Re: [PATCH v6 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:
- [High] Data race and torn reads in vidioc_s_selection due to missing spinlock protection.
- [High] Concurrency between V4L2 controls and IOCTLs bypasses hardware scaling limits, allowing device hangs.
- [Medium] vidioc_s_selection violates V4L2 API contract by returning an error instead of adjusting the rectangle.
--

commit 807ac9fa5320ff60d938b4d5476ad5199ff6b979
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: check scaling factor

This commit introduces checks for the scaling factor to avoid potential hangs
in the RGA hardware when the maximum scaling limit is exceeded. It adds
checks in prepare_streaming, vidioc_s_selection, and when setting the
rotation control.

[ ... ]
> 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);

Can this validation be bypassed due to concurrent calls?

rga_s_ctrl() operates under the V4L2 control lock and ctx->rga->ctrl_lock,
but vidioc_s_selection() operates under the global rga->mutex. If Thread A
calls VIDIOC_S_SELECTION and Thread B calls VIDIOC_S_CTRL concurrently, could
they both independently validate their new states against the old state,
resulting in a combined crop and rotation that exceeds the hardware scaling
limit?

Also, if Thread A is in the middle of preparing a stream and q->streaming is
still 0, could Thread B invoke S_CTRL, see that vb2_is_streaming() is false,
and apply an invalid rotation without validation?

> +			if (ret < 0)
> +				goto s_ctrl_done;
> +		}
>  		ctx->rotate = ctrl->val;
>  		break;

[ ... ]

> @@ -569,10 +611,25 @@ static int vidioc_s_selection(struct file *file, void *priv,
>  		return -EINVAL;
>  	}
>  
> +	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;

Does this violate the V4L2 API specification for VIDIOC_S_SELECTION?

The V4L2 specification requires drivers to adjust the requested rectangle
coordinates and sizes to the closest possible valid values rather than
returning an error code when hardware constraints are violated. Could
returning -EINVAL here break userspace applications that rely on standard
format negotiation?

> +	}
> +
>  	f->crop = s->r;
>  	ctx->cmdbuf_dirty = true;

Can this cause a data race resulting in torn reads?

This updates f->crop, which is a 16-byte struct v4l2_rect, and
ctx->cmdbuf_dirty without holding ctx->rga->ctrl_lock. Since the driver
permits selection changes while streaming, can the M2M worker thread read
these crop rectangles concurrently during rga_hw_setup_cmdbuf() while
holding ctrl_lock?

If the assignment is not atomic, could the worker thread read a torn struct
(such as the new width but the old height) and program the DMA engine with
invalid coordinates, potentially leading to out-of-bounds memory accesses or
device hangs?

>  
> -	return ret;
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-spu-rga3-v6-0-e547152eb9c9@pengutronix.de?part=17