Re: [PATCH v7 14/28] media: rockchip: rga: move cmdbuf to rga_ctx

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unbounded per-file-descriptor DMA memory allocation in `rga_open` allows unprivileged users to exhaust system CMA/DMA memory.

Pre-existing issues:
- [Critical] Use-After-Free vulnerability in `rga_release` during driver unbind, exacerbated by new DMA cleanup code.
- [Critical] Ignored error return from `rga_setup_ctrls` in `rga_open` leads to the use of a destroyed mutex and a Double-Free.
- [Critical] Race condition in `rga_remove()` releases the V4L2 M2M device before unregistering the video node, creating a Use-After-Free window.
--

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

media: rockchip: rga: move cmdbuf to rga_ctx
    
Move the command buffer to the rga_ctx struct in preparation to reuse
an already prepared command buffer. This allows to split the command
buffer setup in a further commit to setup a template for the command
buffer at streamon and only update the buffer addresses in device_run
and trigger the command stream.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 77b8c7ab74274..bf6bbcbfc869b 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -219,6 +219,16 @@ static int rga_open(struct file *file)
>  	ctx = kzalloc_obj(*ctx);
>  	if (!ctx)
>  		return -ENOMEM;
> +
> +	/* Create CMD buffer */
> +	ctx->cmdbuf_virt = dma_alloc_attrs(rga->dev, rga->hw->cmdbuf_size,
> +					   &ctx->cmdbuf_phy, GFP_KERNEL,
> +					   DMA_ATTR_WRITE_COMBINE);

[Severity: High]
Does this allow an unprivileged user to exhaust system CMA/DMA memory?

Since the buffer is now allocated per file descriptor rather than once during
device probe, and there seem to be no driver-level limits on how many times
the V4L2 device can be opened, could a user loop open() or use fork() to
unboundedly allocate DMA memory?

> +	if (!ctx->cmdbuf_virt) {
> +		ret = -ENOMEM;
> +		goto rel_ctx;
> +	}
> +
>  	ctx->rga = rga;
>  	/* Set default formats */
>  	ctx->in = def_frame;

[ ... ]

> @@ -252,6 +260,15 @@ static int rga_open(struct file *file)
>  	mutex_unlock(&rga->mutex);
>  
>  	return 0;

[Severity: Critical]
This is a pre-existing issue, but the return value of rga_setup_ctrls(ctx)
seems to be completely ignored before returning from rga_open().

If control setup fails, rga_setup_ctrls() calls v4l2_ctrl_handler_free(),
which destroys the control handler's mutex and frees its internal structures.

By continuing execution blindly and returning 0, does the subsequent call to
v4l2_ctrl_handler_setup() end up locking a destroyed mutex?

Additionally, since the file descriptor becomes valid, won't rga_release()
eventually call v4l2_ctrl_handler_free() again and cause a double-free?

> +
> +unlock_mutex:
> +	mutex_unlock(&rga->mutex);
> +rel_cmdbuf:
> +	dma_free_attrs(rga->dev, rga->hw->cmdbuf_size, ctx->cmdbuf_virt,
> +		       ctx->cmdbuf_phy, DMA_ATTR_WRITE_COMBINE);
> +rel_ctx:
> +	kfree(ctx);
> +	return ret;
>  }
>  
>  static int rga_release(struct file *file)
> @@ -266,6 +283,10 @@ static int rga_release(struct file *file)
>  	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
>  	v4l2_fh_del(&ctx->fh, file);
>  	v4l2_fh_exit(&ctx->fh);
> +
> +	dma_free_attrs(rga->dev, rga->hw->cmdbuf_size, ctx->cmdbuf_virt,
> +		       ctx->cmdbuf_phy, DMA_ATTR_WRITE_COMBINE);
> +

[Severity: Critical]
This is a pre-existing issue, but because rga is allocated with
devm_kzalloc() in rga_probe(), if a user keeps the file descriptor open
during driver unbind, won't rga be automatically freed by devres?

When the file is later closed, doesn't rga_release() dereference the freed
rga pointer when locking rga->mutex?

Does this new dma_free_attrs() call introduce another use-after-free vector
by dereferencing the already freed rga->dev pointer?

>  	kfree(ctx);
>  
>  	mutex_unlock(&rga->mutex);

[ ... ]

> @@ -759,9 +768,6 @@ static void rga_remove(struct platform_device *pdev)
>  {
>  	struct rockchip_rga *rga = platform_get_drvdata(pdev);
>  
> -	dma_free_attrs(rga->dev, rga->hw->cmdbuf_size, rga->cmdbuf_virt,
> -		       rga->cmdbuf_phy, DMA_ATTR_WRITE_COMBINE);
> -
>  	v4l2_info(&rga->v4l2_dev, "Removing\n");
>  
>  	v4l2_m2m_release(rga->m2m_dev);

[Severity: Critical]
This is a pre-existing issue, but does this function release the V4L2 memory
to memory context before unregistering the video device?

If v4l2_m2m_release() is called before video_unregister_device(), isn't the
video node still fully accessible to userspace during this window?

If a concurrent ioctl is dispatched, could the V4L2 core route it to the
driver, causing it to dereference the already freed rga->m2m_dev?

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