On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote:
Hi guys,
On Wed, May 22, 2019 at 11:14 PM 严小复
<[email protected]<mailto:[email protected]>> 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
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
