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
