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 <[email protected]> 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: [email protected] Reported-by: Sashiko <[email protected]> Link: https://sashiko.dev/#/patchset/[email protected]?part=1 Signed-off-by: GuoHan Zhao <[email protected]> > 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, ðosudev->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(ðosudev->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/[email protected]?part=2
