Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing bounds checking for TIMESTAMP_QUERY, RESET_TIMESTAMP_QUERY, and INDIRECT_CSD CPU jobs allows out-of-bounds kernel memory reads and writes. - [High] Structurally flawed error handling in `v3d_submit_cpu_ioctl` leading to Use-After-Free and NULL pointer dereferences. - [High] Missing validation of `vmap()` return value leads to NULL pointer dereferences in CPU job handlers. - [Medium] Passing duplicate BO handles causes a vmap memory leak in `v3d_copy_query_results()`. -- commit 18b6364dfc49658f5028ca1c3ffd50cc3b4cd3e7 Author: Michael Bommarito <[email protected]> drm/v3d: validate copy-query buffer bounds against destination BO size This commit adds bounds checking to the copy timestamp query and copy performance query extensions. It verifies that the user-supplied destination offset, stride, and query count do not exceed the destination buffer object bounds before queuing the job. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c [ ... ] > +static int > +v3d_cpu_job_check_copy_bounds(struct v3d_cpu_job *job) > +{ [ ... ] > + default: > + return 0; [Severity: Critical] This is a pre-existing issue, but does returning 0 here leave other job types vulnerable to out-of-bounds accesses? The new validation explicitly ignores timestamp query, reset timestamp query, and indirect csd jobs. These unvalidated job types also use user-provided offsets to access kernel memory. For example, in drivers/gpu/drm/v3d/v3d_sched.c:v3d_timestamp_query(): value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset; *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull; And in v3d_rewrite_csd_job_wg_counts_from_indirect(): wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset); ... u32 uniform_idx = indirect_csd->wg_uniform_offsets[i]; ((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i]; Can an attacker provide arbitrary offsets to achieve out-of-bounds reads and writes in kernel memory through these unvalidated job types? > + } > +} [ ... ] > @@ -1317,6 +1399,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void > *data, [ ... ] > if (ret) > goto fail; > > + ret = v3d_cpu_job_check_copy_bounds(cpu_job); > + if (ret) > + goto fail; [Severity: High] This is a pre-existing issue, but does this error handling path cause a use-after-free or NULL pointer dereference? If a failure occurs and the code jumps to the fail block in v3d_submit_cpu_ioctl(), cpu_job is cleaned up: fail: v3d_job_cleanup((void *)cpu_job); ... kvfree(cpu_job->timestamp_query.queries); If v3d_job_init() failed earlier, v3d_job_deallocate(&cpu_job) frees cpu_job and sets the pointer to NULL, making the kvfree() dereference a NULL pointer. If a failure occurs after v3d_job_init() (like the new bounds check), v3d_job_cleanup(cpu_job) drops the kref to 0, freeing the cpu_job struct. The subsequent kvfree() then accesses the freed struct. Is it safe to access cpu_job->timestamp_query.queries after cpu_job has been cleaned up or deallocated? [Severity: High] This isn't a bug introduced by this patch, but is it possible for bo->vaddr to be NULL when accessed in CPU job handlers? In drivers/gpu/drm/v3d/v3d_bo.c:v3d_get_bo_vaddr(): bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP, pgprot_writecombine(PAGE_KERNEL)); The result of vmap() is assigned without a NULL check. If memory allocation fails, bo->vaddr becomes NULL. Later, job handlers unconditionally use it, for example in v3d_timestamp_query(): value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset; *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull; Will this lead to unvalidated writes to a NULL base address under memory pressure? [Severity: Medium] This is a pre-existing issue, but does v3d_copy_query_results() leak memory if the same buffer object handle is passed multiple times? If a user submits a copy timestamp query job and passes the same buffer object handle for both the destination buffer and the timestamp buffer, job->base.bo[0] and job->base.bo[1] resolve to the same struct v3d_bo object. In drivers/gpu/drm/v3d/v3d_sched.c:v3d_copy_query_results(): v3d_get_bo_vaddr(bo); v3d_get_bo_vaddr(timestamp); The second call will overwrite bo->vaddr with a new vmap() allocation, leaking the first virtual memory area mapping. Could this allow userspace to leak vmalloc space? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
