Re: [PATCH 08/17] media: rockchip: rga: add rga_core structure

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing NULL pointer check for devm_kzalloc allocation

Pre-existing issues:
- [High] Shared IRQ handler accesses suspended hardware and dereferences NULL context
- [High] Missing synchronize_irq() in runtime suspend with shared interrupts
- [High] Race condition between pm_runtime_enable and platform_set_drvdata
- [High] Use-After-Free during module removal due to incorrect teardown ordering
--

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

media: rockchip: rga: add rga_core structure
    
Add a rga_core structure to separate the core specific data from the
m2m device. This is done in preparation for multi-core support.

> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c

[ ... ]

> @@ -447,36 +447,34 @@ static void rga_hw_setup_cmdbuf(struct rga_ctx *ctx)
> -static bool rga_handle_irq(struct rockchip_rga *rga)
> +static bool rga_handle_irq(struct rga_core *core)
>  {
>  	int intr;
>  
> -	intr = rga_read(rga, RGA_INT) & 0xf;
> +	intr = rga_read(core, RGA_INT) & 0xf;
>  
> -	rga_mod(rga, RGA_INT, intr << 4, 0xf << 4);
> +	rga_mod(core, RGA_INT, intr << 4, 0xf << 4);
>  
>  	return intr & RGA_INT_COMMAND_FINISHED;
>  }

[Severity: High]
This is a pre-existing issue, but does this handle spurious interrupts
correctly when the hardware is suspended?

If an interrupt arrives on the shared IRQ line while the device is suspended,
the clock-gated hardware typically returns 0xffffffff for rga_read(). The
bitwise AND with RGA_INT_COMMAND_FINISHED will incorrectly evaluate to true.

The handler would then proceed in rga_isr():

    struct rga_ctx *ctx = core->curr;
    WARN_ON(!ctx);
    core->curr = NULL;
    src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);

If no job is active, core->curr is NULL, and dereferencing ctx->fh.m2m_ctx
will cause a kernel panic. Should we verify the PM state before hardware
access in the ISR using pm_runtime_get_if_active()?

> 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

[ ... ]

> @@ -799,20 +803,25 @@ static int rga_probe(struct platform_device *pdev)
>  	if (!rga->hw)
>  		return dev_err_probe(&pdev->dev, -ENODEV, "failed to get match data\n");
>  
> -	rga->dev = &pdev->dev;
>  	spin_lock_init(&rga->ctrl_lock);
>  	mutex_init(&rga->mutex);
>  
> -	ret = rga_parse_dt(rga);
> +	core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
> +	core->rga = rga;
> +	core->dev = &pdev->dev;

[Severity: High]
Could this lead to a NULL pointer dereference? If devm_kzalloc() fails and
returns NULL, assigning core->rga = rga in the next line will immediately
crash. A check like "if (!core) return -ENOMEM;" appears to be missing here.

> +
> +	rga->cores[0] = core;
> +
> +	ret = rga_parse_dt(core);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "Unable to parse OF data\n");
>  
> -	pm_runtime_set_autosuspend_delay(rga->dev, 50);
> -	pm_runtime_enable(rga->dev);
> +	pm_runtime_set_autosuspend_delay(core->dev, 50);
> +	pm_runtime_enable(core->dev);
>  
> -	rga->regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(rga->regs)) {
> -		ret = PTR_ERR(rga->regs);
> +	core->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(core->regs)) {
> +		ret = PTR_ERR(core->regs);
>  		goto err_put_clk;
>  	}

[ ... ]

> @@ -852,7 +861,7 @@ static int rga_probe(struct platform_device *pdev)
>  	video_set_drvdata(vfd, rga);
>  	rga->vfd = vfd;
>  
> -	platform_set_drvdata(pdev, rga);
> +	platform_set_drvdata(pdev, core);

[Severity: High]
This is a pre-existing issue, but does this create a race condition window
between pm_runtime_enable() and platform_set_drvdata()?

If a PM transition occurs right after pm_runtime_enable(core->dev), the
runtime PM callbacks like rga_runtime_suspend() might execute before
platform_set_drvdata(pdev, core) is called.

In rga_runtime_suspend(), dev_get_drvdata() would return NULL, leading to a
crash when attempting to access core->num_clks.

[ ... ]

> @@ -904,23 +914,23 @@ static void rga_remove(struct platform_device *pdev)
>  	video_unregister_device(rga->vfd);
>  	v4l2_device_unregister(&rga->v4l2_dev);
>  
> -	pm_runtime_disable(rga->dev);
> +	pm_runtime_disable(core->dev);
>  }

[Severity: High]
This is a pre-existing issue, but could this teardown sequence result in a
use-after-free vulnerability?

Calling v4l2_m2m_release(rga->m2m_dev) (which occurs just before the quoted
lines) before video_unregister_device() leaves the video node accessible to
userspace after the m2m_dev structure has been freed. Concurrent V4L2 ioctls
from userspace might then interact with freed memory.

>  static int __maybe_unused rga_runtime_suspend(struct device *dev)
>  {
> -	struct rockchip_rga *rga = dev_get_drvdata(dev);
> +	struct rga_core *core = dev_get_drvdata(dev);
>  
> -	clk_bulk_disable_unprepare(rga->num_clks, rga->clks);
> +	clk_bulk_disable_unprepare(core->num_clks, core->clks);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but are we missing a call to synchronize_irq()
before powering down the hardware?

Since the driver can request a shared interrupt line (IRQF_SHARED), disabling
the clocks without waiting for mid-flight IRQ handlers to finish might result
in bus errors or crashes if another device triggers the shared IRQ while
suspending.

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