Re: [PATCH v3 03/22] 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] The IRQ is requested and enabled before the DRM scheduler is fully initialized and before device clocks are enabled, causing potential crashes or bus stalls on spurious/pending interrupts.
- [High] The DRM scheduler is torn down while the device IRQ is still active, allowing the IRQ handler to access destroyed scheduler structures.
- [High] `ethosu_job_fini()` clears the `in_flight_job` without signaling its `done_fence`, causing tasks waiting on the fence to hang indefinitely.
- [High] Use-After-Free during device removal due to missing `drm_dev_enter()`/`drm_dev_exit()` in ioctls.
--

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

accel: ethosu: Fix probe error cleanup
    
This commit fixes a probe error path bug in the Ethos-U NPU driver.
If ethosu_init() or drm_dev_register() fails, the probe function returns
without tearing down the job scheduler or freeing the SRAM pool.
The commit adds proper resource unwinding for these failure paths.

Cc: stable@vger.kernel.org

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 1f6fb5d18cf22..2c8d735767b2a 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)
>  
>  	ret = ethosu_init(ethosudev);
>  	if (ret)
> -		return ret;
> +		goto err_job_fini;

[Severity: High]
This is a pre-existing issue, but does calling ethosu_job_init() before
ethosu_init() leave us vulnerable to spurious interrupts?

In ethosu_probe(), the IRQ is requested and enabled via
devm_request_threaded_irq() (without IRQF_NO_AUTOEN) during
ethosu_job_init(). However, device clocks are not enabled until
ethosu_init() runs.

If a pending or spurious interrupt fires immediately before clocks are enabled:

ethosu_job_irq_handler() {
    u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);
    ...
}

Can reading from memory-mapped registers with disabled clocks stall the bus
or cause a synchronous external abort? Furthermore, if the thread handler
executes, it calls drm_sched_fault() on a scheduler that may not be fully
initialized yet.

>  	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]
This is a pre-existing issue, but does calling ethosu_job_fini() here tear
down the scheduler while the device IRQ is still active?

ethosu_job_fini() calls synchronize_irq() to wait for executing handlers,
but does not disable the IRQ line before calling drm_sched_fini().

Because the IRQ is managed by devres, it remains enabled until devres
teardown, which happens after the probe error path completes. If the device
generates an interrupt in this window, the thread handler can execute:

ethosu_job_handle_irq() {
    ...
    drm_sched_fault(&dev->sched);
}

Could this result in a use-after-free by accessing the already-destroyed
DRM scheduler?

[Severity: High]
This is also a pre-existing issue, but does ethosu_job_fini() clear the
in-flight job without signaling its completion fence?

When ethosu_job_fini() checks READ_ONCE(dev->in_flight_job), if it is
populated, it clears the pointer and resets the hardware:

ethosu_job_fini() {
    ...
    if (READ_ONCE(dev->in_flight_job)) {
        WRITE_ONCE(dev->in_flight_job, NULL);
        ethosu_device_reset(dev);
    }
    ...
}

Because the hardware is reset, the completion interrupt will never arrive.
Without extracting the job and signaling its done_fence (e.g., with
-ECANCELED), could this cause userspace or kernel tasks waiting on the
fence to hang indefinitely during probe failure or driver unbind?

>  	return ret;
>  }
>  
> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index 8dce74db0cb48..ec65305e0cd7d 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c

[ ... ]

> @@ -355,14 +355,10 @@ int ethosu_job_init(struct ethosu_device *edev)
>  	ret = drm_sched_init(&edev->sched, &args);
>  	if (ret) {
>  		dev_err(dev, "Failed to create scheduler: %d\n", ret);
> -		goto err_sched;
> +		return ret;
>  	}
>  
>  	return 0;
> -
> -err_sched:
> -	drm_sched_fini(&edev->sched);
> -	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but are the driver's ioctls
missing drm_dev_enter() and drm_dev_exit() protections?

During a hot-unplug event, ethosu_remove() calls drm_dev_unregister(),
which stops new opens but allows existing file descriptors to persist. It
then tears down the DRM scheduler via ethosu_job_fini().

However, ethosu_ioctl_submit() and the ETHOSU_PERFMON_* ioctls do not verify
the device plug state. A concurrent userspace process holding the file
descriptor can still issue the ETHOSU_SUBMIT ioctl:

ethosu_ioctl_submit_job() {
    ...
    ret = drm_sched_job_init(&ejob->base, &file_priv->sched_entity, ...);
    ...
}

Can this lead to a use-after-free if userspace calls into the DRM scheduler
after dev->sched has been destroyed during device removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-ethosu-fixes-v3-0-490fe215286f@kernel.org?part=3