[PATCH v2 0/3] drm/v3d: Miscellaneous fixes

[PATCH v2 0/3] drm/v3d: Miscellaneous fixes

[Maíra Canal]

Being brief, this small series collects a correctness fix (#1) reported by
Sashiko, a error-path fix (#2) reported by Sashiko, and a cleanup (#3)
found while reading the code.

The first patch is the only notable one: a v3d submission expands into a
chain of jobs (e.g. BIN + RENDER + CACHE CLEAN), but the BO list was only
ever attached to the last job. Since implicit synchronization is added
per-job and gated on job->bo_count, every job before the last one silently
skipped implicit dependencies. In practice the RENDER job could read a
buffer object while another context was still writing it. It's fixed by
looking up the BOs onto every job that references them.

Best regards,
- Maíra

To: Melissa Wen <mwen@igalia.com>
To: Iago Toral Quiroga <itoral@igalia.com>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
Cc: kernel-dev@igalia.com
Cc: dri-devel@lists.freedesktop.org

v1 -> v2: https://lore.kernel.org/r/20260610-v3d-cpu-job-fixes-v1-0-0d9c88989edc@igalia.com

- [1/3] Don't attach implicit dependencies to BIN jobs (Iago Toral)
- [2/3, 3/3] Add Iago's R-b (Iago Toral)

---
Maíra Canal (3):
      drm/v3d: Associate BOs with every job that accesses them
      drm/v3d: Reject invalid indirect BO handle in indirect CSD setup
      drm/v3d: Use write_to_buffer() helper in performance query copy

 drivers/gpu/drm/v3d/v3d_sched.c  | 11 ++---------
 drivers/gpu/drm/v3d/v3d_submit.c | 38 +++++++++++++++++++-------------------
 2 files changed, 21 insertions(+), 28 deletions(-)
---
base-commit: 98b46e693b912eef0e6d497327489113845cbd15
change-id: 20260609-v3d-cpu-job-fixes-fbe2051bee3a

[PATCH v2 1/3] drm/v3d: Associate BOs with every job that accesses them

[Maíra Canal]

A submission can expand into a chain of jobs (e.g. bin + render + cache
clean), but v3d_lookup_bos() only looked up the user's BO list onto the
*last* job of the submission. Every earlier job was left with
bo_count == 0 and an empty bo[] array.

As a consequence, when implicit synchronization happens in
v3d_submit_lock_reservations(), earlier jobs get no implicit
dependencies at all, as the loop is gated on job->bo_count and earlier
jobs don't have any BO attached to them. With that, the RENDER job could
therefore be dispatched to the hardware and read a BO while another context
was still writing it, leading to data corruption.

Fix this by calling v3d_lookup_bos() for each job that references the
submission's BOs, so every job carries its own bo[]/bo_count and picks
up the correct implicit dependencies during reservation locking.

Fixes: dffa9b7a78c4 ("drm/v3d: Add missing implicit synchronization.")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index ee2ac2540ed5..18467e448c91 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -68,7 +68,6 @@ v3d_submit_unlock_reservations(struct v3d_submit *submit)
 /**
  * v3d_lookup_bos() - Sets up job->bo[] with the GEM objects
  * referenced by the job.
- * @dev: DRM device
  * @file_priv: DRM file for this fd
  * @job: V3D job being set up
  * @bo_handles: GEM handles
@@ -82,23 +81,19 @@ v3d_submit_unlock_reservations(struct v3d_submit *submit)
  * failure, because that will happen at `v3d_job_free()`.
  */
 static int
-v3d_lookup_bos(struct v3d_submit *submit, u64 bo_handles, u32 bo_count)
+v3d_lookup_bos(struct drm_file *file_priv, struct v3d_job *job,
+	       u64 bo_handles, u32 bo_count)
 {
-	struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
-
-	last_job->bo_count = bo_count;
-
-	if (!last_job->bo_count) {
-		/* See comment on bo_index for why we have to check
-		 * this.
-		 */
-		drm_warn(&submit->v3d->drm, "Rendering requires BOs\n");
+	if (!bo_count) {
+		drm_warn(&job->v3d->drm, "Rendering requires BOs\n");
 		return -EINVAL;
 	}
 
-	return drm_gem_objects_lookup(submit->file_priv,
+	job->bo_count = bo_count;
+
+	return drm_gem_objects_lookup(file_priv,
 				      (void __user *)(uintptr_t)bo_handles,
-				      last_job->bo_count, &last_job->bo);
+				      job->bo_count, &job->bo);
 }
 
 static void
@@ -446,7 +441,8 @@ v3d_setup_csd_jobs_and_bos(struct v3d_submit *submit,
 	if (IS_ERR(clean_job))
 		return PTR_ERR(clean_job);
 
-	return v3d_lookup_bos(submit, args->bo_handles, args->bo_handle_count);
+	return v3d_lookup_bos(submit->file_priv, &job->base,
+			      args->bo_handles, args->bo_handle_count);
 }
 
 static void
@@ -1085,6 +1081,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
+	ret = v3d_lookup_bos(submit.file_priv, &render->base,
+			     args->bo_handles, args->bo_handle_count);
+	if (ret)
+		goto fail;
+
 	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
 		clean_job = v3d_submit_add_job(&submit, V3D_CACHE_CLEAN);
 		if (IS_ERR(clean_job)) {
@@ -1097,10 +1098,6 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
-	ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
-	if (ret)
-		goto fail;
-
 	ret = v3d_submit_lock_reservations(&submit);
 	if (ret)
 		goto fail;
@@ -1359,7 +1356,8 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 	 * the CSD and clean jobs in the case of indirect CSD job.
 	 */
 	if (args->bo_handle_count) {
-		ret = v3d_lookup_bos(&submit, args->bo_handles, args->bo_handle_count);
+		ret = v3d_lookup_bos(submit.file_priv, &cpu_job->base,
+				     args->bo_handles, args->bo_handle_count);
 		if (ret)
 			goto fail;
 	}

-- 
2.54.0

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

[Maíra Canal]

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.

Bail out with -ENOENT as soon as the lookup fails, so the bad handle is
rejected at submission time.

Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 18467e448c91..3a93dbfe71a2 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);
+	if (!info->indirect)
+		return -ENOENT;
 
 	return 0;
 }

-- 
2.54.0

[PATCH v2 3/3] drm/v3d: Use write_to_buffer() helper in performance query copy

[Maíra Canal]

The copy of performance query results to the output buffer open-codes
the 32-bit/64-bit selection with two nearly identical loops. As the
write_to_buffer() helper already encapsulates the do_64bit decision,
use it instead of open-coding it.

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 53deae1a2eda..17751c8d7f16 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -594,15 +594,8 @@ v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data,
 
 		v3d_perfmon_stop(v3d, perfmon, true);
 
-		if (job->copy.do_64bit) {
-			for (j = 0; j < perfmon->ncounters; j++)
-				write_to_buffer_64(data, offset + j,
-						   perfmon->values[j]);
-		} else {
-			for (j = 0; j < perfmon->ncounters; j++)
-				write_to_buffer_32(data, offset + j,
-						   perfmon->values[j]);
-		}
+		for (j = 0; j < perfmon->ncounters; j++)
+			write_to_buffer(data, offset + j, job->copy.do_64bit, perfmon->values[j]);
 
 		v3d_perfmon_put(perfmon);
 	}

-- 
2.54.0

Re: [PATCH v2 1/3] drm/v3d: Associate BOs with every job that accesses them

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix: `bin` job lacks implicit synchronization dependencies due to missing BO attachment.
--

commit 847118b5a0479c86d14ca61360fb6fc43c737179
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Associate BOs with every job that accesses them

This patch modifies the V3D driver to attach buffer objects (BOs) to every
job in a submission chain rather than just the last job. This is intended to
ensure all jobs receive the proper implicit synchronization dependencies
during reservation locking.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index ee2ac2540ed50..18467e448c91f 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c

[ ... ]

> @@ -1085,6 +1081,11 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto fail;
>  
> +	ret = v3d_lookup_bos(submit.file_priv, &render->base,
> +			     args->bo_handles, args->bo_handle_count);
> +	if (ret)
> +		goto fail;
> +

[Severity: High]
This is a pre-existing issue, but does the bin job also need to have BOs
attached to pick up the correct implicit dependencies?

The commit message indicates that v3d_lookup_bos() should be called for
each job that references the submission's BOs. However, it looks like
v3d_lookup_bos() is only being called for the render job here.

If a userspace client submits a binner command list, the bin job is created
earlier in v3d_submit_cl_ioctl() via v3d_submit_add_job(). Because
v3d_lookup_bos() is not called for it, bin->base.bo_count remains 0.

Could this cause the hardware to execute the bin job without implicit
synchronization, potentially leading to corrupted data if another context
is still writing to the BOs?

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

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

[sashiko-bot]

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

Re: [PATCH v2 3/3] drm/v3d: Use write_to_buffer() helper in performance query copy

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing bounds validation on user-controlled offsets and strides leads to arbitrary out-of-bounds kernel memory accesses.
- [Medium] Lockless read of 64-bit performance monitor counters can lead to torn reads and data races.
--

commit 142086c1a0afebcf8603865523c8298d5a01c455
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Use write_to_buffer() helper in performance query copy

The copy of performance query results to the output buffer open-codes
the 32-bit/64-bit selection with two nearly identical loops. As the
write_to_buffer() helper already encapsulates the do_64bit decision,
use it instead of open-coding it.

> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 53deae1a2eda9..17751c8d7f163 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -594,15 +594,8 @@ v3d_write_performance_query_result(struct v3d_cpu_job *job, void *data,
>  
>  		v3d_perfmon_stop(v3d, perfmon, true);
>  
> -		if (job->copy.do_64bit) {
> -			for (j = 0; j < perfmon->ncounters; j++)
> -				write_to_buffer_64(data, offset + j,
> -						   perfmon->values[j]);
> -		} else {
> -			for (j = 0; j < perfmon->ncounters; j++)
> -				write_to_buffer_32(data, offset + j,
> -						   perfmon->values[j]);
> -		}
> +		for (j = 0; j < perfmon->ncounters; j++)
> +			write_to_buffer(data, offset + j, job->copy.do_64bit, perfmon->values[j]);

[Severity: Medium]
This is a pre-existing issue, but can this lockless read of perfmon->values[j]
cause a torn read or data race on 32-bit architectures?

If a user submits a GPU job and a CPU copy job using the same perfmon ID
concurrently, the GPU job's completion might invoke v3d_perfmon_stop(),
which updates the values array under perfmon->lock, while the CPU job
concurrently reads it locklessly here.

>  
>  		v3d_perfmon_put(perfmon);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this code write outside the bounds
of bo->vaddr?

In v3d_submit.c:v3d_get_cpu_copy_performance_query_params(), the
user-provided offset and stride are accepted and stored without
validating them against the mapped BO size:

	job->copy.offset = copy.offset;
	job->copy.stride = copy.stride;

Later, in v3d_sched.c:v3d_copy_performance_query(), the unvalidated
offset is added to the vmap kernel virtual address:

	data = ((u8 *)bo->vaddr) + copy->offset;

And the destination pointer is incremented by an unvalidated stride for
subsequent writes:

	data += copy->stride;

Can this lead to out-of-bounds kernel memory accesses if an incorrect
offset or stride is supplied via the V3D_SUBMIT_CPU ioctl?

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