Re: [PATCH v2] drm/v3d: release CPU job resources on pre-init failures

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Double-free vulnerability of query arrays introduced by new error handling.

Pre-existing issues:
- [Critical] Deterministic NULL pointer dereference in `drm_sched_job_cleanup()` when pre-init validation fails.
- [High] Memory and syncobj leak in CL, CSD, and TFU ioctls on partial extension parsing failure.
- [High] Memory and WW-Mutex lock leak of nested jobs for INDIRECT_CSD on early failures.
--

--- Patch [1]: [PATCH v2] drm/v3d: release CPU job resources on pre-init failures ---
commit e05d6340b909c6cf82de569c1c19fd20b04f9a1f
Author: Guangshuo Li <lgs201920130244@gmail.com>
Date:   Mon Jul 13 16:05:31 2026 +0000

    drm/v3d: release CPU job resources on pre-init failures
    
    The changes referenced by the Fixes tags moved CPU job resource cleanup
    to v3d_cpu_job_free(), the CPU job kref destructor.
    
    However, CPU job extensions are parsed before v3d_job_init() has
    successfully installed that destructor. Timestamp and performance query
    arrays, their syncobj references, and the indirect CSD GEM reference can
    therefore be acquired before the CPU job is managed by the kref cleanup
    path.
    
    If a later pre-init validation fails, or if v3d_job_init() itself fails,
    the CPU job can be cleaned up or deallocated without running
    v3d_cpu_job_free(). Those extension resources are then leaked.
    
    Move the CPU-job-owned resource release into a helper and call it both
    from the normal CPU job destructor and from the pre-init error paths.
    
    Fixes: b0fe80c0b925 ("drm/v3d: Fix use-after-free of CPU job query arrays on error path")
    Fixes: 6eb6e5acafa4 ("drm/v3d: Release indirect CSD GEM reference on CPU job free")
    Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 1db43c6a078d5..7775fd4a9417b 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c

[ ... ]

> @@ -1311,6 +1316,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  		ret = v3d_get_extensions(file_priv, args->extensions, &se, cpu_job);
>  		if (ret) {
>  			drm_dbg(dev, "Failed to get extensions.\n");
> +			v3d_cpu_job_free_resources(cpu_job);

[Severity: Critical]
Does this new error handling introduce a double-free?

If v3d_get_cpu_timestamp_query_params() fails midway (for instance, if
drm_syncobj_find() returns NULL), it internally cleans up by calling
v3d_timestamp_query_info_free(). This frees the query array via kvfree()
but leaves a dangling pointer in job->timestamp_query.queries since it
isn't set to NULL.

Because v3d_cpu_job_free_resources() is now called on the failure path
here, it unconditionally calls v3d_timestamp_query_info_free() again.
The dangling pointer evaluates to true, resulting in a second kvfree()
on the same address.

[Severity: High]
This is a pre-existing issue, but does a failure in v3d_get_extensions() leak
syncobjs in other ioctls?

While v3d_submit_cpu_ioctl() uses goto fail to clean up the multisync
dependencies via v3d_put_multisync_post_deps(&se), v3d_submit_cl_ioctl() and
v3d_submit_csd_ioctl() use a direct return:

	if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
		ret = v3d_get_extensions(file_priv, args->extensions, &se, NULL);
		if (ret) {
			drm_dbg(dev, "Failed to get extensions.\n");
			return ret;
		}
	}

If a valid MULTI_SYNC extension is processed before an invalid extension,
this direct return bypasses the cleanup, permanently leaking the allocated
se.out_syncs array and the acquired syncobj references.

>  			goto fail;

[Severity: Critical]
This is a pre-existing issue, but can this goto fail result in a
NULL pointer dereference?

If we jump to fail before v3d_job_init() is called, cpu_job has only been
allocated (and zero-initialized). At the fail label, v3d_job_cleanup(cpu_job)
is called, which directly calls drm_sched_job_cleanup(&job->base).

Since the scheduler job wasn't initialized, job->s_fence is NULL, and
drm_sched_job_cleanup() will crash when attempting to read the fence
refcount via kref_read(&job->s_fence->finished.refcount).

>  		}
>  	}
>  
> @@ -1318,12 +1324,14 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  	/* Every CPU job must have a CPU job user extension */
>  	if (!cpu_job->job_type) {
>  		drm_dbg(dev, "CPU job must have a CPU job user extension.\n");
> +		v3d_cpu_job_free_resources(cpu_job);
>  		ret = -EINVAL;
>  		goto fail;

[Severity: High]
This is a pre-existing issue, but does jumping to fail here leak memory
and WW-Mutex locks for nested INDIRECT_CSD jobs?

If v3d_get_extensions() successfully parsed an INDIRECT_CSD extension, it
allocated cpu_job->indirect_csd.job and clean_job, and locked their BO
reservations.

Because the local variables csd_job and clean_job are only assigned later in
this function, they remain NULL during this early goto fail. When the fail
block calls v3d_job_cleanup() on them, it does nothing, bypassing
fail_unreserve and permanently abandoning both the allocations and the locks.

>  	}

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