Re: [PATCH v2 3/5] accel/ethosu: clean up resources on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared interrupt handler accesses hardware registers without verifying if the device is active, risking a synchronous external abort.
- [High] Runtime suspend disables hardware clocks without calling synchronize_irq() for the shared interrupt.
- [High] Missing IS_ERR() check on the return value of devm_platform_ioremap_resource() causes an error pointer dereference.
- [High] The shared IRQ is requested before the hardware is powered on, risking a crash if a shared interrupt fires early.
- [High] Destroying the DRM scheduler in ethosu_remove() creates a Use-After-Free risk for open file descriptors.
- [High] An untrusted hardware register value is used as an array index without bounds checking, potentially leaking kernel memory to the log.
--

--- Patch [3]: [PATCH v2 3/5] accel/ethosu: clean up resources on probe failure ---
commit 596a2469c9dadb88b1da94803fdbc80b578983ef
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>

accel/ethosu: clean up resources on probe failure

ethosu_job_init() creates a scheduler before ethosu_init() and
drm_dev_register(). Errors from either later step return directly, leaving
the scheduler workqueue allocated. A drm_dev_register() failure also leaves
the optional SRAM allocation behind.

Unwind runtime PM, SRAM, and scheduler state in reverse order on probe
failure. Disable the clocks directly when runtime PM is unavailable or its
setup fails.

Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index f3920185389de..6b97d22444a7c 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -272,6 +272,17 @@ static int ethosu_device_suspend(struct device *dev)

[Severity: High]
This is a pre-existing issue, but ethosu_device_suspend() disables hardware
clocks without calling synchronize_irq() for the shared interrupt:

drivers/accel/ethosu/ethosu_drv.c:ethosu_device_suspend() {
	struct ethosu_device *ethosudev = dev_get_drvdata(dev);

	clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks);
	return 0;
}

Could this cause a synchronous external abort if the device enters runtime
suspend while the interrupt handler is actively executing on another CPU?

>  	return 0;
>  }
>  
> +static void ethosu_runtime_suspend(struct ethosu_device *ethosudev)
> +{
> +	struct device *dev = ethosudev->base.dev;
> +
> +	pm_runtime_dont_use_autosuspend(dev);
> +	if (pm_runtime_enabled(dev))
> +		pm_runtime_put_sync_suspend(dev);
> +	else
> +		ethosu_device_suspend(dev);
> +}
> +
>  static int ethosu_sram_init(struct ethosu_device *ethosudev)

[ ... ]

> @@ -355,13 +379,20 @@ static int ethosu_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but devm_platform_ioremap_resource() returns an
ERR_PTR on failure, not NULL.

In ethosu_probe(), its return value is assigned directly without an IS_ERR
check:

drivers/accel/ethosu/ethosu_drv.c:ethosu_probe() {
	...
	ethosudev->regs = devm_platform_ioremap_resource(pdev, 0);

	ethosudev->num_clks = devm_clk_bulk_get_all(&pdev->dev, &ethosudev->clks);
	...
}

If the resource mapping fails, would a subsequent call to readl_relaxed() in
ethosu_init() dereference the error pointer?

[Severity: High]
This is a pre-existing issue, but the shared IRQ is requested before the
hardware is powered on in ethosu_probe():

drivers/accel/ethosu/ethosu_drv.c:ethosu_probe() {
	...
	ret = ethosu_job_init(ethosudev);
	if (ret)
		return ret;

	ret = ethosu_init(ethosudev);
	...
}

If another device on the same shared IRQ line triggers an interrupt between
ethosu_job_init() and ethosu_init(), could the interrupt handler attempt to
access MMIO registers while the clocks are still disabled, causing a crash?

>  
>  	ret = ethosu_init(ethosudev);
>  	if (ret)
> -		return ret;
> +		goto err_job_fini;
>  
>  	ret = drm_dev_register(&ethosudev->base, 0);
>  	if (ret)
> -		pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
> +		goto err_runtime_suspend;
>  
>  	pm_runtime_put_autosuspend(ethosudev->base.dev);
> +	return 0;
> +
> +err_runtime_suspend:
> +	ethosu_runtime_suspend(ethosudev);
> +	ethosu_sram_fini(ethosudev);
> +err_job_fini:
> +	ethosu_job_fini(ethosudev);
>  	return ret;
>  }
>  
> @@ -371,9 +402,7 @@ static void ethosu_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&ethosudev->base);
>  	ethosu_job_fini(ethosudev);

[Severity: High]
This is a pre-existing issue, but destroying the DRM scheduler here in
ethosu_remove() might create a use-after-free risk.

drm_dev_unregister() hides the device node but doesn't revoke existing open
file descriptors. 

If a userspace process still holds an open file descriptor when the driver is
unbound, and subsequently submits an ioctl, will it access the destroyed
scheduler kthreads and freed SRAM?

> -	if (ethosudev->sram)
> -		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
> -			      ethosudev->npu_info.sram_size);
> +	ethosu_sram_fini(ethosudev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I found a pre-existing issue
in ethosu_job_irq_handler().

The shared interrupt handler accesses hardware registers without verifying
if the device is active:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() {
	...
	u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);
	...
}

Because the handler is registered with IRQF_SHARED, couldn't another device
on the same IRQ line trigger an interrupt while the NPU is runtime-suspended?
Accessing clock-gated MMIO here could trigger a synchronous external abort.

[Severity: High]
This is a pre-existing issue, but I found a potential out-of-bounds read
in ethosu_job_timedout().

An untrusted hardware register value is used as an array index without
bounds checking:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() {
	...
	cmdaddr, bocmds[cmdaddr / 4]);
	...
}

If a malformed job forces a timeout and leaves a large offset in the
NPU_REG_QREAD register, could cmdaddr exceed the bounds of the cmd_bo
buffer and leak kernel memory to the log?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717061145.1478139-1-zhaoguohan@kylinos.cn?part=3