Re: [PATCH v2 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] Missing bounds validation on user-controlled offsets and strides leads to arbitrary out-of-bounds kernel memory accesses.
- [Medium] Lockless read of 64-bit performance monitor counters can lead to torn reads and data races.
--

commit 142086c1a0afebcf8603865523c8298d5a01c455
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 open-codes
the 32-bit/64-bit selection with two nearly identical loops. As the
write_to_buffer() helper already encapsulates the do_64bit decision,
use it instead of open-coding it.

> 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: Medium]
This is a pre-existing issue, but can this lockless read of perfmon->values[j]
cause a torn read or data race on 32-bit architectures?

If a user submits a GPU job and a CPU copy job using the same perfmon ID
concurrently, the GPU job's completion might invoke v3d_perfmon_stop(),
which updates the values array under perfmon->lock, while the CPU job
concurrently reads it locklessly here.

>  
>  		v3d_perfmon_put(perfmon);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this code write outside the bounds
of bo->vaddr?

In v3d_submit.c:v3d_get_cpu_copy_performance_query_params(), the
user-provided offset and stride are accepted and stored without
validating them against the mapped BO size:

	job->copy.offset = copy.offset;
	job->copy.stride = copy.stride;

Later, in v3d_sched.c:v3d_copy_performance_query(), the unvalidated
offset is added to the vmap kernel virtual address:

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

And the destination pointer is incremented by an unvalidated stride for
subsequent writes:

	data += copy->stride;

Can this lead to out-of-bounds kernel memory accesses if an incorrect
offset or stride is supplied via the V3D_SUBMIT_CPU ioctl?

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