[FFmpeg-devel,2/4] avcodec/hevc_ps: Fix integer overflow with num_tile_rows

Submitted by Michael Niedermayer on June 13, 2019, 6:32 p.m.

Details

Message ID 20190613183236.19353-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 13, 2019, 6:32 p.m.
Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/hevc_ps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ruiling Song June 15, 2019, 3:07 p.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Michael Niedermayer

> Sent: Friday, June 14, 2019 2:33 AM

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer overflow

> with num_tile_rows

> 

> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in

> type 'int'

> Fixes: 14880/clusterfuzz-testcase-minimized-

> ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536

> 

> Found-by: continuous fuzzing process https://github.com/google/oss-

> fuzz/tree/master/projects/ffmpeg

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

> ---

>  libavcodec/hevc_ps.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c

> index 80df417e4f..0ed6682bb4 100644

> --- a/libavcodec/hevc_ps.c

> +++ b/libavcodec/hevc_ps.c

> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,

> AVCodecContext *avctx,

>          if (pps->num_tile_rows <= 0 ||

>              pps->num_tile_rows >= sps->height) {

>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of

> range: %d\n",

> -                   pps->num_tile_rows - 1);

> +                   pps->num_tile_rows - 1U);

I think the machine code generated here should be the same, right?
So you just tell fuzzer "I am doing subtraction between unsigned numbers", to make it happy?

>              ret = AVERROR_INVALIDDATA;

>              goto err;

>          }

> --

> 2.21.0

> 

> _______________________________________________

> 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".
Michael Niedermayer June 15, 2019, 10:07 p.m.
On Sat, Jun 15, 2019 at 03:07:13PM +0000, Song, Ruiling wrote:
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> > Of Michael Niedermayer
> > Sent: Friday, June 14, 2019 2:33 AM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer overflow
> > with num_tile_rows
> > 
> > Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in
> > type 'int'
> > Fixes: 14880/clusterfuzz-testcase-minimized-
> > ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-
> > fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc_ps.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > index 80df417e4f..0ed6682bb4 100644
> > --- a/libavcodec/hevc_ps.c
> > +++ b/libavcodec/hevc_ps.c
> > @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,
> > AVCodecContext *avctx,
> >          if (pps->num_tile_rows <= 0 ||
> >              pps->num_tile_rows >= sps->height) {
> >              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of
> > range: %d\n",
> > -                   pps->num_tile_rows - 1);
> > +                   pps->num_tile_rows - 1U);
> I think the machine code generated here should be the same, right?
> So you just tell fuzzer "I am doing subtraction between unsigned numbers", to make it happy?

its likely the same machine code, yes. A compiler might produce different code
that break in case of the overflow though ...

thx

[...]
Ruiling Song June 17, 2019, 1:08 a.m.
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Sunday, June 16, 2019 6:07 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer
> overflow with num_tile_rows
> 
> On Sat, Jun 15, 2019 at 03:07:13PM +0000, Song, Ruiling wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On
> Behalf
> > > Of Michael Niedermayer
> > > Sent: Friday, June 14, 2019 2:33 AM
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer
> overflow
> > > with num_tile_rows
> > >
> > > Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in
> > > type 'int'
> > > Fixes: 14880/clusterfuzz-testcase-minimized-
> > > ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> > >
> > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/hevc_ps.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > > index 80df417e4f..0ed6682bb4 100644
> > > --- a/libavcodec/hevc_ps.c
> > > +++ b/libavcodec/hevc_ps.c
> > > @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext
> *gb,
> > > AVCodecContext *avctx,
> > >          if (pps->num_tile_rows <= 0 ||
> > >              pps->num_tile_rows >= sps->height) {
> > >              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of
> > > range: %d\n",
> > > -                   pps->num_tile_rows - 1);
> > > +                   pps->num_tile_rows - 1U);
> > I think the machine code generated here should be the same, right?
> > So you just tell fuzzer "I am doing subtraction between unsigned numbers",
> to make it happy?
> 
> its likely the same machine code, yes. A compiler might produce different
> code
> that break in case of the overflow though ...
Ok, it seems num_tile_columns also need such kind of change.

