[PATCH v2 0/5] accel/ethosu: fix probe and job cleanup errors

[PATCH v2 0/5] accel/ethosu: fix probe and job cleanup errors

[GuoHan Zhao]

Fix error handling and cleanup issues in the Arm Ethos-U driver.

Sashiko reported the original probe, SRAM, and fence cleanup issues while
reviewing the Ethos-U MMIO mapping fix. A follow-up review of v1 found two
more lifecycle issues in scheduler initialization and IRQ teardown, along
with an unchecked DMA mask setup error.

The first two patches handle DMA mask setup and order scheduler and IRQ
lifetimes. The remaining patches unwind probe state, propagate SRAM setup
errors, and fix completion fence ownership.

The reports are available at:

https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1
https://sashiko.dev/#/patchset/20260716082530.1023538-1-zhaoguohan@kylinos.cn?part=1

---
Changes in v2:
- Add DMA mask error handling.
- Initialize the scheduler before requesting the IRQ and release the IRQ
  before scheduler teardown.
- Disable clocks directly on probe failure when runtime PM is unavailable.
- Move the former v1 patches 1/3, 2/3, and 3/3 to patches 3/5, 4/5, and
  5/5, respectively.
- Rebase on the current drm-misc-fixes branch.

v1: https://lore.kernel.org/all/20260716082530.1023538-1-zhaoguohan@kylinos.cn/

GuoHan Zhao (5):
  accel/ethosu: check DMA mask setup
  accel/ethosu: fix scheduler and IRQ lifetime
  accel/ethosu: clean up resources on probe failure
  accel/ethosu: propagate SRAM initialization errors
  accel/ethosu: fix job completion fence cleanup

 drivers/accel/ethosu/ethosu_drv.c | 52 ++++++++++++++++++++++++++-----
 drivers/accel/ethosu/ethosu_job.c | 25 +++++++++------
 2 files changed, 60 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] accel/ethosu: check DMA mask setup

[GuoHan Zhao]

dma_set_mask_and_coherent() can fail when platform DMA operations do not
support the requested 40-bit address mask. Ignoring the error leaves the
previous DMA configuration in place. Probe then continues without ensuring
that DMA addresses are suitable for the NPU.

Return the error so the driver does not bind with an unsupported DMA
configuration.

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>
---
New in v2.

Sashiko report:

> This is a pre-existing issue, but does this code ignore the return value of
> dma_set_mask_and_coherent() which is called earlier in ethosu_probe()?
>
> If the platform only supports 32-bit DMA and the call fails, the default
> 32-bit mask is retained. The DRM core and driver might proceed assuming
> 40-bit DMA is active, handing the NPU physical addresses greater than 4 GB,
> which could lead to silent DMA address truncation and memory corruption.

 drivers/accel/ethosu/ethosu_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index ed9c748a54ad..f3920185389d 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -339,7 +339,9 @@ static int ethosu_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, ethosudev);
 
-	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
+	if (ret)
+		return ret;
 
 	ethosudev->regs = devm_platform_ioremap_resource(pdev, 0);
 
-- 
2.43.0

Re: [PATCH v2 1/5] accel/ethosu: check DMA mask setup

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Validation mismatch allows jobs to run with stale NPU region pointers (UAF / OOB DMA).
- [High] Missing IS_ERR check for devm_platform_ioremap_resource() in ethosu_probe().
- [High] Ignored ethosu_sram_init() failure leaves stale sram_size, causing the NPU to DMA to physical address 0x0.
- [High] ethosu_probe error paths leak DRM scheduler, PM runtime references, and SRAM, leading to a UAF.
- [High] Calling drm_sched_fini on a failed drm_sched_init causes a crash.
- [High] WARN_ON(ret) in ethosu_job_open evaluates to 1 on error, bypassing VFS error checks and causing NULL pointer dereferences later.
- [High] The job's done_fence is leaked on all error paths in ethosu_ioctl_submit_job.
- [High] ethosu_job_run leaves done_fence uninitialized if the job is aborted, causing a refcount underflow later.
- [High] Hardirq handler reads unclocked MMIO registers when IRQ fires early or during runtime suspend.
- [High] Out-of-bounds array read in ethosu_job_timedout using unvalidated hardware register value.
- [High] Potential AB-BA deadlock due to calling pm_runtime_resume_and_get while holding dma_resv locks.
- [High] Uninitialized SRAM memory disclosure via runtime PM race.
--

--- Patch [1]: [PATCH v2 1/5] accel/ethosu: check DMA mask setup ---
commit ea4ab7f2e2a6b6cb0faa43a971f991179714158a
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>
Subject: accel/ethosu: check DMA mask setup

This patch checks the return value of dma_set_mask_and_coherent() to avoid
proceeding with an unsupported DMA configuration. If it fails, the probe
function now properly returns the error.

Link: https://sashiko.dev/#/patchset/20260716082530.1023538-1-zhaoguohan@kylinos.cn?part=1

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index ed9c748a54ad5..f3920185389de 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -339,7 +339,9 @@ static int ethosu_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, ethosudev);
>  
> -	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
> +	if (ret)
> +		return ret;
>  
>  	ethosudev->regs = devm_platform_ioremap_resource(pdev, 0);

[Severity: High]
This is a pre-existing issue, but does this code check the return value of
devm_platform_ioremap_resource()?

If devm_platform_ioremap_resource() fails, it returns an ERR_PTR. Because this
is not checked, ethosu_init() subsequently calls ethosu_device_resume() which
calls ethosu_reset(), passing the error pointer to writel_relaxed(), resulting
in a fatal page fault:

    ethosu_reset()
        writel_relaxed(RESET_PENDING_CSL, ethosudev->regs + NPU_REG_RESET);

Could we add an IS_ERR check here?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
ethosu_probe() clean up the DRM scheduler properly?

If ethosu_probe() fails after ethosu_job_init() (which initializes the
scheduler), it seems to return directly without calling ethosu_job_fini():

    ret = ethosu_init(ethosudev);
    if (ret)
        return ret;

Since ethosudev is devres-managed, it is freed upon probe failure, but the
active scheduler kthreads might remain running and attempt to access the
freed memory.


While reviewing this driver, I also noticed a few other pre-existing issues
outside of this patch's context:

[Severity: Critical]
This isn't a bug introduced by this patch, but does the validation logic
in ethosu_ioctl_submit_job() incorrectly permit jobs with sram_size = 0?

    ethosu_ioctl_submit_job()
        if (i == ETHOSU_SRAM_REGION) {
            if (cmd_info->region_size[i] <= edev->npu_info.sram_size)
                continue;

If a user sets job->sram_size to 0, validation passes. However,
ethosu_job_hw_submit() uses job->sram_size to conditionally update the base:

    ethosu_job_hw_submit()
        if (job->sram_size) {
            writel_relaxed(lower_32_bits(dev->sramphys), ...);
        }

Since it is 0, the base pointer update is skipped. Can this allow the NPU
to execute the command stream using a stale pointer left over from a
previous job, resulting in out-of-bounds DMA?

[Severity: High]
This is a pre-existing issue, but does ethosu_init() ignore the return value
of ethosu_sram_init()?

If ethosu_sram_init() fails, ethosudev->sramphys remains 0, but
ethosudev->npu_info.sram_size was already populated. Since ethosu_init()
doesn't check the return value, it permits the driver to proceed:

    ethosu_init()
        ethosu_sram_init(ethosudev);
        dev_info(ethosudev->base.dev, ...);

If userspace submits a job requesting SRAM, ethosu_job_hw_submit() programs
the NPU using dev->sramphys:

    ethosu_job_hw_submit()
        writel_relaxed(lower_32_bits(dev->sramphys),
                       dev->regs + NPU_REG_BASEP(ETHOSU_SRAM_REGION));

Can this cause the hardware to DMA to physical address 0x0?

[Severity: High]
This is a pre-existing issue, but can the error path in ethosu_job_init()
cause a crash if drm_sched_init() fails?

    ethosu_job_init()
        ret = drm_sched_init(&edev->sched, ...);
        if (ret)
            goto err_sched;
        ...
    err_sched:
        drm_sched_fini(&edev->sched);
        return ret;

If drm_sched_init() fails, it cleans up internally. Does calling
drm_sched_fini() on it again attempt to stop non-existent workqueues or
kthreads, resulting in a NULL pointer dereference?

[Severity: High]
This isn't a bug introduced by this patch, but does ethosu_job_open() return
a positive value on error?

    ethosu_job_open()
        ret = drm_sched_entity_init(...);
        if (ret)
            return WARN_ON(ret);

WARN_ON() evaluates to 1. When ethosu_open() propagates this to VFS, it might
bypass the VFS error checks since 1 is treated as success:

    ethosu_open()
        ret = ethosu_job_open(priv);
        if (ret)
            goto err_put_mod;

Could this bypass assigning file->driver_priv, leading to a NULL pointer
dereference later when ethosu_postclose() passes it to ethosu_job_close()?

[Severity: High]
This is a pre-existing issue, but is the done_fence leaked on error paths in
ethosu_ioctl_submit_job()?

    ethosu_ioctl_submit_job()
        ejob->done_fence = kzalloc_obj(*ejob->done_fence);

If the submission fails later, ethosu_job_err_cleanup() is called, but it
seems to omit freeing the done_fence:

    ethosu_job_err_cleanup()
        ...
        drm_gem_object_put(job->cmd_bo);
        kfree(job);

Can an unprivileged user trigger this leak repeatedly by passing invalid
parameters?

[Severity: High]
This is a pre-existing issue, but can ethosu_job_run() skip initializing the
fence, leading to a refcount underflow?

    ethosu_job_run()
        if (unlikely(job->base.s_fence->finished.error))
            return NULL;

        dma_fence_init(fence, &ethosu_fence_ops, &dev->fence_lock, ...);

If it returns early, job->done_fence remains zero-initialized. Later,
ethosu_job_cleanup() unconditionally calls dma_fence_put(job->done_fence),
which could attempt to decrement a 0-refcount object.

[Severity: High]
This isn't a bug introduced by this patch, but is the IRQ requested before the
device clocks are turned on?

    ethosu_job_init()
        ret = devm_request_threaded_irq(dev, edev->irq,
                                        ethosu_job_irq_handler, ...);

If the device shares this IRQ, and the IRQ fires before ethosu_init() enables
the clocks, ethosu_job_irq_handler() will immediately access MMIO:

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

Can accessing unclocked MMIO registers cause a fatal bus lockup?

[Severity: High]
This is a pre-existing issue, but does ethosu_job_timedout() use an
unvalidated hardware register value as an array index?

    ethosu_job_timedout()
        cmdaddr = readl_relaxed(dev->regs + NPU_REG_QREAD);
        ...
        dev_err(dev->base.dev, "...", cmdaddr, bocmds[cmdaddr / 4]);

If a malicious command stream causes a timeout with an out-of-bounds address
in QREAD, could this perform an out-of-bounds kernel memory read?

[Severity: High]
This is a pre-existing issue, but can ethosu_job_push() cause an AB-BA
deadlock by calling pm_runtime_resume_and_get() while holding dma_resv locks?

    ethosu_job_push()
        ret = drm_gem_lock_reservations(job->region_bo, ...);
        if (ret)
            return ret;
        ...
        ret = pm_runtime_resume_and_get(job->dev->base.dev);

If memory reclaim runs and attempts to shrink shared BOs, the shrinker will
try to acquire the same dma_resv locks. Since pm_runtime can block on
allocations or power domains, could this invert lock ordering?

[Severity: High]
This is a pre-existing issue, but does ethosu_init() expose uninitialized
SRAM via a runtime PM race?

    ethosu_init()
        ret = ethosu_device_resume(ethosudev->base.dev);
        ...
        ethosu_sram_init(ethosudev);

Because ethosu_device_resume() is called before ethosu_sram_init() allocates
the SRAM, the memset_io() zeroing logic is skipped. The device is then marked
active.

If a job is submitted immediately, pm_runtime_resume_and_get() might skip
ethosu_device_resume(), allowing the job to run with uncleared SRAM memory.
Could this leak sensitive data to userspace?

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

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

[GuoHan Zhao]

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>
---
New in v2.

Sashiko reports:

> This is a pre-existing issue, but does the error handling path for
> drm_sched_init() cause a panic if it fails?
>
> drivers/accel/ethosu/ethosu_job.c:ethosu_job_init() {
>     ...
>     ret = drm_sched_init(&edev->sched, &args);
>     if (ret) {
>         dev_err(dev, "Failed to create scheduler: %d\n", ret);
>         goto err_sched;
>     }
>     return 0;
>
> err_sched:
>     drm_sched_fini(&edev->sched);
>     return ret;
> }
>
> If drm_sched_init() fails, it cleans up its own state. The error path
> then branches to err_sched and calls drm_sched_fini() on the partially
> initialized scheduler.
>
> Because the waitqueue head was either destroyed or never initialized,
> its list pointers are NULL. Will drm_sched_fini() invoking
> wake_up_all(&sched->job_scheduled) cause an immediate crash?

> This is a pre-existing issue, but can the devm-managed shared IRQ handler
> outlive the DRM scheduler here?
>
> The IRQ is requested using devm_request_threaded_irq(). During
> ethosu_remove() (and on probe failure), the driver explicitly calls
> ethosu_job_fini(), which destroys the DRM scheduler.
>
> Because the IRQ is managed by devm, it remains fully active until the
> remove function completes. If an interrupt fires in this window, could
> ethosu_job_irq_handler() execute and access the destroyed dev->sched or
> dev->in_flight_job structures, leading to a use-after-free?

 drivers/accel/ethosu/ethosu_job.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index b76924645aaa..5847433e87e7 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,
 					ethosu_job_irq_handler,
 					ethosu_job_irq_handler_thread,
@@ -322,14 +330,6 @@ int ethosu_job_init(struct ethosu_device *edev)
 					edev);
 	if (ret) {
 		dev_err(dev, "failed to request irq\n");
-		return ret;
-	}
-
-	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);
 		goto err_sched;
 	}
 
