Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Double-free of `query_info->queries` array during CPU job extension parsing failure. - [Critical] Arbitrary kernel memory read and write primitive due to missing bounds checks on CPU job offsets. - [High] Race condition and missing implicit synchronization due to unlocked `indirect_csd.indirect` GEM object. - [High] Kernel crash (NULL pointer dereference) in `v3d_get_bo_vaddr()` when mapping imported PRIME buffers. -- commit 4335f305c6546e229d21733e148fae0311963d61 Author: Maíra Canal <[email protected]> drm/v3d: Reject invalid indirect BO handle in indirect CSD setup v3d_get_cpu_indirect_csd_params() looks up the indirect buffer object from a userspace-supplied handle but never checks the result. A bogus or stale handle makes drm_gem_object_lookup() return NULL, which is then stored in info->indirect and only dereferenced later when the indirect CSD job runs, turning a userspace mistake into a NULL pointer dereference in the kernel. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 18467e448c91f..3a93dbfe71a2c 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); [Severity: High] This is a pre-existing issue, but by not adding this buffer to the tracked bo array, are we skipping implicit synchronization? The indirect_csd.indirect GEM object is looked up and stored here, but it doesn't appear to be appended to the job->base.bo array. As a result, does v3d_submit_lock_reservations() bypass dma_resv locking and drm_sched_job_add_implicit_dependencies() completely for this buffer? When the CPU job later executes v3d_rewrite_csd_job_wg_counts_from_indirect(): drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() { ... ((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i]; ... } Will this modify the buffer concurrently without holding locks or waiting for prior GPU operations? Note that the patch "drm/v3d: Associate BOs with every job that accesses them" doesn't seem to cover indirect_csd.indirect. [Severity: High] This is a pre-existing issue, but does mapping this buffer safely handle imported PRIME buffers? If info->indirect is an imported PRIME buffer, drm_gem_shmem_prime_import_sg_table() only populates the scatter-gather table (shmem->sgt) and leaves shmem->pages as NULL. When vmap() is eventually called on it: 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)); ... } Could this pass a NULL page array to vmap() and trigger a kernel crash? [Severity: Critical] This is a pre-existing issue, but are the offsets used for this indirect buffer checked against the bounds of the allocation? In v3d_rewrite_csd_job_wg_counts_from_indirect(), the driver reads and writes using the user-controlled offsets from indirect_csd.offset and indirect_csd.wg_uniform_offsets: 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]; ... } Are these offsets ever bounds-checked against the size of the respective vmapped buffer objects? Could an unprivileged user supply arbitrary 32-bit offsets to achieve out-of-bounds reads and writes relative to the vmap allocations? (Similar missing bounds checks appear to exist in v3d_timestamp_query() and v3d_reset_timestamp_queries()). > + if (!info->indirect) > + return -ENOENT; [Severity: Critical] This isn't a bug introduced by this patch, but could returning an error here trigger a double-free if a timestamp query was parsed earlier in the same job? If a failure occurs here (or in other CPU job parsing functions) and returns an error to v3d_submit_cpu_ioctl(), it seems job cleanup is triggered via v3d_submit_cleanup_jobs(), which unconditionally calls v3d_cpu_job_free(). If an error path already called v3d_timestamp_query_info_free() to clean up the partially initialized array: drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_timestamp_query_params() { ... error: v3d_timestamp_query_info_free(&job->timestamp_query, i); ... } This executes kvfree(query_info->queries) but leaves the pointer dangling. During the subsequent teardown: drivers/gpu/drm/v3d/v3d_submit.c:v3d_cpu_job_free() { ... v3d_timestamp_query_info_free(&job->timestamp_query, job->timestamp_query.count); ... } Will this call kvfree() again on the same dangling pointer, resulting in a double-free? > > return 0; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
