Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing bounds validation for CPU jobs other than COPY operations allows arbitrary out-of-bounds memory accesses. - [High] Missing NULL check for `vmap()` return value causes NULL pointer dereference across all CPU job handlers. - [High] Missing alignment checks for userspace-provided offsets in CPU copy jobs trigger fatal alignment faults on ARM architectures. - [High] Unbounded memory allocation from userspace-controlled counts in CPU job submission. - [High] Repeated calls to `vmap()` leak kernel vmap space when the same BO is supplied multiple times. -- commit 3ca0a930a22512f0b8cf72cfb659ccbf12f82409 Author: Michael Bommarito <[email protected]> drm/v3d: add KUnit reproducer for the copy-query out-of-bounds write Add a KUnit suite (CONFIG_DRM_V3D_COPY_QUERY_KUNIT_TEST, builds under COMPILE_TEST) that drives the real v3d_copy_query_results() CPU-job handler over a shmem-backed BO and exercises the destination-buffer bounds the previous patch adds. Under KASAN the trigger case reproduces the vmalloc-out-of-bounds write on the stock tree and passes once the bounds patch is applied; two in-bounds controls pass on both trees. > diff --git a/drivers/gpu/drm/v3d/v3d_copy_query_kunit.c > b/drivers/gpu/drm/v3d/v3d_copy_query_kunit.c > new file mode 100644 > index 0000000000000..e7a795ed2a3ff > --- /dev/null > +++ b/drivers/gpu/drm/v3d/v3d_copy_query_kunit.c [ ... ] > +static void v3d_copy_test_run(struct kunit *test, u32 offset, u32 stride, > + u32 count) > +{ [ ... ] > + c->bo_array[0] = &c->dst->base.base; > + c->bo_array[1] = &c->ts->base.base; [Severity: High] This is a pre-existing issue, but does the driver handle cases where the same buffer object is passed multiple times in a single job? If userspace passes the same BO handle for both the source and timestamp arrays, the CPU job handler calls v3d_get_bo_vaddr() twice on the identical buffer object. In drivers/gpu/drm/v3d/v3d_bo.c:v3d_get_bo_vaddr(), the mapping is unconditionally overwritten: bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP, pgprot_writecombine(PAGE_KERNEL)); Does the second call to vmap() overwrite the pointer from the first without unmapping it, permanently leaking the first vmap mapping and potentially exhausting virtual memory space? [ ... ] > + c->job.timestamp_query.queries = c->queries; > + c->job.timestamp_query.count = count; [Severity: High] This is a pre-existing issue, but does the ioctl submission path validate the bounds of this user-provided count? During CPU job submission in drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_timestamp_query_params(), the unbounded userspace count is passed directly to the memory allocator: query_info->queries = kvmalloc_objs(struct v3d_timestamp_query, timestamp.count); If an unprivileged user provides an extremely large count value, could this attempt a massive virtual memory allocation, exhausting system memory and triggering the OOM killer? [ ... ] > + v3d_copy_query_results(&c->job); [Severity: High] This is a pre-existing issue, but is it possible for the CPU job handlers to dereference a NULL pointer if mapping fails? In drivers/gpu/drm/v3d/v3d_bo.c:v3d_get_bo_vaddr(), the code maps the buffer: bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP, pgprot_writecombine(PAGE_KERNEL)); If vmap() returns NULL under memory pressure, is this safely handled? Handlers like v3d_copy_query_results() in v3d_sched.c immediately compute addresses using this pointer: data = ((u8 *)bo->vaddr) + copy->offset; Could this result in a NULL pointer dereference and kernel crash? [Severity: High] This is also a pre-existing issue, but does this copy handler properly align memory accesses for userspace-controlled offsets? In drivers/gpu/drm/v3d/v3d_sched.c:v3d_copy_query_results(), the driver computes memory addresses directly from the userspace offsets and performs 64-bit operations: query_addr = ((u8 *)timestamp->vaddr) + queries[i].offset; write_to_buffer(data, 0, copy->do_64bit, *((u64 *)query_addr)); Since V3D hardware typically runs on ARM architectures that strictly require 4-byte or 8-byte alignment for 64-bit load/store instructions, could an unaligned userspace offset trigger a fatal alignment fault and kernel panic? > +} [ ... ] > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 23e19dacfdce2..ec0b94ada73ff 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c [ ... ] > /* Bound the copy-query CPU-job writes; the exec-time copy does not. */ > -static int > +int > v3d_cpu_job_check_copy_bounds(struct v3d_cpu_job *job) > { [Severity: Critical] This is a pre-existing issue, but does this function properly validate bounds for all CPU job types? For CPU jobs other than COPY operations (like TIMESTAMP_QUERY or RESET_TIMESTAMP_QUERY), the switch statement defaults to returning 0 without validation: drivers/gpu/drm/v3d/v3d_submit.c:v3d_cpu_job_check_copy_bounds() { ... default: return 0; } Could a malicious user exploit this by passing arbitrary out-of-bounds offsets? If the offset isn't checked, handlers like v3d_timestamp_query() might add it directly to bo->vaddr and perform out-of-bounds memory writes. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
