Re: [PATCH v3 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] Integer overflow in the rocket_copy_tasks() loop counter leads to an out-of-bounds write in kernel memory.
- [High] The execution fence is not attached to input BOs, allowing concurrent modifications from other execution contexts.
- [High] rocket_job_handle_irq() leaks an IOMMU group reference on every interrupt.
--

commit 47c63098f063d8d22a7ebbc0a4a54fb92597a27b
Author: MoGGuU <Naixumogu@whut.edu.cn>
Date:   Fri Aug 28 05:08:03 2026 +0000

    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.
    
    rocket_job_push() already uses check_add_overflow() for the combined count,
    but stores the result in u32, so it only detects unsigned wraparound. Store
    the result in int so sums above INT_MAX are rejected before the count is
    passed to the reservation helpers.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index bb77b6bf0f231..e6052d1973afa 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 isn't a bug introduced by this patch, but does this function miss attaching
the execution fence to the input BOs?

Later in rocket_job_push(), the inference_done_fence is attached to the output
BOs, but the same is not done for the input 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);
    ...
}

Without attaching a read fence to the input BOs' dma_resv objects, subsequent
jobs that attempt to write to these input BOs will not wait for the NPU to
finish reading. Can this lead to data corruption from concurrent access?

[ ... ]

> @@ -556,6 +556,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 is a pre-existing issue, but can the signed loop counter in
rocket_copy_tasks() overflow and cause an out-of-bounds write?

Since job->task_count is a u32 and not validated against INT_MAX like the BO
counts above, if a user submits a job with task_count >= 0x80000001 (assuming a
large enough memory allocation succeeds), the signed integer `i` in the copy loop
will overflow when it reaches 0x7FFFFFFF and becomes -2147483648:

drivers/accel/rocket/rocket_job.c:rocket_copy_tasks() {
    ...
	for (int i = 0; i < rjob->task_count; i++) {
        ...
		rjob->tasks[i].regcmd = task.regcmd;
    ...
}

Because rjob->task_count is unsigned, the comparison `i < rjob->task_count`
uses unsigned arithmetic. As a result, the negative `i` is treated as a valid
index, causing `rjob->tasks[i].regcmd` to write out-of-bounds at a negative
offset in kernel memory.

[Severity: High]
This is also a pre-existing issue, but does rocket_job_handle_irq() leak an
IOMMU group reference on every interrupt?

In the interrupt handler, an IOMMU group reference is acquired but never
released:

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

Since iommu_group_get() acquires a reference, and iommu_detach_group() does not
consume it, the reference is leaked. Should there be a matching
iommu_group_put() call here to prevent a permanent memory and reference leak?

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