| Message ID | 20240823114451.65930-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: Prevent OOB access in subpic_tiles | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi Frank, thank you for the patch On Fri, Aug 23, 2024 at 7:45 PM Frank Plowman <post@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? 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. > > Patch fixes this issue by replacing != with <, thereby not requiring > subpicture boundaries and tile boundaries to be coincident. > > Signed-off-by: Frank Plowman <post@frankplowman.com> > --- > libavcodec/vvc/ps.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c > index 58496c9fba..ff9a6c7a15 100644 > --- a/libavcodec/vvc/ps.c > +++ b/libavcodec/vvc/ps.c > @@ -384,10 +384,10 @@ static void subpic_tiles(int *tile_x, int *tile_y, > int *tile_x_end, int *tile_y_ > > *tile_x = *tile_y = 0; > > - while (pps->col_bd[*tile_x] != rx) > + while (pps->col_bd[*tile_x] < rx) > (*tile_x)++; > > - while (pps->row_bd[*tile_y] != ry) > + while (pps->row_bd[*tile_y] < ry) > (*tile_y)++; > > *tile_x_end = (*tile_x); > -- > 2.46.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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@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
On Sat, Aug 24, 2024 at 5:01 PM Frank Plowman <post@frankplowman.com> wrote: > 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@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. > oh, thank you for the explanation. Another code may assume subpic coincides with tile. it's my bad. We can merge your patch first and fix another code when we have a valid clip > > >> 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. > Thank you. Sometimes a full check isn't feasible, so we can only do our best. If it's too difficult or the cost is too high, we can leave it as is. > > 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 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c index 58496c9fba..ff9a6c7a15 100644 --- a/libavcodec/vvc/ps.c +++ b/libavcodec/vvc/ps.c @@ -384,10 +384,10 @@ static void subpic_tiles(int *tile_x, int *tile_y, int *tile_x_end, int *tile_y_ *tile_x = *tile_y = 0; - while (pps->col_bd[*tile_x] != rx) + while (pps->col_bd[*tile_x] < rx) (*tile_x)++; - while (pps->row_bd[*tile_y] != ry) + while (pps->row_bd[*tile_y] < ry) (*tile_y)++; *tile_x_end = (*tile_x);
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. 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. Patch fixes this issue by replacing != with <, thereby not requiring subpicture boundaries and tile boundaries to be coincident. Signed-off-by: Frank Plowman <post@frankplowman.com> --- libavcodec/vvc/ps.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)