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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unconditional call to rocket_iommu_domain_put() in the job cleanup path causes a NULL pointer dereference on early submission failures.
- [Critical] Double free of rjob->tasks when rocket_copy_tasks() fails.
- [High] Aborting a DRM scheduler job after calling drm_sched_job_arm() violates the DRM scheduler API contract and corrupts scheduler state.
- [High] The dynamically allocated scheds array leaks in rocket_job_open() on initialization errors and permanently leaks on single-core devices.
- [High] rocket_ioctl_submit() silently ignores errors returned by rocket_ioctl_submit_job().
- [High] Reference count leak of iommu_group in the hardware IRQ handler.
- [High] The job's inference_done_fence is never attached to the input BOs, allowing subsequent writers to corrupt memory.
- [High] PM runtime reference leak and unsignaled fence return on error paths in rocket_job_run().
--

--- Patch [1]: [PATCH] drm/rocket: Check allocations before use ---
commit e94d3a8b04e433fb70c776f865e1fd9a313aff5c
Author: Triet Hoang <triet.hoang.dev@gmail.com>

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 buffer.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index ac51bff39833f..2fee21988d27c 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);
> +	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 *));
>  

[Severity: High]
This is a pre-existing issue, but does aborting the submission after arming
the job violate the DRM scheduler API contract?

In rocket_job_push(), the job is armed before fallible operations:

		drm_sched_job_arm(&job->base);
		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;

If rocket_acquire_object_fences() fails, the submission is aborted. However,
drm_sched_job_arm() is documented as a point of no return. Can aborting here
throw away the assigned fence sequence number and break dependency tracking?

[Severity: High]
This is a pre-existing issue, but does this code leave the input BOs
unprotected from concurrent writers?

Later in rocket_job_push(), the completion fence is attached to the output BOs:

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

But it doesn't appear to attach the fence to the input BOs using
DMA_RESV_USAGE_READ. Does this allow a CPU or GPU write operation to the input
BOs to execute concurrently and overwrite data during the NPU inference?

[ ... ]

> @@ -501,6 +504,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;

[Severity: High]
This is a pre-existing issue, but does this dynamically allocated scheds array
leak on errors or on single-core devices?

In rocket_job_open():

	ret = drm_sched_entity_init(&rocket_priv->sched_entity,
				    DRM_SCHED_PRIORITY_NORMAL,
				    scheds,
				    rdev->num_cores, NULL);
	if (WARN_ON(ret))
		return ret;

The scheds array is not freed if drm_sched_entity_init() fails. Additionally,
when only a single queue (single-core device) is provided, the DRM scheduler
entity does not take ownership of the scheds_list, so entity->sched_list may
not point to the allocated array. Since rocket_job_close() attempts to free
entity->sched_list, could the original scheds array be completely lost and
leaked per file-open?

[Severity: Critical]
This is a pre-existing issue, but could this unconditionally dereference a NULL
pointer on early submission failures?

If a job submission fails early (e.g., in rocket_copy_tasks or
drm_sched_job_init), the error path calls rocket_job_put(rjob). At this point,
rjob->domain has not yet been initialized and is NULL.

In rocket_job_cleanup():

	rocket_iommu_domain_put(job->domain);

And in rocket_iommu_domain_put():

	kref_put(&domain->kref, rocket_iommu_domain_destroy);

Does evaluating &domain->kref when domain is NULL cause a kernel panic that can
be trivially triggered by an unprivileged user through the DRM submit ioctl?

[Severity: Critical]
This is a pre-existing issue, but can this code cause a double free if
rocket_copy_tasks() fails?

In rocket_copy_tasks(), if validation fails:

fail:
	kvfree(rjob->tasks);
	return ret;

The array is freed, but the pointer is not cleared. If the error propagates up
to rocket_ioctl_submit_job(), it jumps to out_cleanup_job, which calls
rocket_job_cleanup().

In rocket_job_cleanup():

	kvfree(job->tasks);

Could this unconditional call on the already-freed pointer cause memory
corruption exploitable by a maliciously crafted ioctl payload?

[Severity: High]
This is a pre-existing issue, but does rocket_ioctl_submit() silently ignore
errors from rocket_ioctl_submit_job()?

In rocket_ioctl_submit():

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

The integer return value of rocket_ioctl_submit_job() is entirely discarded.
If a job fails to submit, userspace will receive a success (0) return value
anyway. Could this cause userspace to hang indefinitely waiting on a completion
fence for a job that was never queued?

[Severity: High]
This is a pre-existing issue, but does this hardware IRQ handler leak an IOMMU
group reference?

In rocket_job_handle_irq(), when a job finishes:

			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 this reference. Because this executes
for every completed job, does this cause a severe resource leak that eventually
results in a refcount overflow or memory exhaustion?

[Severity: High]
This is a pre-existing issue, but does rocket_job_run() leak a power
management reference on error paths?

In 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 pm_runtime_get_sync() fails, it retains the reference, but the function
returns an unsignaled fence without dropping it. Furthermore, if
iommu_attach_group() fails, it also returns without releasing the PM reference
acquired just before it. Does this prevent the device from sleeping and stall
the DRM scheduler on an unsignaled fence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817053949.819142-1-triet.hoang.dev@gmail.com?part=1