Re: [PATCH v6 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:
- [Critical] Moving `dma_free_attrs()` to `rga_release()` exacerbates a Use-After-Free vulnerability.
- [High] Pre-existing Runtime PM race condition in the `rga_probe` error path.
- [High] Pre-existing Use-After-Free in `rga_open` due to ignored return value from `rga_setup_ctrls`.
- [High] Pre-existing Race Condition in `rga_remove` leading to Use-After-Free on the mem2mem device.
--

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

media: rockchip: rga: move cmdbuf to rga_ctx

This commit moves the command buffer to the rga_ctx struct to prepare for
reusing an already prepared command buffer. No sync point is added, as one
command buffer is only used for one conversion at a time.

> 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

[ ... ]

> @@ -230,15 +240,13 @@ static int rga_open(struct file *file)
>  			    ctx->out.fmt->fourcc, def_width, def_height);
>  
>  	if (mutex_lock_interruptible(&rga->mutex)) {
> -		kfree(ctx);
> -		return -ERESTARTSYS;
> +		ret = -ERESTARTSYS;
> +		goto rel_cmdbuf;
>  	}
>  	ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(rga->m2m_dev, ctx, &queue_init);
>  	if (IS_ERR(ctx->fh.m2m_ctx)) {
>  		ret = PTR_ERR(ctx->fh.m2m_ctx);
> -		mutex_unlock(&rga->mutex);
> -		kfree(ctx);
> -		return ret;
> +		goto unlock_mutex;
>  	}
>  	v4l2_fh_init(&ctx->fh, video_devdata(file));
>  	v4l2_fh_add(&ctx->fh, file);

This isn't a bug introduced by this patch, but while reviewing this area I
noticed that rga_open() calls rga_setup_ctrls() just after this block:

	rga_setup_ctrls(ctx);

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

Is it safe to ignore the return value of rga_setup_ctrls() here?

If the control setup fails, rga_setup_ctrls() frees the control handler and
destroys its mutex internally. Because the failure is ignored, the code
proceeds to call v4l2_ctrl_handler_setup() on the destroyed handler, which
could trigger a use-after-free on the mutex and internal control structures.

[ ... ]

> @@ -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);
> +
>  	kfree(ctx);
>  
>  	mutex_unlock(&rga->mutex);

Could this trigger a use-after-free when the file descriptor is kept open
after the device unbinds?

The rga structure is allocated using devm_kzalloc() during probe, which
means it is automatically freed by devres when the device is unbound via
sysfs or module removal. If userspace keeps the V4L2 file descriptor open
after the device is removed, rga_release() will be called on close.

When that happens, dma_free_attrs() dereferences the already freed rga
pointer to access rga->dev and rga->hw. Calling the DMA API with a detached
or destroyed device structure might also trigger IOMMU faults or kernel
panics.

[ ... ]

> @@ -720,19 +741,10 @@ static int rga_probe(struct platform_device *pdev)
>  
>  	pm_runtime_put(rga->dev);
>  
> -	/* Create CMD buffer */
> -	rga->cmdbuf_virt = dma_alloc_attrs(rga->dev, rga->hw->cmdbuf_size,
> -					   &rga->cmdbuf_phy, GFP_KERNEL,
> -					   DMA_ATTR_WRITE_COMBINE);
> -	if (!rga->cmdbuf_virt) {
> -		ret = -ENOMEM;
> -		goto rel_m2m;
> -	}
> -
>  	ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
>  		v4l2_err(&rga->v4l2_dev, "Failed to register video device\n");
> -		goto free_dma;
> +		goto rel_m2m;
>  	}

This isn't a bug introduced by this patch, but could this error path leave
the hardware in an active power state indefinitely?

If video_register_device() fails, the code jumps to rel_m2m, which falls
through to the err_put_clk label:

err_put_clk:
	pm_runtime_disable(rga->dev);

Because an asynchronous pm_runtime_put(rga->dev) is called just before
registration, the pending async suspend work would be abruptly canceled by
the internal barrier in pm_runtime_disable(). Should this use
pm_runtime_put_sync() instead before entering the error path?

[ ... ]

> @@ -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);

This isn't a bug introduced by this patch, but does calling
v4l2_m2m_release() here before video_unregister_device() create a race
condition?

If v4l2_m2m_release(rga->m2m_dev) runs first, it leaves a window where the
video node is still active and accessible to userspace, but the backing
mem2mem device has already been freed.

If a user space program calls an ioctl during this window, could the file
operations access the freed rga->m2m_dev and cause a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-spu-rga3-v6-0-e547152eb9c9@pengutronix.de?part=14