[FFmpeg-devel] [PATCH] lavc/vvc: Prevent OOB access in subpic_tiles

[Frank Plowman]

Hi,

Thanks for your review.

On 24/08/2024 04:40, Nuo Mi wrote:
> Hi Frank,
> thank you for the patch
> On Fri, Aug 23, 2024 at 7:45 PM Frank Plowman <post at frankplowman.com> wrote:
> 
>> The previous logic relied on the subpicture boundaries coinciding with
>> the tile boundaries.  Per 6.3.1 of H.266 (V3), vertical subpicture
>> boundaries are always tile boundaries however the same cannot be said
>> for horizontal subpicture boundaries.
> 
> From the spec:
> "One or both of the following conditions shall be fulfilled for each
> subpicture and tile:
> 
> – All CTUs in a subpicture belong to the same tile.
> 
> – All CTUs in a tile belong to the same subpicture."
> 
> This suggests that the subpicture boundary coincides with a tile boundary,
> right?

Not always.  As an example, consider a picture with only a single tile
but split into two subpictures, one atop the other:

      Tiles       Subpics       Slices
    +-------+    +-------+    +-------+
    |       |    |       |    |       |
    |       |    +-------+    +-------+
    |       |    |       |    |       |
    +-------+    +-------+    +-------+

The first of the conditions is met for both subpics: all CTUs of either
subpic belong to the same tile.  It is clear to see, however, that the
bottom boundary of the top subpicture and the top boundary of the bottom
subpicture do not coincide with a tile boundary.  In conjunction with
the restrictions on subpic and slice layout and those on slice and tile
layout, you get a guarantee that the vertical boundaries of subpics
coincide with vertical boundaries of tiles, but there is no guaranteed
relationship between the horizontal boundaries of subpics and tile
boundaries.

>> Furthermore, it is possible to
>> construct an illegal bitstream where vertical subpicture boundaries are
>> not coincident with tile boundaries.  In these cases, the condition of
>> the while loop would never be satisfied resulting in an OOB read on
>> col_bd/row_bd.
>>
> Can we implement early checks to reject invalid streams?
> If the picture boundaries are incorrect, it indicates a serious error in
> the bitstream.
> 

My first attempt at solving this problem took this approach.  I found
that it was not trivial to write an algorithm which ensures all the
relationships set out in 6.3.1.  As far as I can tell, the restrictions
are only ever set out in prose in that section and never as more formal
requirements on bitstream conformance.  Additionally, these checks
appear to be missing from VTM, so there is no reference implementation
to refer to (aside: VTM is quite happy to encode and decode bitstreams
with invalid partitioning layouts).  As a result, I decided to take the
approach of identifying where the relationships were used and making
changes there instead.

Even if the partitioning structure were validated elsewhere however, the
change to the horizontal boundaries would still be needed as set out
above.  I also think the change to the vertical boundaries does no harm
and serves to prevent an OOB access in the case that validation fails,
however unlikely that may be.

--
All the best,
Frank