diff mbox series

[FFmpeg-devel,1/2] avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps

Message ID 20201114101815.24489-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Nov. 14, 2020, 10:18 a.m. UTC
Fixes: index 26 out of bounds for type 'uint8_t [16]'
Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880

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

Comments

Mark Thompson Nov. 15, 2020, 9:37 p.m. UTC | #1
On 14/11/2020 10:18, Michael Niedermayer wrote:
> Fixes: index 26 out of bounds for type 'uint8_t [16]'
> Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavcodec/cbs_h265_syntax_template.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> index 48fae82d04..8eb6e159f4 100644
> --- a/libavcodec/cbs_h265_syntax_template.c
> +++ b/libavcodec/cbs_h265_syntax_template.c
> @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>                       infer(num_long_term_sps, 0);
>                       idx_size = 0;
>                   }
> +                if (HEVC_MAX_REFS < current->num_long_term_sps)
> +                    return AVERROR_INVALIDDATA;

Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.

>                   ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
>   
>                   for (i = 0; i < current->num_long_term_sps +
> 

It would be good if the commit message could include an explanation derived from the standard, too.

As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.

- Mark
Michael Niedermayer Nov. 21, 2020, 5:37 p.m. UTC | #2
On Sun, Nov 15, 2020 at 09:37:45PM +0000, Mark Thompson wrote:
> On 14/11/2020 10:18, Michael Niedermayer wrote:
> > Fixes: index 26 out of bounds for type 'uint8_t [16]'
> > Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavcodec/cbs_h265_syntax_template.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> > index 48fae82d04..8eb6e159f4 100644
> > --- a/libavcodec/cbs_h265_syntax_template.c
> > +++ b/libavcodec/cbs_h265_syntax_template.c
> > @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
> >                       infer(num_long_term_sps, 0);
> >                       idx_size = 0;
> >                   }
> > +                if (HEVC_MAX_REFS < current->num_long_term_sps)
> > +                    return AVERROR_INVALIDDATA;
> 
> Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.
> 
> >                   ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
> >                   for (i = 0; i < current->num_long_term_sps +
> > 
> 
> It would be good if the commit message could include an explanation derived from the standard, too.
> 
> As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.

Is the change below what you had in mind ?

commit 72c6c46bb2b31b2822331aff461acccd0a4f9159 (HEAD -> master)
Author: Michael Niedermayer <michael@niedermayer.cc>
Date:   Fri Nov 13 23:15:52 2020 +0100

    avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
    
    As far as we can tell the constraint doesn't appear explicitly, but the SPS is allowed to
    define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
    
    Fixes: index 26 out of bounds for type 'uint8_t [16]'
    Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
    
    Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
    Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
index 48fae82d04..30f2bd328c 100644
--- a/libavcodec/cbs_h265_syntax_template.c
+++ b/libavcodec/cbs_h265_syntax_template.c
@@ -1399,7 +1399,7 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
                 unsigned int idx_size;
 
                 if (sps->num_long_term_ref_pics_sps > 0) {
-                    ue(num_long_term_sps, 0, sps->num_long_term_ref_pics_sps);
+                    ue(num_long_term_sps, 0, FFMIN(sps->num_long_term_ref_pics_sps, HEVC_MAX_REFS));
                     idx_size = av_log2(sps->num_long_term_ref_pics_sps - 1) + 1;
                 } else {
                     infer(num_long_term_sps, 0);

[...]
Mark Thompson Nov. 21, 2020, 6:49 p.m. UTC | #3
On 21/11/2020 17:37, Michael Niedermayer wrote:
> On Sun, Nov 15, 2020 at 09:37:45PM +0000, Mark Thompson wrote:
>> On 14/11/2020 10:18, Michael Niedermayer wrote:
>>> Fixes: index 26 out of bounds for type 'uint8_t [16]'
>>> Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavcodec/cbs_h265_syntax_template.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
>>> index 48fae82d04..8eb6e159f4 100644
>>> --- a/libavcodec/cbs_h265_syntax_template.c
>>> +++ b/libavcodec/cbs_h265_syntax_template.c
>>> @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>>                        infer(num_long_term_sps, 0);
>>>                        idx_size = 0;
>>>                    }
>>> +                if (HEVC_MAX_REFS < current->num_long_term_sps)
>>> +                    return AVERROR_INVALIDDATA;
>>
>> Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.
>>
>>>                    ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
>>>                    for (i = 0; i < current->num_long_term_sps +
>>>
>>
>> It would be good if the commit message could include an explanation derived from the standard, too.
>>
>> As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
> 
> Is the change below what you had in mind ?
> 
> commit 72c6c46bb2b31b2822331aff461acccd0a4f9159 (HEAD -> master)
> Author: Michael Niedermayer <michael@niedermayer.cc>
> Date:   Fri Nov 13 23:15:52 2020 +0100
> 
>      avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
>      
>      As far as we can tell the constraint doesn't appear explicitly, but the SPS is allowed to
>      define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.

I meant write a commit message which explains where in the standard the constraint is coming from.  I wrote that because I didn't see any extra constraint written in the standard for num_long_term_sps but num_long_term_ref_pics_sps is indeed bigger, so presumably it must be implied by something else.

>      
>      Fixes: index 26 out of bounds for type 'uint8_t [16]'
>      Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
>      
>      Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>      Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> 
> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> index 48fae82d04..30f2bd328c 100644
> --- a/libavcodec/cbs_h265_syntax_template.c
> +++ b/libavcodec/cbs_h265_syntax_template.c
> @@ -1399,7 +1399,7 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>                   unsigned int idx_size;
>   
>                   if (sps->num_long_term_ref_pics_sps > 0) {
> -                    ue(num_long_term_sps, 0, sps->num_long_term_ref_pics_sps);
> +                    ue(num_long_term_sps, 0, FFMIN(sps->num_long_term_ref_pics_sps, HEVC_MAX_REFS));

Yes to this part.

>                       idx_size = av_log2(sps->num_long_term_ref_pics_sps - 1) + 1;
>                   } else {
>                       infer(num_long_term_sps, 0);
> 
> [...]

- Mark
Michael Niedermayer Jan. 24, 2021, 9:41 p.m. UTC | #4
On Sat, Nov 21, 2020 at 06:49:55PM +0000, Mark Thompson wrote:
> On 21/11/2020 17:37, Michael Niedermayer wrote:
> > On Sun, Nov 15, 2020 at 09:37:45PM +0000, Mark Thompson wrote:
> > > On 14/11/2020 10:18, Michael Niedermayer wrote:
> > > > Fixes: index 26 out of bounds for type 'uint8_t [16]'
> > > > Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >    libavcodec/cbs_h265_syntax_template.c | 2 ++
> > > >    1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> > > > index 48fae82d04..8eb6e159f4 100644
> > > > --- a/libavcodec/cbs_h265_syntax_template.c
> > > > +++ b/libavcodec/cbs_h265_syntax_template.c
> > > > @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
> > > >                        infer(num_long_term_sps, 0);
> > > >                        idx_size = 0;
> > > >                    }
> > > > +                if (HEVC_MAX_REFS < current->num_long_term_sps)
> > > > +                    return AVERROR_INVALIDDATA;
> > > 
> > > Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.
> > > 
> > > >                    ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
> > > >                    for (i = 0; i < current->num_long_term_sps +
> > > > 
> > > 
> > > It would be good if the commit message could include an explanation derived from the standard, too.
> > > 
> > > As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
> > 
> > Is the change below what you had in mind ?
> > 
> > commit 72c6c46bb2b31b2822331aff461acccd0a4f9159 (HEAD -> master)
> > Author: Michael Niedermayer <michael@niedermayer.cc>
> > Date:   Fri Nov 13 23:15:52 2020 +0100
> > 
> >      avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
> >      As far as we can tell the constraint doesn't appear explicitly, but the SPS is allowed to
> >      define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
> 
> I meant write a commit message which explains where in the standard the constraint is coming from.  I wrote that because I didn't see any extra constraint written in the standard for num_long_term_sps but num_long_term_ref_pics_sps is indeed bigger, so presumably it must be implied by something else.

what do you suggest if noone finds that "something else" ?

thx

[...]
Mark Thompson Jan. 25, 2021, 8:36 p.m. UTC | #5
On 24/01/2021 21:41, Michael Niedermayer wrote:
> On Sat, Nov 21, 2020 at 06:49:55PM +0000, Mark Thompson wrote:
>> On 21/11/2020 17:37, Michael Niedermayer wrote:
>>> On Sun, Nov 15, 2020 at 09:37:45PM +0000, Mark Thompson wrote:
>>>> On 14/11/2020 10:18, Michael Niedermayer wrote:
>>>>> Fixes: index 26 out of bounds for type 'uint8_t [16]'
>>>>> Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>     libavcodec/cbs_h265_syntax_template.c | 2 ++
>>>>>     1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
>>>>> index 48fae82d04..8eb6e159f4 100644
>>>>> --- a/libavcodec/cbs_h265_syntax_template.c
>>>>> +++ b/libavcodec/cbs_h265_syntax_template.c
>>>>> @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>>                         infer(num_long_term_sps, 0);
>>>>>                         idx_size = 0;
>>>>>                     }
>>>>> +                if (HEVC_MAX_REFS < current->num_long_term_sps)
>>>>> +                    return AVERROR_INVALIDDATA;
>>>>
>>>> Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.
>>>>
>>>>>                     ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
>>>>>                     for (i = 0; i < current->num_long_term_sps +
>>>>>
>>>>
>>>> It would be good if the commit message could include an explanation derived from the standard, too.
>>>>
>>>> As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
>>>
>>> Is the change below what you had in mind ?
>>>
>>> commit 72c6c46bb2b31b2822331aff461acccd0a4f9159 (HEAD -> master)
>>> Author: Michael Niedermayer <michael@niedermayer.cc>
>>> Date:   Fri Nov 13 23:15:52 2020 +0100
>>>
>>>       avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
>>>       As far as we can tell the constraint doesn't appear explicitly, but the SPS is allowed to
>>>       define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
>>
>> I meant write a commit message which explains where in the standard the constraint is coming from.  I wrote that because I didn't see any extra constraint written in the standard for num_long_term_sps but num_long_term_ref_pics_sps is indeed bigger, so presumably it must be implied by something else.
> 
> what do you suggest if noone finds that "something else" ?

If you have given up on this then I would try to work out what is going on here myself?

- Mark
Michael Niedermayer Jan. 26, 2021, 11:09 p.m. UTC | #6
On Mon, Jan 25, 2021 at 08:36:59PM +0000, Mark Thompson wrote:
> On 24/01/2021 21:41, Michael Niedermayer wrote:
> > On Sat, Nov 21, 2020 at 06:49:55PM +0000, Mark Thompson wrote:
> > > On 21/11/2020 17:37, Michael Niedermayer wrote:
> > > > On Sun, Nov 15, 2020 at 09:37:45PM +0000, Mark Thompson wrote:
> > > > > On 14/11/2020 10:18, Michael Niedermayer wrote:
> > > > > > Fixes: index 26 out of bounds for type 'uint8_t [16]'
> > > > > > Fixes: 24913/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-6261760693370880
> > > > > > 
> > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > >     libavcodec/cbs_h265_syntax_template.c | 2 ++
> > > > > >     1 file changed, 2 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
> > > > > > index 48fae82d04..8eb6e159f4 100644
> > > > > > --- a/libavcodec/cbs_h265_syntax_template.c
> > > > > > +++ b/libavcodec/cbs_h265_syntax_template.c
> > > > > > @@ -1405,6 +1405,8 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
> > > > > >                         infer(num_long_term_sps, 0);
> > > > > >                         idx_size = 0;
> > > > > >                     }
> > > > > > +                if (HEVC_MAX_REFS < current->num_long_term_sps)
> > > > > > +                    return AVERROR_INVALIDDATA;
> > > > > 
> > > > > Please don't put isolated tests in the middle of the template.  If it constrains a value then add it to the constraints on that value.
> > > > > 
> > > > > >                     ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
> > > > > >                     for (i = 0; i < current->num_long_term_sps +
> > > > > > 
> > > > > 
> > > > > It would be good if the commit message could include an explanation derived from the standard, too.
> > > > > 
> > > > > As far as I can tell the constraint doesn't appear explicitly, but the SPS is allowed to define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
> > > > 
> > > > Is the change below what you had in mind ?
> > > > 
> > > > commit 72c6c46bb2b31b2822331aff461acccd0a4f9159 (HEAD -> master)
> > > > Author: Michael Niedermayer <michael@niedermayer.cc>
> > > > Date:   Fri Nov 13 23:15:52 2020 +0100
> > > > 
> > > >       avcodec/cbs_h265_syntax_template: Better check for num_long_term_sps
> > > >       As far as we can tell the constraint doesn't appear explicitly, but the SPS is allowed to
> > > >       define more possible long term frames than are actually allowed to be present at any given moment so we need the tighter bound.
> > > 
> > > I meant write a commit message which explains where in the standard the constraint is coming from.  I wrote that because I didn't see any extra constraint written in the standard for num_long_term_sps but num_long_term_ref_pics_sps is indeed bigger, so presumably it must be implied by something else.
> > 
> > what do you suggest if noone finds that "something else" ?
> 
> If you have given up on this then I would try to work out what is going on here myself?

"given up" is inprecisse, its more a mix between hoping that someone who
knows hevc very well would just look at this and know and that without knowing its
potentially quite a bit of work and the fix seemed important but the
argumentation why exactly we arent allowed to access outside the array
and why its fixed one way and not another seemed more or less detail.
Thats in light of having other lower hanging fruits to work on ...
So yes if you want to investigate, you can go right ahead 

Thanks

[...]
diff mbox series

Patch

diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
index 48fae82d04..8eb6e159f4 100644
--- a/libavcodec/cbs_h265_syntax_template.c
+++ b/libavcodec/cbs_h265_syntax_template.c
@@ -1405,6 +1405,8 @@  static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
                     infer(num_long_term_sps, 0);
                     idx_size = 0;
                 }
+                if (HEVC_MAX_REFS < current->num_long_term_sps)
+                    return AVERROR_INVALIDDATA;
                 ue(num_long_term_pics, 0, HEVC_MAX_REFS - current->num_long_term_sps);
 
                 for (i = 0; i < current->num_long_term_sps +