Re: [PATCH 01/17] media: rockchip: rga: zero cmdbuf in shared code

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent lockless modification of f->crop and ctx->cmdbuf_dirty in vidioc_s_selection races with device_run.
--

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

media: rockchip: rga: zero cmdbuf in shared code

This commit centralizes the zeroing of the command buffer into the shared rga
code instead of individual implementations. It also uses the cmdbuf_size member
for the memset operation.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index b3cb6bf8eb863..bd0afd33affe4 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -41,6 +41,7 @@ static void device_run(void *prv)
>  	spin_lock_irqsave(&rga->ctrl_lock, flags);
>  	if (ctx->cmdbuf_dirty) {
>  		ctx->cmdbuf_dirty = false;
> +		memset(ctx->cmdbuf_virt, 0, rga->hw->cmdbuf_size);
>  		rga->hw->setup_cmdbuf(ctx);

[Severity: High]
This is a pre-existing issue, but can the lockless modification of f->crop
and ctx->cmdbuf_dirty in vidioc_s_selection race with this execution of
device_run?

In vidioc_s_selection, the crop dimensions and dirty flag are updated without
acquiring the rga->ctrl_lock spinlock:

drivers/media/platform/rockchip/rga/rga.c:vidioc_s_selection() {
    ...
    f->crop = s->r;
    ctx->cmdbuf_dirty = true;
    ...
}

Because vidioc_s_selection runs with only rga->mutex held and allows
modifications while streaming, it can execute concurrently with the v4l2_m2m
worker thread running device_run.

Since vidioc_s_selection does not acquire rga->ctrl_lock, hardware
configuration functions like rga_cmd_set_dst_info can read a torn f->crop 
state during rga->hw->setup_cmdbuf:

drivers/media/platform/rockchip/rga/rga-hw.c:rga_cmd_set_dst_info() {
    ...
    dst_h = ctx->out.crop.height;
    dst_w = ctx->out.crop.width;
    dst_x = ctx->out.crop.left;
    dst_y = ctx->out.crop.top;
    ...
    corner_addrs = rga_get_corner_addrs(&ctx->out, addrs, dst_x, dst_y,
                                        dst_w, dst_h);
    ...
}

If a torn read mixes an old, large left and top offset with a new, large
width and height, it bypasses the validation done in vidioc_s_selection.

Does this cause rga_get_corner_addrs to calculate out-of-bounds DMA base
addresses, resulting in the RGA hardware performing out-of-bounds DMA writes
past the allocated video buffers?

>  	}
>  	spin_unlock_irqrestore(&rga->ctrl_lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-spu-rga3multicore-v1-0-3ec2b15675f7@pengutronix.de?part=1