Re: [PATCH 02/10] staging: media: cedrus: reject too large H.264/HEVC SPS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: cedrus: SPS size check in cedrus_check_pic_size() bypassed
--

commit 22d806de7dc3cc25db6d7871ebfc3e81dbda3078
Author: Jernej Skrabec <jernej.skrabec@gmail.com>

staging: media: cedrus: reject too large H.264/HEVC SPS

This commit validates H.264 and HEVC SPS picture dimensions in
cedrus_try_ctrl() against hardware limits and allocated capture buffer
dimensions to prevent memory overruns and hardware hangs.

> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index c3331b4f3f020..45deebddb630e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -28,10 +28,38 @@
>  #include "cedrus_dec.h"
>  #include "cedrus_hw.h"
>  
> +/*
> + * The hardware decodes whatever picture size the bitstream headers describe.
> + * Reject sizes beyond the hardware limits, or beyond the capture buffers once
> + * they are allocated, instead of letting the hardware overrun memory and hang.
> + */
> +static int cedrus_check_pic_size(struct cedrus_ctx *ctx, u32 width, u32 height)
> +{
> +	struct vb2_queue *vq;
> +
> +	if (width > CEDRUS_MAX_WIDTH || height > CEDRUS_MAX_HEIGHT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The hardware decodes whole 16x16 macroblocks, and all capture
> +	 * formats pad the buffers to at least 16 pixels in both directions.
> +	 */
> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +	if (vb2_is_busy(vq) &&
> +	    (width > ALIGN(ctx->src_fmt.width, 16) ||
> +	     height > ALIGN(ctx->src_fmt.height, 16)))
> +		return -EINVAL;

[Severity: High]
Can the check against ctx->src_fmt in cedrus_check_pic_size() be bypassed
when V4L2_CID_STATELESS_H264_SPS or V4L2_CID_STATELESS_HEVC_SPS is set
before capture buffers are allocated?

While vb2_is_busy(vq) is false on the V4L2_BUF_TYPE_VIDEO_CAPTURE queue,
cedrus_check_pic_size() only checks that width and height do not exceed
CEDRUS_MAX_WIDTH and CEDRUS_MAX_HEIGHT (4096x2304). Unlike the HEVC
bit_depth handling in cedrus_try_ctrl(), setting the SPS control while
!vb2_is_busy(vq) does not update ctx->src_fmt or ctx->dst_fmt. In addition,
cedrus_s_fmt_vid_out() in drivers/staging/media/sunxi/cedrus/cedrus_video.c
allows userspace to shrink ctx->src_fmt and ctx->dst_fmt (down to 16x16)
at any time before capture buffers are allocated:

drivers/staging/media/sunxi/cedrus/cedrus_video.c:cedrus_s_fmt_vid_out() {
	...
	peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
				  V4L2_BUF_TYPE_VIDEO_CAPTURE);
	if (vb2_is_busy(peer_vq))
		return -EBUSY;

	return cedrus_s_fmt_vid_out_p(cedrus_file2ctx(file), &f->fmt.pix);
}

Also, if userspace never sets V4L2_CID_STATELESS_H264_SPS, the default
zero-initialized control has V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY unset,
giving a 16x32 picture height while cedrus_open() initializes ctx->src_fmt
and ctx->dst_fmt to 16x16.

When capture buffers are subsequently allocated, cedrus_queue_setup() sizes
the capture buffers from ctx->dst_fmt, and cedrus_h264_start(),
cedrus_write_frame_list(), and cedrus_h265_setup() size the auxiliary DMA
buffers (pic_info_buf, deblk_buf, intra_pred_buf, and mv_col_buf) from
ctx->src_fmt. Neither cedrus_start_streaming() nor the codec setup
callbacks re-validate the active SPS against ctx->src_fmt.

If the larger SPS was set on ctx->hdl before VIDIOC_REQBUFS(CAPTURE) and
subsequent requests do not attach a new SPS control, cedrus_try_ctrl() is
not called again after vb2_is_busy(vq) becomes true. Even if the oversized
SPS was attached to a media request before VIDIOC_REQBUFS(CAPTURE),
cedrus_device_run() in drivers/staging/media/sunxi/cedrus/cedrus_dec.c
ignores the error from v4l2_ctrl_request_setup():

drivers/staging/media/sunxi/cedrus/cedrus_dec.c:cedrus_device_run() {
	...
	if (src_req)
		v4l2_ctrl_request_setup(src_req, &ctx->hdl);
	...
	ctx->current_codec->setup(ctx, &run);
	...
	ctx->current_codec->trigger(ctx);
}

Wouldn't this still allow the hardware to decode using an SPS larger than
the allocated capture and auxiliary DMA buffers, overrunning memory and
hanging the VPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102170303.1973892-1-jernej.skrabec@gmail.com?part=2