@@ -342,6 +342,7 @@ int ethosu_job_init(struct ethosu_device *edev)
 
 void ethosu_job_fini(struct ethosu_device *dev)
 {
+	devm_free_irq(dev->base.dev, dev->irq, dev);
 	drm_sched_fini(&dev->sched);
 }
 
-- 
2.43.0

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

[sashiko-bot]

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

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

[GuoHan Zhao]

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>
---
Changes in v2:
- Apply after the scheduler and IRQ lifetime fix so probe cleanup releases
  the IRQ before destroying the scheduler.
- Disable clocks directly when runtime PM is unavailable.

Sashiko report:

> This is a pre-existing issue, but was not introduced by this patch.
>
> If ethosu_init() or drm_dev_register() fail, the function returns
> directly without calling ethosu_job_fini() or freeing the sram pool.
>
> Because the job scheduler is embedded in the devm-allocated
> ethosu_device, it will be freed when devm cleans up. However, the drm
> scheduler kthreads and timers remain active and will access the freed
> memory.
>
> Can this cause a use-after-free regression leading to a kernel panic?

 drivers/accel/ethosu/ethosu_drv.c | 41 ++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index f3920185389d..6b97d22444a7 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)
 	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)
 {
 	ethosudev->npu_info.sram_size = 0;
@@ -293,6 +304,14 @@ static int ethosu_sram_init(struct ethosu_device *ethosudev)
 	return 0;
 }
 
+static void ethosu_sram_fini(struct ethosu_device *ethosudev)
+{
+	if (ethosudev->sram)
+		gen_pool_free(ethosudev->srampool,
+			      (unsigned long)ethosudev->sram,
+			      ethosudev->npu_info.sram_size);
+}
+
 static int ethosu_init(struct ethosu_device *ethosudev)
 {
 	int ret;
@@ -306,7 +325,7 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 	pm_runtime_use_autosuspend(ethosudev->base.dev);
 	ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
 	if (ret)
-		return ret;
+		goto err_suspend;
 	pm_runtime_get_noresume(ethosudev->base.dev);
 
 	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);
@@ -326,6 +345,11 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 		 ethosudev->npu_info.sram_size / 1024);
 
 	return 0;
