[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 at 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/