Re: [PATCH 13/17] media: rockchip: rga: bind all cores to the master

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unbounded increment of `rga->num_cores` during component re-binding leads to a heap out-of-bounds write on `rga->cores`.
- [High] Missing `component_unbind_all()` in the error paths of `rga_bind()`.
- [Medium] Missing `pm_runtime_disable()` when `component_add()` fails in `rga_core_probe()`.

Pre-existing issues:
- [High] Missing protection against shared interrupts while the device is runtime-suspended causes invalid hardware accesses.
- [High] Prematurely freeing `m2m_dev` in `rga_unbind` causes a use-after-free when open file descriptors are subsequently closed or used concurrently.
--

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

media: rockchip: rga: bind all cores to the master

This commit modifies the RGA driver to bind all core components to the
master device instead of only the first one. It ensures all added cores
report the same hardware version before initialization.

> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -750,14 +751,21 @@ static int rga_core_bind(struct device *dev, struct device *master, void *data)
[ ... ]
>  	pm_runtime_put(core->dev);
>  
> -	rga->cores[0] = core;
> +	rga->cores[rga->num_cores++] = core;

[Severity: High]
If the master device is unbound and then re-bound, does this continuously
increment rga->num_cores without bounds checking?

Looking at rga_unbind(), rga->num_cores is never reset to 0. If a re-bind
occurs, could this write the core pointer out-of-bounds into adjacent heap
memory?

[ ... ]

[Severity: High]
If component_bind_all() succeeds in rga_bind(), but a subsequent step like
v4l2_device_register() fails, the function returns the error directly:

rga_bind() {
    ...
    ret = v4l2_device_register(dev, &rga->v4l2_dev);
    if (ret)
        return ret;
    ...
}

Does this skip calling component_unbind_all()? If so, could this permanently
leak the component bindings, and if retried, cause rga_core_bind() to execute
again and increment rga->num_cores out-of-bounds?

[Severity: Medium]
In rga_core_probe(), pm_runtime_enable() is called, but if component_add()
fails:

rga_core_probe() {
    ...
    ret = component_add(&pdev->dev, &rga_core_ops);
    if (ret < 0) {
        dev_err(&pdev->dev, "failed to register component: %d", ret);
        return ret;
    }
    ...
}

Will returning directly here instead of jumping to the err_put_clk cleanup
label leak the runtime PM enablement state for the device?

[Severity: High]
This is a pre-existing issue, but the driver uses a shared IRQ (IRQF_SHARED)
when lacking an internal IOMMU. The interrupt handler rga_isr() accesses
hardware registers without checking if the device is active:

rga_isr() {
    ...
    if (rga->hw->handle_irq(core)) {
    ...
}

Additionally, rga_runtime_suspend() disables clocks without synchronizing:

rga_runtime_suspend() {
    ...
    clk_bulk_disable_unprepare(core->num_clks, core->clks);
    ...
}

Should rga_isr() use pm_runtime_get_if_active() before access, and should
rga_runtime_suspend() call synchronize_irq() to prevent unclocked hardware
access if a shared interrupt fires while the device is suspended?

[Severity: High]
This isn't a bug introduced by this patch, but rga_unbind() frees the
memory-to-memory device structure prematurely:

rga_unbind() {
    ...
    v4l2_m2m_release(rga->m2m_dev);
    video_unregister_device(rga->vfd);
    ...
}

If userspace applications still hold open file descriptors, closing them
later triggers rga_release() which calls v4l2_m2m_ctx_release(). Can this
lead to a use-after-free when dereferencing the already freed m2m_dev?
Might it be safer to defer the release of m2m_dev until the final V4L2
release callback?

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