Re: [PATCH v4 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] 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 <Naixumogu@whut.edu.cn>

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