Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] The patch prevents PRT mappings from being validated as user queue VAs to avoid NULL pointer dereferences, but userspace can bypass this restriction via a Time-Of-Check to Time-Of-Use (TOCTOU) race, leading to the exact same NULL pointer dereferences. - [High] GPU coredump crashes with a NULL pointer dereference if an Indirect Buffer (IB) resides in a PRT mapping. - [High] Providing a PRT mapping during KFD queue creation permanently leaks `queue_refcount`, leading to a denial of service. - [High] Userspace can completely bypass the user queue eviction synchronization by using `AMDGPU_VA_OP_CLEAR` or `AMDGPU_VA_OP_REPLACE`. -- --- Patch [2]: [PATCH 2/5] drm/amdgpu: reject PRT mappings as user queue buffer VAs --- commit 2673e3bf87d8321b443028125d445c8f3dba144c Author: Junrui Luo <[email protected]> drm/amdgpu: reject PRT mappings as user queue buffer VAs This commit fixes a NULL pointer dereference by rejecting PRT mappings during user queue VA validation. It ensures that only mappings with backing BOs can be used as ring, rptr, or wptr buffers. Cc: [email protected] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index 6d3ed55e9ab4b..bec107216811c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c [ ... ] > @@ -259,6 +259,14 @@ int amdgpu_userq_input_va_validate(struct amdgpu_device > *adev, > if (!va_map) > return -EINVAL; > > + /* > + * A PRT mapping has no backing BO and so can't carry the eviction > + * fence which amdgpu_userq_gem_va_unmap_validate() waits on. Reject it > + * here, otherwise that helper dereferences a NULL bo on GEM unmap. > + */ > + if (!va_map->bo_va->base.bo) > + return -EINVAL; > + [Severity: High] This isn't a bug introduced by this patch, but does this validation fully prevent the NULL pointer dereference? Userspace might bypass this restriction via a Time-Of-Check to Time-Of-Use (TOCTOU) race. After amdgpu_userq_input_va_validate() completes, the VM root PD is temporarily unreserved before mes_userq_mqd_create() is called. A concurrent thread could call AMDGPU_GEM_VA_IOCTL to unmap the wptr_va and remap it as a PRT mapping. When mes_userq_create_wptr_mapping() or amdgpu_userq_fence_read_wptr() executes: drivers/gpu/drm/amd/amdgpu/mes_userqueue.c:mes_userq_create_wptr_mapping() { wptr_mapping = amdgpu_vm_bo_lookup_mapping(vm, wptr >> PAGE_SHIFT); ... obj = wptr_mapping->bo_va->base.bo; ret = drm_exec_lock_obj(&exec, &obj->tbo.base); } Could the mapping now be a PRT mapping, causing obj to be NULL and triggering a kernel panic when drm_exec_lock_obj() dereferences it? [Severity: High] This is also a pre-existing issue, but does the synchronization mechanism this patch aims to protect actually cover all unmap paths? Userspace can submit an AMDGPU_GEM_VA_IOCTL with AMDGPU_VA_OP_CLEAR or AMDGPU_VA_OP_REPLACE instead of AMDGPU_VA_OP_UNMAP. This routes to amdgpu_vm_bo_clear_mappings(), which removes the mappings without checking userq_va_mapped: drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_bo_clear_mappings() { list_for_each_entry_safe(tmp, next, &removed, list) { amdgpu_vm_it_remove(tmp, &vm->va); ... tmp->bo_va = NULL; list_add(&tmp->list, &vm->freed); trace_amdgpu_vm_bo_unmap(NULL, tmp); } } Does this bypass the amdgpu_userq_gem_va_unmap_validate() wait entirely, allowing userspace to unmap active hardware queues while the GPU is still accessing them? [Severity: High] This is a pre-existing issue, but there appears to be another unhandled edge case with PRT mappings causing a similar NULL pointer dereference elsewhere. If userspace submits an Indirect Buffer (IB) pointing to a PRT mapping and triggers a GPU reset, the GPU coredump handler iterates over the IBs to dump them. In amdgpu_devcoredump_print_ibs(): drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c:amdgpu_devcoredump_print_ibs() { mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn); if (!mapping) continue; abo = mapping->bo_va->base.bo; r = drm_exec_lock_obj(&exec, &abo->tbo.base); } Since abo is NULL for PRT mappings, will this unconditionally crash the system during the coredump? [Severity: High] This is a pre-existing issue, but there seems to be one more unhandled PRT mapping flaw in the KFD subsystem. If userspace calls a KFD queue creation IOCTL providing a PRT mapping for one of the queue buffers, kfd_queue_buffer_get() successfully increments the refcount: drivers/gpu/drm/amd/amdkfd/kfd_queue.c:kfd_queue_buffer_get() { mapping = amdgpu_vm_bo_lookup_mapping(vm, user_addr); ... *pbo = amdgpu_bo_ref(mapping->bo_va->base.bo); mapping->bo_va->queue_refcount++; } Because *pbo is set to NULL, kfd_queue_unref_bo_va() skips the decrement during queue destruction: drivers/gpu/drm/amd/amdkfd/kfd_queue.c:kfd_queue_unref_bo_va() { if (*bo) { struct amdgpu_bo_va *bo_va; ... if (bo_va && bo_va->queue_refcount) bo_va->queue_refcount--; } } Does this permanently leak the queue_refcount and prevent the mapping from ever being unmapped? > /* Lookup guarantees start_page is mapped; ensure full span is covered. > */ > if ((end_addr >> AMDGPU_GPU_PAGE_SHIFT) <= va_map->last) { > va_map->bo_va->userq_va_mapped = true; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
