Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Bypassing the is_being_destroyed check allows a concurrent thread to improperly clear the flag, reopening the use-after-free window. Pre-existing issues: - [High] q->mqd is unconditionally freed in destroy_queue_cpsch() even if the function fails with a non-timeout error, leading to a UAF in subsequent operations like update_queue, and permanent memory leaks of the queue structure. -- commit ea1b656c30dda08f56333e614c63a751ee2e05bd Author: Alysa Liu <[email protected]> Date: Mon Aug 31 13:27:54 2026 +0000 drm/amdkfd: fix UAF race in destroy_queue_cpsch wait_on_destroy_queue() drops locks to wait for queue resume, allowing a concurrent destroy to free the queue. Use is_being_destroyed flag to serialize destruction. > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > index 6e7bc983fc0b6..910722be71000 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > @@ -2500,6 +2500,9 @@ static int wait_on_destroy_queue(struct > device_queue_manager *dqm, > if (pdd->qpd.is_debug) > return ret; > > + if (q->properties.is_being_destroyed) > + return -EBUSY; > + [Severity: High] Is it possible for a concurrent thread to bypass the is_being_destroyed check if is_debug becomes true? If Thread A drops locks to wait in wait_event_interruptible(), is_being_destroyed remains true. If a debugger attaches and is_debug becomes true, Thread B calling wait_on_destroy_queue() will return early here and skip the is_being_destroyed check. > q->properties.is_being_destroyed = true; > > if (pdd->process->debug_trap_enabled && q->properties.is_suspended) { [ ... ] > @@ -2605,7 +2611,7 @@ static int destroy_queue_cpsch(struct > device_queue_manager *dqm, > return retval; [Severity: High] This is a pre-existing issue, but does this unconditionally free q->mqd even if an error is returned earlier in this function? If hardware unmap fails (e.g., -ENOMEM from execute_queues_cpsch or -EINVAL from remove_queue_mes), destroy_queue_cpsch() still calls free_mqd() and returns the error. pqm_destroy_queue() receives this error and skips uninit_queue(), leaving the queue in the active list with a dangling q->mqd pointer. A subsequent KFD_IOC_UPDATE_QUEUE ioctl will then dereference the freed q->mqd in update_queue(): drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:update_queue() { ... mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties, minfo); ... } > > failed_try_destroy_debugged_queue: > - > + q->properties.is_being_destroyed = false; [Severity: High] Will unconditionally clearing the flag here re-introduce the use-after-free race? If Thread B bypassed the is_being_destroyed check in wait_on_destroy_queue() due to is_debug becoming true, it will take this error path and clear the flag. Thread A is still waiting, but its protection is now lost, allowing Thread C to free the queue before Thread A wakes up. > dqm_unlock(dqm); > return retval; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
