Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [High] The IRQ handler accesses hardware registers unconditionally without verifying if the device is active via pm_runtime_get_if_active(), risking a system crash on spurious interrupts. - [High] DRM scheduler is finalized before its file-bound entities are destroyed, resulting in a Use-After-Free. - [High] NPU SRAM is not cleared between consecutive jobs from different processes, leading to data leakage. - [High] Missing upper bound check against UAPI array size leads to out-of-bounds read/write in `ethosu_ioctl_perfmon_create`. - [High] Unvalidated hardware register `QREAD` used as an array index causes out-of-bounds memory read in `ethosu_job_timedout`. -- commit 9bfcf97ff7178db9d34db7f9533d5df371a4f57c Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Disable clocks on PM setup failure ethosu_init() enables the clocks directly to reset and query the NPU before runtime PM takes over. If runtime-PM setup fails, the error path returns while those clocks remain enabled. Disable the clocks before returning the setup error to balance the initial direct resume. > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index 2c8d735767b2a..9d063daa1aa7d 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -311,8 +311,10 @@ static int ethosu_init(struct ethosu_device *ethosudev) > pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50); > pm_runtime_use_autosuspend(ethosudev->base.dev); > ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev); > - if (ret) > + if (ret) { > + ethosu_device_suspend(ethosudev->base.dev); > return ret; > + } > pm_runtime_get_noresume(ethosudev->base.dev); > > ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + > NPU_REG_ID); > drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() > { > struct ethosu_device *dev = data; > u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS); > > if (!(status & STATUS_IRQ_RAISED)) > return IRQ_NONE; [Severity: High] This is a pre-existing issue, but does this IRQ handler need to verify if the device is active? If a spurious interrupt occurs while the device is in the RPM_SUSPENDED state with clocks disabled, will this unclocked register access result in a synchronous external abort and a kernel panic? Should this check pm_runtime_get_if_active() before accessing the register? > drivers/accel/ethosu/ethosu_job.c:ethosu_job_fini() > { > drm_sched_fini(&dev->sched); > } > > [ ... ] > > drivers/accel/ethosu/ethosu_job.c:ethosu_job_close() > { > struct drm_sched_entity *entity = ðosu_priv->sched_entity; > > drm_sched_entity_destroy(entity); > } [Severity: High] This is a pre-existing issue, but does this lead to a use-after-free when closing file descriptors? When the device is unbound, ethosu_remove() tears down the scheduler. However, if a userspace process holds a file descriptor open, its sched_entity remains active. When the file descriptor is later closed, ethosu_job_close() calls drm_sched_entity_destroy(). Will drm_sched_entity_destroy() attempt to access the already destroyed and freed scheduler via entity->rq->sched, causing a panic? > 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)); [Severity: High] This is a pre-existing issue, but is there a data leakage risk here? The NPU SRAM is shared globally across jobs. It appears it is only cleared during ethosu_device_reset() when runtime PM resumes. Because the driver uses a 50ms autosuspend delay, consecutive jobs submitted within 50ms do not trigger a suspend/resume cycle and the SRAM is not cleared. Can a malicious process submit a job immediately after another process's job and craft a command stream to DMA the contents of the ETHOSU_SRAM_REGION to its own memory, exfiltrating the previous process's weights or intermediate activations? > drivers/accel/ethosu/ethosu_perfmon.c:ethosu_ioctl_perfmon_create() > { > /* Number of monitored counters cannot exceed HW limits. */ > if (req->ncounters > ethosu->npu_info.pmu_counters) > return -EINVAL; > > [ ... ] > > perfmon = kzalloc_flex(*perfmon, values, req->ncounters); > if (!perfmon) > return -ENOMEM; > > for (i = 0; i < req->ncounters - 1; i++) > perfmon->counters[i] = req->counters[i]; [Severity: High] This is a pre-existing issue, but does this lack an upper bound check on the UAPI array size? The function validates req->ncounters against the hardware-reported count rather than the static UAPI array size limit (DRM_ETHOSU_MAX_PERF_EVENT_COUNTERS, which is 8). If the hardware reports more than 8 counters, and userspace passes req->ncounters > 8, will this loop read past the end of the req->counters array and write past the end of the perfmon->counters array, corrupting the dynamically allocated values array? > drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() > { > [ ... ] > > cmdaddr = readl_relaxed(dev->regs + NPU_REG_QREAD); > > [ ... ] > > dev_err(dev->base.dev, "NPU sched timed out: NPU %s, cmdstream offset > 0x%x: 0x%x\n", > running ? "running" : "stopped", > cmdaddr, bocmds[cmdaddr / 4]); [Severity: High] This is a pre-existing issue, but can this cause an out-of-bounds memory read? Since cmdaddr is read directly from the hardware and entirely unvalidated, if userspace crafts a malformed command stream that hangs the NPU and leaves QREAD with a large value, will cmdaddr / 4 exceed the bounds of the mapped bocmds buffer and cause an invalid memory access? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
