[PATCH AUTOSEL 7.0-6.12] 9p/trans_xen: make cleanup idempotent after dataring alloc errors

[Sasha Levin <sashal@kernel.org>]

From: Yufan Chen <ericterminal@gmail.com>

[ Upstream commit 72cb9ee4f6d80962df17c9763b14e62e28fd85a2 ]

xen_9pfs_front_alloc_dataring() tears down resources on failure but
leaves ring fields stale. If xen_9pfs_front_init() later jumps to the
common error path, xen_9pfs_front_free() may touch the same resources
again, causing duplicate/invalid gnttab_end_foreign_access() calls and
potentially dereferencing a freed intf pointer.

Initialize dataring sentinels before allocation, gate teardown on those
sentinels, and clear ref/intf/data/irq immediately after each release.

This keeps cleanup idempotent for partially initialized rings and
prevents repeated teardown during init failure handling.

Signed-off-by: Yufan Chen <ericterminal@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Message-ID: <20260324153023.86853-2-ericterminal@gmail.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have enough information to make a thorough analysis.

## Phase 1: Commit Message Forensics

**Step 1.1 — Subject line:**
- Record: subsystem=`9p/trans_xen`, action verb=`make` (with implicit
  "fix"), summary=make cleanup idempotent after dataring alloc errors.
  The phrase "make cleanup idempotent" is a classic disguised bug-fix
  verb pattern — it's preventing duplicate teardown.

**Step 1.2 — Tags parsing:**
- Record: `Signed-off-by: Yufan Chen` (author), `Reviewed-by: Stefano
  Stabellini <sstabellini@kernel.org>` (the original author/maintainer
  of trans_xen.c — strong endorsement), `Message-ID:` to lore, `Signed-
  off-by: Dominique Martinet <asmadeus@codewreck.org>` (9p maintainer).
  No `Fixes:`, no `Cc: stable` (expected — that's why this is being
  reviewed). No syzbot, no Reported-by.

**Step 1.3 — Body analysis:**
- Record: Body explains the mechanism precisely —
  `xen_9pfs_front_alloc_dataring()` releases resources on failure but
  leaves pointer/ref fields stale. If init then jumps to common error
  path, `xen_9pfs_front_free()` re-touches them, causing
  "duplicate/invalid `gnttab_end_foreign_access()` calls and potentially
  dereferencing a freed `intf` pointer". Symptom = double teardown + UAF
  on partially initialized rings during init failure.

**Step 1.4 — Hidden bug fix detection:**
- Record: Yes — "make cleanup idempotent" is a textbook hidden bug-fix
  subject. The phrase "potentially dereferencing a freed intf pointer"
  makes the use-after-free explicit. Cover letter (PATCH v3 0/2) states:
  "Patch 1 fixes a potential double-free/Oops during initialization
  failure" and "Tested error paths by forcing init failures on non-Xen
  systems; dmesg confirms the new sentinel-based cleanup correctly
  prevents Oops." So an actual Oops was observed.

## Phase 2: Diff Analysis

**Step 2.1 — Inventory:**
- Record: One file `net/9p/trans_xen.c`, +37/-14 lines, two functions
  changed: `xen_9pfs_front_free()` and
  `xen_9pfs_front_alloc_dataring()`. Single-file surgical fix, scope =
  error path / cleanup only.

**Step 2.2 — Code flow:**
- Record (alloc_dataring): Before — fields are not initialized to
  sentinels; on `out:` path, frees `bytes`/`intf` and revokes
  `ring->ref` unconditionally without clearing the fields. After —
  fields set to NULL/`INVALID_GRANT_REF`/-1 at the top; `out:` only
  frees what's set, then clears the fields after each release.
