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 <[email protected]> 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/[email protected]?part=1
