[FFmpeg-devel] [PATCH 3/3] vp9: recon: Use emulated edge to prevent buffer overflows

[Michael Niedermayer]

Hi

On Sat, Dec 21, 2024 at 12:25:02PM -0500, Ronald S. Bultje wrote:
> Hi Janne,
> 
> On Thu, Dec 19, 2024 at 4:12 PM Janne Grunau <janne-ffmpeg at jannau.net>
> wrote:
> 
> > The arm/aarch64 horizontal filter reads one additional pixel beyond what
> > the filter uses. This can become an issue if the application does not
> > allocate larger buffers than what's required for the pixel data. If the
> > motion vector points to the bottom right edge of the picture this
> > becomes a read buffer overflow. This triggers segfaults in Firefox for
> > video resolutions which result in a page aligned picture size like
> > 1280x640.
> > Prevent this by using emulated edge in this case.
> >
> > Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1881185
> > Signed-off-by: Janne Grunau <janne-ffmpeg at jannau.net>
> > ---
> >  libavcodec/vp9recon.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/libavcodec/vp9recon.c b/libavcodec/vp9recon.c
> > index ef08ed17c8e5..1f1b99a03c8a 100644
> > --- a/libavcodec/vp9recon.c
> > +++ b/libavcodec/vp9recon.c
> > @@ -319,7 +319,11 @@ static av_always_inline void
> > mc_luma_unscaled(VP9TileData *td, const vp9_mc_func
> >      // The arm/aarch64 _hv filters read one more row than what actually is
> >      // needed, so switch to emulated edge one pixel sooner vertically
> >      // (!!my * 5) than horizontally (!!mx * 4).
> > +    // The arm/aarch64 _h filters read one more pixel than what actually
> > is
> > +    // needed, so switch to emulated edge if that would read beyond the
> > bottom
> > +    // right block.
> >      if (x < !!mx * 3 || y < !!my * 3 ||
> > +        ((x + !!mx * 5 > w - bw) && (y + !!my * 5 + 1 > h - bh)) ||
> >          x + !!mx * 4 > w - bw || y + !!my * 5 > h - bh) {
> >          s->vdsp.emulated_edge_mc(td->edge_emu_buffer,
> >                                   ref - !!my * 3 * ref_stride - !!mx * 3 *
> > bytesperpixel,
> >
> 
> Short-term, this is probably a good idea. No objection from me under #if
> ARCH_.
> 
> Long-term, do we want to update the docs for get_buffer()? I realize this
> is an ABI change and probably needs a new function name or at least a major

> bump. But my impression was always that the over-allocation was intended

I can confirm, the overalloc was intended, but this is from the very distant
past. I do not know how this interacts with modern users of the function.
Bascially if everyone _can_ overallocate.


> (same as for input buffers) so SIMD could overread by a few bytes from
> refs... If it is intentional that we don't over-read (which is fine with me
> also), we probably want to fix the SIMD, that seems like it would be more
> efficient and simpler.

[...]

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/20241222/f8778078/attachment.sig>