> 
> thx
> 
> [...]
> --
> Michael     GnuPG fingerprint:
> 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> When the tyrant has disposed of foreign enemies by conquest or treaty, and
> there is nothing more to fear from them, then he is always stirring up
> some war or other, in order that the people may require a leader. -- Plato
James Almer June 17, 2019, 2:10 a.m.
On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc_ps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index 80df417e4f..0ed6682bb4 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>          if (pps->num_tile_rows <= 0 ||
>              pps->num_tile_rows >= sps->height) {
>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> -                   pps->num_tile_rows - 1);
> +                   pps->num_tile_rows - 1U);

The proper fix for this is making pps->num_tile_rows/cols unsigned. The
minimum allowed value for num_tile_{rows,cols}_minus1 is 0.

>              ret = AVERROR_INVALIDDATA;
>              goto err;
>          }
>
Michael Niedermayer June 17, 2019, 9:54 p.m.
On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> > Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> > Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc_ps.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > index 80df417e4f..0ed6682bb4 100644
> > --- a/libavcodec/hevc_ps.c
> > +++ b/libavcodec/hevc_ps.c
> > @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >          if (pps->num_tile_rows <= 0 ||
> >              pps->num_tile_rows >= sps->height) {
> >              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> > -                   pps->num_tile_rows - 1);
> > +                   pps->num_tile_rows - 1U);
> 
> The proper fix for this is making pps->num_tile_rows/cols unsigned. 

I dont think "unsigned int" is wise to use as type here, the semantics 
of unsigned ints are unexpected to many
especially making random subsets of "normal" fields unsigned will make
the codebase slowly "interresting".

is this here ok if num_tile_rows is 0 ?
for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)

i would guess nearly everyone wold say yes without having seen the
discussion about the type. but of course if this is unsigned its not
going to be safe with it being 0. 


> The
> minimum allowed value for num_tile_{rows,cols}_minus1 is 0.

yes


[...]
James Almer June 17, 2019, 10:55 p.m.
On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/hevc_ps.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>> index 80df417e4f..0ed6682bb4 100644
>>> --- a/libavcodec/hevc_ps.c
>>> +++ b/libavcodec/hevc_ps.c
>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>>          if (pps->num_tile_rows <= 0 ||
>>>              pps->num_tile_rows >= sps->height) {
>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
>>> -                   pps->num_tile_rows - 1);
>>> +                   pps->num_tile_rows - 1U);
>>
>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> 
> I dont think "unsigned int" is wise to use as type here, the semantics 
> of unsigned ints are unexpected to many
> especially making random subsets of "normal" fields unsigned will make
> the codebase slowly "interresting".

If you make the actual unsigned values as per the spec be unsigned, it
will not be a random subset of values... And i see plenty of unsigned
and uint*_t fields in hevc_ps.h. These for some reason were given the
wrong type.

> 
> is this here ok if num_tile_rows is 0 ?
> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
> 
> i would guess nearly everyone wold say yes without having seen the
> discussion about the type. but of course if this is unsigned its not
> going to be safe with it being 0. 

pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
it would be a bug. Int is definitely not the right type for it.

By making these two fields unsigned you can remove the
"pps->num_tile_{rows,cols} <= 0" checks, leaving only the
"pps->num_tile_{rows,cols} >= sps->{height,width}" checks. Just add an
av_assert0(pps->num_tile_{rows,cols} > 0) before them and the av_log()
calls, which is also before the loop you quoted, if you want to be sure
no change in the future introduces issues.


> 
> 
>> The
>> minimum allowed value for num_tile_{rows,cols}_minus1 is 0.
> 
> yes
> 
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer June 19, 2019, 9:05 a.m.
On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> > On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> >> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> >>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> >>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/hevc_ps.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>> index 80df417e4f..0ed6682bb4 100644
> >>> --- a/libavcodec/hevc_ps.c
> >>> +++ b/libavcodec/hevc_ps.c
> >>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >>>          if (pps->num_tile_rows <= 0 ||
> >>>              pps->num_tile_rows >= sps->height) {
> >>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> >>> -                   pps->num_tile_rows - 1);
> >>> +                   pps->num_tile_rows - 1U);
> >>
> >> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> > 
> > I dont think "unsigned int" is wise to use as type here, the semantics 
> > of unsigned ints are unexpected to many
> > especially making random subsets of "normal" fields unsigned will make
> > the codebase slowly "interresting".
> 
> If you make the actual unsigned values as per the spec be unsigned, it
> will not be a random subset of values... 

