[PATCH] drm/rocket: Check allocations before use

[PATCH] drm/rocket: Check allocations before use

[Triet Hoang]

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

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/accel/rocket/rocket_job.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..2fee21988d27 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 *));
 
@@ -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;
 
-- 
2.53.0

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

[sashiko-bot]

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

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

[Triet Hoang]

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

Changes in v2:
- Free scheds when drm_sched_entity_init() fails.
- Initialize ret to 0.

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/accel/rocket/rocket_job.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..24e1a61f71e7 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 *));
 
@@ -499,7 +502,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
 							 rdev->num_cores);
 	unsigned int core;
-	int ret;
+	int ret = 0;
+
+	if (!scheds)
+		return -ENOMEM;
 
 	for (core = 0; core < rdev->num_cores; core++)
 		scheds[core] = &rdev->cores[core].sched;
@@ -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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory leak of the dynamically allocated `scheds` array on the success path when `rdev->num_cores == 1`.
- [High] Use-after-free due to `sched_list` being freed before destroying the DRM scheduler entity.
- [High] API contract violation and fence/job leak on the error path after arming the scheduler job.
- [High] Silent discarding of job submission errors in the ioctl handler.
- [High] Data race on input buffer objects due to missing read dependency fences.
--

--- Patch [1]: [PATCH v2] drm/rocket: Check allocations before use ---
commit c8fde6c32c7f279bae49a07582e5353988f62959
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..24e1a61f71e7b 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 the error path later in this function
leak the job and fence?

After drm_sched_job_arm() is called, the job's s_fence is allocated
and an extra reference is taken via dma_fence_get(). If
rocket_acquire_object_fences() subsequently fails, the code jumps to
err_unlock:

rocket_job_push() {
    ...
    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;
    ...
}

Since this path does not call drm_sched_job_cleanup() or release the fence
reference via dma_fence_put(), does it leak these resources?

Furthermore, since drm_sched_job_arm() is a point of no return according to
the DRM scheduler rules, shouldn't all fallible operations be safely
completed before arming the job?


[Severity: High]
This is also a pre-existing issue in rocket_job_push(), but does this omit 
read dependencies for the input buffer objects?

At the end of the function, the execution fence is successfully attached to
the output buffers:

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

However, it seems to skip doing the same for job->in_bos with a 
DMA_RESV_USAGE_READ dependency.

If userspace submits a read job and then a write job to the same buffer
object, will the write job fail to wait for the read to finish, potentially
overwriting data while the NPU is still executing?

[ ... ]

> @@ -499,7 +502,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
>  							 rdev->num_cores);
>  	unsigned int core;
> -	int ret;
> +	int ret = 0;
> +
> +	if (!scheds)
> +		return -ENOMEM;
>  
>  	for (core = 0; core < rdev->num_cores; core++)
>  		scheds[core] = &rdev->cores[core].sched;
> @@ -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: High]
This isn't a bug introduced by this patch, but is it possible that the scheds
array is leaked on the success path when rdev->num_cores is 1?

Looking at drm_sched_entity_init(), it sets entity->sched_list to NULL if
num_sched_list is 1. If rocket_job_open() returns success here, the
scheds pointer is not tracked anywhere else. 

Later, when an unprivileged userspace application closes the device file and
rocket_job_close() is called, kfree(entity->sched_list) will evaluate to
kfree(NULL), leading to a memory leak per file open.

>  
>  void rocket_job_close(struct rocket_file_priv *rocket_priv)

[Severity: High]
This is a pre-existing issue, but does this sequence cause a use-after-free
in rocket_job_close()?

rocket_job_close() {
    ...
    kfree(entity->sched_list);
    drm_sched_entity_destroy(entity);
}

By calling kfree(entity->sched_list) immediately before 
drm_sched_entity_destroy(entity), the memory is freed while the entity might
still be active and flushing pending jobs.

If background scheduler threads attempt to evaluate runqueues or access
sched_list during the teardown phase in drm_sched_entity_destroy(), will
they access prematurely freed memory?


While reviewing the file, I noticed another pre-existing issue outside 
of the modified hunks.

