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

[Ronald S. Bultje]

Hi guys,

On Wed, May 22, 2019 at 11:14 PM 严小复 <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). 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.

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