* Re: [PATCH] accel/ethosu: check MMIO mapping errors in probe
2026-07-16 6:52 [PATCH] accel/ethosu: check MMIO mapping errors in probe zhaoguohan
@ 2026-07-16 7:04 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-16 7:04 UTC (permalink / raw)
To: zhaoguohan; +Cc: dri-devel
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, ðosudev->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(ðosudev->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, ðosu_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
^ permalink raw reply [flat|nested] 2+ messages in thread