[PATCH v5] drm/v3d: bound CPU-job query writes to their destination BO

[PATCH v5] drm/v3d: bound CPU-job query writes to their destination BO

[Michael Bommarito]

The V3D_SUBMIT_CPU CPU jobs take user-supplied offsets and indices and
consume them at exec time without checking that the accesses stay inside
their BO:

  - TIMESTAMP_QUERY and RESET_TIMESTAMP_QUERY write one u64 per query
    into bo[0] at a fully user-controlled per-query offset.
  - COPY_TIMESTAMP_QUERY copies one u64 per query into bo[0] at
    offset + i * stride, and reads each result from a user-controlled
    offset in the source bo[1].
  - COPY_PERFORMANCE_QUERY writes nperfmons * DRM_V3D_MAX_PERF_COUNTERS
    counter slots plus an availability slot into bo[0] at the same
    geometry.
  - INDIRECT_CSD reads three u32 work-group counts from bo[0] at a
    user-controlled offset, then writes each count back into the
    indirect BO at a user-controlled u32 index (wg_uniform_offsets[]).

A render-node user (DRM_RENDER_ALLOW, no master, no capability) can make
the handlers read or write past a BO's vmap mapping.

Validate the full access extent against the BO size once the BOs are
looked up, before the job is queued, rejecting out-of-range geometry
with -EINVAL. The copy extent offset + (count - 1) * stride + write_size
is computed in u64, mirroring the u8 * pointer arithmetic in the
executors: (count - 1) * stride is a u32 * u32 product that is exact in
u64, so one overflow check on the total guards the bound. The
performance slot count and the bare timestamp, copy-source and indirect
offsets are computed in u64 the same way, so a user value cannot wrap
the comparison.

Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job")
Fixes: 9ba0ff3e083f ("drm/v3d: Create a CPU job extension for the timestamp query job")
Fixes: 34a101e64296 ("drm/v3d: Create a CPU job extension for the reset timestamp job")
Fixes: 6745f3e44a20 ("drm/v3d: Create a CPU job extension to copy timestamp query to a buffer")
Fixes: 209e8d2695ee ("drm/v3d: Create a CPU job extension for the copy performance query job")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
v5:
 - Reinstate a minimal NULL check on the indirect BO in the INDIRECT_CSD
   case. On drm-misc-next today (before the parse-time reject in [1]
   lands) a bad indirect GEM handle leaves ->indirect NULL, so this
   bounds check would dereference NULL; the Sashiko review of v4 flagged
   the same. The check is redundant once [1] lands but keeps this patch
   correct on its own.

v4:
 - Rebase onto drm-misc-next (Maira Canal): this is a fix, but a
   reasonably large change and not a critical bug.
 - Reconcile with the "drm/v3d: Miscellaneous fixes" series [1]. Drop the
   INDIRECT_CSD NULL indirect-BO reject: [1] patch 2/3 ("drm/v3d: Reject
   invalid indirect BO handle in indirect CSD setup") already returns
   -ENOENT for a bad handle at parse time, so that check is redundant.
   This patch assumes that series lands first. The out-of-bounds
   offset/index checks here are still novel and unfixed by [1].
 - Declare the loop index i as u32 rather than int (Sashiko, Maira
   Canal): a signed index could go negative once count exceeds INT_MAX.
 - Trim the TIMESTAMP_QUERY comment and restore the v2 wording of the
   COPY_PERFORMANCE_QUERY comment (Maira Canal).
 - INDIRECT_CSD: name the rewrite target dst and the counts BO src, add
   the "3 == x, y, z workgroup dims" and 0xffffffff-sentinel comments,
   and rename the local to indirect_csd (Maira Canal).

[1] https://lore.kernel.org/dri-devel/20260703-v3d-cpu-job-fixes-v3-0-bc51b1f3eeb5@igalia.com/
v3: https://lore.kernel.org/all/20260617125811.3692951-1-michael.bommarito@gmail.com/
v2: https://lore.kernel.org/all/20260614211644.217116-1-michael.bommarito@gmail.com/
v1: https://lore.kernel.org/all/20260614131058.2525157-1-michael.bommarito@gmail.com/

 drivers/gpu/drm/v3d/v3d_submit.c | 134 +++++++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index ee2ac2540ed50..14d0b46a6c105 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -4,6 +4,8 @@
  * Copyright (C) 2023 Raspberry Pi
  */
 
+#include <linux/overflow.h>
+
 #include <drm/drm_print.h>
 #include <drm/drm_syncobj.h>
 
@@ -1300,6 +1302,134 @@ static const unsigned int cpu_job_bo_handle_count[] = {
 	[V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY] = 1,
 };
 
+/* Reject offset + (count - 1) * stride + write_size if it leaves the BO. */
+static int
+v3d_check_copy_extent(struct drm_device *dev, size_t bo_size,
+		      u32 offset, u32 stride, u32 count, u64 write_size)
+{
+	u64 last;
+
+	if (!count)
+		return 0;
+
+	/*
+	 * The executors walk a u8 * cursor, so the furthest written byte is
+	 * offset + (count - 1) * stride + write_size, matching the pointer
+	 * arithmetic in v3d_copy_query_results()/v3d_copy_performance_query().
+	 * (count - 1) * stride is a u32 * u32 product that is exact in u64,
+	 * and offset + write_size stays far below the u64 range, so a single
+	 * overflow check guards the total.
+	 */
+	last = write_size + offset;
+	if (check_add_overflow((u64)(count - 1) * stride, last, &last) ||
+	    last > bo_size) {
+		drm_dbg(dev, "CPU job copy buffer exceeds the destination BO.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* Reject a query CPU job whose writes would land outside their BO. */
+static int
+v3d_cpu_job_bounds_check(struct v3d_cpu_job *job)
+{
+	struct drm_device *dev = &job->base.v3d->drm;
+	struct v3d_timestamp_query_info *tquery = &job->timestamp_query;
+	struct v3d_copy_query_results_info *copy = &job->copy;
+	u32 elem = copy->do_64bit ? sizeof(u64) : sizeof(u32);
+	struct v3d_bo *dst, *src;
+	u64 slots, write_size;
+	u32 i;
+
+	switch (job->job_type) {
+	case V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY:
+	case V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY:
+		/* Each query writes one u64 timestamp slot into bo[0]. */
+		dst = to_v3d_bo(job->base.bo[0]);
+
+		for (i = 0; i < tquery->count; i++) {
+			if ((u64)tquery->queries[i].offset + sizeof(u64) >
+			    dst->base.base.size)
+				goto err_range;
+		}
+		return 0;
+	case V3D_CPU_JOB_TYPE_COPY_TIMESTAMP_QUERY:
+		/* Copies one u64 per query from bo[1] into bo[0]. */
+		dst = to_v3d_bo(job->base.bo[0]);
+		src = to_v3d_bo(job->base.bo[1]);
+
+		for (i = 0; i < tquery->count; i++) {
+			if ((u64)tquery->queries[i].offset + sizeof(u64) >
+			    src->base.base.size)
+				goto err_range;
+		}
+
+		write_size = (copy->availability_bit ? 2 : 1) * elem;
+		return v3d_check_copy_extent(dev, dst->base.base.size,
+					     copy->offset, copy->stride,
+					     tquery->count, write_size);
+	case V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY:
+		/*
+		 * Each query writes nperfmons * DRM_V3D_MAX_PERF_COUNTERS
+		 * counter slots into bo[0], plus an availability slot at
+		 * index ncounters. nperfmons and ncounters are user values,
+		 * so the slot count is computed overflow-safe.
+		 */
+		dst = to_v3d_bo(job->base.bo[0]);
+
+		slots = (u64)job->performance_query.nperfmons *
+			DRM_V3D_MAX_PERF_COUNTERS;
+		if (copy->availability_bit)
+			slots = max(slots,
+				    (u64)job->performance_query.ncounters + 1);
+
+		write_size = slots * elem;
+		return v3d_check_copy_extent(dev, dst->base.base.size,
+					     copy->offset, copy->stride,
+					     job->performance_query.count,
+					     write_size);
+	case V3D_CPU_JOB_TYPE_INDIRECT_CSD: {
+		struct v3d_indirect_csd_info *indirect_csd = &job->indirect_csd;
+
+		/* 3 is the three dimensions (x, y, z) of the workgroup counts. */
+		src = to_v3d_bo(job->base.bo[0]);
+		if ((u64)indirect_csd->offset + 3 * sizeof(u32) >
+		    src->base.base.size)
+			goto err_range;
+
+		/*
+		 * A bad indirect BO handle leaves ->indirect NULL. Reject it
+		 * here so neither this check nor the exec-time rewrite
+		 * dereferences NULL.
+		 */
+		if (!indirect_csd->indirect)
+			return -ENOENT;
+
+		dst = to_v3d_bo(indirect_csd->indirect);
+		for (i = 0; i < 3; i++) {
+			u32 uidx = indirect_csd->wg_uniform_offsets[i];
+
+			/*
+			 * 0xffffffff means "skip this rewrite", so the exec
+			 * path never writes that index and it needs no check.
+			 */
+			if (uidx != 0xffffffff &&
+			    (u64)uidx * sizeof(u32) + sizeof(u32) >
+			    dst->base.base.size)
+				goto err_range;
+		}
+		return 0;
+	}
+	default:
+		return 0;
+	}
+
+err_range:
+	drm_dbg(dev, "CPU job query offset exceeds the BO.\n");
+	return -EINVAL;
+}
+
 /**
  * v3d_submit_cpu_ioctl() - Submits a CPU job to the V3D.
  * @dev: DRM device
@@ -1364,6 +1494,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
 			goto fail;
 	}
 
+	ret = v3d_cpu_job_bounds_check(cpu_job);
+	if (ret)
+		goto fail;
+
 	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
 		ret = v3d_setup_csd_jobs_and_bos(&submit, &cpu_job->indirect_csd.args,
 						 NULL);

base-commit: 1acd235bc8b8cf62da1a2b497f3e8c5cf01a2205
-- 
2.53.0

Re: [PATCH v5] drm/v3d: bound CPU-job query writes to their destination BO

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Double free of query arrays when an error occurs during CPU job parameter parsing.
- [High] vmap memory leak when multiple job BOs point to the same GEM object.
- [High] Unchecked vmap failure leads to NULL pointer dereference in CPU job executors.
- [High] Implicit dma_resv synchronization is bypassed for early jobs in a multi-job submission.
- [High] User-controlled values can trigger WARN_ON and potential panic in CSD job setup.
- [High] Unaligned pointer dereferences in CPU jobs can trigger kernel alignment faults.
--

--- Patch [1]: [PATCH v5] drm/v3d: bound CPU-job query writes to their destination BO ---
commit a2da5ab54906c3c7d9b8afa2e86a50fb3f868e0a
Author: Michael Bommarito <michael.bommarito@gmail.com>
Subject: drm/v3d: bound CPU-job query writes to their destination BO

This commit validates the full access extent against the BO size once
the BOs are looked up, before the job is queued, rejecting out-of-range
geometry with -EINVAL.

Links:
[1] https://lore.kernel.org/dri-devel/20260703-v3d-cpu-job-fixes-v3-0-bc51b1f3eeb5@igalia.com/
v3: https://lore.kernel.org/all/20260617125811.3692951-1-michael.bommarito@gmail.com/
v2: https://lore.kernel.org/all/20260614211644.217116-1-michael.bommarito@gmail.com/
v1: https://lore.kernel.org/all/20260614131058.2525157-1-michael.bommarito@gmail.com/

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index ee2ac2540ed50..14d0b46a6c105 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
[ ... ]
> @@ -1300,6 +1302,134 @@ static const unsigned int cpu_job_bo_handle_count[] = {
[ ... ]
> +	switch (job->job_type) {
> +	case V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY:
> +	case V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY:
> +		/* Each query writes one u64 timestamp slot into bo[0]. */
> +		dst = to_v3d_bo(job->base.bo[0]);
> +
> +		for (i = 0; i < tquery->count; i++) {
> +			if ((u64)tquery->queries[i].offset + sizeof(u64) >
> +			    dst->base.base.size)
> +				goto err_range;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but does this offset
calculation risk triggering a kernel alignment fault?

While this code validates that the user-supplied offset is within bounds,
the executor uses it directly in a 64-bit store without checking alignment:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_timestamp_query() {
    ...
    value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset;
    *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull;
}

If userspace provides a misaligned offset on a strict-alignment
architecture (like ARM32), will this direct cast and dereference trigger
a fatal hardware alignment fault?

[Severity: High]
This isn't a bug introduced by this patch, but does the executor properly
handle vmap() failures?

If vmap() fails due to memory pressure, v3d_get_bo_vaddr() assigns NULL
to bo->vaddr. Later, CPU job executors like v3d_timestamp_query()
calculate the address:

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

If bo->vaddr is NULL, this resolves to just the offset. Will this cause
a fatal page fault by dereferencing an unmapped or user-space address from
the kernel thread?

> +		return 0;
> +	case V3D_CPU_JOB_TYPE_COPY_TIMESTAMP_QUERY:
> +		/* Copies one u64 per query from bo[1] into bo[0]. */
> +		dst = to_v3d_bo(job->base.bo[0]);
> +		src = to_v3d_bo(job->base.bo[1]);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a memory
leak if userspace provides the same GEM object for multiple BOs?

In v3d_copy_query_results(), both BOs are mapped:

    v3d_get_bo_vaddr(bo);
    v3d_get_bo_vaddr(timestamp);

If bo and timestamp resolve to the same struct v3d_bo pointer,
v3d_get_bo_vaddr() will unconditionally overwrite bo->vaddr:

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

Does this permanently leak the first vmap allocation since the cleanup
path will only unmap the second one?

[ ... ]
> +	case V3D_CPU_JOB_TYPE_INDIRECT_CSD: {
> +		struct v3d_indirect_csd_info *indirect_csd = &job->indirect_csd;
> +
> +		/* 3 is the three dimensions (x, y, z) of the workgroup counts. */
> +		src = to_v3d_bo(job->base.bo[0]);
> +		if ((u64)indirect_csd->offset + 3 * sizeof(u32) >
> +		    src->base.base.size)
> +			goto err_range;

[Severity: High]
This isn't a bug introduced by this patch, but can user-controlled values
trigger a kernel crash or WARN_ON in the CSD job setup?

In v3d_rewrite_csd_job_wg_counts_from_indirect(), num_batches is
calculated from user inputs without validating wg_size:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() {
    ...
    num_batches = DIV_ROUND_UP(indirect_csd->wg_size, 16) *
                  (wg_counts[0] * wg_counts[1] * wg_counts[2]);

    if (v3d->ver < 71 || (v3d->ver == 71 && v3d->rev < 6))
        args->cfg[4] = num_batches - 1;
    ...
    WARN_ON(args->cfg[4] == ~0);
}

If userspace submits a job with wg_size = 0, num_batches becomes 0, and
args->cfg[4] underflows to ~0. Will this intentionally trigger the WARN_ON
and panic the kernel if panic_on_warn is enabled?

[ ... ]
> @@ -1364,6 +1494,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  			goto fail;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but do early jobs in a multi-job
submission bypass implicit fencing?

When v3d_lookup_bos() processes the BOs, it assigns them all to the last
job in the submission:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_lookup_bos() {
    ...
    struct v3d_job *last_job = submit->jobs[submit->job_count - 1];
    last_job->bo_count = bo_count;
}

Later, v3d_submit_lock_reservations() iterates through the jobs and only
adds implicit dependencies if job->bo_count > 0:

    for (j = 0; j < job->bo_count; j++) {
        ret = drm_sched_job_add_implicit_dependencies(&job->base,
                                                      job->bo[j],
                                                      true);

Will early jobs like 'bin' or 'csd' be submitted to the GPU scheduler with
no implicit dependencies, potentially racing with active BO users?

> +	ret = v3d_cpu_job_bounds_check(cpu_job);
> +	if (ret)
> +		goto fail;

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error path in
v3d_get_cpu_timestamp_query_params() lead to a double free?

If v3d_get_cpu_timestamp_query_params() encounters an error, it calls:

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

Then the error propagates to v3d_submit_cpu_ioctl(), which jumps to the
fail label and calls v3d_submit_cleanup_jobs(), eventually reaching:

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 second call execute kvfree() again on the dangling pointer?

> +
>  	if (cpu_job->job_type == V3D_CPU_JOB_TYPE_INDIRECT_CSD) {
>  		ret = v3d_setup_csd_jobs_and_bos(&submit, &cpu_job->indirect_csd.args,
>  						 NULL);

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