diff mbox series

[FFmpeg-devel,2/4] avcodec/av1dec: Check tile_group_info size before use

Message ID 20200924202039.30285-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/4] avcodec/svq3: dont crash on free_picture(NULL)
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Sept. 24, 2020, 8:20 p.m. UTC
Fixes: member access within null pointer of type 'TileGroupInfo' (aka 'struct TileGroupInfo')
Fixes: 25725/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-5166692706287616

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

Comments

James Almer Sept. 24, 2020, 8:51 p.m. UTC | #1
On 9/24/2020 5:20 PM, Michael Niedermayer wrote:
> Fixes: member access within null pointer of type 'TileGroupInfo' (aka 'struct TileGroupInfo')
> Fixes: 25725/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-5166692706287616
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/av1dec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index 0bb04a3e44..cf3a78aad8 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -209,6 +209,9 @@ static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_gro
>      uint16_t tile_num, tile_row, tile_col;
>      uint32_t size = 0, size_bytes = 0;
>  
> +    if (s->tile_num != s->raw_frame_header->tile_cols * s->raw_frame_header->tile_rows)
> +        return AVERROR_INVALIDDATA;

This shouldn't happen if a frame header was properly parsed. It sounds
like one wasn't yet s->raw_frame_header was left pointing to it.

Does the following also fix this crash?

> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index 0bb04a3e44..e650d43d9e 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -831,6 +831,9 @@ static int av1_decode_frame(AVCodecContext *avctx, void *frame,
> 
>  end:
>      ff_cbs_fragment_reset(&s->current_obu);
> +    if (ret < 0)
> +        s->raw_frame_header = NULL;
> +
>      return ret;
>  }
>
Michael Niedermayer Sept. 25, 2020, 8:05 a.m. UTC | #2
On Thu, Sep 24, 2020 at 05:51:43PM -0300, James Almer wrote:
> On 9/24/2020 5:20 PM, Michael Niedermayer wrote:
> > Fixes: member access within null pointer of type 'TileGroupInfo' (aka 'struct TileGroupInfo')
> > Fixes: 25725/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-5166692706287616
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/av1dec.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> > index 0bb04a3e44..cf3a78aad8 100644
> > --- a/libavcodec/av1dec.c
> > +++ b/libavcodec/av1dec.c
> > @@ -209,6 +209,9 @@ static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_gro
> >      uint16_t tile_num, tile_row, tile_col;
> >      uint32_t size = 0, size_bytes = 0;
> >  
> > +    if (s->tile_num != s->raw_frame_header->tile_cols * s->raw_frame_header->tile_rows)
> > +        return AVERROR_INVALIDDATA;
> 
> This shouldn't happen if a frame header was properly parsed. It sounds
> like one wasn't yet s->raw_frame_header was left pointing to it.
> 

> Does the following also fix this crash?

I was considering to clear raw_frame_header but went for the smaller change
as it wasnt clear to me if that would have unintended side effects / and
wondered why it was not cleared on any error path ...
But yes if clearing it is ok that is better
and i confirm the suggested change works 

thx

[...]
James Almer Sept. 25, 2020, 1:05 p.m. UTC | #3
On 9/25/2020 5:05 AM, Michael Niedermayer wrote:
> On Thu, Sep 24, 2020 at 05:51:43PM -0300, James Almer wrote:
>> On 9/24/2020 5:20 PM, Michael Niedermayer wrote:
>>> Fixes: member access within null pointer of type 'TileGroupInfo' (aka 'struct TileGroupInfo')
>>> Fixes: 25725/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-5166692706287616
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/av1dec.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>>> index 0bb04a3e44..cf3a78aad8 100644
>>> --- a/libavcodec/av1dec.c
>>> +++ b/libavcodec/av1dec.c
>>> @@ -209,6 +209,9 @@ static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_gro
>>>      uint16_t tile_num, tile_row, tile_col;
>>>      uint32_t size = 0, size_bytes = 0;
>>>  
>>> +    if (s->tile_num != s->raw_frame_header->tile_cols * s->raw_frame_header->tile_rows)
>>> +        return AVERROR_INVALIDDATA;
>>
>> This shouldn't happen if a frame header was properly parsed. It sounds
>> like one wasn't yet s->raw_frame_header was left pointing to it.
>>
> 
>> Does the following also fix this crash?
> 
> I was considering to clear raw_frame_header but went for the smaller change
> as it wasnt clear to me if that would have unintended side effects / and
> wondered why it was not cleared on any error path ...
> But yes if clearing it is ok that is better
> and i confirm the suggested change works 

Yes, it's an oversight. Will send a patchset to make some more thorough
cleaning on error, including this fix.
diff mbox series

Patch

diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index 0bb04a3e44..cf3a78aad8 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -209,6 +209,9 @@  static int get_tiles_info(AVCodecContext *avctx, const AV1RawTileGroup *tile_gro
     uint16_t tile_num, tile_row, tile_col;
     uint32_t size = 0, size_bytes = 0;
 
+    if (s->tile_num != s->raw_frame_header->tile_cols * s->raw_frame_header->tile_rows)
+        return AVERROR_INVALIDDATA;
+
     bytestream2_init(&gb, tile_group->tile_data.data,
                      tile_group->tile_data.data_size);
     s->tg_start = tile_group->tg_start;