On Mon, Jul 15, 2024 at 03:59:12PM +0200, Michael Niedermayer wrote:
> 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

Will apply the original patch with a note that its not the ideal solution
i dont want to leave this issue open while noone seems working on it

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle