Re: [PATCH 06/11] accel: ethosu: Fix probe error cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Asynchronous Runtime PM put on probe error path causes device to remain powered on.
- [High] IRQ remains active while the DRM scheduler is destroyed on probe error and unbind, risking Use-After-Free.
- [High] IRQ handler accesses hardware registers without PM runtime protection, causing bus faults.
- [High] Memory leak of `done_fence` via user-controlled ioctl error path.
--

commit 929da42114076da83041012a4812042cd457402a
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Fix probe error cleanup

This commit unwinds initialized resources on probe error paths to prevent
leaks and leaves the hardware cleanly. It also prevents calling
drm_sched_fini() on a failed scheduler initialization.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index f1af7b3ea0385..41ecfc623d425 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -371,13 +371,23 @@ static int ethosu_probe(struct platform_device *pdev)
>  
>  	ret = ethosu_init(ethosudev);
>  	if (ret)
> -		return ret;
> +		goto err_job_fini;

[Severity: High]
This isn't a bug introduced by this patch, but does the initialization order
risk a bus fault if a spurious interrupt arrives early?

In ethosu_probe(), ethosu_job_init() requests and unmasks the IRQ before
ethosu_init() is called to enable the device clocks.

If a spurious interrupt fires while the device is still powered off (or in
autosuspend), the handler executes unconditionally:

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

Can reading these unclocked memory-mapped registers cause a synchronous
external abort?

>  
>  	ret = drm_dev_register(&ethosudev->base, 0);
>  	if (ret)
> -		pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
> +		goto err_pm_runtime;
> +
> +	pm_runtime_put_autosuspend(ethosudev->base.dev);
> +	return 0;
>  
> +err_pm_runtime:
> +	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
>  	pm_runtime_put_autosuspend(ethosudev->base.dev);

[Severity: High]
This isn't a bug introduced by this patch, but does using
pm_runtime_put_autosuspend() here cause a problem with devres cleanup?

When the probe function fails and returns, devres will immediately call
pm_runtime_disable().

According to the PM subsystem guidelines, using an asynchronous
pm_runtime_put() like pm_runtime_put_autosuspend() when
pm_runtime_disable() follows immediately causes the pending suspend work
to be cancelled.

Can this leave the NPU hardware powered on and in an incorrect PM state?

> +	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]
This isn't a bug introduced by this patch, but could this error path lead to
a use-after-free regression due to the interrupt handler remaining active?

In ethosu_job_init(), the hardware interrupt is requested via
devm_request_threaded_irq().

Because it is devres-managed, the IRQ is not disabled until after the probe
function completely finishes and devres runs.

However, ethosu_job_fini() synchronously destroys the DRM scheduler here.

If a spurious or delayed interrupt fires in the window between
drm_sched_fini() and devres cleanup:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() {
    ...
    drm_sched_fault(&edev->sched);
    ...
}

Will the IRQ handler access the freed scheduler structures?

>  	return ret;
>  }


[Severity: High]
This is a pre-existing issue, but does the error path in
ethosu_ioctl_submit_job() leak the done_fence allocation?

In ethosu_ioctl_submit_job(), ejob->done_fence is dynamically allocated:

drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() {
    ...
    ejob->done_fence = kzalloc_obj(*ejob->done_fence);
    ...
}

If a subsequent step fails (such as drm_gem_object_lookup failing due to an
invalid command stream buffer handle), the code jumps to out_cleanup_job. 

The cleanup function ethosu_job_err_cleanup() frees the job struct and BO
references, but it doesn't appear to call kfree() on ejob->done_fence.

Could unprivileged userspace trigger this repeatedly to drain kernel memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-ethosu-fixes-v1-0-346f9ea8791c@kernel.org?part=6