Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing hardware stop and wait for active jobs in the device removal path leads to DMA Use-After-Free. - [High] The driver registers a shared interrupt handler but fails to check the device PM status before accessing hardware, and its suspend callback lacks IRQ synchronization. - [High] Prematurely calling `drm_sched_fini()` during device removal leads to a Use-After-Free when userspace closes open DRM file entities. - [High] Missing `drm_dev_enter()` and `drm_dev_exit()` protection in `ethosu_ioctl_submit` allows a TOCTOU race with device removal. - [High] Calling `drm_sched_fini()` on `drm_sched_init()` failure causes a crash on uninitialized work items. - [Medium] The device is not synchronously suspended during driver unbind/removal, leaving its hardware clocks permanently enabled. -- --- Patch [2]: [PATCH 2/3] accel/ethosu: propagate SRAM initialization errors --- commit d63b5342071ce711d48106e8b0fded82dbcf48e8 Author: GuoHan Zhao <[email protected]> accel/ethosu: propagate SRAM initialization errors This patch fixes a bug where ethosu_sram_init() errors were ignored in ethosu_init(), leading to jobs programming physical address zero for SRAM. It adds proper error propagation and synchronous suspension in the probe path. 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 d1153b15ca3ee..479156f79bc18 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -320,7 +320,9 @@ static int ethosu_init(struct ethosu_device *ethosudev) > 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); > + ret = ethosu_sram_init(ethosudev); > + if (ret) > + goto err_runtime_suspend; > > dev_info(ethosudev->base.dev, [ ... ] > @@ -389,7 +391,7 @@ static void ethosu_remove(struct platform_device *pdev) > { > struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev); > > drm_dev_unregister(ðosudev->base); > ethosu_job_fini(ethosudev); [Severity: Medium] This is a pre-existing issue, but does this removal path lack a synchronous suspend of the device? Since devres cleanup will eventually call pm_runtime_disable() which clears the state but leaves hardware powered on if it was active, will failing to call pm_runtime_put_sync_suspend() here lead to the clocks and power domains leaking and remaining active indefinitely? [Severity: High] This isn't a bug introduced by this patch, but doesn't ethosu_job_fini() destroy the DRM scheduler via drm_sched_fini() prematurely? If userspace still holds a DRM file descriptor open after the device is unbound, closing the file descriptor will call ethosu_job_close() which invokes drm_sched_entity_destroy() on the already finalized scheduler. Can this cause a use-after-free accessing destroyed DRM scheduler structures? > ethosu_sram_fini(ethosudev); > } [Severity: Critical] This is a pre-existing issue, but is the SRAM freed while the hardware might still be active? If ethosu_remove() is executed while the NPU is actively executing a job, ethosu_sram_fini() will immediately free the SRAM pool via gen_pool_free(). Since there is no halt of the hardware or wait for the in-flight job to finish, could the hardware continue performing DMA read/writes to the freed SRAM pool, leading to a DMA use-after-free? > diff --git a/drivers/accel/ethosu/ethosu_job.c > b/drivers/accel/ethosu/ethosu_job.c > --- a/drivers/accel/ethosu/ethosu_job.c > +++ b/drivers/accel/ethosu/ethosu_job.c > @@ -230,8 +230,8 @@ static irqreturn_t ethosu_job_irq_handler(int irq, void > *data) > { > struct ethosu_device *dev = data; > u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); [Severity: High] This is a pre-existing issue, but this interrupt is requested with IRQF_SHARED in ethosu_job_init(). If the device enters autosuspend and turns off its clocks, could this unconditionally read NPU_REG_STATUS and cause a synchronous external abort or spurious interrupt logic? Should ethosu_job_irq_handler() use pm_runtime_get_if_active() before checking the hardware registers? Additionally, shouldn't ethosu_device_suspend() call synchronize_irq() to ensure no handler is mid-flight before disabling clocks? [ ... ] > @@ -337,9 +337,7 @@ int ethosu_job_init(struct ethosu_device *edev) > ret = drm_sched_init(&edev->sched, &args); > if (ret) { > dev_err(dev, "Failed to create scheduler: %d\n", ret); > goto err_sched; > } > > return 0; > > err_sched: > drm_sched_fini(&edev->sched); [Severity: High] This isn't a bug introduced by this patch, but should drm_sched_fini() be called if drm_sched_init() fails? If drm_sched_init() cleans up its internal resources upon returning an error, would unconditionally calling drm_sched_fini() here attempt to destroy workqueues and free resources that were never successfully initialized, potentially leading to a panic? [ ... ] > @@ -485,9 +483,8 @@ int ethosu_ioctl_submit(struct drm_device *dev, void > *data, struct drm_file *file > { > struct drm_ethosu_submit *args = data; > int ret = 0; [Severity: High] This is a pre-existing issue, but doesn't this ioctl lack drm_dev_enter() and drm_dev_exit() unplug protection? If userspace aggressively calls the DRM_ETHOSU_SUBMIT ioctl while the device is being unbound, can it race with ethosu_remove()? Without drm_dev_enter(), this could push a job onto the scheduler via drm_sched_entity_push_job() while the removal thread is simultaneously tearing it down with drm_sched_fini(), causing a kernel panic. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
