• File: sys/dev/drm/amd/amdgpu/amdgpu_sa.c
• Lines: 77–100 (fini without lock); cf. locked paths at 299, 356, 378
• Severity: Low
• CVSS 3.1: CVSS:3.1/AV:L/AC:H/PR:H/UI:N/S:U:C:N/I:H/A:H
• CWE: CWE-667 Improper Locking
• Confidence: certain
• Status: new
• Related: DF-1988 (radeon_sa.c identical-class bug)

Summary

amdgpu_sa_bo_manager_fini() calls amdgpu_sa_bo_try_free() and iterates olist calling amdgpu_sa_bo_remove_locked() (which does list_del_init + dma_fence_put + kfree) without holding sa_manager->wq.lock.

Every other function that touches these lists — amdgpu_sa_bo_new (line 299), amdgpu_sa_bo_free (line 356), and amdgpu_sa_bo_dump_debug_info (line 378) — correctly acquires the lock.

A concurrent reader of the debugfs entry amdgpu_sa_info (amdgpu_ib.c:409, world-readable 0444) iterating olist under the lock in amdgpu_sa_bo_dump_debug_info can race with the unlocked teardown, dereferencing a sa_bo that fini just freed — use-after-free leading to kernel panic or potentially controlled corruption.

Root cause

amdgpu_sa_bo_manager_fini at amdgpu_sa.c:77-100 performs:

1. amdgpu_sa_bo_try_free(sa_manager) at line 89 — which iterates hole->next onward and calls amdgpu_sa_bo_remove_locked on signaled entries;
2. list_for_each_entry_safe iteration at lines 94-96 calling amdgpu_sa_bo_remove_locked for every remaining sa_bo —

…all without lockmgr(&sa_manager->wq.lock, LK_EXCLUSIVE).

Compare:

• amdgpu_sa_bo_new takes the lock at line 299 before any list operation;
• amdgpu_sa_bo_free takes it at line 356;
• amdgpu_sa_bo_dump_debug_info takes it at line 378.

amdgpu_sa_bo_remove_locked (line 102) performs list_del_init(&sa_bo->olist), list_del_init(&sa_bo->flist), dma_fence_put(sa_bo->fence), and kfree(sa_bo) — all operations that corrupt state if a concurrent locked reader is iterating olist.

This is the identical defect to DF-1988 in radeon_sa.c:77-94 (which also omits the lock in radeon_sa_bo_manager_fini). The amdgpu code was forked from radeon and inherited the bug verbatim, including the comma-operator oddity at line 88 (sa_manager->hole = &sa_manager->olist,).

Threat model

Attacker position: root (or any principal in the debugfs-reading group) with the debugfs entry open for reading concurrently with GPU device teardown (driver unload, PCI hot-remove, or suspend-to-RAM).

The race window is between the fini's list_for_each_entry_safe at line 94 and the debugfs reader's list_for_each_entry at line 379. If the reader's cursor is on a sa_bo that fini frees at line 95 → 111 (kfree), the reader dereferences freed memory on the next iteration.

Impact: kernel panic (A) or, if an attacker can groom the slab to reuse the freed sa_bo, potentially controlled kernel data corruption (I).

In practice the race is narrow and requires privileged access, hence Low severity.

However, the missing lock also means fini itself is unsafe against any concurrent amdgpu_sa_bo_new/amdgpu_sa_bo_free from an in-flight CS ioctl that hasn't fully drained — a more realistic scenario during hot device removal.

Proof of concept

Prerequisites: DragonFlyBSD with AMDGPU loaded, debugfs mounted (typically /sys/kernel/debug), root or group access.

# Thread 1: tight-loop read of the debugfs entry
while true; do
    cat /sys/kernel/debug/dri/0/amdgpu_sa_info > /dev/null 2>&1
done &

# Thread 2: trigger GPU device teardown
kldunload amdgpu   # OR: devctl detach pci0:<device>

amdgpu_sa_bo_dump_debug_info holds wq.lock during iteration; fini frees without it; the reader's cursor lands on a freed sa_bo → UAF → panic.

Expected result: kernel panic with "Fatal trap 12: page fault while in kernel mode" or "freed pointer was not modified" depending on slab allocator hardening.

Reproducibility: ~1 in 10-50 attempts with tight loop timing; higher if many sa_bos are allocated (fill the pool first by issuing many CS submissions before teardown).

No special build needed — shell + devctl/kldunload.

Recommended fix

Acquire sa_manager->wq.lock before touching the lists in fini, matching the pattern in all other functions. Also fix the comma operator at line 88 to a semicolon for clarity.

--- a/sys/dev/drm/amd/amdgpu/amdgpu_sa.c
+++ b/sys/dev/drm/amd/amdgpu/amdgpu_sa.c
@@ -77,6 +77,8 @@
 void amdgpu_sa_bo_manager_fini(struct amdgpu_device *adev,
                               struct amdgpu_sa_manager *sa_manager)
 {
    struct amdgpu_sa_bo *sa_bo, *tmp;
+
+   lockmgr(&sa_manager->wq.lock, LK_EXCLUSIVE);

    if (sa_manager->bo == NULL) {
        dev_err(adev->dev, "no bo for sa manager\n");
@@ -85,7 +87,7 @@
    }

    if (!list_empty(&sa_manager->olist)) {
-       sa_manager->hole = &sa_manager->olist,
+       sa_manager->hole = &sa_manager->olist;
        amdgpu_sa_bo_try_free(sa_manager);
        if (!list_empty(&sa_manager->olist)) {
            dev_err(adev->dev, "sa_manager is not empty, clearing anyway\n");
        }
    }
    list_for_each_entry_safe(sa_bo, tmp, &sa_manager->olist, olist) {
        amdgpu_sa_bo_remove_locked(sa_bo);
    }
+
+   lockmgr(&sa_manager->wq.lock, LK_RELEASE);

    amdgpu_bo_free_kernel(&sa_manager->bo, (u64 *)&sa_manager->gpu_addr, &sa_manager->cpu_ptr);
    sa_manager->size = 0;

References

• sys/dev/drm/amd/amdgpu/amdgpu_sa.c:77-100 — amdgpu_sa_bo_manager_fini without lock
• sys/dev/drm/amd/amdgpu/amdgpu_sa.c:299,356,378 — sibling functions that correctly acquire lock
• sys/dev/drm/amd/amdgpu/amdgpu_sa.c:102-112 — amdgpu_sa_bo_remove_locked does kfree
• sys/dev/drm/amd/amdgpu/amdgpu_ib.c:409,415-417 — debugfs amdgpu_sa_info entry (mode 0444)