Re: [PATCH v2 2/3] drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Double-free of `query_info->queries` array during CPU job extension parsing failure.
- [Critical] Arbitrary kernel memory read and write primitive due to missing bounds checks on CPU job offsets.
- [High] Race condition and missing implicit synchronization due to unlocked `indirect_csd.indirect` GEM object.
- [High] Kernel crash (NULL pointer dereference) in `v3d_get_bo_vaddr()` when mapping imported PRIME buffers.
--

commit 4335f305c6546e229d21733e148fae0311963d61
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

v3d_get_cpu_indirect_csd_params() looks up the indirect buffer object
from a userspace-supplied handle but never checks the result. A bogus
or stale handle makes drm_gem_object_lookup() return NULL, which is
then stored in info->indirect and only dereferenced later when the
indirect CSD job runs, turning a userspace mistake into a NULL pointer
dereference in the kernel.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 18467e448c91f..3a93dbfe71a2c 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -601,6 +601,8 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
>  	       sizeof(indirect_csd.wg_uniform_offsets));
>  
>  	info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);

[Severity: High]
This is a pre-existing issue, but by not adding this buffer to the tracked bo
array, are we skipping implicit synchronization?

The indirect_csd.indirect GEM object is looked up and stored here, but it
doesn't appear to be appended to the job->base.bo array. As a result, does
v3d_submit_lock_reservations() bypass dma_resv locking and
drm_sched_job_add_implicit_dependencies() completely for this buffer?

When the CPU job later executes v3d_rewrite_csd_job_wg_counts_from_indirect():

drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() {
	...
	((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i];
	...
}

Will this modify the buffer concurrently without holding locks or waiting for
prior GPU operations? Note that the patch "drm/v3d: Associate BOs with every
job that accesses them" doesn't seem to cover indirect_csd.indirect.

[Severity: High]
This is a pre-existing issue, but does mapping this buffer safely handle
imported PRIME buffers?

If info->indirect is an imported PRIME buffer,
drm_gem_shmem_prime_import_sg_table() only populates the scatter-gather table
(shmem->sgt) and leaves shmem->pages as NULL.

When vmap() is eventually called on it:

drivers/gpu/drm/v3d/v3d_bo.c:v3d_get_bo_vaddr() {
	...
	bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP,
			 pgprot_writecombine(PAGE_KERNEL));
	...
}

Could this pass a NULL page array to vmap() and trigger a kernel crash?

[Severity: Critical]
This is a pre-existing issue, but are the offsets used for this indirect buffer
checked against the bounds of the allocation?

In v3d_rewrite_csd_job_wg_counts_from_indirect(), the driver reads and writes
using the user-controlled offsets from indirect_csd.offset and
indirect_csd.wg_uniform_offsets:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() {
	...
	wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
	...
	((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i];
	...
}

Are these offsets ever bounds-checked against the size of the respective
vmapped buffer objects? Could an unprivileged user supply arbitrary 32-bit
offsets to achieve out-of-bounds reads and writes relative to the vmap
allocations? (Similar missing bounds checks appear to exist in
v3d_timestamp_query() and v3d_reset_timestamp_queries()).

> +	if (!info->indirect)
> +		return -ENOENT;

[Severity: Critical]
This isn't a bug introduced by this patch, but could returning an error here
trigger a double-free if a timestamp query was parsed earlier in the same job?

If a failure occurs here (or in other CPU job parsing functions) and returns
an error to v3d_submit_cpu_ioctl(), it seems job cleanup is triggered via
v3d_submit_cleanup_jobs(), which unconditionally calls v3d_cpu_job_free().

If an error path already called v3d_timestamp_query_info_free() to clean up
the partially initialized array:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_timestamp_query_params() {
	...
error:
	v3d_timestamp_query_info_free(&job->timestamp_query, i);
	...
}

This executes kvfree(query_info->queries) but leaves the pointer dangling.
During the subsequent teardown:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_cpu_job_free() {
	...
	v3d_timestamp_query_info_free(&job->timestamp_query,
				      job->timestamp_query.count);
	...
}

Will this call kvfree() again on the same dangling pointer, resulting in a
double-free?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-v3d-cpu-job-fixes-v2-0-fec132484fc4@igalia.com?part=2