[FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Michael Niedermayer]

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(+)

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);
-- 
2.17.1

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

Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Rémi Denis-Courmont]

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.

-- 
レミ・デニ-クールモン
http://www.remlab.net/



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

Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step

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

>              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?
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Andreas Rheinhardt]

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

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

Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Rémi Denis-Courmont]

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.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



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

Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Michael Niedermayer]

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

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

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

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA

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