Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
Pre-existing issues: - [Critical] Double free on `rjob->tasks` array during job submission failure. - [Critical] NULL pointer dereference in `rocket_iommu_domain_put()`. - [High] Missing read fence attachment for input BOs causes implicit synchronization data races. - [High] PM usage counter leak and DRM scheduler hang on `rocket_job_run()` error paths. - [High] Calling `drm_sched_job_cleanup()` after `drm_sched_job_arm()` violates the scheduler API contract. - [High] Silent failure handling due to unpropagated return value in `rocket_ioctl_submit()`. - [High] Reference count leak of IOMMU group in `rocket_job_handle_irq()`. - [Medium] Unconditional memory leak of `scheds` array on success for single-core setups. - [Medium] Potential integer overflow in BO array allocation bypasses constraints. -- --- Patch [1]: [PATCH v4 1/2] drm/rocket: Check allocations before use --- commit c89b0a284de194304c5f1c1f130d90f61e831c73 Author: Triet Hoang <[email protected]> drm/rocket: Check allocations before use This commit adds checks for the return values of kvmalloc_array() in rocket_job_push() and kmalloc_objs() in rocket_job_open() to prevent NULL pointer dereferences when allocations fail. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index ac51bff39833f..adcc792541ec1 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); [Severity: Medium] This is a pre-existing issue, but can job->in_bo_count + job->out_bo_count overflow? If these attacker-controlled 32-bit integers wrap around to 0, kvmalloc_array() would return a ZERO_SIZE_PTR. This causes the subsequent memcpy() calls to write out of bounds. > + 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 *)); [ ... ] > @@ -509,9 +515,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > scheds, > rdev->num_cores, NULL); > if (WARN_ON(ret)) > - return ret; > + kfree(scheds); > > - return 0; > + return ret; > } [Severity: Medium] This isn't a bug introduced by this patch, but does this leak the scheds array on success when num_cores == 1? If drm_sched_entity_init() succeeds, it internally sets entity->sched_list = NULL for single-core setups. Since the scheds array is never saved (e.g., to rocket_priv->scheds), it gets leaked. Later, rocket_job_close() will attempt to free entity->sched_list which is NULL, meaning the scheds array is leaked on every file descriptor open/close cycle. Additional pre-existing issues found in drivers/accel/rocket/rocket_job.c: [Severity: Critical] This is a pre-existing issue, but is there a double free on the rjob->tasks array during job submission failure? If rocket_copy_tasks() fails (e.g., due to invalid user pointers), it frees rjob->tasks but does not nullify the pointer: rocket_copy_tasks() { ... fail: kvfree(rjob->tasks); return ret; } The error then propagates up to rocket_ioctl_submit_job(), which calls rocket_job_put() and ultimately rocket_job_cleanup(). The cleanup function unconditionally frees the same dangling pointer: rocket_job_cleanup() { ... kvfree(job->tasks); ... } [Severity: High] This isn't a bug introduced by this patch, but does rocket_job_push() miss attaching read fences for input BOs? The driver correctly attaches write completion fences to out_bos: rocket_job_push() { ... rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence); ... } However, it entirely omits attaching the fence to in_bos. Without a DMA_RESV_USAGE_READ fence, subsequent jobs or operations writing to these input BOs will not wait for this read operation to complete, which could generate read-after-write GPU data races. [Severity: High] This is a pre-existing issue, but could rocket_job_run() leak the PM runtime usage counter and hang the DRM scheduler on error paths? 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 either fails, the function immediately returns the newly created, unsignaled fence. The DRM scheduler will wait forever for this fence, leading to a job timeout and hardware reset. Furthermore, pm_runtime_get_sync() increments the usage counter even on failure, so missing a pm_runtime_put_noidle() call causes a PM counter leak. [Severity: High] This isn't a bug introduced by this patch, but does the error handling for job submission violate the DRM scheduler API contract? In rocket_job_push(), the job is armed first: rocket_job_push() { ... drm_sched_job_arm(&job->base); ... ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); if (ret) goto err_unlock; ... } If acquiring fences fails, the code jumps to the error path and eventually rocket_ioctl_submit_job() calls drm_sched_job_cleanup() on the aborted job: rocket_ioctl_submit_job() { ... out_cleanup_job: if (ret) drm_sched_job_cleanup(&rjob->base); ... } Calling cleanup on an armed job is forbidden by the DRM scheduler API because it invalidates sequence numbers and leaves a dangling initialized fence. [Severity: High] This is a pre-existing issue, but will rocket_ioctl_submit() silently ignore job submission failures? rocket_ioctl_submit() { ... for (i = 0; i < args->job_count; i++) rocket_ioctl_submit_job(dev, file, &jobs[i]); exit: kvfree(jobs); return ret; } The return value of rocket_ioctl_submit_job() is ignored. If a job fails initialization, validation, or pushing to the scheduler, the failure is silenced and the ioctl still returns success to userspace. [Severity: Critical] This isn't a bug introduced by this patch, but might rocket_job_cleanup() cause a NULL pointer dereference? If drm_gem_objects_lookup() fails early in rocket_ioctl_submit_job(), rjob->domain is never initialized and remains NULL. The cleanup path will call rocket_job_cleanup(), which unconditionally passes this NULL domain pointer to rocket_iommu_domain_put(): rocket_job_cleanup() { ... rocket_iommu_domain_put(job->domain); ... } rocket_iommu_domain_put() { kref_put(&domain->kref, rocket_iommu_domain_destroy); } The helper directly dereferences the pointer without checking if it is NULL, which would cause a kernel panic. [Severity: High] This is a pre-existing issue, but is the reference count of the IOMMU group leaked in rocket_job_handle_irq()? rocket_job_handle_irq() { ... 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 it. Because there is no matching iommu_group_put(), this causes a reference count leak on every job completion. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
