Re: [PATCH v7 25/28] media: rockchip: rga: add feature flags

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of `rga_setup_ctrls()` is ignored in `rga_open()`, leading to a Use-After-Destroy vulnerability if control initialization fails.
- [High] The mem2mem device is released before the video device is unregistered in `rga_remove()`, leading to a Use-After-Free.
- [High] The main driver structure `rockchip_rga` is managed by devres (`devm_kzalloc`) but accessed by V4L2 file operations, causing a Use-After-Free on unbind.
- [High] Streaming DMA synchronization is incorrectly called on coherent memory allocated with `dma_alloc_attrs()`, and placed after triggering the hardware.
--

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

media: rockchip: rga: add feature flags

In preparation to the RGA3 addition add feature flags, which can limit
the exposed feature set of the video device, like rotating or selection
support. This is necessary as the RGA3 doesn't initially implement the
full feature set currently exposed by the driver.

[ ... ]

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 1878b4e26360b..8d60e94da32d8 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -177,17 +177,21 @@ static int rga_setup_ctrls(struct rga_ctx *ctx)
>  
>  	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 4);
>  

[ ... ]

>  	if (ctx->ctrl_handler.error) {
>  		int err = ctx->ctrl_handler.error;
>  
>  		v4l2_err(&rga->v4l2_dev, "%s failed\n", __func__);
>  		v4l2_ctrl_handler_free(&ctx->ctrl_handler);
>  		return err;
>  	}
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value
of this function in rga_open() lead to a use-after-destroy vulnerability?

drivers/media/platform/rockchip/rga/rga.c:rga_open() {
...
	rga_setup_ctrls(ctx);

	/* Write the default values to the ctx struct */
	v4l2_ctrl_handler_setup(&ctx->ctrl_handler);
...
}

If rga_setup_ctrls() fails, it calls v4l2_ctrl_handler_free() which destroys
the control handler's embedded lock. Since rga_open() ignores the error and
continues to v4l2_ctrl_handler_setup(), will it attempt to acquire the
destroyed lock?


[Severity: High]
This is a pre-existing issue, but does the teardown order in rga_remove()
cause a use-after-free if the device node is held open?

drivers/media/platform/rockchip/rga/rga.c:rga_remove() {
...
	v4l2_m2m_release(rga->m2m_dev);
	video_unregister_device(rga->vfd);
...
}

If the mem2mem device is released before the video device is unregistered,
could a userspace process with an open file descriptor trigger a
use-after-free when closing it? The rga_release() file operation dereferences
the freed m2m_dev via v4l2_m2m_ctx_release().


[Severity: High]
This is a pre-existing issue, but does managing the main driver structure
with devres lead to a use-after-free on unbind?

drivers/media/platform/rockchip/rga/rga.c:rga_probe() {
...
	rga = devm_kzalloc(&pdev->dev, sizeof(*rga), GFP_KERNEL);
...
}

When the driver is unbound, devres automatically frees the memory. Since
the v4l2 framework allows video device file descriptors to outlive the
driver unbind process, could a userspace application close a lingering file
descriptor and call rga_release() on the freed rga structure?


[Severity: High]
This isn't a bug introduced by this patch, but is streaming dma
synchronization incorrectly used on coherent memory?

drivers/media/platform/rockchip/rga/rga-hw.c:rga_hw_setup_cmdbuf() {
...
	rga_write(rga, RGA_CMD_BASE, ctx->cmdbuf_phy);

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

Since the command buffer is allocated using dma_alloc_attrs() with
DMA_ATTR_WRITE_COMBINE, is it valid to call dma_sync_single_for_device()
on it? The dma api prohibits calling streaming sync functions on addresses
returned by coherent allocation functions. Also, is it problematic that
the sync happens after writing the dma address to the hardware register?

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