Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing bounds checking of user-supplied offsets leads to arbitrary kernel memory read and write in CPU job execution functions. - [Critical] Double free of `query_info->queries` arrays on error paths when processing timestamp or performance query extensions. - [High] Missing return value check for `vmap()` failure in `v3d_get_bo_vaddr()` leads to NULL pointer dereferences. - [High] Memory leak of `se.out_syncs` in non-CPU IOCTLs when `v3d_get_extensions` fails. -- commit 3cd6d13669aae5071cfb72bf592569413ffc98ac Author: Maíra Canal <[email protected]> drm/v3d: Reject invalid indirect BO handle in indirect CSD setup This commit fixes a NULL pointer dereference in the v3d driver caused by failing to check the return value of drm_gem_object_lookup(). It updates v3d_get_cpu_indirect_csd_params() to bail out with -ENOENT if the userspace supplied indirect BO handle is invalid. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 9d23935b05dd0..656b0bbd4f8e3 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c > @@ -601,6 +601,8 @@ v3d_get_cpu_indirect_csd_params(struct drm_file > *file_priv, > sizeof(indirect_csd.wg_uniform_offsets)); > > info->indirect = drm_gem_object_lookup(file_priv, > indirect_csd.indirect); > + if (!info->indirect) > + return -ENOENT; > > return 0; > } [Severity: Critical] This isn't a bug introduced by this patch, but while reviewing the CPU job code, does this lack bounds checking on the user-supplied offsets? In drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect(): wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset); ((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i]; And in v3d_timestamp_query(): value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset; *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull; When a user submits a CPU job (like indirect CSD or timestamp query), can they pass arbitrary offset values? Since these offsets are added to the vmap'd kernel addresses of the BOs during job execution in the scheduler thread, could this allow arbitrary read and write access to kernel memory? [Severity: High] This is a pre-existing issue, but does this code fail to handle vmap() returning NULL? 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)); If vmap() fails due to memory pressure, bo->vaddr is set to NULL but no error is returned. Callers then unconditionally use the mapped address assuming success: drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() { ... v3d_get_bo_vaddr(bo); wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset); ... } Could this trigger a NULL pointer dereference when vmalloc space is exhausted? [Severity: High] This isn't a bug introduced by this patch, but does this early return leak se.out_syncs? In drivers/gpu/drm/v3d/v3d_submit.c, non-CPU IOCTLs handle extension parsing like this: drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cl_ioctl() { ... ret = v3d_get_extensions(file_priv, args->extensions, &se, NULL); if (ret) { drm_dbg(dev, "Failed to get extensions.\n"); return ret; } ... } If a user provides a valid DRM_V3D_EXT_ID_MULTI_SYNC extension followed by an invalid one in the linked list, the MULTI_SYNC handler successfully allocates se->out_syncs. When parsing the invalid extension fails, v3d_get_extensions() returns an error. By returning directly instead of jumping to the fail_submit cleanup label, do we leak the struct v3d_submit_outsync allocations? [Severity: Critical] This is a pre-existing issue, but can this sequence lead to a double free? When v3d_get_cpu_timestamp_query_params() encounters an error (like an invalid syncobj), it calls v3d_timestamp_query_info_free(): drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_timestamp_query_params() { ... error: v3d_timestamp_query_info_free(&job->timestamp_query, i); return err; } This frees the array but leaves the pointer intact: drivers/gpu/drm/v3d/v3d_sched.c:v3d_timestamp_query_info_free() { ... kvfree(query_info->queries); } The IOCTL then propagates the error and jumps to cleanup, which unconditionally destroys the job: drivers/gpu/drm/v3d/v3d_submit.c:v3d_cpu_job_free() { ... v3d_timestamp_query_info_free(&job->timestamp_query, job->timestamp_query.count); } Since count remains 0, the inner loop is skipped, but does kvfree() get called again on the dangling query_info->queries pointer? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
