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