Re: [FFmpeg-devel] [PATCH 1/6] avcodec/eatgq: : Check index increments in tgq_decode_block()

From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/6] avcodec/eatgq: : Check index increments in tgq_decode_block()
Date: Sun, 15 Jan 2023 18:21:19 +0100
Message-ID: <20230115172119.GU1949656@pb2> (raw)
In-Reply-To: <Y8Np6tmypQAO/n9K@4eb231a0d1b36cedda43a2a005befe4d>

[-- Attachment #1.1: Type: text/plain, Size: 2912 bytes --]

On Sun, Jan 15, 2023 at 01:50:18PM +1100, Peter Ross wrote:
> On Thu, Jan 12, 2023 at 12:54:27AM +0100, Michael Niedermayer wrote:
> > Fixes: out of array access
> > Fixes: 48567/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_EATGQ_fuzzer-6743211456724992
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/eatgq.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/eatgq.c b/libavcodec/eatgq.c
> > index 89e9f20880..beb9f4d046 100644
> > --- a/libavcodec/eatgq.c
> > +++ b/libavcodec/eatgq.c
> > @@ -56,7 +56,7 @@ static av_cold int tgq_decode_init(AVCodecContext *avctx)
> >      return 0;
> >  }
> >  
> > -static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb)
> > +static int tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb)
> >  {
> >      const uint8_t *scantable = ff_zigzag_direct;
> >      int i, j, value;
> > @@ -64,6 +64,8 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb
> >      for (i = 1; i < 64;) {
> >          switch (show_bits(gb, 3)) {
> >          case 4:
> > +            if (i >= 63)
> > +                return AVERROR_INVALIDDATA;
> >              block[scantable[i++]] = 0;
> >          case 0:
> >              block[scantable[i++]] = 0;
> > @@ -73,6 +75,8 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb
> >          case 1:
> >              skip_bits(gb, 2);
> >              value = get_bits(gb, 6);
> > +            if (value > 64 - i)
> > +                return AVERROR_INVALIDDATA;
> >              for (j = 0; j < value; j++)
> >                  block[scantable[i++]] = 0;
> >              break;
> > @@ -100,6 +104,7 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb
> >          }
> >      }
> >      block[0] += 128 << 4;
> > +    return 0;
> >  }
> >  
> >  static void tgq_idct_put_mb(TgqContext *s, int16_t (*block)[64], AVFrame *frame,
> > -- 
> > 2.17.1
> 
> Hi Michael,
> 
> The return value of tgq_decode() is not checked anywhere.
> 
> Need to add something like:
> 
> -            tgq_decode_block(s, s->block[i], &gb);
> +            if ((ret = tgq_decode_block(s, s->block[i], &gb)) < 0)
> +                return ret;

will apply with this 

thx

> 
> (From earlier attempt at fixing this: https://patchwork.ffmpeg.org/project/ffmpeg/patch/1ab7c3994301a243fc64d59d6a08e3a2b364e411.1666774269.git.pross@xvid.org/)
> 
> -- Peter
> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".