[FFmpeg-devel] [PATCH] lavc/vvc: Validate alf_list indexes

[Nuo Mi]

On Mon, Feb 5, 2024 at 10:04 PM Frank Plowman <post at frankplowman.com> wrote:

> On 05/02/2024 13:45, Nuo Mi wrote:
> > On Sun, Feb 4, 2024 at 1:22 AM <post at frankplowman.com> wrote:
> >
> >> From: Frank Plowman <post at frankplowman.com>
> >>
> >> Fixes crashes when decoding illegal bitstreams found by fuzzing.
> >>
> >> Signed-off-by: Frank Plowman <post at frankplowman.com>
> >> ---
> >>   libavcodec/vvc/vvc_ctu.c | 17 +++++++++++++++--
> >>   1 file changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/vvc/vvc_ctu.c b/libavcodec/vvc/vvc_ctu.c
> >> index d166b16a19..f601c6c95e 100644
> >> --- a/libavcodec/vvc/vvc_ctu.c
> >> +++ b/libavcodec/vvc/vvc_ctu.c
> >> @@ -2207,7 +2207,7 @@ static void hls_sao(VVCLocalContext *lc, const int
> >> rx, const int ry)
> >>       }
> >>   }
> >>
> >> -static void alf_params(VVCLocalContext *lc, const int rx, const int ry)
> >> +static int alf_params(VVCLocalContext *lc, const int rx, const int ry)
> >>   {
> >>       const VVCFrameContext *fc     = lc->fc;
> >>       const H266RawSliceHeader *sh  = lc->sc->sh.r;
> >> @@ -2233,6 +2233,11 @@ static void alf_params(VVCLocalContext *lc, const
> >> int rx, const int ry)
> >>                   c_idx == CB ? sh->sh_alf_cb_enabled_flag :
> >> sh->sh_alf_cr_enabled_flag;
> >>               if (alf_enabled_flag) {
> >>                   const VVCALF *aps =
> >> fc->ps.alf_list[sh->sh_alf_aps_id_chroma];
> >> +                if (!aps) {
> >> +                    // Slice header refers to an APS which does not
> exist
> >> +                    return AVERROR_INVALIDDATA;
> >> +                }
> >> +
> >>
> > Thank you Frank.
> > Since all "if conditions" are related to slices, could we perform the
> check
> > at sh_derive()?
>
> Yes, I had another look at this yesterday and came to the same
> conclusion.  Do you think it's better to derive pointers to the relevant
> APSs or just to validate the indices into fps->aps_list?
>
> Check the fps->aps_list[i] should be enough.
Make sure you run all the conformance tests, only conformance tests know
what change is right :)