Re: [PATCH v4 1/2] drm/rocket: Check allocations before use

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double free on `rjob->tasks` array during job submission failure.
- [Critical] NULL pointer dereference in `rocket_iommu_domain_put()`.
- [High] Missing read fence attachment for input BOs causes implicit synchronization data races.
- [High] PM usage counter leak and DRM scheduler hang on `rocket_job_run()` error paths.
- [High] Calling `drm_sched_job_cleanup()` after `drm_sched_job_arm()` violates the scheduler API contract.
- [High] Silent failure handling due to unpropagated return value in `rocket_ioctl_submit()`.
- [High] Reference count leak of IOMMU group in `rocket_job_handle_irq()`.
- [Medium] Unconditional memory leak of `scheds` array on success for single-core setups.
- [Medium] Potential integer overflow in BO array allocation bypasses constraints.
--

--- Patch [1]: [PATCH v4 1/2] drm/rocket: Check allocations before use ---
commit c89b0a284de194304c5f1c1f130d90f61e831c73
Author: Triet Hoang <[email protected]>

drm/rocket: Check allocations before use

This commit adds checks for the return values of kvmalloc_array() in
rocket_job_push() and kmalloc_objs() in rocket_job_open() to prevent NULL
pointer dereferences when allocations fail.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index ac51bff39833f..adcc792541ec1 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -192,6 +192,9 @@ static int rocket_job_push(struct rocket_job *job)
>  
>  	bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void *),
>  			     GFP_KERNEL);

[Severity: Medium]
This is a pre-existing issue, but can job->in_bo_count + job->out_bo_count
overflow?

If these attacker-controlled 32-bit integers wrap around to 0,
kvmalloc_array() would return a ZERO_SIZE_PTR. This causes the subsequent
memcpy() calls to write out of bounds.

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

[ ... ]

> @@ -509,9 +515,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  				    scheds,
>  				    rdev->num_cores, NULL);
>  	if (WARN_ON(ret))
> -		return ret;
> +		kfree(scheds);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but does this leak the scheds
array on success when num_cores == 1?

If drm_sched_entity_init() succeeds, it internally sets
entity->sched_list = NULL for single-core setups. Since the scheds array
is never saved (e.g., to rocket_priv->scheds), it gets leaked.

Later, rocket_job_close() will attempt to free entity->sched_list which is
NULL, meaning the scheds array is leaked on every file descriptor open/close
cycle.

Additional pre-existing issues found in drivers/accel/rocket/rocket_job.c:

[Severity: Critical]
This is a pre-existing issue, but is there a double free on the rjob->tasks
array during job submission failure?

If rocket_copy_tasks() fails (e.g., due to invalid user pointers), it frees
rjob->tasks but does not nullify the pointer:

rocket_copy_tasks() {
    ...
fail:
    kvfree(rjob->tasks);
    return ret;
}

The error then propagates up to rocket_ioctl_submit_job(), which calls
rocket_job_put() and ultimately rocket_job_cleanup(). The cleanup function
unconditionally frees the same dangling pointer:

rocket_job_cleanup() {
    ...
    kvfree(job->tasks);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does rocket_job_push() miss
attaching read fences for input BOs?

The driver correctly attaches write completion fences to out_bos:

rocket_job_push() {
    ...
    rocket_attach_object_fences(job->out_bos, job->out_bo_count,
                                job->inference_done_fence);
    ...
}

However, it entirely omits attaching the fence to in_bos. Without a
DMA_RESV_USAGE_READ fence, subsequent jobs or operations writing to these
input BOs will not wait for this read operation to complete, which could
generate read-after-write GPU data races.

[Severity: High]
This is a pre-existing issue, but could rocket_job_run() leak the PM runtime
usage counter and hang the DRM scheduler on error paths?

rocket_job_run() {
    ...
    ret = pm_runtime_get_sync(core->dev);
    if (ret < 0)
        return fence;

    ret = iommu_attach_group(job->domain->domain, core->iommu_group);
    if (ret < 0)
        return fence;
    ...
}

If either fails, the function immediately returns the newly created, unsignaled
fence. The DRM scheduler will wait forever for this fence, leading to a job
timeout and hardware reset. Furthermore, pm_runtime_get_sync() increments the
usage counter even on failure, so missing a pm_runtime_put_noidle() call
causes a PM counter leak.

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling for
job submission violate the DRM scheduler API contract?

In rocket_job_push(), the job is armed first:

rocket_job_push() {
    ...
        drm_sched_job_arm(&job->base);
        ...
        ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count,
                                           &job->base, false);
        if (ret)
            goto err_unlock;
    ...
}

If acquiring fences fails, the code jumps to the error path and eventually
rocket_ioctl_submit_job() calls drm_sched_job_cleanup() on the aborted job:

rocket_ioctl_submit_job() {
    ...
out_cleanup_job:
    if (ret)
        drm_sched_job_cleanup(&rjob->base);
    ...
}

Calling cleanup on an armed job is forbidden by the DRM scheduler API because
it invalidates sequence numbers and leaves a dangling initialized fence.

[Severity: High]
This is a pre-existing issue, but will rocket_ioctl_submit() silently ignore
job submission failures?

rocket_ioctl_submit() {
    ...
    for (i = 0; i < args->job_count; i++)
        rocket_ioctl_submit_job(dev, file, &jobs[i]);

exit:
    kvfree(jobs);

    return ret;
}

The return value of rocket_ioctl_submit_job() is ignored. If a job fails
initialization, validation, or pushing to the scheduler, the failure is
silenced and the ioctl still returns success to userspace.

[Severity: Critical]
This isn't a bug introduced by this patch, but might rocket_job_cleanup()
cause a NULL pointer dereference?

If drm_gem_objects_lookup() fails early in rocket_ioctl_submit_job(),
rjob->domain is never initialized and remains NULL. The cleanup path will
call rocket_job_cleanup(), which unconditionally passes this NULL domain
pointer to rocket_iommu_domain_put():

rocket_job_cleanup() {
    ...
    rocket_iommu_domain_put(job->domain);
    ...
}

rocket_iommu_domain_put() {
    kref_put(&domain->kref, rocket_iommu_domain_destroy);
}

The helper directly dereferences the pointer without checking if it is NULL,
which would cause a kernel panic.

[Severity: High]
This is a pre-existing issue, but is the reference count of the IOMMU group
leaked in rocket_job_handle_irq()?

rocket_job_handle_irq() {
    ...
            iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

iommu_group_get() increments the reference count of the IOMMU group, but
iommu_detach_group() does not consume it. Because there is no matching
iommu_group_put(), this causes a reference count leak on every job completion.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1