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

[sashiko-bot@kernel.org]

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