Re: [PATCH v7 06/28] media: rockchip: rga: fix too small buffer size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Teardown asymmetry and Use-After-Free during device unbind due to premature resource release.
- [Critical] Use-After-Free of the main rockchip_rga structure due to devres lifecycle mismatch.
- [High] DMA API violation and potential out-of-bounds memory invalidation on command buffer.
--

commit f24ab77ddd82ca43a87c0f75d653428b04cd703c
Author: Sven Püschel <s.pueschel@pengutronix.de>
Subject: media: rockchip: rga: fix too small buffer size

This commit fixes the RGA_CMDBUF_SIZE macro to represent the command
buffer size in bytes rather than in number of u32 elements. It adjusts
the memset() in rga_cmd_set() to use the correct byte size.

> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> index 43ed742a16492..d1618bb247501 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> @@ -414,7 +414,7 @@ static void rga_cmd_set(struct rga_ctx *ctx,
>  {
>  	struct rockchip_rga *rga = ctx->rga;
>  
> -	memset(rga->cmdbuf_virt, 0, RGA_CMDBUF_SIZE * 4);
> +	memset(rga->cmdbuf_virt, 0, RGA_CMDBUF_SIZE);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed the driver
allocates the command buffer using dma_alloc_attrs() with
DMA_ATTR_WRITE_COMBINE, yet it calls dma_sync_single_for_device() later
in this function:

	/* sync CMD buf for RGA */
	dma_sync_single_for_device(rga->dev, rga->cmdbuf_phy,
		PAGE_SIZE, DMA_BIDIRECTIONAL);

The DMA API prohibits calling streaming sync functions on coherent or
write-combined allocations.

Because the memory is mapped as write-combined, the implicit write barrier
provided by writel() when writing to the command register should be sufficient
to drain the write buffers, making the sync unnecessary.

Additionally, the sync size is hardcoded to PAGE_SIZE (4096 bytes) instead
of the actual buffer size. If the system allocator packs small allocations
into a sub-page coherent pool, syncing 4096 bytes could cause out-of-bounds
cache invalidation for adjacent memory objects.

Could we remove this unnecessary sync or switch to a proper streaming DMA
allocation?

>  
>  	rga_cmd_set_src_addr(ctx, src->dma_desc_pa);
>  	/*

Additionally, while reviewing the driver, I noticed a couple of other
pre-existing lifetime and teardown issues:

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential use-after-free
during device unbind due to premature resource release.

In rga_remove(), the driver tears down core resources by calling
v4l2_m2m_release(rga->m2m_dev) before video_unregister_device(rga->vfd).

Because the video device is still registered, user space could issue an ioctl
(e.g., VIDIOC_QBUF) concurrently.

The V4L2 core routes this to the m2m handlers, which would dereference the
already-freed m2m_dev.

Furthermore, the hardware isn't stopped and the devres-managed IRQ isn't
disabled during teardown. If an active hardware job completes after
rga_remove() returns, the interrupt handler will access freed memory.

Should the driver unregister the video device and halt the hardware before
releasing the m2m context?

[Severity: Critical]
This is a pre-existing issue, but there appears to be a use-after-free
vulnerability related to the devres lifecycle of the main rockchip_rga
structure.

The core device structure (struct rockchip_rga) is allocated using
devm_kzalloc(), which tightly binds its lifetime to the platform device.

However, the V4L2 character device can be held open by user space
independently of the hardware's bind/unbind lifecycle.

If the platform device is unbound, rga_remove() executes and the devres
manager frees the rockchip_rga structure immediately.

When user space later closes its file descriptor, the V4L2 release callback
(rga_release()) is invoked and attempts to acquire a mutex
(mutex_lock(&rga->mutex)) embedded within the already-freed structure.

Should the core structure's lifetime be decoupled from devres and instead
be reference-counted?

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