Re: [PATCH v7 16/28] media: rockchip: rga: reuse cmdbuf contents

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Prematurely releasing `ctrl_lock` in `device_run()` creates a race condition with V4L2 control updates, leading to mismatched DMA offsets and potential out-of-bounds memory writes.
- [Critical] Modifying `ctx->cmdbuf_dirty` and crop settings in `vidioc_s_selection()` without holding `ctrl_lock` causes a data race with `device_run()`, leading to out-of-bounds DMA reads/writes.
--

commit 1de4a02a0f92b85a965f47ad17f866f83247fbba
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: reuse cmdbuf contents

This commit reuses the command buffer contents instead of completely
rewriting it for every frame. A new cmdbuf_dirty flag tracks whether
the command buffer needs to be rewritten on the next frame.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -38,6 +38,11 @@ static void device_run(void *prv)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&rga->ctrl_lock, flags);
> +	if (ctx->cmdbuf_dirty) {
> +		ctx->cmdbuf_dirty = false;
> +		rga->hw->setup_cmdbuf(ctx);
> +	}
> +	spin_unlock_irqrestore(&rga->ctrl_lock, flags);
>  
>  	rga->curr = ctx;
>  
> @@ -47,8 +52,6 @@ static void device_run(void *prv)
>  	dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  
>  	rga->hw->start(rga, vb_to_rga(src), vb_to_rga(dst));
> -
> -	spin_unlock_irqrestore(&rga->ctrl_lock, flags);
>  }

[Severity: Critical]
Could dropping the ctrl_lock before calling rga->hw->start() lead to an
out-of-bounds DMA write?

In device_run(), rga->hw->setup_cmdbuf() is called while holding ctrl_lock,
configuring the hardware transformation flags (e.g., rot_mode based on
ctx->rotate).

After the lock is dropped, rga->hw->start() executes and calls
rga_cmd_set_dst_info(), which reads ctx->rotate again to calculate the
destination DMA start address:

device_run()
  rga->hw->start()
    rga_cmd_set()
      rga_cmd_set_dst_info()
        switch (ctx->rotate) {
        ...

Since device_run() runs asynchronously, could userspace concurrently call
VIDIOC_S_EXT_CTRLS, allowing rga_s_ctrl() to modify ctx->rotate during this
lockless window? This might leave the hardware command buffer with a rot_mode
of 0 but destination offsets calculated for a 90-degree rotation, causing the
hardware to start DMA at an incorrect coordinate and draw linearly past the
buffer bounds.

[ ... ]
> @@ -564,6 +570,7 @@ static int vidioc_s_selection(struct file *file, void *priv,
>  	}
>  
>  	f->crop = s->r;
> +	ctx->cmdbuf_dirty = true;
>  
>  	return ret;
>  }

[Severity: Critical]
Does modifying ctx->cmdbuf_dirty and f->crop without holding ctrl_lock cause a
data race with device_run()?

If device_run() executes concurrently and reads cmdbuf_dirty as false, it
would skip calling setup_cmdbuf(), leaving the old active dimensions in
RGA_SRC_ACT_INFO.

Later, device_run() unconditionally calls rga_cmd_set_src_info(), which reads
the new f->crop.left and f->crop.top to calculate the DMA start offset. If
the new crop offset is large but the hardware retains the old, larger active
width, could the calculated DMA end address exceed the buffer's bounds,
leading to an out-of-bounds DMA read or write?

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