[FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns

[James Almer]

On 6/26/2019 9:41 AM, Michael Niedermayer wrote:
> On Tue, Jun 25, 2019 at 10:30:45AM -0300, James Almer wrote:
>> On 6/25/2019 5:55 AM, 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 at niedermayer.cc>
>>> ---
>>>  libavcodec/hevc_ps.c | 23 +++++++++++++----------
>>>  libavcodec/hevc_ps.h |  4 ++--
>>>  2 files changed, 15 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>> index 80df417e4f..07d220a5c8 100644
>>> --- a/libavcodec/hevc_ps.c
>>> +++ b/libavcodec/hevc_ps.c
>>> @@ -1584,22 +1584,25 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
>>>      pps->entropy_coding_sync_enabled_flag = get_bits1(gb);
>>>  
>>>      if (pps->tiles_enabled_flag) {
>>> -        pps->num_tile_columns = get_ue_golomb_long(gb) + 1;
>>> -        pps->num_tile_rows    = get_ue_golomb_long(gb) + 1;
>>> -        if (pps->num_tile_columns <= 0 ||
>>> -            pps->num_tile_columns >= sps->width) {
>>> +        int num_tile_columns_minus1 = get_ue_golomb(gb);
>>> +        int num_tile_rows_minus1    = get_ue_golomb(gb);
>>> +
>>> +        if (num_tile_columns_minus1 < 0 ||
>>
>> num_tile_columns_minus1 can never be < 0 for an int now that you're
>> using get_ue_golomb(gb). It returns values from 0 to 8190.
>> Add an av_assert0, at most. A value < 0 would mean there's a huge bug in
>> golomb.h, and not invalid bitstream data.
>>
>> And since you got rid of the "- 1" calculation below, which was your
>> original concern, you could also just make this unsigned. There's really
>> no need for an int at all.
> 
> If you look at get_ue_golomb() in golomb.h
> static inline int get_ue_golomb(GetBitContext *gb)
> ...
> there is a case that does:
>     return AVERROR_INVALIDDATA;
>     
> That is a negative value an should be checked for before
> truncating to uint8_t. There is no gurantee that error codes
> when cast to uint8_t would not map to valid values.

I had not seen that. The doxy for the function mentioned the valid range
it can return, but made no mention of the chance for an AVERROR* value.

int is ok and the patch should be good, then. Sorry for the noise.

Any opinion about the struct field size? Is uint8_t enough? Your
original suggestion of uint16_t may be a safer bet, as i mentioned.
Looking at CBS Mark limited these fields to the max allowed value level
6.2 allows for them (20 and 22), but we're not doing that here, so
num_tile_[cols,rows}_minus1 could in theory and in some extreme cases go
beyond 255.

> 
> also i belive that returning an error in cases that are
> outside 0 to 8190 is the correct thing to do, so the
> implementation which returns this appears to do the correct
> thing to me.
> 
> Thanks
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>