Re: [PATCH v3 04/22] accel: ethosu: Disable clocks on PM setup failure

[sashiko-bot@kernel.org]

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) <robh@kernel.org>

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 = &ethosu_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/20260908-ethosu-fixes-v3-0-490fe215286f@kernel.org?part=4