[FFmpeg-devel] [PATCH] libavcodec/vp9: fix ref-frame size judging method
mryancen at gmail.com
Mon Jun 17 06:43:26 EEST 2019
Hi, RonaldHave you see the reply of Haihao？
He mentioned that decode_frame_header() has use s->s.refs[s->s.h.refidx[i]].f without safe check.
For this suggestion, We must avoid the point on vp9.c 794-795.Could I restore the null point in decode_frame_header()？
-------- 原始信息 --------由： "Xiang, Haihao" <haihao.xiang at intel.com> 日期: 2019/5/27 12:53 (GMT+08:00) 收件人： rsbultje at gmail.com, ffmpeg-devel at ffmpeg.org 抄送： "Yan, CenX" <cenx.yan at intel.com> 主题： Re: [FFmpeg-devel] [PATCH] libavcodec/vp9: fix ref-frame size judging method
On Mon, 2019-05-27 at 04:44 +0000, Xiang, Haihao wrote:
> 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, in
> which some reference is missing, e.g. s->s.refs is NULL (but the rest is
Do you mean s->s.refs.f ? s->s.refs 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 assigned as per the "spec". Let's
> imagine that we encounter a frame in which s->s.refs is used as an active
> reference in this frame, for example s->s.h.refidx = 5. Right now, with the
> code in git/master, we abort decoding. Your patch will make it continue
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-
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, which means s->s.h.refidx[b->ref] =
> 5), and have prediction code that does something like vp9_mc_template.c line
> ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref]];
> 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
> Note how especially *none of these functions* check anywhere whether s-
> >s.refs[s->s.h.refidx[b->ref]] (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 becomes NULL, then send a subsequent frame
> using refs by having s->s.h.refidx = 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]]], 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
> HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode)
> if this is still unclear.
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
ffmpeg-devel mailing list
ffmpeg-devel at ffmpeg.org
To unsubscribe, visit link above, or email
ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel