Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing BO deduplication causes -EALREADY on in-place operations. - [High] Missing read fence attachment for input BOs breaks implicit synchronization. - [High] Integer overflow in task parsing loop allows out-of-bounds memory write. -- commit 88dcb66252c59bcede3a94150e673b09305b3bd6 Author: MoGGuU <[email protected]> accel/rocket: Validate BO handle counts on job submission The input and output BO handle counts are __u32, while GEM lookup and reservation helpers take int counts. A count above INT_MAX cannot be represented safely by the GEM lookup helper. Reject each count above INT_MAX before looking up the BOs. [ ... ] > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index 3141f210fcd1b..13f4915c1b07c 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -190,7 +190,7 @@ static int rocket_job_push(struct rocket_job *job) > struct rocket_device *rdev = job->rdev; > struct drm_gem_object **bos; > struct ww_acquire_ctx acquire_ctx; > - u32 bo_count; > + int bo_count; > int ret = 0; > > if (check_add_overflow(job->in_bo_count, job->out_bo_count, &bo_count)) > return -EINVAL; > > bos = kvmalloc_array(bo_count, sizeof(*bos), 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 *)); > > ret = drm_gem_lock_reservations(bos, bo_count, &acquire_ctx); [Severity: High] This is a pre-existing issue, but does this submission path handle duplicate BOs correctly? Since the in_bos and out_bos arrays are concatenated without deduplication, if userspace submits a job where the same BO handle appears in both arrays (e.g., an in-place accelerator operation), drm_gem_lock_reservations() will lock the array sequentially. When it hits the duplicate object, dma_resv_lock_interruptible() detects the ww_mutex is already held by the same context and returns -EALREADY. Since drm_gem_lock_reservations() does not handle -EALREADY, it will abort the submission. > if (ret) > goto err; [ ... ] > scoped_guard(mutex, &rdev->sched_lock) { > drm_sched_job_arm(&job->base); > > job->inference_done_fence = > dma_fence_get(&job->base.s_fence->finished); > > kref_get(&job->refcount); /* put by scheduler job completion */ > > drm_sched_entity_push_job(&job->base); > } > > rocket_attach_object_fences(job->out_bos, job->out_bo_count, > job->inference_done_fence); [Severity: High] This is a pre-existing issue, but does this code properly synchronize the input BOs? The completion fence is exclusively attached to the output buffers. Because the fence is never attached to in_bos (which should receive a read usage fence), the DMA reservation object of the input buffers remains unaware of this job's execution. Could this lead to a write-after-read data corruption if subsequent jobs acquire a write lock on those input buffers without waiting for this job to finish reading? [ ... ] > @@ -557,6 +557,11 @@ static int rocket_ioctl_submit_job(struct drm_device > *dev, struct drm_file *file > if (job->task_count == 0) > return -EINVAL; > > + /* GEM lookup takes a signed object count. */ > + if (job->in_bo_handle_count > INT_MAX || > + job->out_bo_handle_count > INT_MAX) > + return -EINVAL; > + [Severity: High] This isn't a bug introduced by this patch, but since we are limiting the BO counts to INT_MAX to prevent integer overflow, should we do the same for job->task_count? If job->task_count exceeds INT_MAX (e.g., 0x80000001), it can cause an integer overflow in rocket_copy_tasks(): drivers/accel/rocket/rocket_job.c:rocket_copy_tasks() { for (int i = 0; i < rjob->task_count; i++) { struct drm_rocket_task task = {0}; ... rjob->tasks[i].regcmd = task.regcmd; } When the signed counter i reaches 0x7FFFFFFF, i++ wraps it to negative. The loop condition i < rjob->task_count promotes i to u32, so it continues, leading to out-of-bounds writes using a negative array index. > rjob = kzalloc_obj(*rjob); > if (!rjob) > return -ENOMEM; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
