Re: [PATCH v2 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:
- [Critical] Integer overflow in `rocket_copy_tasks` allows arbitrary out-of-bounds write of user data.
- [High] iommu_group_get() refcount leak in rocket_job_handle_irq.
- [High] Missing implicit execution fence for input BOs allows write-after-read hazards.
--

commit 335ce4beaa240be7a9d792c09e9cc0672c21caac
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, or a combined
count above INT_MAX, cannot be represented safely at those call sites.

Reject such counts before looking up the BOs.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index bb77b6bf0f231..7e3d123afc5ad 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -556,6 +556,12 @@ static int rocket_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
>  	if (job->task_count == 0)
>  		return -EINVAL;
>  
> +	/* GEM lookup and reservation helpers take signed object counts. */
> +	if (job->in_bo_handle_count > INT_MAX ||
> +	    job->out_bo_handle_count > INT_MAX ||
> +	    job->in_bo_handle_count > INT_MAX - job->out_bo_handle_count)
> +		return -EINVAL;
> +
>  	rjob = kzalloc_obj(*rjob);
>  	if (!rjob)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but looking at rocket_job_handle_irq():

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq()
			iommu_detach_group(NULL, iommu_group_get(core->dev));
			dma_fence_signal(core->in_flight_job->done_fence);

Does this leak the IOMMU group reference?

iommu_group_get() increments the reference count of the returned IOMMU group,
but iommu_detach_group() doesn't consume this reference. Since there is no
matching iommu_group_put(), won't this reference leak on every successfully
completed job?

[Severity: High]
This is a pre-existing issue, but looking at rocket_job_push():

drivers/accel/rocket/rocket_job.c:rocket_job_push()
		ret = rocket_acquire_object_fences(job->out_bos, job->out_bo_count, &job->base, true);
		if (ret)
			goto err_unlock;

		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);

Does this omit the required implicit execution fence attachment for input BOs?

The execution fence (inference_done_fence) is attached to out_bos, but it
seems in_bos are skipped. If in_bos don't receive the execution fence, could
this cause a write-after-read hazard if subsequent operations write to these
BOs while the NPU is still reading them?

[Severity: Critical]
This is a pre-existing issue, but looking at 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};

		if (copy_from_user(&task,
				   u64_to_user_ptr(job->tasks) + i * job->task_struct_size,
				   sizeof(task))) {
			drm_dbg(dev, "Failed to copy incoming tasks\n");
			ret = -EFAULT;
			goto fail;
		}

[ ... ]

		rjob->tasks[i].regcmd = task.regcmd;
		rjob->tasks[i].regcmd_count = task.regcmd_count;
	}

Does this code overflow the i counter?

Since int i is signed and rjob->task_count is a u32, if a user submits a job
with a task_count greater than INT_MAX, could i increment past 0x7FFFFFFF and
overflow to negative values? This would mean the loop condition
i < rjob->task_count evaluates to true due to unsigned promotion, but the
array access rjob->tasks[i] would use a negative index, leading to an
out-of-bounds write with user-controlled data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827170608.39511-1-Naixumogu@whut.edu.cn?part=1