| Message ID | 20240618134826.2189719-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avcodec/utils: apply the same alignment to YUV410 as we do to YUV420 when motion estimation is used | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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). - Andreas
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 [...]
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 [...]
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:
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(+)