[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

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

[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

[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

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

[Markus Elfring]

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

I see opportunities for corresponding collateral evolution.
How do you think about to apply the attribute “__free(kvfree)” also in such
a function implementation by another update step?
https://elixir.bootlin.com/linux/v7.2-rc7/source/include/linux/slab.h#L1420

Regards,
Markus

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

[Markus Elfring]

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

Why do you think that an additional blank would be helpful at this place?

Regards,
Markus

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

[Triet Hoang]

> I see opportunities for corresponding collateral evolution.
> How do you think about to apply the attribute “__free(kvfree)” also in such
> a function implementation by another update step?

I am not sure I understand your idea.
Could you explain more? This attribute is a new thing to media

Regards,
Triet

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

[Triet Hoang]

> This will look like 
> if (WARN_ON(ret) || !(&rocket_priv->sched_entity->sched_list))
>  	kfree(scheds);
> How do you think about that

Hi Igor,

This is a bad idea. Sorry about that and just forget it.

Regards,
Triet

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

[Triet Hoang]

> This will look like 
> if (WARN_ON(ret) || !(&rocket_priv->sched_entity->sched_list))
>  	kfree(scheds);
> How do you think about that

Hi Igor,

This is a bad idea. Sorry about that and just forget it.

Regards,
Triet

[PATCH v5 1/2] 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
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.

Changes in v5:
- Add check overflow before kvmalloc_array() in rocket_job_push().
---
 drivers/accel/rocket/rocket_job.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

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

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

[Triet Hoang]

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/
Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
Changses in v5:
- Free rocket_priv->scheds instead of entity->sched_list in rocket_job_close().
---
 drivers/accel/rocket/rocket_drv.h | 1 +
 drivers/accel/rocket/rocket_job.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

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 15e93c355c38..50b6e0e06ed8 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -519,6 +519,8 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv)
 				    rdev->num_cores, NULL);
 	if (WARN_ON(ret))
 		kfree(scheds);
+	else
+		rocket_priv->scheds = scheds;
 
 	return ret;
 }
@@ -527,7 +529,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 v5] drm/rocket: Check allocations before use

[Triet Hoang]

Hi Everyone,

Thanks for the detailed review. This is my first time contributing to the
kernel and using the mailing list, so I really appreciate your feedback.

The thread has become quite long, so I'll leave it here for now and wait
for feedback from Tomeu before making further changes.

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

I don't have the hardware to test these cases myself, so it would be very
helpful if you could run the patch (v5) and share any feedback. It would also
help me better understand the behavior of the driver and the testing
process.

Thanks again for taking the time to review and test it.

Regards,
Triet

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

[sashiko-bot]

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 <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
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/20260818041505.1579320-1-triet.hoang.dev@gmail.com?part=1

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

[Markus Elfring]

>> I see opportunities for corresponding collateral evolution.
>> How do you think about to apply the attribute “__free(kvfree)” also in such
>> a function implementation by another update step?
> 
> I am not sure I understand your idea.
> Could you explain more? This attribute is a new thing to media
There are programming interfaces supported to some degree
for the application of scope-based resource management.
https://elixir.bootlin.com/linux/v7.2-rc7/source/include/linux/cleanup.h#L10

Would you like to take any adjustment possibilities better into account accordingly?

Regards,
Markus

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

[Triet Hoang]

> > There are programming interfaces supported to some degree
> for the application of scope-based resource management.
> https://elixir.bootlin.com/linux/v7.2-rc7/source/include/linux/cleanup.h#L10
> 
> Would you like to take any adjustment possibilities better into account accordingly?

Thanks for explaining. I am happy to continue working on this, but I think
this would be more of a refactoring to use the new API than a bug fix.
Furthermore, I don't have much experience with the new cleanup API yet, so
I think it would be better to consider this as a separate change in the
future, after these patches have been reviewed and I have gained more
experience with the new API.

Anyway, thanks for suggesting this. I'll take a look at it and keep it in
mind for future changes.

Regards,
Triet