+
+err_suspend:
+	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
+	ethosu_device_suspend(ethosudev->base.dev);
+	return ret;
 }
 
 static int ethosu_probe(struct platform_device *pdev)
@@ -355,13 +379,20 @@ static int ethosu_probe(struct platform_device *pdev)
 
 	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);
-	if (ethosudev->sram)
-		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
-			      ethosudev->npu_info.sram_size);
+	ethosu_sram_fini(ethosudev);
 }
 
 static const struct of_device_id dt_match[] = {
-- 
2.43.0

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

[sashiko-bot]

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

[PATCH v2 4/5] accel/ethosu: propagate SRAM initialization errors

[GuoHan Zhao]

ethosu_sram_init() returns -ENOMEM if allocation from a declared SRAM
pool fails. ethosu_init() ignores the error after recording the pool size.
Probe then registers a device with a nonzero SRAM size and a zero sramphys
address, allowing jobs to program address zero for SRAM.

Propagate the error and synchronously suspend the device before the probe
path releases the scheduler.

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>
---
Changes in v2:
- Renumber from patch 2/3 to 4/5.
- Use the common runtime suspend helper when SRAM setup fails.

Sashiko report:

> This is a pre-existing issue, but was not introduced by this patch.
>
> In ethosu_sram_init(), if gen_pool_dma_alloc() fails, it returns an
> error. However, ethosu_init() ignores this return value.
>
> Because npu_info.sram_size was already set to gen_pool_size(), it remains
> non-zero while sramphys remains 0.
>
> When a user submits a job requesting sram, the size check passes, and
> ethosu_job_hw_submit() configures the hardware to use physical sram
> address 0x0.
>
> Could this result in a memory corruption regression or an IOMMU fault?

 drivers/accel/ethosu/ethosu_drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
index 6b97d22444a7..c825b67e9cba 100644
--- a/drivers/accel/ethosu/ethosu_drv.c
+++ b/drivers/accel/ethosu/ethosu_drv.c
@@ -331,7 +331,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 	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);
+	ret = ethosu_sram_init(ethosudev);
+	if (ret)
+		goto err_runtime_suspend;
 
 	dev_info(ethosudev->base.dev,
 		 "Ethos-U NPU, arch v%ld.%ld.%ld, rev r%ldp%ld, cmd stream ver%ld, %d MACs, %dKB SRAM\n",
@@ -346,6 +348,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
 
 	return 0;
 
+err_runtime_suspend:
+	ethosu_runtime_suspend(ethosudev);
+	return ret;
 err_suspend:
 	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
 	ethosu_device_suspend(ethosudev->base.dev);
-- 
2.43.0

Re: [PATCH v2 4/5] accel/ethosu: propagate SRAM initialization errors

[sashiko-bot]

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

Pre-existing issues:
- [High] Unsafe hardware access in shared IRQ handler without Runtime PM check.
- [High] Missing synchronize_irq() in runtime suspend callback.
- [High] SRAM is allocated but left uninitialized during probe, leading to potential use of uninitialized memory.
--

--- Patch [4]: [PATCH v2 4/5] accel/ethosu: propagate SRAM initialization errors ---
commit 4382c986c5c63a85ef8db5d2db6b39f2605d7154
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>

accel/ethosu: propagate SRAM initialization errors

ethosu_sram_init() returns -ENOMEM if allocation from a declared SRAM
pool fails. ethosu_init() ignores the error after recording the pool size.
Probe then registers a device with a nonzero SRAM size and a zero sramphys
address, allowing jobs to program address zero for SRAM.

Propagate the error and synchronously suspend the device before the probe
path releases the scheduler.

Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 6b97d22444a7c..c825b67e9cba0 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -331,7 +331,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  	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);
> +	ret = ethosu_sram_init(ethosudev);
> +	if (ret)
> +		goto err_runtime_suspend;

[Severity: High]
This is a pre-existing issue, but is the SRAM left uninitialized during the
first active window after probe?

Earlier in ethosu_init(), we call ethosu_device_resume(), which attempts to
zero the SRAM via ethosu_reset(). However, since ethosu_sram_init() hasn't
run yet, SRAM isn't allocated and zeroing is skipped.

