Re: [PATCH 05/11] accel: ethosu: Ensure SRAM region size matches job

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Kernel panic due to NULL pointer dereference in `drm_sched_job_cleanup` on uninitialized job.
- [High] Memory leak of `done_fence` on error paths controllable by userspace.
- [High] Lockless TOCTOU race in IRQ handler causing premature signaling of unrelated jobs.
--

commit ac51039957292dc327d760628e45a0de5289b27b
Author: Rob Herring (Arm) <robh@kernel.org>

    accel: ethosu: Ensure SRAM region size matches job
    
    It is possible for userspace to set the job SRAM size to 0, but then still
    have SRAM accesses in the command stream. When the job SRAM size is 0,
    setting the region base register is skipped and a stale base address from
    a prior job is used.
    
    Check the region size against the job's SRAM size instead of just the size
    of the SRAM. The job's SRAM size was already checked against the total SRAM
    size.

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index c33f2877b3850..0982722a9195a 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -441,13 +441,13 @@ static int ethosu_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
>  			if (!cmd_info->region_size[i])
>  				continue;
>  			if (i == ETHOSU_SRAM_REGION) {
> -				if (cmd_info->region_size[i] <= edev->npu_info.sram_size)
> +				if (cmd_info->region_size[i] <= ejob->sram_size)
>  					continue;
>  
>  				dev_err(dev->dev,
> -					"cmd stream region %d size greater than SRAM size (%llu > %u)\n",
> +					"cmd stream region %d size greater than job SRAM size (%llu > %u)\n",
>  					i, cmd_info->region_size[i],
> -					edev->npu_info.sram_size);
> +					ejob->sram_size);
>  				ret = -EINVAL;
>  				goto out_cleanup_job;

[Severity: High]
This isn't a bug introduced by this patch, but this newly modified error path
can trigger a memory leak. If the region size validation fails here, we jump
to out_cleanup_job. Does this leak the dynamically allocated
ejob->done_fence?

In ethosu_job_err_cleanup(), the job is freed:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() {
    ...
	drm_gem_object_put(job->cmd_bo);

	kfree(job);
}

But it doesn't appear to free job->done_fence, allowing unprivileged
userspace to trigger a memory leak by intentionally submitting jobs with
mismatched SRAM region sizes.

>  			}

[Severity: Critical]
This is a pre-existing issue, but there is a potential NULL pointer
dereference in the error handling path for this function.

If kzalloc_obj for ejob->done_fence fails earlier in
ethosu_ioctl_submit_job(), the code jumps to out_cleanup_job without
initializing the scheduler job:

drivers/accel/ethosu/ethosu_job.c:ethosu_ioctl_submit_job() {
    ...
	ejob->done_fence = kzalloc_obj(*ejob->done_fence);
	if (!ejob->done_fence) {
		ret = -ENOMEM;
		goto out_cleanup_job;
	}
    ...
out_cleanup_job:
	if (ret)
		drm_sched_job_cleanup(&ejob->base);
}

Because drm_sched_job_init() was never called, ejob->base.s_fence is NULL.
Will this cause a NULL pointer dereference when drm_sched_job_cleanup()
attempts to read the refcount?

drivers/gpu/drm/scheduler/sched_main.c:drm_sched_job_cleanup() {
    ...
	if (kref_read(&job->s_fence->finished.refcount)) {
    ...
}


[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a lockless
TOCTOU race in the IRQ handler that could cause premature signaling of
unrelated jobs.

In ethosu_job_handle_irq(), the in_flight_job is read and signaled locklessly:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_handle_irq() {
    ...
	job = READ_ONCE(dev->in_flight_job);
	if (job) {
		WRITE_ONCE(dev->in_flight_job, NULL);
		dma_fence_signal(job->done_fence);
	}
    ...
}

If the threaded IRQ handler is delayed and a hardware timeout occurs
concurrently, ethosu_job_timedout() clears in_flight_job and the scheduler
starts the next job.

When the delayed IRQ thread resumes, could it read the new in_flight_job,
clear it, and erroneously signal the new job's fence before it has finished
executing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-ethosu-fixes-v1-0-346f9ea8791c@kernel.org?part=5