Re: [PATCH 3/3] drm/v3d: Use write_to_buffer() helper in performance query copy

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] User-controlled offsets, strides, and counts are used to access Buffer Object (BO) memory without any bounds checking, leading to arbitrary out-of-bounds kernel memory reads and writes.
- [Critical] The function `v3d_get_bo_vaddr()` does not check if `vmap()` returns NULL, which leads to a subsequent NULL pointer dereference.
--

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

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

The copy of performance query results to the output buffer is refactored
to use the write_to_buffer() helper instead of open-coding the
32-bit/64-bit selection. This replaces two nearly identical loops.

> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 53deae1a2eda9..17751c8d7f163 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -594,15 +594,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: Critical]
This is a pre-existing issue, but does this write out of bounds?

Userspace submits a CPU job (e.g., V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY)
providing offset and stride via DRM ioctls. These values are parsed without
being validated against the actual allocated size of the target BO.

In v3d_submit.c:v3d_get_cpu_copy_performance_query_params():
	job->copy.offset = copy.offset;
	job->copy.stride = copy.stride;

The unvalidated offset is used to compute the base write pointer in
v3d_sched.c:v3d_copy_performance_query():
	data = ((u8 *)bo->vaddr) + copy->offset;

This loop then blindly writes to data while incrementing the pointer by
copy->stride without checking boundaries, which can lead to arbitrary
out-of-bounds kernel memory reads and writes. Any unprivileged userspace
process that can open the DRM device can trigger this via
DRM_IOCTL_V3D_SUBMIT_CPU.

A similar unvalidated out-of-bounds access is present in other CPU jobs like
indirect CSD:

v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect():
	wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);

>  
>  		v3d_perfmon_put(perfmon);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does v3d_get_bo_vaddr() dereference a NULL
pointer when vmap() fails?

When the system is under memory pressure, vmap() can fail and return NULL.
However, v3d_get_bo_vaddr() assigns this NULL pointer directly to bo->vaddr
without checking:

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

Callers compute offsets from bo->vaddr and proceed to read or write to it
without any NULL checks. For example:

v3d_sched.c:v3d_copy_performance_query():
	v3d_get_bo_vaddr(bo);

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

This is reachable by an untrusted userspace program triggering CPU jobs via
DRM ioctls while the system is under memory pressure.

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