no, but it would require very precisse knowledge of the types used in the
spec though. Iam not sure everyone working on the code has this knowledge


> And i see plenty of unsigned
> and uint*_t fields in hevc_ps.h. These for some reason were given the
> wrong type.

uint8/16 is not unsigned in this sense in C. So these are not a problem
at all. Ill cover uint32 below, but first consider uint8/16 and unsigned

int main(){
    uint8_t u8 = 0;
    unsigned u = 0;

    printf("is u-1 negative: %d, is u8-1 negative: %d\n", u-1 < 0, u8-1 < 0);
}
This produces:
is u-1 negative: 0, is u8-1 negative: 1

the uint8_t type is a signed int in C expressions, unsigned is a unsigned int in C

Now about uint32_t, that you probably dont want to hear about but it should
behave platform dependant. On a system that has 32bit ints its unsigned, on a platform
where int is 64bit it should be signed.
We did not run into this yet because platforms with 64bit int are uncommon.
the world choose to keep int 32bit even on 64bit architectures in general.

But because of that use of uint32_t except for array types is probably simply
a bad idea.

Iam not sure today if i like C, normally i like it ...

Thanks


[...]
Michael Niedermayer June 19, 2019, 9:22 a.m.
On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> > On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> >> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> >>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> >>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/hevc_ps.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>> index 80df417e4f..0ed6682bb4 100644
> >>> --- a/libavcodec/hevc_ps.c
> >>> +++ b/libavcodec/hevc_ps.c
> >>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >>>          if (pps->num_tile_rows <= 0 ||
> >>>              pps->num_tile_rows >= sps->height) {
> >>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> >>> -                   pps->num_tile_rows - 1);
> >>> +                   pps->num_tile_rows - 1U);
> >>
> >> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
[...]
> > 
> > is this here ok if num_tile_rows is 0 ?
> > for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
> > 
> > i would guess nearly everyone wold say yes without having seen the
> > discussion about the type. but of course if this is unsigned its not
> > going to be safe with it being 0. 
> 
> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
> it would be a bug. Int is definitely not the right type for it.

i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
makes much sense but sure i can change it to unsigned if preferred.

[...]
James Almer June 19, 2019, 3:54 p.m.
On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
>>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavcodec/hevc_ps.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>>>> index 80df417e4f..0ed6682bb4 100644
>>>>> --- a/libavcodec/hevc_ps.c
>>>>> +++ b/libavcodec/hevc_ps.c
>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>>>>          if (pps->num_tile_rows <= 0 ||
>>>>>              pps->num_tile_rows >= sps->height) {
>>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
>>>>> -                   pps->num_tile_rows - 1);
>>>>> +                   pps->num_tile_rows - 1U);
>>>>
>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> [...]
>>>
>>> is this here ok if num_tile_rows is 0 ?
>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
>>>
>>> i would guess nearly everyone wold say yes without having seen the
>>> discussion about the type. but of course if this is unsigned its not
>>> going to be safe with it being 0. 
>>
>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
>> it would be a bug. Int is definitely not the right type for it.
> 
> i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
> makes much sense but sure i can change it to unsigned if preferred.

Of course it doesn't. In pretty much any sample it will be at least 1
and at most 22, which is the max value allowed by hevc level 6.2 in
table A.6. Only if the stream reports an undefined level it could go up
to, if i'm reading this right, sps->ctb_height and not sps->height as
the decoder is currently checking. This means you can even replace
get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.

In any case, the bitstream value is unsigned, so the struct field should
be unsigned as well. Having it be signed and assigning it a value using
a function that potentially returns huge unsigned values introduced this
issue to being with, so instead of working around it using type
promotion, just make it follow the spec.

> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer June 19, 2019, 6:13 p.m.
On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote:
> On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
> > On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
> >> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> >>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> >>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> >>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> >>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> >>>>>
> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>> ---
> >>>>>  libavcodec/hevc_ps.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>>>> index 80df417e4f..0ed6682bb4 100644
> >>>>> --- a/libavcodec/hevc_ps.c
> >>>>> +++ b/libavcodec/hevc_ps.c
> >>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >>>>>          if (pps->num_tile_rows <= 0 ||
> >>>>>              pps->num_tile_rows >= sps->height) {
> >>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> >>>>> -                   pps->num_tile_rows - 1);
> >>>>> +                   pps->num_tile_rows - 1U);
> >>>>
> >>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> > [...]
> >>>
> >>> is this here ok if num_tile_rows is 0 ?
> >>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
> >>>
> >>> i would guess nearly everyone wold say yes without having seen the
> >>> discussion about the type. but of course if this is unsigned its not
> >>> going to be safe with it being 0. 
> >>
> >> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
> >> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
> >> it would be a bug. Int is definitely not the right type for it.
> > 
> > i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
> > makes much sense but sure i can change it to unsigned if preferred.
> 
> Of course it doesn't. In pretty much any sample it will be at least 1
> and at most 22, which is the max value allowed by hevc level 6.2 in
> table A.6. Only if the stream reports an undefined level it could go up
> to, if i'm reading this right, sps->ctb_height and not sps->height as
> the decoder is currently checking. This means you can even replace
> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.
> 
> In any case, the bitstream value is unsigned, so the struct field should
> be unsigned as well. Having it be signed and assigning it a value using
> a function that potentially returns huge unsigned values introduced this
> issue to being with, so instead of working around it using type
> promotion, just make it follow the spec.

what would be your feeling/oppinon about making the field uint16_t ?
this would make it unsigned in the struct but avoid the problems with
unsigned int ?

Thanks

[...]
James Almer June 19, 2019, 6:59 p.m.
On 6/19/2019 3:13 PM, Michael Niedermayer wrote:
> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote:
>> On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
>>> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
>>>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
>>>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
>>>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
>>>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
>>>>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
>>>>>>>
>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>> ---
>>>>>>>  libavcodec/hevc_ps.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>>>>>> index 80df417e4f..0ed6682bb4 100644
>>>>>>> --- a/libavcodec/hevc_ps.c
>>>>>>> +++ b/libavcodec/hevc_ps.c
>>>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>>>>>>          if (pps->num_tile_rows <= 0 ||
>>>>>>>              pps->num_tile_rows >= sps->height) {
>>>>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
>>>>>>> -                   pps->num_tile_rows - 1);
>>>>>>> +                   pps->num_tile_rows - 1U);
>>>>>>
>>>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
>>> [...]
>>>>>
>>>>> is this here ok if num_tile_rows is 0 ?
>>>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
>>>>>
>>>>> i would guess nearly everyone wold say yes without having seen the
>>>>> discussion about the type. but of course if this is unsigned its not
>>>>> going to be safe with it being 0. 
>>>>
>>>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
>>>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
>>>> it would be a bug. Int is definitely not the right type for it.
>>>
>>> i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
>>> makes much sense but sure i can change it to unsigned if preferred.
>>
>> Of course it doesn't. In pretty much any sample it will be at least 1
>> and at most 22, which is the max value allowed by hevc level 6.2 in
>> table A.6. Only if the stream reports an undefined level it could go up
>> to, if i'm reading this right, sps->ctb_height and not sps->height as
>> the decoder is currently checking. This means you can even replace
>> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.
>>
>> In any case, the bitstream value is unsigned, so the struct field should
>> be unsigned as well. Having it be signed and assigning it a value using
>> a function that potentially returns huge unsigned values introduced this
>> issue to being with, so instead of working around it using type
>> promotion, just make it follow the spec.
> 
> what would be your feeling/oppinon about making the field uint16_t ?
> this would make it unsigned in the struct but avoid the problems with
> unsigned int ?

That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also
change it to use get_ue_golomb() while at it. And please do it for both
rows and columns. Can be in separate patches if you want the rows one to
explicitly address the fuzzing testcase.

Can you also confirm my suspicion that the checks should be comparing
the values with sps->ctb_height/weight and not with sps->height/weight?

> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
James Almer June 20, 2019, 4:04 a.m.
On 6/19/2019 3:59 PM, James Almer wrote:
> On 6/19/2019 3:13 PM, Michael Niedermayer wrote:
>> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote:
>>> On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
>>>> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
>>>>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
>>>>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
>>>>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
>>>>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
>>>>>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
>>>>>>>>
>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>> ---
>>>>>>>>  libavcodec/hevc_ps.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>>>>>>> index 80df417e4f..0ed6682bb4 100644
>>>>>>>> --- a/libavcodec/hevc_ps.c
>>>>>>>> +++ b/libavcodec/hevc_ps.c
>>>>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>>>>>>>          if (pps->num_tile_rows <= 0 ||
>>>>>>>>              pps->num_tile_rows >= sps->height) {
>>>>>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
>>>>>>>> -                   pps->num_tile_rows - 1);
>>>>>>>> +                   pps->num_tile_rows - 1U);
>>>>>>>
>>>>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
>>>> [...]
>>>>>>
>>>>>> is this here ok if num_tile_rows is 0 ?
>>>>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
>>>>>>
>>>>>> i would guess nearly everyone wold say yes without having seen the
>>>>>> discussion about the type. but of course if this is unsigned its not
>>>>>> going to be safe with it being 0. 
>>>>>
>>>>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
>>>>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
>>>>> it would be a bug. Int is definitely not the right type for it.
>>>>
>>>> i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
>>>> makes much sense but sure i can change it to unsigned if preferred.
>>>
>>> Of course it doesn't. In pretty much any sample it will be at least 1
>>> and at most 22, which is the max value allowed by hevc level 6.2 in
>>> table A.6. Only if the stream reports an undefined level it could go up
>>> to, if i'm reading this right, sps->ctb_height and not sps->height as
>>> the decoder is currently checking. This means you can even replace
>>> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.
>>>
>>> In any case, the bitstream value is unsigned, so the struct field should
>>> be unsigned as well. Having it be signed and assigning it a value using
>>> a function that potentially returns huge unsigned values introduced this
>>> issue to being with, so instead of working around it using type
>>> promotion, just make it follow the spec.
>>
>> what would be your feeling/oppinon about making the field uint16_t ?
>> this would make it unsigned in the struct but avoid the problems with
>> unsigned int ?
> 
> That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also
> change it to use get_ue_golomb() while at it. And please do it for both
> rows and columns. Can be in separate patches if you want the rows one to
> explicitly address the fuzzing testcase.
> 
> Can you also confirm my suspicion that the checks should be comparing
> the values with sps->ctb_height/weight and not with sps->height/weight?

Ok, so yes, it is. Should be changed in a separate patch as well.

> 
>>
>> Thanks
>>
>> [...]
>>
>>
>> _______________________________________________
>> 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".
>>
>
Michael Niedermayer June 25, 2019, 8:23 a.m.
On Wed, Jun 19, 2019 at 03:59:47PM -0300, James Almer wrote:
> On 6/19/2019 3:13 PM, Michael Niedermayer wrote:
> > On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote:
> >> On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
> >>> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
> >>>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> >>>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> >>>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> >>>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> >>>>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> >>>>>>>
> >>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>>>> ---
> >>>>>>>  libavcodec/hevc_ps.c | 2 +-
> >>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>>>>>> index 80df417e4f..0ed6682bb4 100644
> >>>>>>> --- a/libavcodec/hevc_ps.c
> >>>>>>> +++ b/libavcodec/hevc_ps.c
> >>>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >>>>>>>          if (pps->num_tile_rows <= 0 ||
> >>>>>>>              pps->num_tile_rows >= sps->height) {
> >>>>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> >>>>>>> -                   pps->num_tile_rows - 1);
> >>>>>>> +                   pps->num_tile_rows - 1U);
> >>>>>>
> >>>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> >>> [...]
> >>>>>
> >>>>> is this here ok if num_tile_rows is 0 ?
> >>>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
> >>>>>
> >>>>> i would guess nearly everyone wold say yes without having seen the
> >>>>> discussion about the type. but of course if this is unsigned its not
> >>>>> going to be safe with it being 0. 
> >>>>
> >>>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
> >>>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
> >>>> it would be a bug. Int is definitely not the right type for it.
> >>>
> >>> i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
> >>> makes much sense but sure i can change it to unsigned if preferred.
> >>
> >> Of course it doesn't. In pretty much any sample it will be at least 1
> >> and at most 22, which is the max value allowed by hevc level 6.2 in
> >> table A.6. Only if the stream reports an undefined level it could go up
> >> to, if i'm reading this right, sps->ctb_height and not sps->height as
> >> the decoder is currently checking. This means you can even replace
> >> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.
> >>
> >> In any case, the bitstream value is unsigned, so the struct field should
> >> be unsigned as well. Having it be signed and assigning it a value using
> >> a function that potentially returns huge unsigned values introduced this
> >> issue to being with, so instead of working around it using type
> >> promotion, just make it follow the spec.
> > 
> > what would be your feeling/oppinon about making the field uint16_t ?
> > this would make it unsigned in the struct but avoid the problems with
> > unsigned int ?
> 
> That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also
> change it to use get_ue_golomb() while at it. And please do it for both
> rows and columns. Can be in separate patches if you want the rows one to
> explicitly address the fuzzing testcase.

