Re: [PATCH 12/17] media: rockchip: rga: move core initialization from bind to probe

[sashiko-bot@kernel.org]

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

New issues:
- [High] NULL pointer dereference in `rga_isr()` due to early shared IRQ registration.
- [Medium] Missing `pm_runtime_disable()` on `component_add()` error path.

Pre-existing issues:
- [High] Unprotected hardware access in shared interrupt handler.
- [High] Missing `synchronize_irq()` in runtime suspend for shared IRQs.
- [High] Use-After-Free when the master device is unbound.
- [High] Missing NULL check for `devm_kzalloc` allocation.
--

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

media: rockchip: rga: move core initialization from bind to probe

This commit moves the core initialization from the core binding function to
the core probing function to better match the actual initialization sequence.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 952377ae467f5..0413b8518dfc8 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c

[ ... ]

> @@ -740,21 +740,49 @@ static int rga_parse_dt(struct rga_core *core)
>  
>  static int rga_core_probe(struct platform_device *pdev)
>  {
>  	struct rga_core *core;
>  	const struct rga_hw *hw;
>  	int ret = 0;
>  	int irq;
>  
>  	if (!pdev->dev.of_node)
>  		return -ENODEV;
>  
> +	hw = of_device_get_match_data(&pdev->dev);
> +	if (!hw)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "failed to get match data\n");
> +
>  	core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
> -	core->rga = rga;
>  	core->dev = &pdev->dev;

[Severity: High]
This isn't a bug introduced by this patch, but does this devm_kzalloc()
allocation need a null pointer check?

If the allocation fails under memory pressure, the immediate assignment to
core->dev = &pdev->dev will cause a null pointer dereference.

[ ... ]

> @@ -775,7 +803,7 @@ static int rga_core_bind(struct device *dev, struct device *master, void *data)
>  	}
>  
>  	ret = devm_request_irq(core->dev, irq, rga_isr,
> -			       rga_has_internal_iommu(rga) ? 0 : IRQF_SHARED,
> +			       hw->has_internal_iommu ? 0 : IRQF_SHARED,
>  			       dev_name(core->dev), core);

[Severity: High]
Since devm_request_irq() is now called here in rga_core_probe(), could this
trigger an early null pointer dereference in rga_isr()?

When hw->has_internal_iommu is false, the interrupt is registered as
IRQF_SHARED. For shared interrupts, the handler can be invoked immediately by
another device sharing the line. However, core->rga is not initialized until
rga_core_bind() runs later.

If the interrupt service routine fires early, it will evaluate
rga->hw->handle_irq(core) and unconditionally dereference the null core->rga
pointer.

[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler safely
access hardware registers?

Because rga_isr() is registered as IRQF_SHARED, it can be invoked by another
device while this device is runtime-suspended and unclocked. The handler
unconditionally calls rga->hw->handle_irq(core), which accesses registers.
Reading powered-off registers typically causes a synchronous external abort.

Should the interrupt service routine use pm_runtime_get_if_active() before
accessing registers?

[Severity: High]
This is another pre-existing issue related to the shared interrupt, but is
there a missing synchronize_irq() in the runtime suspend path?

In rga_runtime_suspend(), clocks are disabled without synchronizing the
interrupt. If an interrupt fires on another CPU just before clocks are
disabled, the handler could actively read registers exactly when the hardware
is powered down.

[Severity: High]
This is a pre-existing issue, but could tying the shared interrupt to the
core device lifetime cause a use-after-free when the master device is
unbound?

The shared interrupt is requested via devm_request_irq(core->dev, ...). When
the master device is unbound, its rga structure is freed. Since the core
device is not removed during unbind, the devm-managed interrupt is not freed,
leaving the shared handler active.

A subsequent interrupt will execute rga_isr(), which dereferences the
freed core->rga pointer.

[ ... ]

> @@ -833,11 +825,20 @@ static int rga_core_probe(struct platform_device *pdev)
>  
>  	ret = component_add(&pdev->dev, &rga_core_ops);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to register component: %d", ret);
>  		return ret;
>  	}

[Severity: Medium]
If component_add() fails, does this bypass the pm_runtime_disable() cleanup
at the err_put_clk label?

Returning directly instead of using a goto would leak the device's Runtime
PM usage counter.

>  	return 0;
> +
> +err_put_clk:
> +	pm_runtime_disable(core->dev);
> +
> +	return ret;
>  }

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