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

[sashiko-bot@kernel.org]

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