| Message ID | 20170313023637.1848-3-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 45198477de19ccb00729b7eec07d81494f0353e0 |
| Headers | show |
On 3/13/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > Benchmarks with START_TIMER indicate that the code is faster with unsigned, > (that is > with the patch), there was quite some fluctuation in the numbers so this may > be just > random > > Fixes: 811/clusterfuzz-testcase-6465493076541440 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/simple_idct_template.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > So this fixes remote code execution? Please state what commit fixes.
On Mon, Mar 13, 2017 at 09:51:40AM +0100, Paul B Mahol wrote: > On 3/13/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > Benchmarks with START_TIMER indicate that the code is faster with unsigned, > > (that is > > with the patch), there was quite some fluctuation in the numbers so this may > > be just > > random > > > > Fixes: 811/clusterfuzz-testcase-6465493076541440 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/simple_idct_template.c | 36 ++++++++++++++++++------------------ > > 1 file changed, 18 insertions(+), 18 deletions(-) > > > > So this fixes remote code execution? It fixes integer overflows which are undefined behavior, i very much doubt this allows any RCE in practive anywhere but strictly speaking undefined means anything can happen > > Please state what commit fixes. Please see the first line of the commit message, which git puts in the subject of the mail: Subject: Re: [FFmpeg-devel] [PATCH 3/4] avcodec/simple_idct_template: Fix several integer overflows [...]
On Mon, Mar 13, 2017 at 12:53:27PM +0100, Michael Niedermayer wrote: > On Mon, Mar 13, 2017 at 09:51:40AM +0100, Paul B Mahol wrote: > > On 3/13/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > Benchmarks with START_TIMER indicate that the code is faster with unsigned, > > > (that is > > > with the patch), there was quite some fluctuation in the numbers so this may > > > be just > > > random > > > > > > Fixes: 811/clusterfuzz-testcase-6465493076541440 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/simple_idct_template.c | 36 ++++++++++++++++++------------------ > > > 1 file changed, 18 insertions(+), 18 deletions(-) > > > > > > > So this fixes remote code execution? > > It fixes integer overflows which are undefined behavior, > i very much doubt this allows any RCE in practive anywhere but > strictly speaking undefined means anything can happen > > > > > > Please state what commit fixes. > > Please see the first line of the commit message, which git puts in > the subject of the mail: > > Subject: Re: [FFmpeg-devel] [PATCH 3/4] avcodec/simple_idct_template: Fix several integer overflows i will apply the patch in 24h unless there is an objection [...]
On Tue, Mar 14, 2017 at 10:20:45PM +0100, Michael Niedermayer wrote: > On Mon, Mar 13, 2017 at 12:53:27PM +0100, Michael Niedermayer wrote: > > On Mon, Mar 13, 2017 at 09:51:40AM +0100, Paul B Mahol wrote: > > > On 3/13/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > Benchmarks with START_TIMER indicate that the code is faster with unsigned, > > > > (that is > > > > with the patch), there was quite some fluctuation in the numbers so this may > > > > be just > > > > random > > > > > > > > Fixes: 811/clusterfuzz-testcase-6465493076541440 > > > > > > > > Found-by: continuous fuzzing process > > > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/simple_idct_template.c | 36 ++++++++++++++++++------------------ > > > > 1 file changed, 18 insertions(+), 18 deletions(-) > > > > > > > > > > So this fixes remote code execution? > > > > It fixes integer overflows which are undefined behavior, > > i very much doubt this allows any RCE in practive anywhere but > > strictly speaking undefined means anything can happen > > > > > > > > > > Please state what commit fixes. > > > > Please see the first line of the commit message, which git puts in > > the subject of the mail: > > > > Subject: Re: [FFmpeg-devel] [PATCH 3/4] avcodec/simple_idct_template: Fix several integer overflows > > i will apply the patch in 24h unless there is an objection applied [...]
diff --git a/libavcodec/simple_idct_template.c b/libavcodec/simple_idct_template.c index f5744e0a39..c669767761 100644 --- a/libavcodec/simple_idct_template.c +++ b/libavcodec/simple_idct_template.c @@ -112,7 +112,7 @@ static inline void FUNC(idctRowCondDC_extrashift)(int16_t *row, int extra_shift) static inline void FUNC(idctRowCondDC)(int16_t *row, int extra_shift) #endif { - int a0, a1, a2, a3, b0, b1, b2, b3; + SUINT a0, a1, a2, a3, b0, b1, b2, b3; #if HAVE_FAST_64BIT #define ROW0_MASK (0xffffLL << 48 * HAVE_BIGENDIAN) @@ -187,14 +187,14 @@ static inline void FUNC(idctRowCondDC)(int16_t *row, int extra_shift) MAC(b3, -W1, row[7]); } - row[0] = (a0 + b0) >> (ROW_SHIFT + extra_shift); - row[7] = (a0 - b0) >> (ROW_SHIFT + extra_shift); - row[1] = (a1 + b1) >> (ROW_SHIFT + extra_shift); - row[6] = (a1 - b1) >> (ROW_SHIFT + extra_shift); - row[2] = (a2 + b2) >> (ROW_SHIFT + extra_shift); - row[5] = (a2 - b2) >> (ROW_SHIFT + extra_shift); - row[3] = (a3 + b3) >> (ROW_SHIFT + extra_shift); - row[4] = (a3 - b3) >> (ROW_SHIFT + extra_shift); + row[0] = (int)(a0 + b0) >> (ROW_SHIFT + extra_shift); + row[7] = (int)(a0 - b0) >> (ROW_SHIFT + extra_shift); + row[1] = (int)(a1 + b1) >> (ROW_SHIFT + extra_shift); + row[6] = (int)(a1 - b1) >> (ROW_SHIFT + extra_shift); + row[2] = (int)(a2 + b2) >> (ROW_SHIFT + extra_shift); + row[5] = (int)(a2 - b2) >> (ROW_SHIFT + extra_shift); + row[3] = (int)(a3 + b3) >> (ROW_SHIFT + extra_shift); + row[4] = (int)(a3 - b3) >> (ROW_SHIFT + extra_shift); } #define IDCT_COLS do { \ @@ -253,25 +253,25 @@ static inline void FUNC(idctSparseCol_extrashift)(int16_t *col) static inline void FUNC(idctSparseColPut)(pixel *dest, int line_size, int16_t *col) { - int a0, a1, a2, a3, b0, b1, b2, b3; + SUINT a0, a1, a2, a3, b0, b1, b2, b3; IDCT_COLS; - dest[0] = av_clip_pixel((a0 + b0) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a0 + b0) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a1 + b1) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a1 + b1) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a2 + b2) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a2 + b2) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a3 + b3) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a3 + b3) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a3 - b3) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a3 - b3) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a2 - b2) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a2 - b2) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a1 - b1) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a1 - b1) >> COL_SHIFT); dest += line_size; - dest[0] = av_clip_pixel((a0 - b0) >> COL_SHIFT); + dest[0] = av_clip_pixel((int)(a0 - b0) >> COL_SHIFT); } static inline void FUNC(idctSparseColAdd)(pixel *dest, int line_size,
Benchmarks with START_TIMER indicate that the code is faster with unsigned, (that is with the patch), there was quite some fluctuation in the numbers so this may be just random Fixes: 811/clusterfuzz-testcase-6465493076541440 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/simple_idct_template.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)