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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Loop variable 'i' declared as signed 'int' causes negative indexing and out-of-bounds reads when 'tquery->count' exceeds INT_MAX.

Pre-existing issues:
- [Critical] Uninitialized CPU job structure cleanup causes a NULL pointer dereference in `drm_sched_job_cleanup()`.
- [Critical] Cleaning up CPU jobs that have already been pushed to the DRM scheduler queue causes severe state corruption and Use-After-Free.
- [High] Memory and WW mutex leaks for INDIRECT_CSD clean_job BO references on intermediate error paths.
- [High] Missing check for vmap() allocation failure leads to NULL pointer dereferences in CPU job executors.
- [High] The INDIRECT_CSD CPU job accesses the indirect GEM BO without extracting its implicit dependencies, causing it to race with prior GPU jobs.
- [High] Double vmap() on the same Buffer Object permanently leaks virtual memory mappings.
- [High] User-controlled wg_counts integer overflow evaluates to 0, triggering a WARN_ON and potential Denial of Service.
--

commit 176c9c3615bb9c83354ef76d006e602c0a5318e8
Author: Michael Bommarito <michael.bommarito@gmail.com>
Subject: drm/v3d: bound CPU-job query writes to their destination BO

The commit adds bounds checking for CPU jobs to ensure that user-supplied
offsets and indices do not result in reads or writes outside their
associated Buffer Objects (BOs), preventing potential out-of-bounds memory
accesses by render-node users.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 1db43c6a078d5..0d3a388fbba57 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
[ ... ]
> +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;
> +	int 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]. The
> +		 * offset is cast to u64 so a 0xffffffff query offset cannot wrap
> +		 * the addition past the BO size.
> +		 */
> +		dst = to_v3d_bo(job->base.bo[0]);
> +
> +		for (i = 0; i < tquery->count; i++) {

[Severity: Medium]
Is it possible for tquery->count to exceed the maximum value of a signed int?
If so, does the counter i wrap around to a negative number, causing an
out-of-bounds read when accessing tquery->queries[i].offset?

> +			if ((u64)tquery->queries[i].offset + sizeof(u64) >
> +			    dst->base.base.size)
> +				goto err_range;
> +		}
> +		return 0;
[ ... ]
> @@ -1346,6 +1478,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  		if (ret)
>  			goto fail;
>  
> +		ret = v3d_cpu_job_bounds_check(cpu_job);
> +		if (ret)
> +			goto fail;
> +

[Severity: High]
This is a pre-existing issue, but does jumping to the fail label here bypass
the fail_unreserve cleanup path?

For INDIRECT_CSD jobs, v3d_setup_csd_jobs_and_bos() dynamically allocates jobs
and locks the reservations for clean_job->bo. Does bypassing the
fail_unreserve label permanently leak the WW mutexes and cause deadlocks for
subsequent operations on these BOs?

>  		ret = v3d_lock_bo_reservations(&cpu_job->base, &acquire_ctx);
>  		if (ret)
>  			goto fail;

While reviewing this patch, I also noticed a few pre-existing issues in the
surrounding code:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cpu_ioctl() {
    ...
    if (args->bo_handle_count != cpu_job_bo_handle_count[cpu_job->job_type]) {
        drm_dbg(dev, "This CPU job was not submitted with the proper number of BOs.\n");
        ret = -EINVAL;
        goto fail;
    }
    ...
fail:
    v3d_job_cleanup((void *)cpu_job);
}

[Severity: Critical]
This is a pre-existing issue, but since the error occurs before
v3d_job_init() is called, cpu_job->base.s_fence is NULL. Does the
subsequent call to drm_sched_job_cleanup() unconditionally dereference
job->s_fence->finished.refcount, causing a kernel panic?


drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cpu_ioctl() {
    ...
    v3d_push_job(&cpu_job->base);
    ...
    case V3D_CPU_JOB_TYPE_INDIRECT_CSD:
        ret = drm_sched_job_add_dependency(&csd_job->base.base,
                                           dma_fence_get(cpu_job->base.done_fence));
        if (ret)
            goto fail_unreserve;
    ...
fail_unreserve:
    ...
fail:
    v3d_job_cleanup((void *)cpu_job);
}

[Severity: Critical]
This is a pre-existing issue, but when goto fail_unreserve falls through
to the fail label and calls v3d_job_cleanup(cpu_job), does this corrupt the
state of jobs that have already been armed and pushed to the active DRM
scheduler queue?


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

[Severity: High]
This is a pre-existing issue, but the result of vmap() is assigned directly
to bo->vaddr. If vmap() returns NULL due to memory fragmentation, do CPU
job executors like v3d_rewrite_csd_job_wg_counts_from_indirect() blindly
dereference this NULL pointer?


drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_indirect_csd_params() {
    ...
    info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);
    ...
}

[Severity: High]
This is a pre-existing issue, but since the indirect BO is not added to
cpu_job->base.bo, it appears to bypass v3d_lock_bo_reservations(). Does this
allow v3d_rewrite_csd_job_wg_counts_from_indirect() to read and modify the BO
while a prior GPU dispatch might still be writing to it?


drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() {
    ...
    v3d_get_bo_vaddr(bo);
    v3d_get_bo_vaddr(indirect);
    ...
}

[Severity: High]
This is a pre-existing issue, but if a user provides the exact same GEM
object handle for both the primary bo and the indirect BO, does the second
vmap() silently overwrite the vaddr pointer from the first, permanently
leaking the prior virtual memory mapping?


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]);
    ...
    args->cfg[4] = num_batches - 1;
    ...
    WARN_ON(args->cfg[4] == ~0);
}

[Severity: High]
This is a pre-existing issue, but since wg_counts are user-supplied 32-bit
unsigned integers, can specific inputs cleanly overflow the product to 0?
Does this underflow args->cfg[4] to 0xFFFFFFFF and trigger the WARN_ON,
potentially crashing systems with panic_on_warn enabled?

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