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

[Michael Niedermayer]

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 at 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     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20190625/a4849b3b/attachment.sig>