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

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

Pre-existing issues:
- [Critical] Double free vulnerability in job submission error path
- [Critical] NULL pointer dereference when tearing down a failed job initialization
- [High] Integer overflow in `drm_gem_lock_reservations` causes complete locking bypass
- [High] Aborting an armed DRM scheduler job violates API contract and corrupts sequence numbers
- [High] Missing `DMA_RESV_USAGE_READ` fence attachment leads to Write-After-Read data corruption
- [High] Memory leak of `scheds` array on success path for single-core devices
--

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

drm/rocket: Check allocations before use

Check the result of kvmalloc_array() in rocket_job_push() and
kmalloc_objs() in rocket_job_open() before using the allocated
buffers.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index ac51bff39833f..c4a40fe6ab973 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -189,9 +189,15 @@ static int rocket_job_push(struct rocket_job *job)
>  	struct drm_gem_object **bos;
>  	struct ww_acquire_ctx acquire_ctx;
>  	int ret = 0;
> +	size_t bos_count;
> +
> +	if (check_add_overflow(job->in_bo_count, job->out_bo_count, &bos_count))
> +		return -EOVERFLOW;
> +
> +	bos = kvmalloc_array(bos_count, sizeof(void *), GFP_KERNEL);
> +	if (!bos)
> +		return -ENOMEM;
>  
> -	bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void *),
> -			     GFP_KERNEL);
>  	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 *));

[Severity: Critical]
This is a pre-existing issue, but is there a potential double free during
the job submission error path?

If rocket_copy_tasks() fails (for example, due to an invalid
copy_from_user()), it calls kvfree(rjob->tasks) but does not set the
pointer to NULL:

rocket_copy_tasks()
    kvfree(rjob->tasks);
    // rjob->tasks is left dangling

When the error propagates up to rocket_ioctl_submit_job(), it calls
rocket_job_put(rjob) which triggers the cleanup routine. The cleanup
function unconditionally frees the tasks array a second time:

rocket_job_cleanup()
    kvfree(job->tasks);

Could this double-free be triggered from unprivileged user space?

[Severity: Critical]
This is also a pre-existing issue, but could we hit a NULL pointer
dereference when tearing down a failed job initialization?

In rocket_ioctl_submit_job(), rjob is allocated with kzalloc_obj(), which
leaves rjob->domain initialized to NULL. If an error occurs before the
IOMMU domain is fetched (for example, if drm_sched_job_init() fails),
the error path invokes rocket_job_cleanup().

The cleanup function unconditionally calls rocket_iommu_domain_put() on
the domain pointer:

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

Since domain is NULL, would this immediate dereference cause a kernel
panic reachable by userspace?

>  	ret = drm_gem_lock_reservations(bos, job->in_bo_count + job->out_bo_count, &acquire_ctx);

[Severity: High]
This isn't a bug introduced by this patch, but does the sum of the buffers
overflow when passed to drm_gem_lock_reservations()?

While this patch adds check_add_overflow() to prevent size_t overflow
during the array allocation, the unchecked sum is still passed directly to
drm_gem_lock_reservations().

Since drm_gem_lock_reservations() accepts a signed int for the count,
could providing individually valid but large arrays (e.g., 0x40000000 each)
result in a sum of 0x80000000?

If this implicitly casts to a negative integer (-2147483648), it appears
drm_gem_lock_reservations() would skip its loop and return success
immediately. Would this bypass the GEM locking mechanism and lead to data
races?

>  	if (ret)
>  		goto err;
>  
>  	scoped_guard(mutex, &rdev->sched_lock) {
>  		drm_sched_job_arm(&job->base);

[Severity: High]
This is a pre-existing issue, but does this error path violate the DRM
scheduler API contract?

In rocket_job_push(), the job is armed, establishing a point of no return.
Subsequently, the driver attempts to acquire object fences. If this fails,
the function returns an error:

>  
>  		job->inference_done_fence = dma_fence_get(&job->base.s_fence->finished);
>  
>  		ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false);
>  		if (ret)
>  			goto err_unlock;

The DRM scheduler documentation explicitly forbids aborting a job once
drm_sched_job_arm() is called; it must be pushed. Does aborting the job
here orphan the sequence number and corrupt the scheduler state?

[ ... ]

>  	rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence);

[Severity: High]
This is a pre-existing issue, but is the completion fence missing for the
input buffers?

After the job is pushed, rocket_attach_object_fences() attaches the
completion fence to out_bos. However, it doesn't appear to attach the fence
to the input buffers (in_bos) with DMA_RESV_USAGE_READ.

Without this attachment, does the DRM reservation system know the hardware
is reading from these buffers? Could a subsequent operation write to these
buffers without waiting, resulting in a write-after-read data corruption?

[ ... ]

> @@ -501,6 +507,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  	unsigned int core;
>  	int ret;
>  
> +	if (!scheds)
> +		return -ENOMEM;
> +
>  	for (core = 0; core < rdev->num_cores; core++)
>  		scheds[core] = &rdev->cores[core].sched;
>  
> @@ -509,9 +518,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: High]
This is a pre-existing issue, but is there a memory leak of the scheds
array on the success path for single-core devices?

When drm_sched_entity_init() is called with num_sched_list equal to
rdev->num_cores, and num_cores is 1, the DRM core sets entity->sched_list
to NULL internally.

Because rocket_job_open() returns without freeing scheds or storing the
pointer locally when entity->sched_list is internally nulled, is this
dynamically allocated array leaked?

Could a malicious userspace program exhaust kernel memory by repeatedly
opening and closing the device node?

>  }
>  
>  void rocket_job_close(struct rocket_file_priv *rocket_priv)

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