- Record (front_free): Before — uses `if (priv->rings[i].irq > 0)` and
  unconditionally calls `gnttab_end_foreign_access(ring->ref, NULL)` and
  `free_page(ring->intf)`. After — uses `if (ring->irq >= 0)` then
  resets to -1; checks `ring->ref != INVALID_GRANT_REF`; clears
  intf/ref/data.in/data.out/irq after each release.

**Step 2.3 — Bug mechanism:**
- Record: This is BOTH (a) error path / resource leak fixes AND (d)
  memory safety fixes:
  - **Double-free of `ring->intf`**: `xen_9pfs_front_alloc_dataring()`
    calls `free_page((unsigned long)ring->intf)` on failure but leaves
    the pointer pointing to freed memory. Init then calls
    `xen_9pfs_front_free()` whose check `if (!priv->rings[i].intf)
    break;` does NOT trip (stale non-NULL pointer), so
    `free_page((unsigned long)priv->rings[i].intf)` runs again → kernel
    page double-free.
  - **Double `gnttab_end_foreign_access` on `ring->ref`**: same path re-
    revokes a stale grant ref.
  - **Use-after-free of `ring->intf`**: if alloc failed at the
    `xenbus_alloc_evtchn` stage, `ring->data.in` was set, then `bytes`
    was freed by alloc_dataring's cleanup. On the second pass through
    front_free, the `if (ring->data.in)` branch dereferences
    `ring->intf->ring_order` and `ring->intf->ref[j]` (already-freed
    page) → UAF read; then calls `gnttab_end_foreign_access` on stale
    grant refs and `free_pages_exact` on already-freed `data.in`.

**Step 2.4 — Fix quality:**
- Record: Obviously correct — sentinel-based teardown is a standard
  idempotent-cleanup pattern. Each release is gated by a sentinel and
  the field is invalidated afterward. The change `irq > 0` → `irq >= 0`
  is also a defensive correction (with explicit `-1` init, this is the
  proper check). No new locking, no new APIs, no behaviour change on the
  success path. Regression risk is very low.

## Phase 3: Git History Investigation

**Step 3.1 — Blame:**
- Record: The buggy alloc_dataring code came from `71ebd71921e45`
  ("xen/9pfs: connect to the backend"), part of v4.12-rc1 (Apr 2017).
  Bug has been latent in every kernel since v4.12, so all currently-
  supported LTS trees (5.4, 5.10, 5.15, 6.1, 6.6, 6.12, 6.18+) carry it.

**Step 3.2 — Fixes: target:**
- Record: No `Fixes:` tag in the commit. The introducing commit
  `71ebd71921e45` is in mainline since v4.12, so it definitely exists in
  every active stable tree.

