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 <[email protected]> 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/[email protected]?part=3
