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

[sashiko-bot@kernel.org]

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