Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The newly added bounds check for V3D_CPU_JOB_TYPE_INDIRECT_CSD jobs unconditionally dereferences the indirect BO, leading to a NULL pointer dereference if userspace provides an invalid GEM handle. Pre-existing issues: - [High] Missing NULL check on `vmap()` return value leads to kernel oops in CPU jobs during vmalloc exhaustion. -- --- Patch [1]: [PATCH v4] drm/v3d: bound CPU-job query writes to their destination BO --- commit 4b1e346690c50e3a4b036e29530bc03eb7a927ba Author: Michael Bommarito <[email protected]> Subject: drm/v3d: bound CPU-job query writes to their destination BO This patch validates user-supplied offsets and indices for V3D_SUBMIT_CPU jobs against the bounds of their destination buffer objects before queuing. It prevents render-node users from reading or writing past a BO's vmap mapping by verifying the access extent and returning -EINVAL for invalid geometry. Fixes: 18b8413b25b7 ("drm/v3d: Create a CPU job extension for a indirect CSD job") Fixes: 9ba0ff3e083f ("drm/v3d: Create a CPU job extension for the timestamp query job") Fixes: 34a101e64296 ("drm/v3d: Create a CPU job extension for the reset timestamp job") Fixes: 6745f3e44a20 ("drm/v3d: Create a CPU job extension to copy timestamp query to a buffer") Fixes: 209e8d2695ee ("drm/v3d: Create a CPU job extension for the copy performance query job") > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index ee2ac2540ed50..277823b774bed 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; > + u32 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]. */ > + dst = to_v3d_bo(job->base.bo[0]); [Severity: High] This isn't a bug introduced by this patch, but is it possible for the CPU job executors to trigger a NULL pointer dereference if vmap fails? During high memory pressure, vmap() can return NULL, but the result is assigned without being checked: v3d_get_bo_vaddr() { ... bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP, pgprot_writecombine(PAGE_KERNEL)); } Later, the CPU job executor unconditionally calculates an offset against bo->vaddr and writes to it: v3d_timestamp_query() { ... v3d_get_bo_vaddr(bo); for (int i = 0; i < timestamp_query->count; i++) { value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset; *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull; ... } Does the driver need to check if bo->vaddr is valid before allowing these writes to proceed? > + > + for (i = 0; i < tquery->count; i++) { > + if ((u64)tquery->queries[i].offset + sizeof(u64) > > + dst->base.base.size) > + goto err_range; > + } > + return 0; [ ... ] > + case V3D_CPU_JOB_TYPE_INDIRECT_CSD: { > + struct v3d_indirect_csd_info *indirect_csd = &job->indirect_csd; > + > + /* 3 is the three dimensions (x, y, z) of the workgroup counts. > */ > + src = to_v3d_bo(job->base.bo[0]); > + if ((u64)indirect_csd->offset + 3 * sizeof(u32) > > + src->base.base.size) > + goto err_range; > + > + dst = to_v3d_bo(indirect_csd->indirect); > + for (i = 0; i < 3; i++) { > + u32 uidx = indirect_csd->wg_uniform_offsets[i]; > + > + /* > + * 0xffffffff means "skip this rewrite", so the exec > + * path never writes that index and it needs no check. > + */ > + if (uidx != 0xffffffff && > + (u64)uidx * sizeof(u32) + sizeof(u32) > > + dst->base.base.size) > + goto err_range; [Severity: High] Can dst be NULL here, leading to a NULL pointer dereference when accessing dst->base.base.size? If userspace provides an invalid indirect GEM handle, drm_gem_object_lookup() returns NULL: v3d_get_cpu_indirect_csd_params() { ... info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect); return 0; } Since this lookup allows the ioctl to proceed with a NULL indirect BO, does this bounds check need to verify dst is not NULL before dereferencing it? > + } > + return 0; > + } > + default: > + return 0; > + } > + > +err_range: > + drm_dbg(dev, "CPU job query offset exceeds the BO.\n"); > + return -EINVAL; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
