On Mon, Apr 22, 2024 at 05:46:10PM -0300, James Almer wrote: > On 4/22/2024 5:40 PM, Mark Thompson wrote: > > On 22/04/2024 02:31, Michael Niedermayer wrote: > > > Found-by-reviewing: CID1419833 Untrusted loop bound > > > > > > Sponsored-by: Sovereign Tech Fund > > > Signed-off-by: Michael Niedermayer > > > --- > > > libavcodec/cbs_h2645.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > > > index fe2e383ff33..1a45d424bae 100644 > > > --- a/libavcodec/cbs_h2645.c > > > +++ b/libavcodec/cbs_h2645.c > > > @@ -709,7 +709,11 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, > > > start = bytestream2_tell(&gbc); > > > for(i = 0; i < num_nalus; i++) { > > > + if (bytestream2_get_bytes_left(&gbc) < 2) > > > + return AVERROR_INVALIDDATA; > > > size = bytestream2_get_be16(&gbc); > > > + if (bytestream2_get_bytes_left(&gbc) < size) > > > + return AVERROR_INVALIDDATA; > > > bytestream2_skip(&gbc, size); > > > } > > > end = bytestream2_tell(&gbc); > > > > Seems fair. > > > > The problem looks more general with missing bounds checks in all the H.266 code around this, though? Compare with H.26[45], which have checks on all the reads - seems like H.266 should be doing that. > > > > Thanks, > > Not against this approach, but since the bytestream2_get_* functions return > 0, never overread the buffer or move the internal pointer, wouldn't it be > enough to just ensure end > start? > Particularly in ff_h2645_packet_split(), we can return an error if length > (in this case being set to end - start) is < 4. The patch adds the same kind of check as are used in the AV_CODEC_ID_HEVC case earlier in the file already I think what is done should approximately stay in sync thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who would give up essential Liberty, to purchase a little temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin