Re: [PATCH v2 2/5] accel/ethosu: fix scheduler and IRQ lifetime

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] ethosu_init() ignores the return value of ethosu_sram_init().
- [Critical] The driver leaves NPU_REG_BASEP(ETHOSU_SRAM_REGION) unmodified when job->sram_size == 0 and no BO handle is provided for the SRAM region, allowing an attacker to force the NPU to use a stale DMA address from a previously freed BO.
- [High] The ethosu_probe() error paths lack cleanup for resources initialized during probe, leading to memory leaks and leaked DRM scheduler workqueues.
- [High] The return value of devm_platform_ioremap_resource() is not checked for errors, leading to a crash on failure.
- [High] Calling drm_sched_job_cleanup() on an uninitialized job causes a null pointer dereference.
- [High] Racing ethosu_ioctl_submit with device unbind causes kernel panics due to missing drm_dev_enter() and incorrect scheduler lifetime.
- [High] The job->done_fence allocated during job submission is leaked on error paths and causes a refcount underflow warning during scheduler teardown if the job is cancelled.
- [High] Shared IRQ handler accesses hardware registers while clocks are disabled.
--

--- Patch [2]: [PATCH v2 2/5] accel/ethosu: fix scheduler and IRQ lifetime ---
commit 5c0b05e99c708b0d7c384370684640c706d58902
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>

accel/ethosu: fix scheduler and IRQ lifetime

ethosu_job_init() requests the managed IRQ before initializing the DRM
scheduler. If drm_sched_init() fails, the error path calls drm_sched_fini()
on a partially initialized scheduler. The IRQ also remains registered until
devres cleanup after normal or probe-error paths destroy the scheduler.

Initialize the scheduler before requesting the IRQ. Scheduler setup failure
then needs no teardown, while IRQ setup failure can tear down a valid
scheduler. Free the IRQ explicitly before drm_sched_fini() to prevent
interrupt handlers from accessing scheduler state during teardown.

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/20260716082530.1023538-1-zhaoguohan@kylinos.cn?part=1
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index b76924645aaaf..5847433e87e7e 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -315,6 +315,14 @@ int ethosu_job_init(struct ethosu_device *edev)
>  	if (edev->irq < 0)
>  		return edev->irq;
>  
> +	edev->fence_context = dma_fence_context_alloc(1);
> +
> +	ret = drm_sched_init(&edev->sched, &args);
> +	if (ret) {
> +		dev_err(dev, "Failed to create scheduler: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = devm_request_threaded_irq(dev, edev->irq,

[ ... ]

[Severity: High]
This is a pre-existing issue, but the ethosu_probe() error paths lack cleanup
for resources initialized during probe:

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

If ethosu_init() or drm_dev_register() fails, the function returns directly
without calling ethosu_job_fini().

Does this leak the DRM scheduler initialized by ethosu_job_init(), including
the scheduler's submit_wq workqueue and threads?

[Severity: High]
This is a pre-existing issue, but the return value of
devm_platform_ioremap_resource() is not checked for errors:

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 devm_platform_ioremap_resource() fails, it returns an ERR_PTR. 

Because the return value is not checked with IS_ERR(), could ethosudev->regs
hold an invalid error pointer that causes a crash when dereferenced later in
ethosu_reset()?

[Severity: High]
This is a pre-existing issue, but drm_sched_job_cleanup() might be called on
an uninitialized job:

drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() {
    ...
	ejob->done_fence = kzalloc_obj(*ejob->done_fence);
	if (!ejob->done_fence) {
		ret = -ENOMEM;
		goto out_cleanup_job;
	}
    ...
out_cleanup_job:
	if (ret)
		drm_sched_job_cleanup(&ejob->base);
    ...
}

If the allocation for ejob->done_fence fails, the error path calls
drm_sched_job_cleanup() on ejob->base. However, drm_sched_job_init() was
never called, meaning ejob->base.s_fence is NULL.

Will this cause a null pointer dereference when drm_sched_job_cleanup()
attempts to read the refcount?

[Severity: Critical]
This is a pre-existing issue, but ethosu_init() ignores the return value of
ethosu_sram_init():

drivers/accel/ethosu/ethosu_drv.c:ethosu_init() {
    ...
	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);
	ethosudev->npu_info.config = config = readl_relaxed(ethosudev->regs + NPU_REG_CONFIG);

	ethosu_sram_init(ethosudev);
    ...
}

If gen_pool_dma_alloc() fails inside ethosu_sram_init(), it returns -ENOMEM,
but ethosu_init() completely ignores this return code.

Could the driver proceed as if SRAM is fully available, programming physical
address 0 into the NPU SRAM base registers when a job runs?

[Severity: High]
This is a pre-existing issue, but ethosu_ioctl_submit() appears to lack
drm_dev_enter() and drm_dev_exit() protection:

drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit() {
	struct drm_ethosu_submit *args = data;
    ...

While ethosu_remove() tears down the scheduler:

drivers/accel/ethosu/ethosu_drv.c:ethosu_remove() {
    ...
	drm_dev_unregister(&ethosudev->base);
	ethosu_job_fini(ethosudev);
    ...
}

Since ethosu_remove() destroys the DRM scheduler synchronously via
ethosu_job_fini(), userspace might continue invoking submit ioctls on open
file descriptors.

Can this result in a use-after-free if submissions are pushed to the
scheduler entity and attempt to use the destroyed dev->sched workqueues
and locks?

[Severity: High]
This is a pre-existing issue, but the job->done_fence allocated during job
submission seems to be leaked on error paths:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() {
	unsigned int i;

	for (i = 0; i < job->region_cnt; i++)
		drm_gem_object_put(job->region_bo[i]);

	drm_gem_object_put(job->cmd_bo);

	kfree(job);
}

If job submission fails after ejob->done_fence is allocated,
ethosu_job_err_cleanup() frees the job but never frees the done_fence.

Additionally, if a job is cancelled before execution, dma_fence_init() is
never called, leaving the fence refcount at 0.

drivers/accel/ethosu/ethosu_job.c:ethosu_job_cleanup() {
    ...
	dma_fence_put(job->done_fence);
    ...
}

Could dma_fence_put() then trigger a refcount underflow warning during
scheduler teardown?

[Severity: Critical]
This is a pre-existing issue, but the NPU SRAM base address might remain
unmodified when job->sram_size is 0:

drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() {
    ...
		/* Can only omit a BO handle if the region is not used or used for SRAM */
		if (!job->region_bo_handles[i]) {
			if (!cmd_info->region_size[i])
				continue;
			if (i == ETHOSU_SRAM_REGION) {
				if (cmd_info->region_size[i] <= edev->npu_info.sram_size)
					continue;
    ...
}

drivers/accel/ethosu/ethosu_job.c:ethosu_job_hw_submit() {
    ...
	if (job->sram_size) {
		writel_relaxed(lower_32_bits(dev->sramphys),
			       dev->regs + NPU_REG_BASEP(ETHOSU_SRAM_REGION));
    ...
}

The ioctl validation allows an empty BO handle for ETHOSU_SRAM_REGION if the
command stream region size fits in the hardware SRAM. However,
ethosu_job_hw_submit() only programs the hardware SRAM base if
job->sram_size is greater than 0.

If a user submits a job with a command stream that accesses the SRAM region
while setting job->sram_size to 0, does this leave a stale DMA address in the
hardware register from a previously freed BO, potentially allowing arbitrary
use-after-free DMA access?

[Severity: High]
This is a pre-existing issue, but the shared IRQ handler accesses hardware
registers unconditionally:

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

The driver requests the IRQ using IRQF_SHARED and supports runtime PM.

If another device on the same shared IRQ line triggers an interrupt while
this device is runtime-suspended and its clocks are disabled, could reading
dev->regs + NPU_REG_STATUS cause a synchronous external abort (bus fault)?

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