Re: [FFmpeg-devel] [PATCH 1/7] avcodec/utils: apply the same alignment to YUV410 as we do to YUV420 when motion estimation is used

From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/7] avcodec/utils: apply the same alignment to YUV410 as we do to YUV420 when motion estimation is used
Date: Mon, 15 Jul 2024 15:59:12 +0200
Message-ID: <20240715135912.GH4991@pb2> (raw)
In-Reply-To: <AS8P250MB074442C0B626098F444C01218FCE2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM>

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

On Tue, Jun 18, 2024 at 07:28:18PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > The snow encoder uses block based motion estimation which can read out of array if
> > insufficient alignment is used
> > 
> > Fixes: out of array access
> > Fixes: 68963/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SNOW_fuzzer-4979988435632128
> > Fixes: 68969/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SNOW_fuzzer-6239933667803136.fuzz
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/utils.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 337c00e789a..7914f799041 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -259,6 +259,9 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height,
> >          if (s->codec_id == AV_CODEC_ID_SVQ1) {
> >              w_align = 64;
> >              h_align = 64;
> > +        } else if (s->codec_id == AV_CODEC_ID_SNOW) {
> > +            w_align = 16;
> > +            h_align = 16;
> >          }
> >          break;
> >      case AV_PIX_FMT_RGB555:
> 
> avcodec_align_dimensions2() is for decoders and happens to be used by
> ff_encode_alloc_frame(), too. But decoders should not be required to add
> more padding because the decoder needs more. Instead the encoder should
> add more padding itself (by using more than 2 * EDGE_WIDTH).

If you have verified that the decoder alignment is sufficient and 4:1:0 does not
need the alignment that 4:2:0 has for snow, then yes i can mess with some
EDGE_WIDTH uses in snow to fix the encoder crash

Again the snow decoder for 4:2:0 has 16x16 alignment, 4:1:0 has not
and its identical in the encoders
This is why it crashes, the patch corrects this difference.
I do not think thats a great differernce to have

If we keep the 4:2:0 and 4:1:0 difference in alignment then
what should i do about the encoder ?
adjust EDGE_WIDTH for 4:1:0 only ? because 4:2:0 has the buffer dimensions aligned correctly
so it doesnt need it bumped up?

That would result in 4:2:0 having the alignment from avcodec_align_dimensions2() for
decoder and encoder
and for 4:1:0 we would achieve something comparable with EDGE_WIDTH encoder side only
and hope the decoder doesnt need it

This just doesnt sound like a step in the right direction
Also this as a security fix should be simple

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is a danger to trust the dream we wish for rather than
the science we have, -- Dr. Kenneth Brown

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