[Severity: High]
This is a pre-existing issue, but does this loop silently discard job 
submission errors?

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

By ignoring the return value of rocket_ioctl_submit_job(), if submitting a
job fails due to memory allocation failure, invalid task count, or failed GEM
lookups, the loop continues and eventually returns 0 to userspace.

Will this give userspace no indication that one or more jobs were silently
dropped, potentially causing applications to hang while waiting for jobs
that were never pushed?

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

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

[Markus Elfring]

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

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n669
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n145
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2-rc7#n34


> Changes in v2:
…
> ---
>  drivers/accel/rocket/rocket_job.c | 12 +++++++++---
…

Please move patch version descriptions behind the marker line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n795

Regards,
Markus

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

[Triet Hoang]

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

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
Changes in v2:
- Free scheds when drm_sched_entity_init() fails.
- Initialize ret to 0.

Changes in v3:
- Move patch version descriptions below the '---' marker.

 drivers/accel/rocket/rocket_job.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..24e1a61f71e7 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 *));
 
@@ -499,7 +502,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
 							 rdev->num_cores);
 	unsigned int core;
-	int ret;
+	int ret = 0;
+
+	if (!scheds)
+		return -ENOMEM;
 
 	for (core = 0; core < rdev->num_cores; core++)
 		scheds[core] = &rdev->cores[core].sched;
@@ -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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Critical NULL pointer dereference in `rocket_job_cleanup()` leading to kernel panic.
- [High] Memory leak of `scheds` array in `rocket_job_open` on success.
- [High] Fallible operations placed after `drm_sched_job_arm()` violate the strict DRM scheduler contract.
- [High] Ignored return value in `rocket_ioctl_submit()` silently swallows failed job submissions.
--

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

drm/rocket: Check allocations before use

This commit adds missing allocation failure checks for kvmalloc_array() in
rocket_job_push() and kmalloc_objs() in rocket_job_open(). If the
allocations fail, the code now returns -ENOMEM to prevent using null
pointers.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index ac51bff39833f..24e1a61f71e7b 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 isn't a bug introduced by this patch, but can
rocket_acquire_object_fences() fail after drm_sched_job_arm() is called?

In rocket_job_push(), a job is armed via drm_sched_job_arm(), which
allocates its fence sequence numbers. Immediately after, a fallible
operation is called:

rocket_job_push() {
        ...
        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 job is aborted without being
pushed to the ring. Does this violate the DRM scheduler contract where
drm_sched_job_arm() is a point of no return? Aborting the job after it is
called creates permanent sequence number gaps.

[ ... ]

> @@ -499,7 +502,10 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
>  	struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
>  							 rdev->num_cores);
>  	unsigned int core;
> -	int ret;
> +	int ret = 0;
> +
> +	if (!scheds)
> +		return -ENOMEM;
>  
>  	for (core = 0; core < rdev->num_cores; core++)
>  		scheds[core] = &rdev->cores[core].sched;
> @@ -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: High]
This is a pre-existing issue, but does rocket_job_open() leak the scheds
array on success when the device only has 1 scheduler?

When rdev->num_cores == 1, drm_sched_entity_init() intentionally does not
store the sched_list pointer in the entity. rocket_job_open() then returns
success without saving or freeing the scheds pointer.

Additionally, in rocket_job_close(), the cleanup path calls
kfree(entity->sched_list), which is a no-op when it is NULL, leaving the
array permanently leaked.

[Severity: Critical]
This isn't a bug introduced by this patch, but can the cleanup path trigger
a NULL pointer dereference?

If a user submits a job with a malformed parameter and
rocket_ioctl_submit_job() fails early during rocket_copy_tasks(),
rocket_job_put() cleans up the partially initialized job. At this stage,
job->domain is still NULL.

rocket_job_cleanup() unconditionally calls the put function:

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

Which then dereferences the pointer without a NULL check:

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

Should rocket_job_cleanup() or rocket_iommu_domain_put() verify the domain
pointer is valid before attempting to drop the reference?

[Severity: High]
This is a pre-existing issue, but does rocket_ioctl_submit() silently drop
failed jobs?

If multiple jobs are submitted and one fails to process due to invalid BO
handles or memory allocation failures:

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

