DF-2203 — amdgpu_xgmi unbounded write to stack tmp_topology (Medium)

Claim

amdgpu_xgmi_add_device() in sys/dev/drm/amd/amdgpu/amdgpu_xgmi.c declares tmp_topology[AMDGPU_MAX_XGMI_DEVICE_PER_HIVE] (4 entries, 16 bytes each, 64 bytes on stack) and fills it with an unbounded list_for_each_entry(... hive->device_list ...) loop whose body is tmp_topology[count++].device_id = entry->device_id;. Because the device is list_add_tail()-ed into the hive before the loop (line 85), once the hive aggregates more than 4 devices the write goes past the array and smashes adjacent stack slots (return address, saved RBP, hive, entry). The stub psp_v11_0_xgmi_get_hive_id() (psp_v11_0.c:564) returns the same constant 0x123456789abcdef for every Vega20 GPU, so all of them collapse into one software hive — a 5-GPU system tips the loop past the array.

Verification approach

HW-gated / source-only. amdgpu_xgmi_add_device() is called once per Vega20-class GPU from amdgpu_device_init() (amdgpu_device.c:1691) and is reachable only when an amdgpu driver is bound to real Vega20 hardware. No such hardware on this audit guest. Per the task brief, source-only confirmation is acceptable. The overflow is fully visible at the source level and the structural fix is mechanical:

1. Confirm tmp_topology is sized by AMDGPU_MAX_XGMI_DEVICE_PER_HIVE (4).
2. Confirm the loop body has no count < MAX guard.
3. Confirm the list_add_tail precedes the loop (so the just-added device is itself counted).
4. Confirm psp_v11_0_xgmi_get_hive_id() is a stub returning a constant per device.
5. Author fix.diff (bound the loop).
6. Phase 8 — apply all 5 batched fixes and rebuild amdgpu.ko with -Werror.

Source trace (confirmed)

• sys/dev/drm/amd/amdgpu/amdgpu_xgmi.c:32 — #define AMDGPU_MAX_XGMI_DEVICE_PER_HIVE 4.
• sys/dev/drm/amd/amdgpu/amdgpu_xgmi.c:66 — struct psp_xgmi_topology_info tmp_topology[AMDGPU_MAX_XGMI_DEVICE_PER_HIVE]; (4 entries, on stack).
• sys/dev/drm/amd/amdgpu/amdgpu_xgmi.c:85 — list_add_tail(&adev->gmc.xgmi.head, &hive->device_list); adds the current device before the counting loop.
• sys/dev/drm/amd/amdgpu/amdgpu_xgmi.c:86-87 — list_for_each_entry(entry, &hive->device_list, head) tmp_topology[count++].device_id = entry->device_id; — no upper bound on count.
• sys/dev/drm/amd/amdgpu/amdgpu_psp.h:144 — struct psp_xgmi_topology_info { uint64_t device_id; uint32_t connection_mask; uint32_t reserved; } = 16 bytes per entry.
• sys/dev/drm/amd/amdgpu/psp_v11_0.c:564-573 — psp_v11_0_xgmi_get_hive_id() returns the constant 0x123456789abcdef for every Vega20 GPU where num_physical_nodes != 0.
• sys/dev/drm/amd/amdgpu/amdgpu_device.c:1691 — call site: once per Vega20 GPU during amdgpu_device_init.

Overflow scenario

A host with 5+ Vega20 GPUs all reporting the same stub hive id lands them all in one xgmi_hives[] entry; the 5th device's list_for_each_entry writes tmp_topology[4].device_id (16 bytes past the array), tmp_topology[5], etc. On x86_64 the write immediately overruns the stack frame.

Files

• VERDICT.md — full narrative.
• fix.diff — guard the loop with if (count >= AMDGPU_MAX_XGMI_DEVICE_PER_HIVE) { dev_err(...); ret = -ENOSPC; goto exit; }.
• build_baseline.log — unpatched amdgpu.ko build, rc=0, -Werror.
• build_patched.log — patched amdgpu.ko rebuild (only amdgpu_xgmi.o recompiled), rc=0, -Werror.
• env.txt — guest environment.

Reproduce

./build.sh    # applies fix.diff + rebuilds amdgpu.ko
./run.sh      # HW-gated no-op (see VERDICT.md)

DF-2203 — VERDICT