If a job is submitted within the 50ms autosuspend delay window after probe
finishes, pm_runtime_resume_and_get() will increment the usage counter but
skip ethosu_device_resume() since the device is already active.

Does this mean the first job could run with uninitialized garbage data in
the SRAM, leading to unpredictable hardware behavior?

>  
>  	dev_info(ethosudev->base.dev,
>  		 "Ethos-U NPU, arch v%ld.%ld.%ld, rev r%ldp%ld, cmd stream ver%ld, %d MACs, %dKB SRAM\n",
[ ... ]
> @@ -346,6 +348,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  
>  	return 0;
>  
> +err_runtime_suspend:
> +	ethosu_runtime_suspend(ethosudev);
> +	return ret;
>  err_suspend:
>  	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
>  	ethosu_device_suspend(ethosudev->base.dev);

[Severity: High]
This is a pre-existing issue, but does ethosu_device_suspend() need to call
synchronize_irq() before disabling the clocks?

Looking at ethosu_device_suspend(), it calls clk_bulk_disable_unprepare()
directly without synchronizing with the interrupt handler:

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;
}

If the autosuspend timer expires and ethosu_device_suspend() executes while
an interrupt is mid-flight on another CPU, could the clocks be disabled
while the IRQ handler is actively accessing hardware registers, resulting in
a bus fault?

[Severity: High]
This is a pre-existing issue, but is it safe for ethosu_job_irq_handler()
to access hardware registers without checking if the device is active?

When the device enters runtime suspend and its clocks are disabled, a
different device sharing the same IRQ line could trigger an interrupt.

The ethosu_job_irq_handler() unconditionally reads NPU_REG_STATUS:

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

Without calling pm_runtime_get_if_active() to check if the device is active
first, could this lead to a bus fault and kernel panic if the device is
suspended and unclocked?

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

[PATCH v2 5/5] accel/ethosu: fix job completion fence cleanup

[GuoHan Zhao]

ethosu_ioctl_submit_job() allocates done_fence before validating buffer
handles. Errors after allocation call ethosu_job_err_cleanup(), which frees
the job but leaks the uninitialized fence.

A scheduler dependency error also lets ethosu_job_run() return before
dma_fence_init(). Normal cleanup then passes a zeroed refcount to
dma_fence_put().

Release done_fence in the common cleanup path and use
dma_fence_was_initialized() to distinguish initialized fences from raw
allocations.

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>
---
Changes in v2:
- Renumber from patch 3/3 to 5/5; no code changes.

Sashiko reports:

> 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);
> }
>
> This is a pre-existing issue, but was not introduced by this patch.
>
> ethosu_ioctl_submit_job() allocates done_fence. If a subsequent
> validation fails, the error path calls ethosu_job_err_cleanup().
>
> This function frees the job structure but misses freeing job->done_fence.
>
> Can an unprivileged user repeatedly call the submit ioctl with invalid
> handles to trigger a done_fence leak?

> drivers/accel/ethosu/ethosu_job.c:ethosu_job_run() {
>     ...
>         if (unlikely(job->base.s_fence->finished.error))
>                 return NULL;
>
>         dma_fence_init(fence, &ethosu_fence_ops, &dev->fence_lock,
>                        dev->fence_context, ++dev->emit_seqno);
>     ...
> }
>
> This is a pre-existing issue, but was not introduced by this patch.
>
> If job->base.s_fence->finished.error is true, the function returns early
> without calling dma_fence_init() on job->done_fence.
>
> When the job is later destroyed, ethosu_job_cleanup() unconditionally
> calls dma_fence_put() on the fence:
>
> drivers/accel/ethosu/ethosu_job.c:ethosu_job_cleanup() {
>     ...
>         dma_fence_put(job->done_fence);
>     ...
> }
>
> Since the fence is zero-initialized, will calling kref_put() on it
> violate the API and cause a refcount underflow?

 drivers/accel/ethosu/ethosu_job.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
index 5847433e87e7..b68a905d22f8 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -152,6 +152,13 @@ static void ethosu_job_err_cleanup(struct ethosu_job *job)
 
 	drm_gem_object_put(job->cmd_bo);
 
+	if (job->done_fence) {
+		if (dma_fence_was_initialized(job->done_fence))
+			dma_fence_put(job->done_fence);
+		else
+			dma_fence_free(job->done_fence);
+	}
+
 	kfree(job);
 }
 
@@ -162,7 +169,6 @@ static void ethosu_job_cleanup(struct kref *ref)
 
 	pm_runtime_put_autosuspend(job->dev->base.dev);
 
-	dma_fence_put(job->done_fence);
 	dma_fence_put(job->inference_done_fence);
 
 	ethosu_job_err_cleanup(job);
-- 
2.43.0

Re: [PATCH v2 5/5] accel/ethosu: fix job completion fence cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling drm_sched_job_cleanup() on an uninitialized struct drm_sched_job causes a NULL pointer dereference in the ethosu_ioctl_submit_job() error path.
- [High] The shared IRQ handler accesses hardware registers without ensuring the device is active via Runtime PM, and the suspend callback fails to synchronize IRQs, risking fatal bus faults.
--

--- Patch [5]: [PATCH v2 5/5] accel/ethosu: fix job completion fence cleanup ---
commit b390c1806154d7460d3ff212d56c250283785f11
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>
Subject: accel/ethosu: fix job completion fence cleanup

This commit addresses memory leaks and refcount underflows in the Ethos-U NPU
driver's job cleanup path. It ensures that the done_fence is properly released
during error cleanup and uses dma_fence_was_initialized() to handle raw
allocations gracefully.

Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index 5847433e87e7e..b68a905d22f83 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -152,6 +152,13 @@ static void ethosu_job_err_cleanup(struct ethosu_job *job)
>  
>  	drm_gem_object_put(job->cmd_bo);
>  
> +	if (job->done_fence) {
> +		if (dma_fence_was_initialized(job->done_fence))
> +			dma_fence_put(job->done_fence);
> +		else
> +			dma_fence_free(job->done_fence);
> +	}
> +
>  	kfree(job);
>  }
>  

[Severity: High]
This is a pre-existing issue, but was not introduced by this patch.

When the done_fence allocation fails in ethosu_ioctl_submit_job(), it jumps
to out_cleanup_job:

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

Because drm_sched_job_init() was not called yet, ejob->base is completely
zero-initialized, meaning ejob->base.s_fence is NULL.

When drm_sched_job_cleanup() attempts to read the refcount:

drivers/gpu/drm/scheduler/sched_main.c:drm_sched_job_cleanup() {
    ...
	if (kref_read(&job->s_fence->finished.refcount)) {
    ...
}

Will this result in a NULL pointer dereference because the function assumes
job->s_fence is a valid pointer?


[Severity: High]
This isn't a bug introduced by this patch, but there appears to be an issue
with how shared interrupts interact with runtime PM.

The NPU interrupt is registered with the IRQF_SHARED flag:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_init() {
    ...
	ret = devm_request_threaded_irq(dev, edev->irq,
				ethosu_job_irq_handler,
				ethosu_job_irq_handler_thread,
				IRQF_SHARED, KBUILD_MODNAME,
				edev);
    ...
}

If another device sharing the IRQ line triggers an interrupt while the NPU is
runtime-suspended, the handler executes and reads MMIO registers:

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

	if (!(status & STATUS_IRQ_RAISED))
		return IRQ_NONE;
    ...
}

Could this unconditionally read registers without verifying the device's PM
state via pm_runtime_get_if_active()?

Furthermore, the runtime suspend callback disables clocks:

drivers/accel/ethosu/ethosu_drv.c:ethosu_device_suspend() {
    ...
	clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks);
	return 0;
}

Since this doesn't call synchronize_irq() to wait for executing handlers,
could concurrent IRQ execution cause invalid hardware writes or a bus fault
when accessing powered-off MMIO registers?

> @@ -162,7 +169,6 @@ static void ethosu_job_cleanup(struct kref *ref)
>  
>  	pm_runtime_put_autosuspend(job->dev->base.dev);
>  
> -	dma_fence_put(job->done_fence);
>  	dma_fence_put(job->inference_done_fence);
>  
>  	ethosu_job_err_cleanup(job);

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