[FFmpeg-devel] [PATCH] libavcodec/vp9: fix ref-frame size judging method

[Xiang, Haihao]

Please ignore this email because my comment was mixed with yours.

Thanks
Haihao

> On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote:
> Hi guys,
> 
> On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen at gmail.com<mailto:
> mryancen at gmail.com>> wrote:
> code”, I'm little confused about the red word,would you mean encode process
> need validity checks or there need to check the reference id of each frame?
> 
> And this patch will act on decode process, all references have already been
> appointed by the stream.
> 
> No. As said before, the *decode* process needs such checks.
> 
> But since you don't seem to understand, let me try to be more helpful.
> 
> If you have an array of references, and we refer to that as s->s.refs[8], in
> which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is
> fine).
> 
> Do you mean s->s.refs[5].f ? s->s.refs[5] is not a pointer.
> 
> Now let's also say that we have a header in s->s.h in which there is an array
> of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's
> imagine that we encounter a frame in which s->s.refs[5] is used as an active
> reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the
> code in git/master, we abort decoding. Your patch will make it continue
> decoding.
> 
> If so, even without Cen's patch, there is still a similar issue because the
> reference is used without any check in decode_frame_header () in vp9.c line
> 794-795
> 
>     AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f;
>    int refw = ref->width, refh = ref->height;
> 
> So I presumed s->s.refs[s->s.h.refidx[i]].f is assigned somewhere, otherwise
> we must add a check here.
> 
> 
> 
> So now, imagine that we encounter, in this frame, an inter block in which we
> use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] =
> 5), and have prediction code that does something like vp9_mc_template.c line
> 39:
> 
>     ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]];
> 
> Then later on this code will call a function which may in some cases be called
> mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in
> vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on
> line 383 of same file). This function now calls a DSP function in line 331:
> 
> mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1);
> 
> which directly accesses the reference pixels (see e.g. vp9dsp_template.c line
> 2044).
> 
> Note how especially *none of these functions* check anywhere whether s-
> >s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that,
> because ... the header check already did that, so the check was redundant.
> 
> But! You are *removing* that header check, so in this brave new world which
> will exist after applying your patch, what will happen is that I can craft a
> special stream where s->s.refs[5] becomes NULL, then send a subsequent frame
> using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the
> block uses inter coding and uses reference 0 so that this causes access into
> s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is
> undefined behaviour by the C standard, which means a bad person could craft
> such a stream that would allow this bad person to break into your computer and
> steal all your data. In more straightforward cases, it might also crash
> Firefox and VLC, which use the FFmpeg VP9 decoder.
> 
> Note also that having your data stolen or your application crashing is
> considered *bad*. We *don't want that*. Therefore, as I've tried to say a few
> times already, if you remove the header check, you need to add a per-block
> check instead, so that Firefox/VLC don't crash and so that bad persons cannot
> steal your data.
> 
> Please add such a check and test using a fuzzer that the check prevents
> crashes as described above. Similar checks may be needed in other places also,
> since there's multiple places where the software decoder does MC and accesses
> references. Good luck finding these places by reading the code and fuzzing
> away.
> 
> HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode)
> if this is still unclear.
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".