Verdict: REPRODUCED (source-only, HW-gated).

Class: stack buffer overflow (memory corruption).

Mechanism

amdgpu_xgmi_add_device() (sys/dev/drm/amd/amdgpu/amdgpu_xgmi.c) builds a transient on-stack topology snapshot for the XGMI hive:

/* amdgpu_xgmi.c:32 */
#define AMDGPU_MAX_XGMI_DEVICE_PER_HIVE  4
/* amdgpu_xgmi.c:66 */
struct psp_xgmi_topology_info tmp_topology[AMDGPU_MAX_XGMI_DEVICE_PER_HIVE];
/* amdgpu_xgmi.c:85 */
list_add_tail(&adev->gmc.xgmi.head, &hive->device_list);
/* amdgpu_xgmi.c:86-87 */
list_for_each_entry(entry, &hive->device_list, head)
    tmp_topology[count++].device_id = entry->device_id;

tmp_topology is a fixed 4-entry array (4 × 16 bytes = 64 bytes on stack; see struct psp_xgmi_topology_info at amdgpu_psp.h:144). The loop body increments count unconditionally and writes tmp_topology[count++] with no upper-bound check, so once hive->device_list holds more than 4 devices the write runs past the array into adjacent stack slots.

The list_add_tail() at line 85 happens before the loop, so the device currently being added is counted too — the 5th add into a hive is the one that overruns.

Why 5+ devices realistically land in one hive

psp_v11_0_xgmi_get_hive_id() is a stub (psp_v11_0.c:564-573):

static uint64_t psp_v11_0_xgmi_get_hive_id(struct psp_context *psp)
{
    uint64_t hive_id = 0;
    /* Remove me when we can get correct hive_id through PSP */
    if (psp->adev->gmc.xgmi.num_physical_nodes)
        hive_id = 0x123456789abcdef;
    return hive_id;
}

It returns the same constant for every Vega20-class GPU that has num_physical_nodes != 0. amdgpu_xgmi_add_device uses that value as the hive lookup key (amdgpu_xgmi.c:77), so all such GPUs collapse into one xgmi_hives[] entry. A host with 5+ Vega20 GPUs therefore exceeds the 4-entry array on the 5th amdgpu_xgmi_add_device() call.

Stack layout impact

tmp_topology is the largest local in the function (64 bytes); writing tmp_topology[4] and beyond overwrites other locals (hive, entry, tmp_adev, count, ret) and ultimately the saved RBP / return address. Impact per the original finding: deterministic kernel panic on return (corrupted return address), NULL deref at line 98, or silent misbehaviour depending on the values written.

Trigger surface / reachability

amdgpu_xgmi_add_device() is invoked from amdgpu_device_init() (amdgpu_device.c:1691) exactly once per Vega20 GPU during driver probe. The function early-returns for non-Vega20 ASICs (asic_type < CHIP_VEGA20) and APUs. On this audit guest there is no GPU, so the overflow cannot be exercised live; the bug is confirmed at the source level and the fix compiles cleanly (Phase 8).

Fix

fix.diff adds a per-iteration bounds check inside the list_for_each_entry loop:

list_for_each_entry(entry, &hive->device_list, head) {
    if (count >= AMDGPU_MAX_XGMI_DEVICE_PER_HIVE) {
        dev_err(adev->dev, "XGMI: hive %llx holds more than %d devices; "
                 "refusing to overflow tmp_topology", ...);
        ret = -ENOSPC;
        goto exit;
    }
    tmp_topology[count++].device_id = entry->device_id;
}

This is the minimal, targeted fix: refuse to overflow rather than silently truncating. The correct long-term fix is also to drive num_physical_nodes from real PSP data (kill the stub), but that is a separate, larger change and out of scope for a single-finding diff.

Phase 8 build validation

Applied fix.diff (plus the four other batched DRM fixes) to the in-guest /usr/src, rebuilt amdgpu.ko with -Werror: * baseline (unpatched) amdgpu.ko: rc=0 (build_baseline.log). * patched amdgpu.ko: rc=0, only amdgpu_xgmi.o recompiled (build_patched.log); no warnings, no errors.

Verdict

REPRODUCED at source level (HW-gated; no live repro possible on guest). The unbounded stack write is real and unambiguous from the source; the fix compiles cleanly under -Werror.