[FFmpeg-devel] [PATCH 2/5] avcodec/hevc/hevcdec: Do not allow changes to parameter sets between slices

[Michael Niedermayer]

On Tue, Jun 25, 2024 at 10:59:14AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-06-24 01:01:34)
> > The slice code detects changes by comparing the pps index.
> > That way the code cannot detect changes if the sets can change.
> > 
> > Fixes: out of array access
> > Fixes: 69585/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6389604524556288
> > Fixes: 69601/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5069068613255168
> > Fixes: 69621/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6187334182174720
> > 
> > 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/hevcdec.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/hevc/hevcdec.c b/libavcodec/hevc/hevcdec.c
> > index 1d2e53afc32..d68d454537a 100644
> > --- a/libavcodec/hevc/hevcdec.c
> > +++ b/libavcodec/hevc/hevcdec.c
> > @@ -3221,17 +3221,20 @@ static int decode_nal_unit(HEVCContext *s, const H2645NAL *nal)
> >          ret = ff_hevc_decode_nal_vps(&gb, s->avctx, &s->ps);
> >          if (ret < 0)
> >              goto fail;
> > +        s->sh.pps_id = -1;
> 
> This is backwards. If the problem is that the current check does not
> detect the change, then the check should be fixed.

The specification explcitly doesnt allow the active *PS to change

"Any PPS NAL unit containing the value of pps_pic_parameter_set_id for the active PPS RBSP for a coded picture (and
consequently for the layer containing the coded picture) shall have the same content as that of the active PPS RBSP for the
coded picture, unless it follows the last VCL NAL unit of the coded picture and precedes the first VCL NAL unit of another
coded picture."

"Any SPS NAL unit with nuh_layer_id equal to 0 containing the value of sps_seq_parameter_set_id for the active SPS
RBSP for the base layer for a CVS shall have the same content as that of the active SPS RBSP for the base layer for the
CVS, unless it follows the last access unit of the CVS and precedes the first VCL NAL unit and the first SEI NAL unit
containing an active parameter sets SEI message (when present) of another CVS."

"Any SPS NAL unit with any nuh_layer_id value containing the value of sps_seq_parameter_set_id for the active SPS
RBSP for a particular layer shall have the same content as that of the active SPS RBSP for the particular layer unless it
follows the access unit auA containing the last coded picture for which the active SPS RBSP for the particular layer is
required to be active for the particular layer and precedes the first NAL unit succeeding auA in decoding order that activates
an SPS RBSP with the same value of seq_parameter_set_id."

So if anything fancy is wanted, the way to go is not to parse it in the first place.
Because if it matches then the parsing was a waste of time. If it mismatches, that
would be invalid and would just lead to failure anyway

But really the primary goal is to fix the out of array access and be easy to
backport. I am not trying to fix this in a fancy way.
The simpler and more robust the fix is the better.

thx

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

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240626/2b474672/attachment.sig>