**Step 3.3 — File history:**
- Record: Recent related fixes on this file that are already in stable:
  `e43c608f40c06` ("9p/xen: fix release of IRQ"), `7ef3ae82a6ebb`
  ("9p/xen: fix init sequence"), `ea4f1009408ef` ("9p/xen: Fix UAF in
  xen_9pfs_front_remove"), `ce8ded2e61f47` ("9p/xen: protect
  xen_9pfs_front_free against concurrent calls"). All are small
  stability fixes. The current patch is standalone and not part of a
  multi-patch dependent series; series cover letter shows it splits into
  2/2 patches but patch 2 (parser cleanup with kstrtouint) is
  independent.

**Step 3.4 — Author context:**
- Record: Yufan Chen is a contributor; the patch was reviewed by Stefano
  Stabellini who is the original author/long-time maintainer of
  `trans_xen.c` (copyright at top of file). Authoritative review.

**Step 3.5 — Dependencies:**
- Record: Uses `INVALID_GRANT_REF`, defined in
  `include/xen/grant_table.h` since `bce21a2b48ede` (v5.12-rc3). This
  macro is present in all current stable LTS trees (verified in 5.15 —
  `#define INVALID_GRANT_REF ((grant_ref_t)-1)` at line 57). No other
  dependencies. Self-contained patch.

## Phase 4: Mailing List Research

**Step 4.1 — b4 dig:**
- Record: `b4 dig -c 72cb9ee4f6d80` matched by patch-id, returned `https
  ://lore.kernel.org/all/20260324153023.86853-2-ericterminal@gmail.com/`
  (v3 1/2).
- `b4 dig -a` showed evolution: v1 (single patch, 2026-02-25), v2 (1/4
  in mixed series, 2026-02-25), v3 (1/2 in dedicated 9p/trans_xen
  series, 2026-03-24). Applied version is the latest.
- v3 cover letter: "Patch 1 fixes a potential double-free/Oops during
  initialization failure by making the dataring cleanup idempotent."
  Confirms the author treats this as a stability/bug fix.

**Step 4.2 — Reviewers:**
- Record: Reviewed-by Stefano Stabellini (subsystem maintainer), CC'd
  Eric Van Hensbergen (ericvh@kernel.org), Lucho Ionkov
  (lucho@ionkov.net), and the v9fs list. The right people reviewed it.

**Step 4.3 — Bug report:**
- Record: No external bug report. Bug discovered by code inspection and
  confirmed by deliberate fault injection during testing (per the v3
  cover letter). No syzbot.

**Step 4.4 — Series context:**
- Record: 2-patch series. Patch 2 ("replace simple_strto* with
  kstrtouint") is unrelated parser modernization and not stable
  material. This patch (1/2) is fully standalone — no dependency on
  patch 2.

**Step 4.5 — Stable list:**
- Record: No prior discussion on stable list found via b4 dig. Author
  did not Cc stable, but recent precedent shows similar 9p/xen
  idempotency-style fixes (`e43c608`, `7ef3ae82`, `ea4f1009`,
  `ce8ded2e`) were backported to 5.15.y, 6.1.y, 6.6.y, 6.12.y as stable-
  eligible bug fixes.

## Phase 5: Code Semantic Analysis

**Step 5.1 — Functions modified:**
- Record: `xen_9pfs_front_free()`, `xen_9pfs_front_alloc_dataring()`.

**Step 5.2 — Callers:**
- Record: `xen_9pfs_front_alloc_dataring` is called from
  `xen_9pfs_front_init` (in a loop over `XEN_9PFS_NUM_RINGS`).
  `xen_9pfs_front_free` is called from `xen_9pfs_front_remove` (xenbus
  driver remove callback) AND from `xen_9pfs_front_init` error path.
  Critical: both callers are in the device probe/teardown flow, which is
  exactly the scenario the patch protects against.

**Step 5.3 — Callees:**
- Record: `gnttab_end_foreign_access`, `free_page`, `free_pages_exact`,
  `unbind_from_irqhandler`, `cancel_work_sync`.
  `gnttab_end_foreign_access(ref, NULL)` calls into
  `gnttab_try_end_foreign_access` → `_gnttab_end_foreign_access_ref` →
  indirect into the gnttab interface; reentering with stale ref produces
  warnings or worse on backend interaction.

**Step 5.4 — Reachability:**
- Record: Triggered from `xenbus_driver` callback chain when a 9pfs
  frontend tries to come up and any of these fails: `get_zeroed_page`
  (memory pressure), `gnttab_grant_foreign_access` (grant-table
  exhaustion — realistic on busy Xen guests), `alloc_pages_exact`,
  `xenbus_alloc_evtchn` (event-channel exhaustion),
  `bind_evtchn_to_irqhandler`. Reachable on every 9pfs frontend probe
  under resource pressure or hostile/buggy backend.

**Step 5.5 — Similar patterns:**
- Record: Idempotent-cleanup-with-sentinels is the same pattern used
  throughout xen frontends. The previous 9p/xen fixes (`e43c608`,
  `ce8ded2e`) target the same teardown function and were backported to
  stable.

## Phase 6: Cross-Referencing & Stable Tree Analysis

**Step 6.1 — Code presence:**
- Record: Verified by reading `git show
  stable/linux-6.6.y:net/9p/trans_xen.c` and `git show
  stable/linux-6.12.y:net/9p/trans_xen.c` — both contain the same buggy
  `xen_9pfs_front_alloc_dataring()` cleanup pattern and the same
  `xen_9pfs_front_free()` un-gated double-teardown. Bug present in 5.4,
  5.10, 5.15, 6.1, 6.6, 6.12, 6.18 (all active LTS).

**Step 6.2 — Backport complications:**
- Record: 6.12.y file matches mainline structure almost exactly — minor
  context-only deltas. 6.6.y / 6.1.y / 5.15.y use `priv->num_rings`
  instead of the constant in the loop and have a slightly different
  `xen_9pfs_front_free` outline (no `priv->rings` NULL check at the top
  in 6.6) — those need trivial mechanical adjustment.
  `INVALID_GRANT_REF` is available in all active LTS. Expected
  difficulty: clean-to-minor.

**Step 6.3 — Related fixes already in stable:**
- Record: Verified — `2bb3ee1bf2375` (6.6), `b9e26059664bd` (6.1),
  `4950408793b11` (5.15), `530bc9f03a102` (6.12) are the IRQ-double-free
  fix; `592fb738d8682`/`91b4763da3ee6`/`db94e06c24cd4`/`e978643c4c9c0`
  are the init-sequence fix; `a5d00dff97118` is the concurrent-
  front_free protection. None of these address the alloc-failure
  idempotency bug — this patch fills a remaining gap.

## Phase 7: Subsystem Context

**Step 7.1 — Subsystem:**
- Record: `net/9p/` — 9P virtual filesystem transport, Xen-specific.
  Criticality: PERIPHERAL globally but IMPORTANT for users who actually
  use 9P over Xen (e.g., Edera and other Xen-based confidential-
  computing/lightweight-VM stacks who recently submitted other 9p/xen
  fixes).

**Step 7.2 — Activity:**
- Record: Active subsystem with periodic stability-fix submissions in
  2024–2026; multiple recent patches went to stable.

## Phase 8: Impact and Risk

**Step 8.1 — Affected population:**
- Record: Users of Xen 9pfs frontend. Niche but real (Edera, others
  using 9p mounts in Xen guests).

**Step 8.2 — Trigger conditions:**
- Record: Failure during second-ring allocation in
  `xen_9pfs_front_init`. Triggers include memory pressure, grant-table
  exhaustion, evtchn exhaustion, malicious/buggy Xen backend. Not user-
  triggerable from unprivileged userspace, but a malicious backend can
  deliberately starve the frontend (Xen security model assumes the
  backend is more privileged but a frontend should not crash on backend
  misbehaviour).

**Step 8.3 — Severity:**
- Record: When triggered → kernel page double-free + grant ref double-
  revoke + use-after-free read on a freed page. Failure mode: kernel
  oops / panic / memory corruption. Severity: CRITICAL.

**Step 8.4 — Risk-benefit:**
- Record: Benefit = high (eliminates a confirmed Oops on init failure,
  idempotent cleanup is universally desirable). Risk = very low — pure
  error-path tightening, sentinel-based, no behaviour change on success
  path, reviewed by the original author Stefano Stabellini, tested with
  deliberate fault injection.

## Phase 9: Final Synthesis

**Evidence FOR backport:**
- Real bug — double-free of kernel page, use-after-free, double grant-
  ref revoke during init failure (CRITICAL severity)
- Reproduced (Oops) by author with fault injection in virtme-ng
- Reviewed by the original author/maintainer of the file (Stefano
  Stabellini)
- Small, surgical, single file (+37/-14)
- Bug present since v4.12 — affects every active LTS
- All dependencies (`INVALID_GRANT_REF`) present in stable trees
- Recent precedent: 4 similar 9p/xen fixes have been backported to
  5.15/6.1/6.6/6.12
- Idempotent cleanup is a textbook stable-friendly pattern, no
  behavioural change on success path

**Evidence AGAINST:**
- No `Fixes:` tag, no `Cc: stable` (expected, not a real negative
  signal)
- No external user bug report (but author observed Oops during testing)
- Niche subsystem (9p over Xen)

**Stable rules checklist:**
1. Obviously correct and tested? Yes — sentinel pattern, reviewed by
   maintainer, fault-injected by author.
2. Real bug? Yes — confirmed Oops.
3. Important issue? Yes — double-free / UAF (CRITICAL).
4. Small and contained? Yes — 51 lines, one file.
5. No new features? Correct — pure cleanup hardening.
6. Applies to stable? Yes for 6.12.y essentially clean; 6.6/6.1/5.15
   need trivial loop-variable adjustment.

**Decision:** This is a small, well-reviewed, fault-injection-confirmed
fix for a memory-safety bug (double-free + UAF) that has been latent in
Xen 9p frontend code since 2017 and exists in every active LTS tree. It
matches the pattern of multiple similar 9p/xen stability fixes already
backported to stable. Backport-worthy.

## Verification

- [Phase 1] Read commit message and v3 cover letter from saved mbox
  `/tmp/9pxen-thread.mbox` — confirmed "fixes a potential double-
  free/Oops during initialization failure" and "Tested error paths by
  forcing init failures... dmesg confirms the new sentinel-based cleanup
  correctly prevents Oops".
- [Phase 1] Confirmed Reviewed-by from Stefano Stabellini in the mbox
  thread.
- [Phase 2] Read full pre-fix `net/9p/trans_xen.c` and post-fix;
  manually traced ring-1 alloc failure scenarios at four distinct
  failure points and confirmed each leads to either double
  `free_page(intf)`, double `gnttab_end_foreign_access(ref)`, or UAF
  read of `ring->intf->ring_order`/`ring->intf->ref[j]`.
- [Phase 3] `git log --oneline --follow net/9p/trans_xen.c` showed
  `71ebd71921e45` as origin; `git describe --contains 71ebd71921e45` →
  `v4.12-rc1~103^2~31`.
- [Phase 3] `git show 71ebd71921e45` confirmed the pre-existing buggy
  `xen_9pfs_front_alloc_dataring`+`xen_9pfs_front_free` cleanup pattern
  was introduced in 2017.
- [Phase 3] Found prior related fixes (`e43c608`, `7ef3ae82`,
  `ea4f1009`, `ce8ded2e`) on the same file with their stable tree
  counterparts.
- [Phase 4] `b4 dig -c 72cb9ee4f6d80` returned the lore URL for v3 1/2.
- [Phase 4] `b4 dig -c 72cb9ee4f6d80 -a` showed v1 → v2 → v3 evolution;
  applied version is latest.
- [Phase 4] `b4 dig -m /tmp/9pxen-thread.mbox` saved full thread; read
  entire mbox with Read tool.
- [Phase 4] No `Cc: stable` and no NAKs in the thread; only Reviewed-by
  from Stefano.
- [Phase 5] Confirmed `xen_9pfs_front_alloc_dataring` is called from
  `xen_9pfs_front_init` (loop over rings) and `xen_9pfs_front_free` is
  called from both `xen_9pfs_front_init` error path and
  `xen_9pfs_front_remove`.
- [Phase 5] Read `drivers/xen/grant-table.c` to confirm
  `gnttab_end_foreign_access` chain — invoking on a stale ref re-enters
  the gnttab interface.
- [Phase 6] Verified `INVALID_GRANT_REF` exists in
  `include/xen/grant_table.h` of mainline (line 57) and
  `stable/linux-5.15.y` — backport-friendly.
- [Phase 6] Read `stable/linux-6.6.y:net/9p/trans_xen.c` and
  `stable/linux-6.12.y:net/9p/trans_xen.c` and confirmed the buggy code
  pattern is present in both.
- [Phase 6] Verified previous 9p/xen fixes were backported to
  5.15/6.1/6.6/6.12 stable branches via `git log <branch> --
  net/9p/trans_xen.c`.
- [Phase 8] Failure mode verified by manual trace: double-free of a
  kernel page + use-after-free read on stale `ring->intf` + double
  grant-revoke → CRITICAL.
- UNVERIFIED: No external bug report or syzbot reproducer; severity
  rests on author's fault-injection result and direct code analysis
  (both consistent with each other).

**YES**

 net/9p/trans_xen.c | 51 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 47af5a10e9212..85b9ebfaa17a6 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -283,25 +283,33 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
 
 			cancel_work_sync(&ring->work);
 
-			if (!priv->rings[i].intf)
+			if (!ring->intf)
 				break;
-			if (priv->rings[i].irq > 0)
-				unbind_from_irqhandler(priv->rings[i].irq, ring);
-			if (priv->rings[i].data.in) {
-				for (j = 0;
-				     j < (1 << priv->rings[i].intf->ring_order);
+			if (ring->irq >= 0) {
+				unbind_from_irqhandler(ring->irq, ring);
+				ring->irq = -1;
+			}
+			if (ring->data.in) {
+				for (j = 0; j < (1 << ring->intf->ring_order);
 				     j++) {
 					grant_ref_t ref;
 
-					ref = priv->rings[i].intf->ref[j];
+					ref = ring->intf->ref[j];
 					gnttab_end_foreign_access(ref, NULL);
+					ring->intf->ref[j] = INVALID_GRANT_REF;
 				}
-				free_pages_exact(priv->rings[i].data.in,
-				   1UL << (priv->rings[i].intf->ring_order +
-					   XEN_PAGE_SHIFT));
+				free_pages_exact(ring->data.in,
+						 1UL << (ring->intf->ring_order +
+							 XEN_PAGE_SHIFT));
+				ring->data.in = NULL;
+				ring->data.out = NULL;
+			}
+			if (ring->ref != INVALID_GRANT_REF) {
+				gnttab_end_foreign_access(ring->ref, NULL);
+				ring->ref = INVALID_GRANT_REF;
 			}
-			gnttab_end_foreign_access(priv->rings[i].ref, NULL);
-			free_page((unsigned long)priv->rings[i].intf);
+			free_page((unsigned long)ring->intf);
+			ring->intf = NULL;
 		}
 		kfree(priv->rings);
 	}
@@ -334,6 +342,12 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
 	int ret = -ENOMEM;
 	void *bytes = NULL;
 
+	ring->intf = NULL;
+	ring->data.in = NULL;
+	ring->data.out = NULL;
+	ring->ref = INVALID_GRANT_REF;
+	ring->irq = -1;
+
 	init_waitqueue_head(&ring->wq);
 	spin_lock_init(&ring->lock);
 	INIT_WORK(&ring->work, p9_xen_response);
@@ -379,9 +393,18 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
 		for (i--; i >= 0; i--)
 			gnttab_end_foreign_access(ring->intf->ref[i], NULL);
 		free_pages_exact(bytes, 1UL << (order + XEN_PAGE_SHIFT));
+		ring->data.in = NULL;
+		ring->data.out = NULL;
+	}
+	if (ring->ref != INVALID_GRANT_REF) {
+		gnttab_end_foreign_access(ring->ref, NULL);
+		ring->ref = INVALID_GRANT_REF;
+	}
+	if (ring->intf) {
+		free_page((unsigned long)ring->intf);
+		ring->intf = NULL;
 	}
-	gnttab_end_foreign_access(ring->ref, NULL);
-	free_page((unsigned long)ring->intf);
+	ring->irq = -1;
 	return ret;
 }
 
-- 
2.53.0