[FFmpeg-devel] [PATCH] avcodec/fft_template: Fix multiple runtime error: signed integer overflow: -1943918714 - 1935113003 cannot be represented in type 'int'

Michael Niedermayer michael at niedermayer.cc
Sat May 27 04:56:42 EEST 2017


On Fri, May 26, 2017 at 07:06:44PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, May 26, 2017 at 6:34 PM, Michael Niedermayer <michael at niedermayer.cc
> > wrote:
> 
> > On Fri, May 26, 2017 at 06:07:35PM -0400, Ronald S. Bultje wrote:
> > > Hi,
> > >
> > > On Fri, May 26, 2017 at 5:50 PM, Michael Niedermayer
> > <michael at niedermayer.cc
> > > > wrote:
> > >
> > > > On Fri, May 26, 2017 at 11:18:12PM +0200, Hendrik Leppkes wrote:
> > > > > On Fri, May 26, 2017 at 11:11 PM, Michael Niedermayer
> > > > > <michael at niedermayer.cc> wrote:
> > > > > > On Fri, May 26, 2017 at 03:20:14PM +0100, Rostislav Pehlivanov
> > wrote:
> > > > > >> On 26 May 2017 at 12:21, wm4 <nfxjfg at googlemail.com> wrote:
> > > > > >>
> > > > > >> > On Thu, 25 May 2017 16:10:49 +0200
> > > > > >> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > >> >
> > > > > >> > > Fixes: 1735/clusterfuzz-testcase-minimized-5350472347025408
> > > > > >> > >
> > > > > >> > > 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/fft_template.c | 50 +++++++++++++++++++++++-------
> > > > > >> > -----------------
> > > > > >> > >  1 file changed, 25 insertions(+), 25 deletions(-)
> > > > > >> > >
> > > > > >> > > diff --git a/libavcodec/fft_template.c
> > b/libavcodec/fft_template.c
> > > > > >> > > index 480557f49f..e3a37e5d69 100644
> > > > > >> > > --- a/libavcodec/fft_template.c
> > > > > >> > > +++ b/libavcodec/fft_template.c
> > > > > >> > > @@ -249,7 +249,7 @@ static void fft_calc_c(FFTContext *s,
> > > > FFTComplex *z)
> > > > > >> > {
> > > > > >> > >
> > > > > >> > >      int nbits, i, n, num_transforms, offset, step;
> > > > > >> > >      int n4, n2, n34;
> > > > > >> > > -    FFTSample tmp1, tmp2, tmp3, tmp4, tmp5, tmp6, tmp7, tmp8;
> > > > > >> > > +    SUINT tmp1, tmp2, tmp3, tmp4, tmp5, tmp6, tmp7, tmp8;
> > > > > >> >
> > > > > >> > I want this SUINT thing gone, not have more of it.
> > > > > >> > _______________________________________________
> > > > > >> > ffmpeg-devel mailing list
> > > > > >> > ffmpeg-devel at ffmpeg.org
> > > > > >> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > >> >
> > > > > >>
> > > > > >> I agree, especially here.
> > > > > >
> > > > > >> Overflows should be left to happen in transforms if the input is
> > > > corrupt.
> > > > > >
> > > > > > signed int overflow is not allowed in C, if that is what you meant.
> > > > > >
> > > > > >
> > > > >
> > > > > Its "undefined", which means what the result will be is not defined -
> > > > > which in such a DSP function is irrelevant, if all it causes is
> > > > > corrupt output ... from corrupt input.
> > > >
> > > > no, this is not correct.
> > > > undefined behavior does not mean the effect will be limited to
> > > > the output.
> > > > It could cause the process to hard lockup, trigger an exception or
> > > > set a flag causing errors in later unrelated code.
> > >
> > >
> >
> > > We've discussed this before, if you believe this to be exploitable, why
> > > allow it in our repository at all? I know of no other project that even
> > > remotely allows such ridiculous things. Please limit exploitable code to
> > > your personal tree, ffmpeg is not a rootkit.
> >
> > please calm down, you make all kinds of statments and implications in
> > the text above which are not true.
> >
> > This specific code in git triggers undefined behavior, the patch fixes
> > this undefined behavior.
> > If that is exploitable (which i neither claim it is nor that it isnt)
> > its a issue that exists before the patch but not afterwards.
> 
> 

> *unsigned* removes the exploit. SUINT keeps it, and is therefore part of a
> rootkit.

SUINT is defined to unsigned, if unsigned removes the issue
so does SUINT.

If an attacker can convince his victim to define or redefine a symbol
during build then SUINT is neither needed nor anything an attacker
would choose.

redefining SUINT makes sense to developers though to test for
arithmetic overflow. It also keeps a clear seperation between

unsigned values in unsigned types and
signed values in unsigned types  aka "SUINT"


>
> Or there is no exploit, in which case SUINT is not necessary.
>
> ??
>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct answer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170527/0db2cd0d/attachment.sig>


More information about the ffmpeg-devel mailing list