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

[PATCH v3 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)

v2 -> v3: https://lore.kernel.org/r/20260618-v3d-cpu-job-fixes-v2-0-fec132484fc4@igalia.com

- [1/3] Add a comment clarifing why we don't attach BOs to BIN jobs (Iago Toral)
- [1/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 | 44 +++++++++++++++++++++++-----------------
 2 files changed, 27 insertions(+), 28 deletions(-)
---
base-commit: 903f8773ee96c5dc5fb9aec65227f39fd3e7a1dc
change-id: 20260609-v3d-cpu-job-fixes-fbe2051bee3a

[PATCH v3 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.")
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_submit.c | 42 ++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index ee2ac2540ed5..9d23935b05dd 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,17 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
+	/*
+	 * We don't associate the BOs with the BIN job. Fences are only
+	 * attached to the last job in the submission chain, and BIN jobs
+	 * don't need implicit dependencies since depending on results from
+	 * another context is not a realistic scenario for binning.
+	 */
+	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 +1104,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 +1362,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

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

[sashiko-bot]

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

New issues:
- [Critical] V3D_CACHE_CLEAN jobs execute after their associated buffer objects (BOs) are freed, causing the GPU L2 cache to write dirty lines back to unmapped or reassigned physical memory.

Pre-existing issues:
- [High] The BIN job explicitly skips acquiring implicit dependencies, which violates the dma-buf implicit sync contract for imported buffers and allows the BIN job to execute concurrently with writers.
--

commit 9c4142c6fc75015db16c542de5fec66fe98c34f8
Author: Maíra Canal <mcanal@igalia.com>

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

This commit addresses an implicit synchronization issue by calling
v3d_lookup_bos() for each job that references the submission's BOs. This
gives earlier jobs in the submission chain their own implicit dependencies
during reservation locking.

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

[ ... ]

> @@ -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);
>  }

[Severity: Critical]
Does attaching the BOs exclusively to the CSD job leave the appended
CACHE_CLEAN job without references to keep the BOs alive?

If userspace submits a job with the flush cache flag and immediately closes
the BO handles, the CSD job's completion might drop the final reference to
the BOs. 

If the BOs are freed and unmapped before the CACHE_CLEAN job executes
v3d_clean_caches(), could the synchronous flush write dirty lines back to
reallocated physical memory?

[ ... ]

> @@ -1085,6 +1081,17 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto fail;
>  
> +	/*
> +	 * We don't associate the BOs with the BIN job. Fences are only
> +	 * attached to the last job in the submission chain, and BIN jobs
> +	 * don't need implicit dependencies since depending on results from
> +	 * another context is not a realistic scenario for binning.
> +	 */
> +	ret = v3d_lookup_bos(submit.file_priv, &render->base,
> +			     args->bo_handles, args->bo_handle_count);

[Severity: High]
This is a pre-existing issue, but does skipping the implicit dependencies
for the BIN job violate the dma-buf implicit sync contract for imported
buffers?

If userspace imports a dma-buf that is currently being written by another
context (like a compute shader or video decoder) and uses it for a BIN job,
the DRM scheduler will execute the BIN job immediately because it was given
zero implicit dependencies.

Could this cause the hardware to read from the buffer while the other context
is still writing to it, resulting in data corruption?