The loop entirely ignores the return value of rocket_ioctl_submit_job(),
skips the failed job, and continues. The ioctl ultimately returns 0 to
userspace, which might leave userspace falsely believing all jobs were
successfully queued.

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

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

[Markus Elfring]

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

* Would an other word wrap variant be nicer here?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n669

* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n145
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2-rc7#n34


Regards,
Markus

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

[Triet Hoang]

Hi Markus,

Thanks for the review. I'll update the patch accordingly in the next version.

Regards,
Triet

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

[Igor Paunovic]

Hi Triet,

Thanks for picking this up -- the rocket driver has few enough eyes on it
that allocation-check patches are welcome.

The rocket_job_push() half looks right to me. The early return skips the
err: label, but bos is NULL there anyway, and the caller
(rocket_ioctl_submit_job()) takes the goto out_cleanup_job path, which
does drm_sched_job_cleanup() and rocket_job_put(). Nothing is leaked and
nothing is armed yet, so returning early is safe.

On rocket_job_open(), the v2 change fixes the error path, but I think it
only covers half of what was reported. The other half is still there:
the array leaks on a single-core device even when nothing fails.

drm_sched_entity_init() stores the caller's array only when it will
actually need it:

	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;

(drivers/gpu/drm/scheduler/sched_entity.c, unchanged in current
mainline). And rocket_job_close() frees exactly that field:

	kfree(entity->sched_list);
	drm_sched_entity_destroy(entity);

So when rdev->num_cores == 1, drm_sched_entity_init() succeeds,
entity->sched_list is NULL, rocket_job_close() frees NULL, and the array
that rocket_job_open() allocated is never freed. One pointer per open(),
unbounded across open/close cycles.

That is not a hypothetical configuration. The RK3576 series currently on
the list enables exactly one core on the ROCK 4D -- its commit message
says so in as many words ("Only rknn_core_0 is enabled: the driver binds
one core per node and the second core is left to whoever can test it").
Any RK3588 DT that leaves a single core enabled lands in the same place.

I would suggest not depending on drm_sched_entity_init()'s internal
choice about sched_list at all: keep the pointer in rocket_file_priv and
free it unconditionally in rocket_job_close(). That covers one core and
many cores with the same line, and it stops rocket_job_close() from
reaching into a scheduler-internal field to decide what it owns. But
that is a bigger change than the one you set out to make, so it may be
better as a separate patch -- Tomeu's call.

Two smaller things:

  - With the check added, ret is assigned unconditionally from
    drm_sched_entity_init(), so the "int ret = 0" initialiser in v2 is
    no longer doing anything.

  - Heads-up on collision: I have a patch in flight that touches these
    same lines of rocket_job_open() ("accel/rocket: keep core slots
    stable across unbind and rebind", part of a two-patch lifecycle
    series). Whichever of us lands first, the other rebases -- I am happy
    for that to be me. Worth mentioning because in my version the count
    passed to drm_sched_entity_init() is the number of *live* cores, so
    once cores are unbound down to one, the leak above starts happening
    on a multi-core board too, at runtime.

I have not run your patch, so no tag from me. If it would help, I can
test it on RK3588 with three cores and again with two of them unbound,
and check the single-core case with kmemleak.

Regards,
Igor

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

[Triet Hoang]

From: Triet Hoang <triet.hoang.dev@gmail.com>

Hi Igor,

Thanks for the suggestion. I agree that keeping the allocation in rocket_file_priv 
would make the ownership clearer. I will implement this idea as a seperate patch.

Best Regards,
Triet

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

[Triet Hoang]

From: Triet Hoang <triet.hoang.dev@gmail.com>

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

Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
Changes in v2:
- Free scheds when drm_sched_entity_init() fails.
- Initialize ret to 0.

Changes in v3:
- Move patch version descriptions below the '---' marker.

Changes in v4:
- Remove unnecessary initialization of ret to 0.
- Adjust commit message word wrapping.
- Add Fixes tag.

 drivers/accel/rocket/rocket_job.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index ac51bff39833..adcc792541ec 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 *));
 
@@ -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;
 
