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

[Michael Niedermayer]

On Wed, Jun 26, 2019 at 10:29:05AM -0300, James Almer wrote:
> 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.

ok, will apply with uint16_t

Thanks


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190630/91bc0440/attachment.sig>