> +	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)) {

[Severity: Critical]
Similar to the CSD job in v3d_setup_csd_jobs_and_bos(), does associating
the BOs exclusively with the RENDER job leave the cache clean job without
references to the BOs?

If the BOs are freed after the RENDER job completes, could the CACHE_CLEAN job
flush the L2 cache back to freed physical addresses?

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

[PATCH v3 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 9d23935b05dd..656b0bbd4f8e 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

Re: [PATCH v3 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] Missing bounds checking of user-supplied offsets leads to arbitrary kernel memory read and write in CPU job execution functions.
- [Critical] Double free of `query_info->queries` arrays on error paths when processing timestamp or performance query extensions.
- [High] Missing return value check for `vmap()` failure in `v3d_get_bo_vaddr()` leads to NULL pointer dereferences.
- [High] Memory leak of `se.out_syncs` in non-CPU IOCTLs when `v3d_get_extensions` fails.
--

commit 3cd6d13669aae5071cfb72bf592569413ffc98ac
Author: Maíra Canal <mcanal@igalia.com>

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

This commit fixes a NULL pointer dereference in the v3d driver caused by
failing to check the return value of drm_gem_object_lookup(). It updates
v3d_get_cpu_indirect_csd_params() to bail out with -ENOENT if the userspace
supplied indirect BO handle is invalid.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 9d23935b05dd0..656b0bbd4f8e3 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;
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing the CPU job
code, does this lack bounds checking on the user-supplied offsets?

In 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];

And in v3d_timestamp_query():
    value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset;
    *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull;

When a user submits a CPU job (like indirect CSD or timestamp query), can
they pass arbitrary offset values? 

Since these offsets are added to the vmap'd kernel addresses of the BOs
during job execution in the scheduler thread, could this allow arbitrary
read and write access to kernel memory?

[Severity: High]
This is a pre-existing issue, but does this code fail to handle vmap()
returning NULL?

In 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));

If vmap() fails due to memory pressure, bo->vaddr is set to NULL but no
error is returned.

Callers then unconditionally use the mapped address assuming success:

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

Could this trigger a NULL pointer dereference when vmalloc space is
exhausted?

[Severity: High]
This isn't a bug introduced by this patch, but does this early return leak
se.out_syncs?

In drivers/gpu/drm/v3d/v3d_submit.c, non-CPU IOCTLs handle extension parsing
like this:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cl_ioctl() {
    ...
    ret = v3d_get_extensions(file_priv, args->extensions, &se, NULL);
    if (ret) {
        drm_dbg(dev, "Failed to get extensions.\n");
        return ret;
    }
    ...
}

If a user provides a valid DRM_V3D_EXT_ID_MULTI_SYNC extension followed by
an invalid one in the linked list, the MULTI_SYNC handler successfully
allocates se->out_syncs.

When parsing the invalid extension fails, v3d_get_extensions() returns an
error. By returning directly instead of jumping to the fail_submit cleanup
label, do we leak the struct v3d_submit_outsync allocations?

[Severity: Critical]
This is a pre-existing issue, but can this sequence lead to a double free?

When v3d_get_cpu_timestamp_query_params() encounters an error (like an
invalid syncobj), it calls v3d_timestamp_query_info_free():

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

This frees the array but leaves the pointer intact:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_timestamp_query_info_free() {
    ...
    kvfree(query_info->queries);
}

The IOCTL then propagates the error and jumps to cleanup, which unconditionally
destroys the job:

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

Since count remains 0, the inner loop is skipped, but does kvfree() get
called again on the dangling query_info->queries pointer?

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

[PATCH v3 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 e950cee31bcb..21ab9cd83685 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -605,15 +605,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 v3 3/3] drm/v3d: Use write_to_buffer() helper in performance query copy

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The V3D CPU job execution functions do not validate userspace-provided offsets, strides, and query counts against the GEM buffer size, allowing trivial out-of-bounds kernel memory reads and writes.
--

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

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

This commit updates the performance query result copy logic to use the
write_to_buffer() helper. This replaces the open-coded 32-bit and 64-bit
selection loops with a single simplified loop.

> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index e950cee31bcba..21ab9cd836856 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -605,15 +605,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: High]
This is a pre-existing issue, but does this code properly validate the bounds
of the destination buffer before writing?

Looking at v3d_write_performance_query_result() and the surrounding job
execution functions like v3d_copy_performance_query(), it appears the offset,
stride, and count values provided by userspace are used without being checked
against the GEM object size.

For example, when calculating the initial data pointer and incrementing it:

    data = ((u8 *)bo->vaddr) + copy->offset;
    for (int i = 0; i < performance_query->count; i++) {
        // ...
        data += copy->stride;
    }

The same pattern seems to exist in v3d_copy_query_results() for timestamp
queries:

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

Since these userspace-provided parameters are added directly to the buffer's
kernel virtual address, could this allow an out-of-bounds memory access if
large offset, stride, or count parameters are passed via the CPU job ioctl?

>  
>  		v3d_perfmon_put(perfmon);
>  	}

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