Re: [PATCH v7 17/28] media: rockchip: rga: check scaling factor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unprotected concurrent accesses and TOCTOU races between `VIDIOC_S_CTRL`, `VIDIOC_S_SELECTION`, and `VIDIOC_STREAMON` allow bypass of scaling limits and cause struct tearing.

Pre-existing issues:
- [High] Unsynchronized modification of `f->crop` and `ctx->cmdbuf_dirty` in `vidioc_s_selection` leads to potential data races and struct tearing when read by `device_run`.
- [High] Integer overflow in `vidioc_s_selection` bounds check allows out-of-bounds crop values.
--

commit 51e5c59f315703fdbb1a5279ab9158d07280eca5
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: check scaling factor

Check the scaling factor to avoid potential problems. This is relevant
for the upcoming RGA3 support, as it can hang when the scaling factor
is exceeded.

> diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
> index ffc6162b2e681..dcaba66f5c1fc 100644
> --- a/drivers/media/platform/rockchip/rga/rga-buf.c
> +++ b/drivers/media/platform/rockchip/rga/rga-buf.c
[ ... ]
> @@ -197,6 +197,33 @@ static void rga_buf_return_buffers(struct vb2_queue *q,
>  	}
>  }
>  
> +static int rga_buf_prepare_streaming(struct vb2_queue *q)
> +{
> +	struct rga_ctx *ctx = vb2_get_drv_priv(q);
> +	const struct rga_hw *hw = ctx->rga->hw;
> +	int ret;
> +
> +	/* It's safe to check the streaming state of the other queue,
> +	 * as the streamon ioctl's can't race due to the lock set in
> +	 * the queue_init function.
> +	 */
> +	if ((V4L2_TYPE_IS_OUTPUT(q->type) &&
> +	     vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx))) ||
> +	    (V4L2_TYPE_IS_CAPTURE(q->type) &&
> +	     vb2_is_streaming(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx)))) {
> +		/*
> +		 * As the other side is already streaming,
> +		 * check that the max scaling factor isn't exceeded.
> +		 */
> +		ret = rga_check_scaling(hw, &ctx->in.crop, &ctx->out.crop,
> +					ctx->rotate);

[Severity: High]
Does this code introduce a TOCTOU race with VIDIOC_S_CTRL?

rga_buf_prepare_streaming() checks the scaling factor just before the V4L2
core sets q->streaming = 1. If VIDIOC_S_CTRL executes in this window, its
vb2_is_streaming() check evaluates to false, bypassing the limit check and
applying an invalid rotation just as streaming starts.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
[ ... ]
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 394b14b9469df..22954bbae55fc 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -127,7 +127,9 @@ static int rga_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct rga_ctx *ctx = container_of(ctrl->handler, struct rga_ctx,
>  					   ctrl_handler);
> +	const struct rga_hw *hw = ctx->rga->hw;
>  	unsigned long flags;
> +	int ret = 0;
>  
>  	spin_lock_irqsave(&ctx->rga->ctrl_lock, flags);
>  	switch (ctrl->id) {
> @@ -138,6 +140,13 @@ static int rga_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ctx->vflip = ctrl->val;
>  		break;
>  	case V4L2_CID_ROTATE:
> +		if (vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx)) &&
> +		    vb2_is_streaming(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx))) {
> +			ret = rga_check_scaling(hw, &ctx->in.crop,
> +						&ctx->out.crop, ctrl->val);

[Severity: High]
Are there concurrent access issues and TOCTOU races between rga_s_ctrl() and
vidioc_s_selection() here?

This check relies on crop_in and crop_out, but rga_s_ctrl() uses the control
handler lock and ctrl_lock, while vidioc_s_selection() uses rga->mutex.
rga_s_ctrl() reads ctx->in.crop and ctx->out.crop here while holding
ctrl_lock, but vidioc_s_selection() modifies them concurrently without
taking ctrl_lock, which could lead to struct tearing.

Additionally, concurrent calls to VIDIOC_S_CTRL and VIDIOC_S_SELECTION could
validate their new values against the other's stale values, independently
passing the checks but resulting in an invalid combined state.

> +			if (ret < 0)
> +				goto s_ctrl_done;
> +		}
>  		ctx->rotate = ctrl->val;
>  		break;
>  	case V4L2_CID_BG_COLOR:
[ ... ]
> @@ -569,10 +611,25 @@ static int vidioc_s_selection(struct file *file, void *priv,
>  		return -EINVAL;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does the bounds check just before this
code allow integer overflows?

If an attacker provides a large s->r.left (e.g., 0x7FFFFFFF) and a huge
s->r.width (e.g., 0x80000001), their 32-bit unsigned addition wraps around
to 0 in the check:

	if (s->r.left + s->r.width > f->pix.width ||
	    s->r.top + s->r.height > f->pix.height ||

Since 0 is not greater than f->pix.width, the check passes. The massive width
value is then stored and programmed into the RGA hardware registers, bypassing
dimension constraints and leading to out-of-bounds DMA access.

> +	if (vb2_is_streaming(v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx)) &&
> +	    vb2_is_streaming(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx))) {
> +		int ret = 0;
> +
> +		if (V4L2_TYPE_IS_OUTPUT(s->type))
> +			ret = rga_check_scaling(rga->hw, &s->r, &ctx->out.crop,
> +						ctx->rotate);
> +		else
> +			ret = rga_check_scaling(rga->hw, &ctx->in.crop, &s->r,
> +						ctx->rotate);
> +
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	f->crop = s->r;
>  	ctx->cmdbuf_dirty = true;

[Severity: High]
This is a pre-existing issue, but can the unsynchronized modification of
f->crop and ctx->cmdbuf_dirty here lead to struct tearing when read by
device_run()?

vidioc_s_selection() modifies f->crop (a 16-byte v4l2_rect struct) and sets
ctx->cmdbuf_dirty = true holding only rga->mutex. Concurrently, device_run()
(running asynchronously in a worker thread) takes ctrl_lock, checks
cmdbuf_dirty, and reads the crop dimensions via setup_cmdbuf().

This lack of synchronization allows setup_cmdbuf() to read partially updated
bounds or process a dirty state before the crop updates are fully visible,
potentially programming the hardware with arbitrary dimensions.

>  
> -	return ret;
> +	return 0;
>  }
>  
>  static const struct v4l2_ioctl_ops rga_ioctl_ops = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-spu-rga3-v7-0-3f33e8c7145f@pengutronix.de?part=17