Re: [PATCH v2 02/19] accel: ethosu: Fix probe error cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Active devm-managed IRQ during probe error paths can trigger use-after-free or system crash due to destroyed scheduler.

Pre-existing issues:
- [High] IRQ handler is registered before device clocks are enabled, risking system crash on pending or spurious interrupt.
--

commit 884e296c2a78cb8e0657cc20be2687762177ac12
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Fix probe error cleanup

This patch unwinds initialized resources on both failure paths in probe:
ethosu_init() and drm_dev_register(). It also avoids calling drm_sched_fini()
after a failed drm_sched_init().

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index df76253d01a64..c684e28ac7871 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -375,9 +375,20 @@ static int ethosu_probe(struct platform_device *pdev)
>  

[Severity: High]
This is a pre-existing issue, but is it safe that the IRQ handler is registered
and unmasked before the device clocks are enabled?

In ethosu_probe(), ethosu_job_init() is called before ethosu_init():

ethosu_drv.c:ethosu_probe() {
	...
	ret = ethosu_job_init(ethosudev);
	if (ret)
		return ret;

	ret = ethosu_init(ethosudev);
	...
}

Inside ethosu_job_init(), devm_request_threaded_irq() unmasks the IRQ. If a
pending interrupt from a prior boot or a spurious interrupt fires, the handler
ethosu_job_irq_handler() executes and immediately reads NPU_REG_STATUS.

However, the hardware clocks are not enabled until ethosu_init() calls
ethosu_device_resume(). Could accessing this register with disabled clocks
trigger a synchronous external abort and crash the system?

>  	ret = ethosu_init(ethosudev);
>  	if (ret)
> -		return ret;
> +		goto err_job_fini;
>  
>  	ret = drm_dev_register(&ethosudev->base, 0);
> +	if (ret)
> +		goto err_sram_free;
> +
> +	return 0;
> +
> +err_sram_free:
> +	if (ethosudev->sram)
> +		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
> +			      ethosudev->npu_info.sram_size);
> +err_job_fini:
> +	ethosu_job_fini(ethosudev);

[Severity: High]
Could the devm-managed IRQ trigger a use-after-free or a crash during this
teardown window on probe failure?

This patch adds ethosu_job_fini() to the probe error path. When
ethosu_job_fini() executes, it tears down the DRM scheduler via
drm_sched_fini().

Because the IRQ was allocated via devm_request_threaded_irq() in
ethosu_job_init() and is not explicitly disabled before ethosu_job_fini(), the
interrupt remains active until the probe function returns and the devres
cleanup runs.

If an interrupt fires in this window, the handler could access the destroyed
scheduler when calling drm_sched_fault(&dev->sched). Additionally, since
ethosu_init() already suspended the device on its successful path, could the
handler crash the system by reading the status register with clocks disabled?

>  	return ret;
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-ethosu-fixes-v2-0-3767738756a4@kernel.org?part=2