| Message ID | 20221002154323.21278-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/bonk: Check step | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit : > Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented > in type 'int' Fixes: > 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481 > 067503616 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/bonk.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c > index 409694f710d..32f7c9b9bdb 100644 > --- a/libavcodec/bonk.c > +++ b/libavcodec/bonk.c > @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int > entries, int base_2_part) if (!dominant) > n_zeros += steplet; > > + if (step > INT_MAX/9*8) > + return AVERROR_INVALIDDATA; > + > step += step / 8; > } else if (steplet > 0) { > int actual_run = read_uint_max(s, steplet - 1); No problem with this patch *specifically* but wouldn't it be more effective to fix that sort of issue with checked arithmetic, e.g. something like: if (av_ckd_add(&step, step, step / 8)) return AVERROR_INVALIDDATA; ...especially on 64-bit systems whence this is really just an add. This also avoids having to figure out what the exact boundary value is.
On 10/2/2022 1:13 PM, Rémi Denis-Courmont wrote: > Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit : >> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented >> in type 'int' Fixes: >> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481 >> 067503616 >> >> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/bonk.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c >> index 409694f710d..32f7c9b9bdb 100644 >> --- a/libavcodec/bonk.c >> +++ b/libavcodec/bonk.c >> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int >> entries, int base_2_part) if (!dominant) >> n_zeros += steplet; >> >> + if (step > INT_MAX/9*8) >> + return AVERROR_INVALIDDATA; >> + >> step += step / 8; >> } else if (steplet > 0) { >> int actual_run = read_uint_max(s, steplet - 1); > > No problem with this patch *specifically* but wouldn't it be more effective to > fix that sort of issue with checked arithmetic, e.g. something like: > > if (av_ckd_add(&step, step, step / 8)) That's __builtin_add_overflow() from gcc/clang. There's av_sat_add32(), which will clip the result to fit in a 32-bit variable. So i guess it could be used and then just check for step == INT32_MAX and error out, but that's slower than what this patch is doing. > return AVERROR_INVALIDDATA; > > ...especially on 64-bit systems whence this is really just an add. This also > avoids having to figure out what the exact boundary value is. What 64-bit arch has sizeof(int) == 8?
James Almer: > On 10/2/2022 1:13 PM, Rémi Denis-Courmont wrote: >> Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a >> écrit : >>> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be >>> represented >>> in type 'int' Fixes: >>> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481 >>> 067503616 >>> >>> Found-by: continuous fuzzing process >>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/bonk.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c >>> index 409694f710d..32f7c9b9bdb 100644 >>> --- a/libavcodec/bonk.c >>> +++ b/libavcodec/bonk.c >>> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, >>> int >>> entries, int base_2_part) if (!dominant) >>> n_zeros += steplet; >>> >>> + if (step > INT_MAX/9*8) >>> + return AVERROR_INVALIDDATA; >>> + >>> step += step / 8; >>> } else if (steplet > 0) { >>> int actual_run = read_uint_max(s, steplet - 1); >> >> No problem with this patch *specifically* but wouldn't it be more >> effective to >> fix that sort of issue with checked arithmetic, e.g. something like: >> >> if (av_ckd_add(&step, step, step / 8)) > > That's __builtin_add_overflow() from gcc/clang. > > There's av_sat_add32(), which will clip the result to fit in a 32-bit > variable. So i guess it could be used and then just check for step == > INT32_MAX and error out, but that's slower than what this patch is doing. > The result of av_sat_add32() being INT32_MAX/MIN does not imply that the result has been saturated. Apart from that, the documentation of av_sat_add32() pretty much presumes that INT_MIN/MAX == INT32_MIN/MAX. >> return AVERROR_INVALIDDATA; >> >> ...especially on 64-bit systems whence this is really just an add. >> This also >> avoids having to figure out what the exact boundary value is. > > What 64-bit arch has sizeof(int) == 8? IIRC on Cray computers all integer types that don't have a size of 1 (character types) have size 8. Notice that there are several places in our codebase where we pretty much presume int/unsigned to be 32bits (and to have no padding). - Andreas
Le sunnuntaina 2. lokakuuta 2022, 19.26.21 EEST James Almer a écrit : > On 10/2/2022 1:13 PM, Rémi Denis-Courmont wrote: > > Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit : > >> Fixes: signed integer overflow: 2040812214 + 255101526 cannot be > >> represented in type 'int' Fixes: > >> 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-47914 > >> 81 > >> 067503616 > >> > >> Found-by: continuous fuzzing process > >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> > >> libavcodec/bonk.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c > >> index 409694f710d..32f7c9b9bdb 100644 > >> --- a/libavcodec/bonk.c > >> +++ b/libavcodec/bonk.c > >> @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int > >> entries, int base_2_part) if (!dominant) > >> > >> n_zeros += steplet; > >> > >> + if (step > INT_MAX/9*8) > >> + return AVERROR_INVALIDDATA; > >> + > >> > >> step += step / 8; > >> > >> } else if (steplet > 0) { > >> > >> int actual_run = read_uint_max(s, steplet - 1); > > > > No problem with this patch *specifically* but wouldn't it be more > > effective to> > > fix that sort of issue with checked arithmetic, e.g. something like: > > if (av_ckd_add(&step, step, step / 8)) > > That's __builtin_add_overflow() from gcc/clang. I took the name and paramater order ckd_add from Cnext's <stdckdint.h>. > There's av_sat_add32(), which will clip the result to fit in a 32-bit > variable. So i guess it could be used and then just check for step == > INT32_MAX and error out, but that's slower than what this patch is doing. Yes but the saturation behaviour is what makes it slower, and it's totally unnecessary here. On x86-64, GCC generates this for Michael's code: cmp $0x71c71c70,%edi jg 1f test %edi,%edi lea 0x7(%rdi),%eax cmovns %edi,%eax sar $0x3,%eax add %edi,%eax ret 1: cs nopw 0x0(%rax,%rax,1) mov $0xffffffea,%eax ret cs nopw 0x0(%rax,%rax,1) And this if you use checked overflowing arithmetic: test %edi,%edi lea 0x7(%rdi),%eax mov $0xffffffea,%edx cmovns %edi,%eax sar $0x3,%eax add %edi,%eax cmovo %edx,%eax ret > > return AVERROR_INVALIDDATA; > > > > ...especially on 64-bit systems whence this is really just an add. This > > also avoids having to figure out what the exact boundary value is. > > What 64-bit arch has sizeof(int) == 8? None. But the compiler can freely upgrade the addition to 64-bit internally.
On Sun, Oct 02, 2022 at 07:13:39PM +0300, Rémi Denis-Courmont wrote: > Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit : > > Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented > > in type 'int' Fixes: > > 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481 > > 067503616 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/bonk.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c > > index 409694f710d..32f7c9b9bdb 100644 > > --- a/libavcodec/bonk.c > > +++ b/libavcodec/bonk.c > > @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int > > entries, int base_2_part) if (!dominant) > > n_zeros += steplet; > > > > + if (step > INT_MAX/9*8) if you want the exact limit for any INT_MAX its: (INT_MAX + 1ULL) / 9 * 8 + (INT_MAX + 1ULL) % 9 - 1; though probably a fixed bit value would be better anyway for reproducability > > + return AVERROR_INVALIDDATA; > > + > > step += step / 8; > > } else if (steplet > 0) { > > int actual_run = read_uint_max(s, steplet - 1); > > No problem with this patch *specifically* but wouldn't it be more effective to > fix that sort of issue with checked arithmetic, e.g. something like: > > if (av_ckd_add(&step, step, step / 8)) > return AVERROR_INVALIDDATA; > > ...especially on 64-bit systems whence this is really just an add. This also > avoids having to figure out what the exact boundary value is. If someone has a set of testfiles for this decoder then I can look at this and cleanup the code more completely. i dont know if a step > C vs overflow check is better but i have the suspicion there are more problems as multiple statments around this seem unreachable. And iam also missing some check that i would expect to be in there I found links to datafilehost.com to test files but they are all expired so i have no test files, so iam a bit cautious with changes ... CCing piotr who linked to the files on datafilehost, maybe he still has them. thx [...]
diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c index 409694f710d..32f7c9b9bdb 100644 --- a/libavcodec/bonk.c +++ b/libavcodec/bonk.c @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int entries, int base_2_part) if (!dominant) n_zeros += steplet; + if (step > INT_MAX/9*8) + return AVERROR_INVALIDDATA; + step += step / 8; } else if (steplet > 0) { int actual_run = read_uint_max(s, steplet - 1);
Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented in type 'int' Fixes: 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481067503616 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/bonk.c | 3 +++ 1 file changed, 3 insertions(+)