Re: [PATCH AUTOSEL 6.18-6.6] drm/amdkfd: fix UAF race in destroy_queue_cpsch

[sashiko-bot@kernel.org]

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 <Alysa.Liu@amd.com>
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/20260831133314.4125787-446-sashal@kernel.org?part=1