Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues: - [Medium] Loop variable 'i' declared as signed 'int' causes negative indexing and out-of-bounds reads when 'tquery->count' exceeds INT_MAX. Pre-existing issues: - [Critical] Uninitialized CPU job structure cleanup causes a NULL pointer dereference in `drm_sched_job_cleanup()`. - [Critical] Cleaning up CPU jobs that have already been pushed to the DRM scheduler queue causes severe state corruption and Use-After-Free. - [High] Memory and WW mutex leaks for INDIRECT_CSD clean_job BO references on intermediate error paths. - [High] Missing check for vmap() allocation failure leads to NULL pointer dereferences in CPU job executors. - [High] The INDIRECT_CSD CPU job accesses the indirect GEM BO without extracting its implicit dependencies, causing it to race with prior GPU jobs. - [High] Double vmap() on the same Buffer Object permanently leaks virtual memory mappings. - [High] User-controlled wg_counts integer overflow evaluates to 0, triggering a WARN_ON and potential Denial of Service. -- commit 176c9c3615bb9c83354ef76d006e602c0a5318e8 Author: Michael Bommarito <[email protected]> Subject: drm/v3d: bound CPU-job query writes to their destination BO The commit adds bounds checking for CPU jobs to ensure that user-supplied offsets and indices do not result in reads or writes outside their associated Buffer Objects (BOs), preventing potential out-of-bounds memory accesses by render-node users. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 1db43c6a078d5..0d3a388fbba57 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c [ ... ] > +static int > +v3d_cpu_job_bounds_check(struct v3d_cpu_job *job) > +{ > + struct drm_device *dev = &job->base.v3d->drm; > + struct v3d_timestamp_query_info *tquery = &job->timestamp_query; > + struct v3d_copy_query_results_info *copy = &job->copy; > + u32 elem = copy->do_64bit ? sizeof(u64) : sizeof(u32); > + struct v3d_bo *dst, *src; > + u64 slots, write_size; > + int i; > + > + switch (job->job_type) { > + case V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY: > + case V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY: > + /* > + * Each query writes one u64 timestamp slot into bo[0]. The > + * offset is cast to u64 so a 0xffffffff query offset cannot > wrap > + * the addition past the BO size. > + */ > + dst = to_v3d_bo(job->base.bo[0]); > + > + for (i = 0; i < tquery->count; i++) { [Severity: Medium] Is it possible for tquery->count to exceed the maximum value of a signed int? If so, does the counter i wrap around to a negative number, causing an out-of-bounds read when accessing tquery->queries[i].offset? > + if ((u64)tquery->queries[i].offset + sizeof(u64) > > + dst->base.base.size) > + goto err_range; > + } > + return 0; [ ... ] > @@ -1346,6 +1478,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void > *data, > if (ret) > goto fail; > > + ret = v3d_cpu_job_bounds_check(cpu_job); > + if (ret) > + goto fail; > + [Severity: High] This is a pre-existing issue, but does jumping to the fail label here bypass the fail_unreserve cleanup path? For INDIRECT_CSD jobs, v3d_setup_csd_jobs_and_bos() dynamically allocates jobs and locks the reservations for clean_job->bo. Does bypassing the fail_unreserve label permanently leak the WW mutexes and cause deadlocks for subsequent operations on these BOs? > ret = v3d_lock_bo_reservations(&cpu_job->base, &acquire_ctx); > if (ret) > goto fail; While reviewing this patch, I also noticed a few pre-existing issues in the surrounding code: drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cpu_ioctl() { ... if (args->bo_handle_count != cpu_job_bo_handle_count[cpu_job->job_type]) { drm_dbg(dev, "This CPU job was not submitted with the proper number of BOs.\n"); ret = -EINVAL; goto fail; } ... fail: v3d_job_cleanup((void *)cpu_job); } [Severity: Critical] This is a pre-existing issue, but since the error occurs before v3d_job_init() is called, cpu_job->base.s_fence is NULL. Does the subsequent call to drm_sched_job_cleanup() unconditionally dereference job->s_fence->finished.refcount, causing a kernel panic? drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cpu_ioctl() { ... v3d_push_job(&cpu_job->base); ... case V3D_CPU_JOB_TYPE_INDIRECT_CSD: ret = drm_sched_job_add_dependency(&csd_job->base.base, dma_fence_get(cpu_job->base.done_fence)); if (ret) goto fail_unreserve; ... fail_unreserve: ... fail: v3d_job_cleanup((void *)cpu_job); } [Severity: Critical] This is a pre-existing issue, but when goto fail_unreserve falls through to the fail label and calls v3d_job_cleanup(cpu_job), does this corrupt the state of jobs that have already been armed and pushed to the active DRM scheduler queue? 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)); ... } [Severity: High] This is a pre-existing issue, but the result of vmap() is assigned directly to bo->vaddr. If vmap() returns NULL due to memory fragmentation, do CPU job executors like v3d_rewrite_csd_job_wg_counts_from_indirect() blindly dereference this NULL pointer? drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_indirect_csd_params() { ... info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect); ... } [Severity: High] This is a pre-existing issue, but since the indirect BO is not added to cpu_job->base.bo, it appears to bypass v3d_lock_bo_reservations(). Does this allow v3d_rewrite_csd_job_wg_counts_from_indirect() to read and modify the BO while a prior GPU dispatch might still be writing to it? drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() { ... v3d_get_bo_vaddr(bo); v3d_get_bo_vaddr(indirect); ... } [Severity: High] This is a pre-existing issue, but if a user provides the exact same GEM object handle for both the primary bo and the indirect BO, does the second vmap() silently overwrite the vaddr pointer from the first, permanently leaking the prior virtual memory mapping? drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() { ... num_batches = DIV_ROUND_UP(indirect_csd->wg_size, 16) * (wg_counts[0] * wg_counts[1] * wg_counts[2]); ... args->cfg[4] = num_batches - 1; ... WARN_ON(args->cfg[4] == ~0); } [Severity: High] This is a pre-existing issue, but since wg_counts are user-supplied 32-bit unsigned integers, can specific inputs cleanly overflow the product to 0? Does this underflow args->cfg[4] to 0xFFFFFFFF and trigger the WARN_ON, potentially crashing systems with panic_on_warn enabled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