will do but this requires intermediate int, ill post a new patch
get_ue_golomb() can return negative error codes. storing this in an 8bit
variable would truncate the error code and could fold it into a valid
value.


[...]
Michael Niedermayer June 25, 2019, 8:24 a.m.
On Thu, Jun 20, 2019 at 01:04:45AM -0300, James Almer wrote:
> On 6/19/2019 3:59 PM, James Almer wrote:
> > On 6/19/2019 3:13 PM, Michael Niedermayer wrote:
> >> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote:
> >>> On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
> >>>> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
> >>>>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> >>>>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> >>>>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> >>>>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> >>>>>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> >>>>>>>>
> >>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>>>>> ---
> >>>>>>>>  libavcodec/hevc_ps.c | 2 +-
> >>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>>>>>>> index 80df417e4f..0ed6682bb4 100644
> >>>>>>>> --- a/libavcodec/hevc_ps.c
> >>>>>>>> +++ b/libavcodec/hevc_ps.c
> >>>>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >>>>>>>>          if (pps->num_tile_rows <= 0 ||
> >>>>>>>>              pps->num_tile_rows >= sps->height) {
> >>>>>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> >>>>>>>> -                   pps->num_tile_rows - 1);
> >>>>>>>> +                   pps->num_tile_rows - 1U);
> >>>>>>>
> >>>>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> >>>> [...]
> >>>>>>
> >>>>>> is this here ok if num_tile_rows is 0 ?
> >>>>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
> >>>>>>
> >>>>>> i would guess nearly everyone wold say yes without having seen the
> >>>>>> discussion about the type. but of course if this is unsigned its not
> >>>>>> going to be safe with it being 0. 
> >>>>>
> >>>>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
> >>>>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
> >>>>> it would be a bug. Int is definitely not the right type for it.
> >>>>
> >>>> i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
> >>>> makes much sense but sure i can change it to unsigned if preferred.
> >>>
> >>> Of course it doesn't. In pretty much any sample it will be at least 1
> >>> and at most 22, which is the max value allowed by hevc level 6.2 in
> >>> table A.6. Only if the stream reports an undefined level it could go up
> >>> to, if i'm reading this right, sps->ctb_height and not sps->height as
> >>> the decoder is currently checking. This means you can even replace
> >>> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.
> >>>
> >>> In any case, the bitstream value is unsigned, so the struct field should
> >>> be unsigned as well. Having it be signed and assigning it a value using
> >>> a function that potentially returns huge unsigned values introduced this
> >>> issue to being with, so instead of working around it using type
> >>> promotion, just make it follow the spec.
> >>
> >> what would be your feeling/oppinon about making the field uint16_t ?
> >> this would make it unsigned in the struct but avoid the problems with
> >> unsigned int ?
> > 
> > That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also
> > change it to use get_ue_golomb() while at it. And please do it for both
> > rows and columns. Can be in separate patches if you want the rows one to
> > explicitly address the fuzzing testcase.
> > 

> > Can you also confirm my suspicion that the checks should be comparing
> > the values with sps->ctb_height/weight and not with sps->height/weight?
> 
> Ok, so yes, it is. Should be changed in a separate patch as well.

will post it together with the other updated patch

thanks

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index 80df417e4f..0ed6682bb4 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -1596,7 +1596,7 @@  int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
         if (pps->num_tile_rows <= 0 ||
             pps->num_tile_rows >= sps->height) {
             av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
-                   pps->num_tile_rows - 1);
+                   pps->num_tile_rows - 1U);
             ret = AVERROR_INVALIDDATA;
             goto err;
         }