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) <[email protected]> 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/[email protected]?part=5
