[PATCH AUTOSEL 6.18-6.12] drm/amdgpu: Guard reads in pcie state readout
From: Sasha Levin
Date: Mon Oct 19 2026 - 12:58:36 EST
From: Lijo Lazar <lijo.lazar@xxxxxxx>
[ Upstream commit 230753e46a4a9d04dad6a9b5dbaeb7fd52add7d0 ]
Internal US/DS switch may not be exposed in passthrough. Guard the
upstream port reads to avoid a NULL dereference.
Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx>
Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
---
LLM Generated explanations, may be completely bogus:
I have enough to decide: this is a NULL-deref on a sysfs read for MI300
passthrough, the buggy code is in 6.18.53, and the patch is small and
already reviewed.
---
## Phase 1: Commit message forensics
**Step 1.1: Subject line**
Record: `[drm/amdgpu] [Guard] Guard reads in PCIe state readout` —
“Guard” here is a missing-NULL-check / skip-on-missing-hardware fix, not
a feature.
**Step 1.2: Tags**
Record:
- `Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx>` (author; AMD amdgpu
contributor)
- `Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>` (amdgpu
maintainer)
- `Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>` (maintainer
merge)
- No `Fixes:`, `Reported-by:`, `Cc: stable`, `Link:`, `Tested-by:`, or
`Reviewed-by:` (absence of Fixes/Cc-stable is expected for this
review)
- Ignore `[Upstream commit 230753e46a4a...]` and `Signed-off-by: Sasha
Levin` (pipeline)
**Step 1.3: Body**
Record:
- Bug: internal US/DS PCIe switch may not be visible in passthrough
- Symptom: NULL dereference when reading upstream-port config
- Root cause: `pci_upstream_bridge()` can return NULL; the code still
uses the pointer
- No stack trace, no version list, no reproducer in the message
**Step 1.4: Hidden bug fix?**
Record: Not hidden. Explicit NULL-deref fix. Extra vendor/type checks
also avoid treating a hypervisor virtual bridge as the GPU’s internal
switch.
---
## Phase 2: Diff analysis
**Step 2.1: Inventory**
Record:
- File: `drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c` only
- ~42 insertions, ~17 deletions (59-line churn)
- New helper `aqua_vanjaram_get_us_pdev()`;
`aqua_vanjaram_read_pcie_state()` updated
- Classification: single-file surgical fix
**Step 2.2: Code flow**
Record:
- **Before:** `ds_pdev = pci_upstream_bridge(adev->pdev); us_pdev =
pci_upstream_bridge(ds_pdev);` then unconditional
`pcie_capability_read_word(us_pdev, ...)`,
`pci_find_ext_capability(us_pdev, ...)`,
`pci_read_config_dword(us_pdev, ...)`.
- **After:** helper requires ATI downstream port then ATI/AMD upstream
port; if either walk fails, skip those config reads. SMN register dump
still runs.
- Path: sysfs `reg_state` read (normal userspace debug path), not
probe/error-only.
**Step 2.3: Bug mechanism**
Record: **NULL pointer dereference** (memory safety).
- `pci_upstream_bridge()` in `include/linux/pci.h` returns NULL on a
root bus.
- `pci_upstream_bridge(NULL)` calls `pci_physfn(dev)` which does
`dev->is_virtfn` — immediate oops if the GPU has no parent.
- Typical VFIO topology (GPU under a QEMU root port): `ds_pdev` is non-
NULL, `us_pdev` is NULL, then `pcie_capability_read_word(NULL, ...)`
oopses.
- Helper matches the existing pattern in
`amdgpu_device_cache_switch_state()` /
`amdgpu_device_get_aspm_pdev()`.
**Step 2.4: Fix quality**
Record: Obviously correct; same topology checks already used nearby.
Residual: skipped fields are not zeroed (AER fields were already
conditional). That is much less severe than an oops. No new API, no
lock, no ABI change. Low regression risk.
---
## Phase 3: Git history
**Step 3.1: Blame**
Record: Unguarded walk introduced in `081a6eda2b2509` (“drm/amdgpu: Read
aquavanjaram PCIE register state”, 2023-11-29, Lijo Lazar). In this tree
since well before v6.18. Contained in v6.8-era history
(`locking_urgent_for_v6.8_rc2~...`).
**Step 3.2: Fixes: tag**
Record: No `Fixes:` tag. Introducer `081a6eda2b2509` **is** an ancestor
of `v6.18.53` (`git merge-base --is-ancestor` exit 0).
**Step 3.3: File history**
Record: Related NULL-guard already in this tree: `9b608fe94870f`
“drm/amdgpu: Check swus/ds for switch state save” (in v6.18.53; GitLab
drm/amd#4602). Same topology bug, different path (reset/save). This
patch is the sysfs readout counterpart. Standalone, not `N/M`.
**Step 3.4: Author**
Record: Lijo Lazar is a regular amdgpu author of this file. Maintainer
Alex Deucher acked and applied.
**Step 3.5: Dependencies**
Record: None. Uses existing `pci_upstream_bridge`,
`PCI_VENDOR_ID_ATI/AMD`, `pci_pcie_type`. No new helpers from later
series. Can apply standalone.
---
## Phase 4: Mailing list / external
**Step 4.1: Original discussion**
Record: `b4 dig -c 230753e46a4a9d04dad6a9b5dbaeb7fd52add7d0` →
https://patch.msgid.link/20260617093518.753118-1-lijo.lazar@xxxxxxx
`b4 dig -a`: **v1 only**, 2026-06-17. No later rewrite.
**Step 4.2: Recipients (`b4 dig -w`)**
Record: `amd-gfx@xxxxxxxxxxxxxxxxxxxxx`, Hawking Zhang, Alex Deucher,
Asad Kamal, Candice Li. Right list and maintainer.
**Step 4.3: Review (mbox `/tmp/pcie-guard.mbox`)**
Record:
- Kevin Wang: under VFIO the guest topology is virtualized; suggested
hiding the whole interface in PT/SRIOV.
- Lijo: SMN/endpoint reads still useful; this patch only skips the
physical-link (US port) reads that need real switch ports.
- Alex Deucher: `Acked-by` (2026-06-19). No NAK. No `Cc: stable` in
review.
**Step 4.4: Series**
Record: Single standalone patch.
**Step 4.5: Stable list**
Record: lore.kernel.org/stable fetch returned 403. UNVERIFIED whether
stable@ discussed it. Does not change the technical verdict.
---
## Phase 5: Semantic / call chain
**Step 5.1: Functions**
Record: `aqua_vanjaram_get_us_pdev` (new),
`aqua_vanjaram_read_pcie_state` (modified).
**Step 5.2: Callers**
Record:
- `aqua_vanjaram_read_pcie_state` ← `aqua_vanjaram_get_reg_state` (PCIE
case)
- ← `amdgpu_asic_get_reg_state` ← `amdgpu_sysfs_reg_state_get`
- Sysfs bin attr `reg_state` mode **0444**, offset
`AMDGPU_SYS_REG_STATE_PCIE = 0x2000`
- `get_reg_state` is only on `aqua_vanjaram_asic_funcs` (`soc15.c`)
**Step 5.3: Callees**
Record: `pci_upstream_bridge`, `pcie_capability_read_word`,
`pci_find_ext_capability`, `pci_read_config_dword`, plus existing SMN
reads (`aqua_read_smn` / `RREG32_PCIE`).
**Step 5.4: Reachability**
Record: Userspace-reachable: `read()` on `/sys/devices/.../reg_state` at
offset 0x2000 (or via drm sysfs). Not interrupt/hot path. Any process
that can read that 0444 file can trigger it.
**Step 5.5: Similar patterns**
Record: Same US/DS walk in `amdgpu_device_cache_switch_state()` (already
NULL-checked in this tree) and `amdgpu_device_get_aspm_pdev()`.
---
## Phase 6: Local tree (v6.18.53)
**Step 6.1: Does the bug exist here?**
Record:
- `git describe HEAD`: `v6.18.53-5049-g0f0bdc2b0dffb`;
`VERSION/PATCHLEVEL/SUBLEVEL` = 6.18.53
- `git merge-base --is-ancestor 9bd596d9e90da v6.18.53` → **no** (fix
not in 6.18.53)
- `git merge-base --is-ancestor 081a6eda2b2509 v6.18.53` → **yes** (bug
is in 6.18.53)
- At `v6.18.53` lines 653–670: unguarded `pci_upstream_bridge` + config
reads
**Step 6.2: Backport difficulty**
Record: `git apply --check` of both the candidate and upstream diffs
onto the v6.18.53 file **succeeds** (hunk offset only). Context around
`pcie_reg_addrs` / `aqua_vanjaram_read_pcie_state` matches. **Clean
apply.**
**Step 6.3: Related fixes already here?**
Record: Switch-state save is guarded (`9b608fe94870f` in v6.18.53).
Sysfs PCIe readout is **not**. No earlier version of this helper in
6.18.53.
---
## Phase 7: Subsystem context
**Step 7.1**
Record: amdgpu / Aqua Vanjaram (Instinct MI300 family: GC 9.4.3, 9.4.4,
9.5.0). **IMPORTANT** (datacenter GPU driver), not core mm/VFS, not
staging.
**Step 7.2**
Record: File is actively developed, but this readout path has been
unchanged since 2023. Mature buggy code, not a brand-new feature.
---
## Phase 8: Impact and risk
**Step 8.1: Who is affected**
Record: Driver-specific: MI300/MI308/MI350 (`IP_VERSION(9,4,3/4)` and
`(9,5,0)`). Highest impact: **VFIO/QEMU passthrough guests** where the
internal US/DS switch is hidden. Bare metal with the real AMD switch is
unaffected.
**Step 8.2: Trigger**
Record: Reading sysfs `reg_state` at PCIE offset. Common for AMD
debug/RAS dumps; also trivial to hit (`dd`/`pread` at 0x2000).
Unprivileged users can read 0444 sysfs. Not a boot-every-time path.
**Step 8.3: Failure mode**
Record: Kernel oops / NULL deref. **Severity: HIGH** (crash; local DoS
from sysfs). Not silent corruption.
**Step 8.4: Risk-benefit**
Record:
- Benefit: stops a real oops on MI300 passthrough when diagnostics read
PCIe state — production cloud/HPC setup.
- Risk: ~25-line helper + `if (us_pdev)` around existing reads; mirrors
code already in this tree. **Low.**
- Ratio: strongly favors backport.
---
## Phase 9: Synthesis
**Step 9.1: Evidence**
For:
- Real NULL deref, reachable from sysfs
- Buggy code in this 6.18.53 tree since 2023
- Passthrough is a stated, realistic trigger
- Same topology already caused a user-visible bug (drm/amd#4602) on the
sibling path
- Small, obviously correct, maintainer-acked
- Applies cleanly to v6.18.53
- No new features/APIs
Against:
- Affects one ASIC family, debug sysfs, not a universal path
- No `Reported-by` / public crash log on *this* function
- Kevin argued for hiding the whole interface (not a NAK of the NULL
check)
- Skipped struct fields left uninitialized (minor, pre-existing for AER)
Unresolved:
- Stable@ discussion (lore 403)
- Whether a specific production crash dump exists for this sysfs path
(unverified; mechanism is still verified in code)
**Step 9.2: Stable rules**
1. Obviously correct and tested? **PASS** (mirror of existing checks;
maintainer Acked-by)
2. Real bug affecting users? **PASS** (oops on MI300 passthrough sysfs
read)
3. Important (crash/security/corruption/deadlock)? **PASS** — oops /
local DoS, HIGH
4. Small and contained? **PASS** — one file, one helper + guard
5. No new features/APIs? **PASS**
6. Applies to local tree? **PASS** — clean apply; buggy code present
**Step 9.3: Exceptions**
Record: None (not a device-ID/quirk/DT/build/doc-only change). Qualifies
as a normal crash fix.
**Step 9.4: Decision**
Backport. This tree is 6.18.53; the unguarded walk is present; the fix
is a small NULL-deref guard for a userspace-reachable path on hardware
this kernel already supports.
---
### What the commit solves for this tree
`aqua_vanjaram_read_pcie_state()` walks to the GPU’s internal upstream
switch and reads PCIe/AER config with no NULL or topology check. In
passthrough that switch is often missing, so `pci_upstream_bridge()`
returns NULL and the kernel oopses. The helper skips those reads when
the ATI/AMD US/DS chain is not there, while still dumping SMN regs.
### Risk vs benefit
Benefit is avoiding a reproducible oops on Instinct MI300-class GPUs in
VFIO guests. Risk is a narrow, identical-to-existing-pattern check.
Applies cleanly to 6.18.53.
### Verification
- [Phase 1] Parsed tags: author SOB, Alex Deucher Acked-by + SOB; no
Fixes/Reported-by/Cc-stable
- [Phase 2] Diff: helper + `if (us_pdev)` around config reads in
`aqua_vanjaram.c`; SMN loop unchanged
- [Phase 2] `pci_upstream_bridge()` / `pci_physfn()` read: NULL parent →
NULL deref
- [Phase 3] `git blame v6.18.53`: lines from `081a6eda2b2509` (2023)
- [Phase 3] `git merge-base --is-ancestor 081a6eda2b2509 v6.18.53`: yes
(bug in this tree)
- [Phase 3] `git merge-base --is-ancestor 9bd596d9e90da v6.18.53`: no
(fix not in 6.18.53)
- [Phase 3] Related `9b608fe94870f` already in v6.18.53 (switch-state
save)
- [Phase 4] `b4 dig -c 230753e46a4a...`: lore thread; v1 only
- [Phase 4] mbox: Kevin design comment; Lijo kept SMN reads; Alex Acked-
by; no NAK
- [Phase 4] lore.kernel.org/stable: **UNVERIFIED** (HTTP 403)
- [Phase 5] Callers: sysfs `reg_state` 0444 → PCIE offset 0x2000; only
`aqua_vanjaram_asic_funcs`
- [Phase 6] `git describe HEAD` / Makefile: 6.18.53; unguarded code at
v6.18.53:653–670
- [Phase 6] `git apply --check` of candidate and upstream patches onto
v6.18.53 file: success (offset only)
- [Phase 8] Failure mode: NULL deref oops on sysfs read in passthrough,
HIGH
- UNVERIFIED: no public crash log specifically for this sysfs function
(mechanism still verified)
**YES**
drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c | 59 +++++++++++++++-------
1 file changed, 42 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
index 15e70415c35b4..b73e114206cc1 100644
--- a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
+++ b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
@@ -613,6 +613,29 @@ static struct aqua_reg_list pcie_reg_addrs[] = {
{ smreg_0x1A380088, 6, DW_ADDR_INCR },
};
+/*
+ * Return the GPU's internal US switch port, or NULL if it is not visible
+ * (e.g. passthrough) or the EP is parented under an unrelated bridge.
+ */
+static struct pci_dev *aqua_vanjaram_get_us_pdev(struct amdgpu_device *adev)
+{
+ struct pci_dev *ds_pdev, *us_pdev;
+
+ ds_pdev = pci_upstream_bridge(adev->pdev);
+ if (!ds_pdev || ds_pdev->vendor != PCI_VENDOR_ID_ATI ||
+ pci_pcie_type(ds_pdev) != PCI_EXP_TYPE_DOWNSTREAM)
+ return NULL;
+
+ us_pdev = pci_upstream_bridge(ds_pdev);
+ if (!us_pdev ||
+ (us_pdev->vendor != PCI_VENDOR_ID_ATI &&
+ us_pdev->vendor != PCI_VENDOR_ID_AMD) ||
+ pci_pcie_type(us_pdev) != PCI_EXP_TYPE_UPSTREAM)
+ return NULL;
+
+ return us_pdev;
+}
+
static ssize_t aqua_vanjaram_read_pcie_state(struct amdgpu_device *adev,
void *buf, size_t max_size)
{
@@ -620,7 +643,7 @@ static ssize_t aqua_vanjaram_read_pcie_state(struct amdgpu_device *adev,
uint32_t start_addr, incrx, num_regs, szbuf;
struct amdgpu_regs_pcie_v1_0 *pcie_regs;
struct amdgpu_smn_reg_data *reg_data;
- struct pci_dev *us_pdev, *ds_pdev;
+ struct pci_dev *us_pdev;
int aer_cap, r, n;
if (!buf || !max_size)
@@ -652,25 +675,27 @@ static ssize_t aqua_vanjaram_read_pcie_state(struct amdgpu_device *adev,
}
}
- ds_pdev = pci_upstream_bridge(adev->pdev);
- us_pdev = pci_upstream_bridge(ds_pdev);
+ us_pdev = aqua_vanjaram_get_us_pdev(adev);
+ if (us_pdev) {
+ pcie_capability_read_word(us_pdev, PCI_EXP_DEVSTA,
+ &pcie_regs->device_status);
+ pcie_capability_read_word(us_pdev, PCI_EXP_LNKSTA,
+ &pcie_regs->link_status);
+
+ aer_cap = pci_find_ext_capability(us_pdev, PCI_EXT_CAP_ID_ERR);
+ if (aer_cap) {
+ pci_read_config_dword(us_pdev,
+ aer_cap + PCI_ERR_COR_STATUS,
+ &pcie_regs->pcie_corr_err_status);
+ pci_read_config_dword(us_pdev,
+ aer_cap + PCI_ERR_UNCOR_STATUS,
+ &pcie_regs->pcie_uncorr_err_status);
+ }
- pcie_capability_read_word(us_pdev, PCI_EXP_DEVSTA,
- &pcie_regs->device_status);
- pcie_capability_read_word(us_pdev, PCI_EXP_LNKSTA,
- &pcie_regs->link_status);
-
- aer_cap = pci_find_ext_capability(us_pdev, PCI_EXT_CAP_ID_ERR);
- if (aer_cap) {
- pci_read_config_dword(us_pdev, aer_cap + PCI_ERR_COR_STATUS,
- &pcie_regs->pcie_corr_err_status);
- pci_read_config_dword(us_pdev, aer_cap + PCI_ERR_UNCOR_STATUS,
- &pcie_regs->pcie_uncorr_err_status);
+ pci_read_config_dword(us_pdev, PCI_PRIMARY_BUS,
+ &pcie_regs->sub_bus_number_latency);
}
- pci_read_config_dword(us_pdev, PCI_PRIMARY_BUS,
- &pcie_regs->sub_bus_number_latency);
-
pcie_reg_state->common_header.structure_size = szbuf;
pcie_reg_state->common_header.format_revision = 1;
pcie_reg_state->common_header.content_revision = 0;
--
2.53.0