Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [High] The shared interrupt handler accesses hardware registers without verifying if the device is active, risking a synchronous external abort. - [High] Runtime suspend disables hardware clocks without calling synchronize_irq() for the shared interrupt. - [High] Missing IS_ERR() check on the return value of devm_platform_ioremap_resource() causes an error pointer dereference. - [High] The shared IRQ is requested before the hardware is powered on, risking a crash if a shared interrupt fires early. - [High] Destroying the DRM scheduler in ethosu_remove() creates a Use-After-Free risk for open file descriptors. - [High] An untrusted hardware register value is used as an array index without bounds checking, potentially leaking kernel memory to the log. -- --- Patch [3]: [PATCH v2 3/5] accel/ethosu: clean up resources on probe failure --- commit 596a2469c9dadb88b1da94803fdbc80b578983ef Author: GuoHan Zhao <[email protected]> accel/ethosu: clean up resources on probe failure ethosu_job_init() creates a scheduler before ethosu_init() and drm_dev_register(). Errors from either later step return directly, leaving the scheduler workqueue allocated. A drm_dev_register() failure also leaves the optional SRAM allocation behind. Unwind runtime PM, SRAM, and scheduler state in reverse order on probe failure. Disable the clocks directly when runtime PM is unavailable or its setup fails. 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_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index f3920185389de..6b97d22444a7c 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -272,6 +272,17 @@ static int ethosu_device_suspend(struct device *dev) [Severity: High] This is a pre-existing issue, but ethosu_device_suspend() disables hardware clocks without calling synchronize_irq() for the shared interrupt: drivers/accel/ethosu/ethosu_drv.c:ethosu_device_suspend() { struct ethosu_device *ethosudev = dev_get_drvdata(dev); clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks); return 0; } Could this cause a synchronous external abort if the device enters runtime suspend while the interrupt handler is actively executing on another CPU? > return 0; > } > > +static void ethosu_runtime_suspend(struct ethosu_device *ethosudev) > +{ > + struct device *dev = ethosudev->base.dev; > + > + pm_runtime_dont_use_autosuspend(dev); > + if (pm_runtime_enabled(dev)) > + pm_runtime_put_sync_suspend(dev); > + else > + ethosu_device_suspend(dev); > +} > + > static int ethosu_sram_init(struct ethosu_device *ethosudev) [ ... ] > @@ -355,13 +379,20 @@ static int ethosu_probe(struct platform_device *pdev) [Severity: High] This is a pre-existing issue, but devm_platform_ioremap_resource() returns an ERR_PTR on failure, not NULL. In ethosu_probe(), its return value is assigned directly without an IS_ERR check: 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 the resource mapping fails, would a subsequent call to readl_relaxed() in ethosu_init() dereference the error pointer? [Severity: High] This is a pre-existing issue, but the shared IRQ is requested before the hardware is powered on in ethosu_probe(): drivers/accel/ethosu/ethosu_drv.c:ethosu_probe() { ... ret = ethosu_job_init(ethosudev); if (ret) return ret; ret = ethosu_init(ethosudev); ... } If another device on the same shared IRQ line triggers an interrupt between ethosu_job_init() and ethosu_init(), could the interrupt handler attempt to access MMIO registers while the clocks are still disabled, causing a crash? > > ret = ethosu_init(ethosudev); > if (ret) > - return ret; > + goto err_job_fini; > > ret = drm_dev_register(ðosudev->base, 0); > if (ret) > - pm_runtime_dont_use_autosuspend(ethosudev->base.dev); > + goto err_runtime_suspend; > > pm_runtime_put_autosuspend(ethosudev->base.dev); > + return 0; > + > +err_runtime_suspend: > + ethosu_runtime_suspend(ethosudev); > + ethosu_sram_fini(ethosudev); > +err_job_fini: > + ethosu_job_fini(ethosudev); > return ret; > } > > @@ -371,9 +402,7 @@ static void ethosu_remove(struct platform_device *pdev) > > drm_dev_unregister(ðosudev->base); > ethosu_job_fini(ethosudev); [Severity: High] This is a pre-existing issue, but destroying the DRM scheduler here in ethosu_remove() might create a use-after-free risk. drm_dev_unregister() hides the device node but doesn't revoke existing open file descriptors. If a userspace process still holds an open file descriptor when the driver is unbound, and subsequently submits an ioctl, will it access the destroyed scheduler kthreads and freed SRAM? > - if (ethosudev->sram) > - gen_pool_free(ethosudev->srampool, (unsigned > long)ethosudev->sram, > - ethosudev->npu_info.sram_size); > + ethosu_sram_fini(ethosudev); > } [Severity: High] This isn't a bug introduced by this patch, but I found a pre-existing issue in ethosu_job_irq_handler(). The shared interrupt handler accesses hardware registers without verifying if the device is active: drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() { ... u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); ... } Because the handler is registered with IRQF_SHARED, couldn't another device on the same IRQ line trigger an interrupt while the NPU is runtime-suspended? Accessing clock-gated MMIO here could trigger a synchronous external abort. [Severity: High] This is a pre-existing issue, but I found a potential out-of-bounds read in ethosu_job_timedout(). An untrusted hardware register value is used as an array index without bounds checking: drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() { ... cmdaddr, bocmds[cmdaddr / 4]); ... } If a malformed job forces a timeout and leaves a large offset in the NPU_REG_QREAD register, could cmdaddr exceed the bounds of the cmd_bo buffer and leak kernel memory to the log? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