@@ -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;
 }
 
 void rocket_job_close(struct rocket_file_priv *rocket_priv)
-- 
2.53.0

[PATCH v4 2/2] drm/rocket: Keep scheduler allocation in rocket_file_priv

[Triet Hoang]

From: Triet Hoang <triet.hoang.dev@gmail.com>

Keep the scheduler allocation in rocket_file_priv
and free it unconditionally in rocket_job_close().

Suggested-by: Igor Paunovic <royalnet026@gmail.com>
Link: https://lore.kernel.org/all/20260817093009.22359-1-royalnet026@gmail.com/#t
Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/accel/rocket/rocket_drv.h | 1 +
 drivers/accel/rocket/rocket_job.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/accel/rocket/rocket_drv.h b/drivers/accel/rocket/rocket_drv.h
index 2c673bb99ccc..9421e48ec5d8 100644
--- a/drivers/accel/rocket/rocket_drv.h
+++ b/drivers/accel/rocket/rocket_drv.h
@@ -23,6 +23,7 @@ struct rocket_file_priv {
 	struct drm_mm mm;
 	struct mutex mm_lock;
 
+	struct drm_gpu_scheduler **scheds;
 	struct drm_sched_entity sched_entity;
 };
 
diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index adcc792541ec..ff1d9e802024 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -514,8 +514,11 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 				    DRM_SCHED_PRIORITY_NORMAL,
 				    scheds,
 				    rdev->num_cores, NULL);
+
 	if (WARN_ON(ret))
 		kfree(scheds);
+	else
+		rocket_priv->scheds = scheds;
 
 	return ret;
 }
@@ -525,6 +528,7 @@ void rocket_job_close(struct rocket_file_priv *rocket_priv)
 	struct drm_sched_entity *entity = &rocket_priv->sched_entity;
 
 	kfree(entity->sched_list);
+	kfree(rocket_priv->scheds);
 	drm_sched_entity_destroy(entity);
 }
 
-- 
2.53.0

Re: [PATCH v4 2/2] drm/rocket: Keep scheduler allocation in rocket_file_priv

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Deterministic double free in rocket_job_close() for multi-core devices.
--

--- Patch [2]: [PATCH v4 2/2] drm/rocket: Keep scheduler allocation in rocket_file_priv ---
commit 3f1ce60677689fe14af070b96e041b01b9fbc718
Author: Triet Hoang <triet.hoang.dev@gmail.com>

drm/rocket: Keep scheduler allocation in rocket_file_priv

Keep the scheduler allocation in rocket_file_priv and free it
unconditionally in rocket_job_close().

Link: https://lore.kernel.org/all/20260817093009.22359-1-royalnet026@gmail.com/#t

[ ... ]
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index adcc792541ec1..ff1d9e802024d 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -525,6 +528,7 @@ void rocket_job_close(struct rocket_file_priv *rocket_priv)
>  	struct drm_sched_entity *entity = &rocket_priv->sched_entity;
>  
>  	kfree(entity->sched_list);
> +	kfree(rocket_priv->scheds);

[Severity: Critical]
Does this cause a double free of the scheds array on multi-core devices?

When the device has multiple cores (rdev->num_cores > 1),
drm_sched_entity_init() internally assigns the provided scheds array to
entity->sched_list. Since the patch additionally saves this same pointer
to rocket_priv->scheds, both entity->sched_list and rocket_priv->scheds
will point to the exact same heap allocation. 

By calling kfree() on both sequentially, are we freeing the same memory twice?

>  	drm_sched_entity_destroy(entity);
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817140110.148873-1-89897892+HMTCT@users.noreply.github.com?part=2

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

[sashiko-bot]

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 <triet.hoang.dev@gmail.com>

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/20260817140110.148873-1-89897892+HMTCT@users.noreply.github.com?part=1

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

[Triet Hoang]

Hi Igor,

After reading the code carefully, I think that we can check 
if entity->sched_list is NULL or not, then we can free the array
that rocket_job_open() allocated.

This will look like 
if (WARN_ON(ret) || !(&rocket_priv->sched_entity->sched_list))
 	kfree(scheds);

How do you think about that

Best Regards,
Triet