Re: [PATCH v5 1/3] accel/rocket: Validate BO handle counts on job submission

[sashiko-bot@kernel.org]

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 <Naixumogu@whut.edu.cn>

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/20260828064152.37822-1-Naixumogu@whut.edu.cn?part=1