| Message ID | 20191216231946.11260-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
Dec 16, 2019, 23:19 by michael@niedermayer.cc: > Fixes: out of array read > Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > Just zero the entire ATRAC9ChannelData->band_ext_data and return if !get_bits(gb, 5). That way mode 0 won't change the signal and mode 1, 2, 3 and 4 will have minimal effect.The 5 bits that are read are meant to correspond to the length (already known) of the band extension data to be read. I'm not sure what Sony were thinking if its 0. And ping me on IRC next time.
Dec 26, 2019, 13:57 by dev@lynne.ee: > Dec 16, 2019, 23:19 by michael@niedermayer.cc: > >> Fixes: out of array read >> Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> Actually nevermind, patch is good as-is. I think a 0 len just means to reuse the parameters from the previous. So clipping them should be fine. You should replace the FFMIN(val, (len << 1) - 1) with a av_clip_uintp2(val, len) which does exactly that.
On 12/26/2019 1:24 PM, Lynne wrote: > Dec 26, 2019, 13:57 by dev@lynne.ee: > >> Dec 16, 2019, 23:19 by michael@niedermayer.cc: >> >>> Fixes: out of array read >>> Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> > > Actually nevermind, patch is good as-is. I think a 0 len just means to reuse the parameters from the previous. So clipping them should be fine. > You should replace the FFMIN(val, (len << 1) - 1) with a av_clip_uintp2(val, len) which does exactly that. I guess he didn't use it because c->band_ext_data is an int array. A quick look at the code shows it apparently can't be negative, so av_clip_uintp2() should be safe, but maybe the type should be changed to unsigned to avoid confusion. > _______________________________________________ > 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". >
On 12/26/2019 1:30 PM, James Almer wrote: > On 12/26/2019 1:24 PM, Lynne wrote: >> Dec 26, 2019, 13:57 by dev@lynne.ee: >> >>> Dec 16, 2019, 23:19 by michael@niedermayer.cc: >>> >>>> Fixes: out of array read >>>> Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 >>>> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> >> >> Actually nevermind, patch is good as-is. I think a 0 len just means to reuse the parameters from the previous. So clipping them should be fine. >> You should replace the FFMIN(val, (len << 1) - 1) with a av_clip_uintp2(val, len) which does exactly that. > > I guess he didn't use it because c->band_ext_data is an int array. A > quick look at the code shows it apparently can't be negative, so > av_clip_uintp2() should be safe, but maybe the type should be changed to > unsigned to avoid confusion. Nevermind, didn't realize av_clip_uintp2() takes ints as input arguments.
On Thu, Dec 26, 2019 at 05:24:31PM +0100, Lynne wrote: > Dec 26, 2019, 13:57 by dev@lynne.ee: > > > Dec 16, 2019, 23:19 by michael@niedermayer.cc: > > > >> Fixes: out of array read > >> Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 > >> > >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > > Actually nevermind, patch is good as-is. I think a 0 len just means to reuse the parameters from the previous. So clipping them should be fine. > You should replace the FFMIN(val, (len << 1) - 1) with a av_clip_uintp2(val, len) which does exactly that. i think both the FFMIN and av_clip_uintp2 do the same thing here, unless iam missing something. But i agree The later is a bit prettier, and as you prefer it too, ill change it and will apply with that Thanks [...]
Dec 27, 2019, 21:35 by michael@niedermayer.cc: > On Thu, Dec 26, 2019 at 05:24:31PM +0100, Lynne wrote: > >> Dec 26, 2019, 13:57 by dev@lynne.ee: >> >> > Dec 16, 2019, 23:19 by michael@niedermayer.cc: >> > >> >> Fixes: out of array read >> >> Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 >> >> >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> >> >> Actually nevermind, patch is good as-is. I think a 0 len just means to reuse the parameters from the previous. So clipping them should be fine. >> You should replace the FFMIN(val, (len << 1) - 1) with a av_clip_uintp2(val, len) which does exactly that. >> > > i think both the FFMIN and av_clip_uintp2 do the same thing here, unless > iam missing something. But i agree > The later is a bit prettier, and as you prefer it too, ill change it and will apply with that > Why did you use av_clip_uintp2_c() instead of av_clip_uintp2()?
On Sat, Dec 28, 2019 at 02:03:37PM +0100, Lynne wrote: > Dec 27, 2019, 21:35 by michael@niedermayer.cc: > > > On Thu, Dec 26, 2019 at 05:24:31PM +0100, Lynne wrote: > > > >> Dec 26, 2019, 13:57 by dev@lynne.ee: > >> > >> > Dec 16, 2019, 23:19 by michael@niedermayer.cc: > >> > > >> >> Fixes: out of array read > >> >> Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 > >> >> > >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> >> > >> > >> Actually nevermind, patch is good as-is. I think a 0 len just means to reuse the parameters from the previous. So clipping them should be fine. > >> You should replace the FFMIN(val, (len << 1) - 1) with a av_clip_uintp2(val, len) which does exactly that. > >> > > > > i think both the FFMIN and av_clip_uintp2 do the same thing here, unless > > iam missing something. But i agree > > The later is a bit prettier, and as you prefer it too, ill change it and will apply with that > > > > Why did you use av_clip_uintp2_c() instead of av_clip_uintp2()? Because len is not a constant, av_clip_uintp2() only works with constants (it will fail to build even on ARM for example if av_clip_uintp2() is used) Thx [...]
diff --git a/libavcodec/atrac9dec.c b/libavcodec/atrac9dec.c index 5415d1348e..53c5848057 100644 --- a/libavcodec/atrac9dec.c +++ b/libavcodec/atrac9dec.c @@ -223,8 +223,18 @@ static inline int parse_band_ext(ATRAC9Context *s, ATRAC9BlockData *b, b->channel[0].band_ext = get_bits(gb, 2); b->channel[0].band_ext = ext_band > 2 ? b->channel[0].band_ext : 4; - if (!get_bits(gb, 5)) + if (!get_bits(gb, 5)) { + for (int i = 0; i <= stereo; i++) { + ATRAC9ChannelData *c = &b->channel[i]; + const int count = at9_tab_band_ext_cnt[c->band_ext][ext_band]; + for (int j = 0; j < count; j++) { + int len = at9_tab_band_ext_lengths[c->band_ext][ext_band][j]; + c->band_ext_data[j] = FFMIN(c->band_ext_data[j], (1<<len) - 1); + } + } + return 0; + } for (int i = 0; i <= stereo; i++) { ATRAC9ChannelData *c = &b->channel[i];
Fixes: out of array read Fixes: 19327/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5679823087468544 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/atrac9dec.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)