Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The job's completion fence is not attached to input BOs, causing a Write-After-Read data corruption vulnerability. - [High] Missing practical upper bound for BO handle counts allows hard lockups and memory exhaustion. - [High] Signed integer overflow in task copy loop leads to out-of-bounds array access and memory corruption. -- commit b8a6c0c385bb7b291f18a1ccd50949d83223c9d8 Author: Haoming Gao <[email protected]> accel/rocket: Validate BO handle counts on job submission This commit validates the input and output BO handle counts during job submission to ensure they do not exceed INT_MAX. It also changes the type of the combined count in rocket_job_push to prevent silent truncation before passing it to GEM reservation helpers. > 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)) [Severity: High] This is a pre-existing issue, but does rocket_job_push() leave input BOs unprotected against concurrent writers? Later in rocket_job_push(), the completion fence is only attached to output BOs: drivers/accel/rocket/rocket_job.c:rocket_job_push() { ... rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence); ... } Because the completion fence is never recorded on the input BOs with a read usage flag, could this lead to a write-after-read data corruption? If userspace submits a job that reads an input BO, and subsequently a second job that writes to the same BO, the scheduler might execute the second job immediately without waiting for the first job to finish reading, allowing the input data to be overwritten prematurely. [ ... ] > @@ -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; [Severity: High] This isn't a bug introduced by this patch, but does rocket_copy_tasks() have an integer overflow in the task copy loop? In rocket_ioctl_submit_job(), task_count comes from userspace as a u32 and is passed to rocket_copy_tasks(). If job->task_count exceeds INT_MAX (e.g., 0x80000005) and the array allocation succeeds, the iteration uses a signed int: 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; rjob->tasks[i].regcmd_count = task.regcmd_count; } ... } Because i is compared against the unsigned rjob->task_count, the loop continues after i overflows and wraps to negative values. Will this result in an out-of-bounds array access and memory corruption when writing to rjob->tasks[i] with a negative index? > > + /* 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 does bounding the BO counts to INT_MAX still allow for hard lockups? If userspace submits a job with in_bo_handle_count set to INT_MAX, the check passes and drm_gem_objects_lookup() will allocate massive arrays and iterate over 2 billion times inside spin_lock(&filp->table_lock). Could holding a spinlock for that duration exceed the NMI watchdog timeout and cause a system crash due to a hard lockup? > rjob = kzalloc_obj(*rjob); > if (!rjob) > return -ENOMEM; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
