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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added bounds check for V3D_CPU_JOB_TYPE_INDIRECT_CSD jobs unconditionally dereferences the indirect BO, leading to a NULL pointer dereference if userspace provides an invalid GEM handle.

Pre-existing issues:
- [High] Missing NULL check on `vmap()` return value leads to kernel oops in CPU jobs during vmalloc exhaustion.
--

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

This patch validates user-supplied offsets and indices for V3D_SUBMIT_CPU
jobs against the bounds of their destination buffer objects before queuing.
It prevents render-node users from reading or writing past a BO's vmap
mapping by verifying the access extent and returning -EINVAL for invalid
geometry.

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")

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index ee2ac2540ed50..277823b774bed 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;
> +	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]);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the CPU
job executors to trigger a NULL pointer dereference if vmap fails?

During high memory pressure, vmap() can return NULL, but the result is
assigned without being checked:

v3d_get_bo_vaddr() {
    ...
    bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP,
                     pgprot_writecombine(PAGE_KERNEL));
}

Later, the CPU job executor unconditionally calculates an offset against
bo->vaddr and writes to it:

v3d_timestamp_query() {
    ...
    v3d_get_bo_vaddr(bo);

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

Does the driver need to check if bo->vaddr is valid before allowing these
writes to proceed?

> +
> +		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_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;
> +
> +		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;

[Severity: High]
Can dst be NULL here, leading to a NULL pointer dereference when accessing
dst->base.base.size?

If userspace provides an invalid indirect GEM handle, drm_gem_object_lookup()
returns NULL:

v3d_get_cpu_indirect_csd_params() {
    ...
    info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);
    
    return 0;
}

Since this lookup allows the ioctl to proceed with a NULL indirect BO, does
this bounds check need to verify dst is not NULL before dereferencing it?

> +		}
> +		return 0;
> +	}
> +	default:
> +		return 0;
> +	}
> +
> +err_range:
> +	drm_dbg(dev, "CPU job query offset exceeds the BO.\n");
> +	return -EINVAL;
> +}

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