Re: [PATCH 2/2] drm/amdgpu/userq: hold the doorbell xa lock during hang reset

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected xa_for_each iteration allows concurrent Use-After-Free.
- [High] Lockless and unprotected access to userq->last_fence can result in a Use-After-Free.
- [Medium] Unprotected state modification of queue->state causes a data race.
--

--- Patch [2]: [PATCH 2/2] drm/amdgpu/userq: hold the doorbell xa lock during hang reset ---
commit c3c720e398b80f11731a92d4798c53c135bb4886
Author: Junrui Luo <[email protected]>

drm/amdgpu/userq: hold the doorbell xa lock during hang reset

This commit fixes a use-after-free vulnerability in the AMDGPU driver.
It adds an xa_lock_irqsave() lock around an xa_for_each() loop in
mes_userq_detect_and_reset() to prevent the queue from being freed
while iterating.

> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> index 4e44a581a78a2..f4d12e4b2d483 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c

[ ... ]

> @@ -230,6 +230,13 @@ static int mes_userq_detect_and_reset(struct amdgpu_device *adev,
>  	if (r) {
>  		dev_err(adev->dev, "Failed to detect and reset queues, err (%d)\n", r);
>  	} else if (hung_db_num) {
> +		/*
> +		 * The doorbell xarray is device wide, so this walks queues
> +		 * owned by other drm_files too. Hold its lock: the free path
> +		 * erases the entry under the same lock strictly before it
> +		 * frees the queue, so an entry found here stays allocated.
> +		 */
> +		xa_lock_irqsave(&adev->userq_doorbell_xa, flags);
>  		xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
xa_for_each() iteration here, I noticed several other functions iterate
adev->userq_doorbell_xa locklessly.

For example, in amdgpu_userq_stop_sched_for_enforce_isolation():

    xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
        uqm = queue->userq_mgr;
        cancel_delayed_work_sync(&uqm->resume_work);
        mutex_lock(&uqm->userq_mutex);

Similar loops exist in amdgpu_userq_start_sched_for_enforce_isolation() and
amdgpu_userq_suspend().

The xa_for_each macro only uses rcu_read_lock() internally during the lookup.

Could an unprivileged user trigger amdgpu_userq_destroy() concurrently and
free the queue, causing a use-after-free when the loop body dereferences
queue->userq_mgr?

>  			if (queue->queue_type == queue_type) {
>  				for (i = 0; i < hung_db_num; i++) {
>  					if (queue->doorbell_index == db_array[i]) {
>  						queue->state = AMDGPU_USERQ_STATE_HUNG;

[Severity: Medium]
This is a pre-existing issue, but does assigning queue->state here without
holding queue->userq_mgr->userq_mutex cause a data race?

Concurrently, actions like amdgpu_userq_start_sched_for_enforce_isolation() or
PM resume can call amdgpu_userq_map_helper() under userq_mutex, which also
evaluates and sets queue->state.

Could this lead to lost updates where the AMDGPU_USERQ_STATE_HUNG state is
overwritten?

>  						found_hung_queue = true;
>  						atomic_inc(&adev->gpu_reset_counter);
>  						amdgpu_userq_fence_driver_force_completion(queue);

[Severity: High]
This is a pre-existing issue, but is it safe to call
amdgpu_userq_fence_driver_force_completion() here?

This function reads userq->last_fence without taking
queue->userq_mgr->userq_mutex or holding rcu_read_lock():

amdgpu_userq_fence_driver_force_completion()
    struct dma_fence *f = userq->last_fence;
    if (f) {
        struct amdgpu_userq_fence *fence = to_amdgpu_userq_fence(f);

Concurrently, amdgpu_userq_fence_init() (called under userq_mutex) updates
userq->last_fence and drops the old fence reference via dma_fence_put().

Since xa_lock_irqsave() maps to a sleeping lock on PREEMPT_RT kernels and
does not disable interrupts, the RCU grace period could end.

Could this allow the fence memory to be freed before to_amdgpu_userq_fence(f)
accesses f->ops, resulting in a use-after-free?

> -						drm_dev_wedged_event(adev_to_drm(adev), DRM_WEDGE_RECOVERY_NONE, NULL);
>  					}
>  				}
>  			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2