Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] amdkfd: Double-Checked Locking vulnerability exposes unpinned doorbell BO to mmap - [High] drm/amdkfd: TOCTOU race in doorbell allocation exposing unpinned BO --
commit 8544c203ebbbee60344c5d18d017125a93e18d7c Author: Henry Martin <[email protected]> amdkfd: fix doorbell allocation race creating duplicate allocations The commit adds a per-process-device mutex doorbell_mutex inside kfd_alloc_process_doorbells() to fix a check-and-allocate race condition where concurrent callers could create duplicate doorbell allocations. > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c > b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c > index fdcf7f2d1b5b4..55acadc986bee 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c > @@ -257,12 +257,21 @@ int kfd_alloc_process_doorbells(struct kfd_dev *kfd, > struct kfd_process_device * > int r; > struct qcm_process_device *qpd = &pdd->qpd; > > + mutex_lock(&pdd->doorbell_mutex); > + > + /* Another thread may have won the check-and-allocate race */ > + if (qpd->proc_doorbells) { > + mutex_unlock(&pdd->doorbell_mutex); > + return 0; > + } > + > /* Allocate bitmap for dynamic doorbell allocation */ > qpd->doorbell_bitmap = bitmap_zalloc(KFD_MAX_NUM_OF_QUEUES_PER_PROCESS, [ ... ] > /* Allocate doorbells for this process */ > r = amdgpu_bo_create_kernel(kfd->adev, > kfd_doorbell_process_slice(kfd), > PAGE_SIZE, > AMDGPU_GEM_DOMAIN_DOORBELL, > &qpd->proc_doorbells, > NULL, > NULL); > if (r) { > DRM_ERROR("Failed to allocate process doorbells\n"); > goto err; > } > + > + mutex_unlock(&pdd->doorbell_mutex); [Severity: Critical] Does moving the mutex inside this function create a double-checked locking vulnerability since the outer check is lockless? When kfd_get_process_doorbells() is called via mmap, it does a lockless check: if (!pdd->qpd.proc_doorbells) { if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd)) /* phys_addr_t 0 is error */ return 0; } Concurrently, if another thread is inside kfd_alloc_process_doorbells() and calls amdgpu_bo_create_kernel() while holding the doorbell_mutex, it assigns the new BO pointer to &qpd->proc_doorbells before the BO is fully pinned. Can the thread calling mmap() observe the non-NULL proc_doorbells pointer, bypass the mutex entirely, and use the unpinned BO? If it calls amdgpu_bo_gpu_offset() on the unpinned BO, could it calculate a bogus physical address and map unintended memory to userspace? > return 0; > > err: > bitmap_free(qpd->doorbell_bitmap); > qpd->doorbell_bitmap = NULL; > +unlock: > + mutex_unlock(&pdd->doorbell_mutex); > return r; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
