Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
Pre-existing issues: - [Critical] Unconditional call to rocket_iommu_domain_put() in the job cleanup path causes a NULL pointer dereference on early submission failures. - [Critical] Double free of rjob->tasks when rocket_copy_tasks() fails. - [High] Aborting a DRM scheduler job after calling drm_sched_job_arm() violates the DRM scheduler API contract and corrupts scheduler state. - [High] The dynamically allocated scheds array leaks in rocket_job_open() on initialization errors and permanently leaks on single-core devices. - [High] rocket_ioctl_submit() silently ignores errors returned by rocket_ioctl_submit_job(). - [High] Reference count leak of iommu_group in the hardware IRQ handler. - [High] The job's inference_done_fence is never attached to the input BOs, allowing subsequent writers to corrupt memory. - [High] PM runtime reference leak and unsignaled fence return on error paths in rocket_job_run(). -- --- Patch [1]: [PATCH] drm/rocket: Check allocations before use --- commit e94d3a8b04e433fb70c776f865e1fd9a313aff5c Author: Triet Hoang <[email protected]> drm/rocket: Check allocations before use Check the result of kvmalloc_array() in rocket_job_push() and kmalloc_objs() in rocket_job_open() before using the allocated buffer. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index ac51bff39833f..2fee21988d27c 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -192,6 +192,9 @@ static int rocket_job_push(struct rocket_job *job) > > bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void > *), > GFP_KERNEL); > + if (!bos) > + return -ENOMEM; > + > memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); > memcpy(&bos[job->in_bo_count], job->out_bos, job->out_bo_count * > sizeof(void *)); > [Severity: High] This is a pre-existing issue, but does aborting the submission after arming the job violate the DRM scheduler API contract? In rocket_job_push(), the job is armed before fallible operations: drm_sched_job_arm(&job->base); job->inference_done_fence = dma_fence_get(&job->base.s_fence->finished); ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); if (ret) goto err_unlock; If rocket_acquire_object_fences() fails, the submission is aborted. However, drm_sched_job_arm() is documented as a point of no return. Can aborting here throw away the assigned fence sequence number and break dependency tracking? [Severity: High] This is a pre-existing issue, but does this code leave the input BOs unprotected from concurrent writers? Later in rocket_job_push(), the completion fence is attached to the output BOs: rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence); But it doesn't appear to attach the fence to the input BOs using DMA_RESV_USAGE_READ. Does this allow a CPU or GPU write operation to the input BOs to execute concurrently and overwrite data during the NPU inference? [ ... ] > @@ -501,6 +504,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > unsigned int core; > int ret; > > + if (!scheds) > + return -ENOMEM; > + > for (core = 0; core < rdev->num_cores; core++) > scheds[core] = &rdev->cores[core].sched; [Severity: High] This is a pre-existing issue, but does this dynamically allocated scheds array leak on errors or on single-core devices? In rocket_job_open(): ret = drm_sched_entity_init(&rocket_priv->sched_entity, DRM_SCHED_PRIORITY_NORMAL, scheds, rdev->num_cores, NULL); if (WARN_ON(ret)) return ret; The scheds array is not freed if drm_sched_entity_init() fails. Additionally, when only a single queue (single-core device) is provided, the DRM scheduler entity does not take ownership of the scheds_list, so entity->sched_list may not point to the allocated array. Since rocket_job_close() attempts to free entity->sched_list, could the original scheds array be completely lost and leaked per file-open? [Severity: Critical] This is a pre-existing issue, but could this unconditionally dereference a NULL pointer on early submission failures? If a job submission fails early (e.g., in rocket_copy_tasks or drm_sched_job_init), the error path calls rocket_job_put(rjob). At this point, rjob->domain has not yet been initialized and is NULL. In rocket_job_cleanup(): rocket_iommu_domain_put(job->domain); And in rocket_iommu_domain_put(): kref_put(&domain->kref, rocket_iommu_domain_destroy); Does evaluating &domain->kref when domain is NULL cause a kernel panic that can be trivially triggered by an unprivileged user through the DRM submit ioctl? [Severity: Critical] This is a pre-existing issue, but can this code cause a double free if rocket_copy_tasks() fails? In rocket_copy_tasks(), if validation fails: fail: kvfree(rjob->tasks); return ret; The array is freed, but the pointer is not cleared. If the error propagates up to rocket_ioctl_submit_job(), it jumps to out_cleanup_job, which calls rocket_job_cleanup(). In rocket_job_cleanup(): kvfree(job->tasks); Could this unconditional call on the already-freed pointer cause memory corruption exploitable by a maliciously crafted ioctl payload? [Severity: High] This is a pre-existing issue, but does rocket_ioctl_submit() silently ignore errors from rocket_ioctl_submit_job()? In rocket_ioctl_submit(): for (i = 0; i < args->job_count; i++) rocket_ioctl_submit_job(dev, file, &jobs[i]); The integer return value of rocket_ioctl_submit_job() is entirely discarded. If a job fails to submit, userspace will receive a success (0) return value anyway. Could this cause userspace to hang indefinitely waiting on a completion fence for a job that was never queued? [Severity: High] This is a pre-existing issue, but does this hardware IRQ handler leak an IOMMU group reference? In rocket_job_handle_irq(), when a job finishes: iommu_detach_group(NULL, iommu_group_get(core->dev)); iommu_group_get() increments the reference count of the IOMMU group, but iommu_detach_group() does not consume this reference. Because this executes for every completed job, does this cause a severe resource leak that eventually results in a refcount overflow or memory exhaustion? [Severity: High] This is a pre-existing issue, but does rocket_job_run() leak a power management reference on error paths? In rocket_job_run(): ret = pm_runtime_get_sync(core->dev); if (ret < 0) return fence; ret = iommu_attach_group(job->domain->domain, core->iommu_group); if (ret < 0) return fence; If pm_runtime_get_sync() fails, it retains the reference, but the function returns an unsignaled fence without dropping it. Furthermore, if iommu_attach_group() fails, it also returns without releasing the PM reference acquired just before it. Does this prevent the device from sleeping and stall the DRM scheduler on an unsignaled fence? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
