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

[Ronald S. Bultje]

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
(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.

Ronald