Re: [PATCH] accel/ethosu: check MMIO mapping errors in probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing cleanup of job scheduler and SRAM pool on probe error paths causes a Use-After-Free.
- [Critical] Ignored return value from ethosu_sram_init() leads to potential memory corruption.
- [High] Registering a shared IRQ handler before enabling hardware clocks can cause a bus fault.
- [High] User-triggerable memory leak of done_fence in ethosu_ioctl_submit_job error paths.
- [High] Refcount underflow caused by calling dma_fence_put() on an uninitialized done_fence.
--

--- Patch [1]: [PATCH] accel/ethosu: check MMIO mapping errors in probe ---
commit 211e95fc7f28959e17e3b5a23f5f4f5aaf071bdd
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>
accel/ethosu: check MMIO mapping errors in probe

devm_platform_ioremap_resource() returns an error pointer when the register
resource cannot be mapped. ethosu_probe() stores it and continues until
initialization dereferences it through MMIO accessors. This patch returns
the mapping error before initializing the device.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index ed9c748a54ad5..b2901eb8a7a03 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -342,6 +342,8 @@ static int ethosu_probe(struct platform_device *pdev)
>  	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
>  
>  	ethosudev->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ethosudev->regs))
> +		return PTR_ERR(ethosudev->regs);
>  
>  	ethosudev->num_clks = devm_clk_bulk_get_all(&pdev->dev, &ethosudev->clks);
>  	if (ethosudev->num_clks < 0)

I noticed some additional pre-existing issues in this driver.

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

        ret = drm_dev_register(&ethosudev->base, 0);
        if (ret)
                pm_runtime_dont_use_autosuspend(ethosudev->base.dev);

        pm_runtime_put_autosuspend(ethosudev->base.dev);
        return ret;
}

[Severity: Critical]
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:ethosu_probe() {
    ...
        ret = ethosu_job_init(ethosudev);
        if (ret)
                return ret;

        ret = ethosu_init(ethosudev);
    ...
}

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

ethosu_job_init() registers the interrupt handler with IRQF_SHARED before
ethosu_init() resumes PM runtime and enables the clocks.

If another device on the same IRQ line triggers an interrupt in this window,
the handler will execute and read NPU_REG_STATUS:

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

Will reading from an unclocked NPU peripheral crash the system?

drivers/accel/ethosu/ethosu_drv.c:ethosu_init() {
    ...
        ethosu_sram_init(ethosudev);
    ...
}

[Severity: Critical]
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_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);
}

[Severity: High]
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);
    ...
}

[Severity: High]
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?

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