From c83eefd525be14a1460e44f7102ef02d934eb8b6 Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Fri, 13 Nov 2026 13:54:17 -0800 Subject: [PATCH 1/5] 9p/xen: only let an errno come back from a control request's Rlerror xen_9pfs_front_ctl() answers a backend Rlerror with -(int)ecode, and the ecode is the backend's to choose. An ecode of 0 came back as 0 and any ecode above INT_MAX came back positive, and the function's contract is "reply length or negative errno", so a caller took either for a good reply of that length. xen_9pfs_front_suspend() then read total and next from beyond the short error reply and computed the chunk length from the bogus "reply length": an ecode of 0xfff00000 had it copy about 1 MiB of uninitialised buffer into a session chunk and later hand it to the backend in Tsession_restore, and 0x80000010 had it memcpy() some 2 GiB past the reply buffer. Take only a plausible errno from an Rlerror, 1..MAX_ERRNO, and answer anything else with -EPROTO, so a control request can only ever return a negative errno or a reply that carries the id and body it asked for. Signed-off-by: Ariadne Conill (cherry picked from commit 47a8726039adec952c8043d70d152e9c89d69d54) --- net/9p/trans_xen.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index 81ced50975d590..fd168fc48f633e 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -585,8 +585,19 @@ static int xen_9pfs_front_ctl(struct xen_9pfs_front_priv *priv, reply_len = priv->ctl_reply_len; if (reply_len < P9_HDRSZ || get_unaligned_le32(reply) != reply_len) return -EPROTO; - if (reply[4] == P9_RLERROR && reply_len >= P9_HDRSZ + 4) - return -(int)get_unaligned_le32(reply + P9_HDRSZ); + if (reply[4] == P9_RLERROR) { + u32 ecode; + + /* The backend chooses the ecode; only an errno may come back + * from here, or a caller would take it for a reply length. + */ + if (reply_len < P9_HDRSZ + 4) + return -EPROTO; + ecode = get_unaligned_le32(reply + P9_HDRSZ); + if (!ecode || ecode > MAX_ERRNO) + return -EPROTO; + return -(int)ecode; + } if (reply[4] != id || reply_len < P9_HDRSZ + body) return -EPROTO; return reply_len; From 60ac278be8dd93b8fbbd2f9eee5cfcad6eda6bbb Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Fri, 13 Nov 2026 13:54:46 -0800 Subject: [PATCH 2/5] 9p/xen: do not dereference priv->client while replaying a saved session The Connected handler replays the fids saved at suspend from xenwatch, after do_suspend() has returned and processes run again. It checked priv->client once and then read priv->client->msize for every chunk, each of which waits up to 5 s on the backend. Meanwhile a umount or a fresh mount blocked on the still-disconnected frontend can be killed: p9_client_destroy() then runs p9_xen_close(), which clears priv->client with no lock the replay holds, and frees the client. The next chunk dereferences NULL, or the freed client, and xenwatch oopses, which stops all xenbus processing for the guest. Record the owning client and its msize when the session is saved, with userspace frozen and the client known to be alive, and have the replay use those. It compares priv->client with the owner by address only, before starting and before each chunk, and gives up if they differ: a client destroyed in between does not want its fids back, and nothing is dereferenced that could have gone away. Signed-off-by: Ariadne Conill (cherry picked from commit 1aed70cf1c48c43bf139090100a5d3eae2ecb287) --- net/9p/trans_xen.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index fd168fc48f633e..e4155cbe147904 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -90,6 +90,12 @@ struct xen_9pfs_front_priv { * they survive into a restored or forked domain. */ struct list_head session; + /* The client whose fids those are, and its msize. The restore only + * compares the pointer: nothing keeps the client alive until then, and + * if it was destroyed in between its fids are not wanted back. + */ + struct p9_client *session_client; + u32 session_msize; /* A control request on ring 0, answered with tag P9_NOTAG. */ bool ctl_pending; struct completion ctl_done; @@ -627,6 +633,8 @@ static int xen_9pfs_front_suspend(struct xenbus_device *dev) client = priv->client; if (!READ_ONCE(priv->connected) || !client) goto out; + priv->session_client = client; + priv->session_msize = client->msize; /* Let what is in flight finish first, so that the saved fids match * what the guest has seen answered. @@ -994,15 +1002,21 @@ static void xen_9pfs_front_changed(struct xenbus_device *dev, } /* Replay the fids saved at suspend before any request can use - * them. + * them. Processes are running again by now, so the client + * that owns them can be destroyed under us at any point, which + * p9_xen_close() records by clearing priv->client: never + * dereference it here, only compare it with the owner. */ down_read(&priv->rings_sem); ret = 0; flags = P9_XEN_SESSION_RESET; - if (!list_empty(&priv->session) && priv->client && + if (!list_empty(&priv->session) && + READ_ONCE(priv->client) == priv->session_client && xenbus_read_unsigned(dev->otherend, "feature-session-transfer", 0)) { list_for_each_entry(chunk, &priv->session, list) { + if (READ_ONCE(priv->client) != priv->session_client) + break; len = P9_HDRSZ + 8 + chunk->len; req = kvmalloc(len, GFP_KERNEL); if (!req) { @@ -1012,7 +1026,7 @@ static void xen_9pfs_front_changed(struct xenbus_device *dev, put_unaligned_le32(len, req); req[4] = P9_XEN_TSESSION_RESTORE; put_unaligned_le16(P9_NOTAG, req + 5); - put_unaligned_le32(priv->client->msize, + put_unaligned_le32(priv->session_msize, req + P9_HDRSZ); put_unaligned_le32(flags, req + P9_HDRSZ + 4); memcpy(req + P9_HDRSZ + 8, chunk->data, From c251fe6300793f0c14f6a1f49b086201631fa583 Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Fri, 13 Nov 2026 13:55:11 -0800 Subject: [PATCH 3/5] 9p/xen: keep a saved session across a suspend that has nothing to save xen_9pfs_front_suspend() freed the session saved by the previous suspend first thing, before checking whether the frontend is connected. A guest suspended again before the backend had reconnected it, which is what happens when a restore is retried or a saved domain is forked more than once, thus lost the fids it was carrying, and the eventual restore reset the session instead of replaying them. Only free the old session once there is a connected client whose fids replace it. A frontend the backend has not reconnected since the last suspend has nothing newer to save, and the Connected handler frees the session once it has been replayed, so what is kept is never stale. Signed-off-by: Ariadne Conill (cherry picked from commit 373e994daa5e55335e3d57d136196a2b98f14892) --- net/9p/trans_xen.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index e4155cbe147904..0af047cfbf6c53 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -625,14 +625,17 @@ static int xen_9pfs_front_suspend(struct xenbus_device *dev) if (!priv) return 0; - xen_9pfs_front_session_free(priv); if (!xenbus_read_unsigned(dev->otherend, "feature-session-transfer", 0)) return 0; down_read(&priv->rings_sem); client = priv->client; + /* A frontend the backend has not reconnected since the last suspend + * has nothing newer to save: keep what that suspend saved. + */ if (!READ_ONCE(priv->connected) || !client) goto out; + xen_9pfs_front_session_free(priv); priv->session_client = client; priv->session_msize = client->msize; From aeab5323e9e8cc88f9e24e46182592a349c6a684 Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Fri, 13 Nov 2026 13:55:29 -0800 Subject: [PATCH 4/5] 9p/xen: end foreign access on the grant refs we granted, not the intf page's xen_9pfs_front_free_rings() took the data pages' ring_order and grant refs from the intf page, which is shared with the backend and writable by it. Upstream only freed rings from remove(), with the backend gone; since the frontend reconnects after a restore it also frees them from xen_9pfs_front_disconnect() on every resume, while a backend that still serves the old rings may be running. A backend that rewrote ref[] had the frontend end foreign access on grant refs that belong to other devices, and one that rewrote ring_order had it walk past the array or free the wrong number of pages. Keep the refs in the ring as they are granted and use those, with the order the frontend chose, to tear the ring down. Signed-off-by: Ariadne Conill (cherry picked from commit 88cb2b50c54ff61f5e55a6580d6b6583a5a0f78e) --- net/9p/trans_xen.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index 0af047cfbf6c53..f5e35eb8ea3715 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -61,6 +61,10 @@ struct xen_9pfs_dataring { struct xen_9pfs_data_intf *intf; grant_ref_t ref; + /* The data pages' grant refs, as granted. intf->ref[] holds the + * backend's copy and the backend can rewrite it. + */ + grant_ref_t data_refs[1 << XEN_9PFS_RING_ORDER]; int evtchn; int irq; /* protect a ring from concurrent accesses */ @@ -423,16 +427,15 @@ static void xen_9pfs_front_free_rings(struct xen_9pfs_front_priv *priv) 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); - j++) { - grant_ref_t ref; - - ref = priv->rings[i].intf->ref[j]; - gnttab_end_foreign_access(ref, NULL); - } + /* Not from the intf page: a backend that is still + * running, as it is on resume, can have rewritten + * ring_order and ref[] there. + */ + for (j = 0; j < (1 << priv->ring_order); j++) + gnttab_end_foreign_access(ring->data_refs[j], + NULL); free_pages_exact(priv->rings[i].data.in, - 1UL << (priv->rings[i].intf->ring_order + + 1UL << (priv->ring_order + XEN_PAGE_SHIFT)); } gnttab_end_foreign_access(priv->rings[i].ref, NULL); @@ -734,6 +737,7 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev, virt_to_gfn(bytes + i * XEN_PAGE_SIZE), 0); if (ret < 0) goto out; + ring->data_refs[i] = ret; ring->intf->ref[i] = ret; } ring->intf->ring_order = order; @@ -754,7 +758,7 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev, out: if (bytes) { for (i--; i >= 0; i--) - gnttab_end_foreign_access(ring->intf->ref[i], NULL); + gnttab_end_foreign_access(ring->data_refs[i], NULL); free_pages_exact(bytes, 1UL << (order + XEN_PAGE_SHIFT)); } gnttab_end_foreign_access(ring->ref, NULL); From 403c8cc309a903131cf913d852a28998d2a30487 Mon Sep 17 00:00:00 2001 From: Ariadne Conill Date: Fri, 13 Nov 2026 13:55:30 -0800 Subject: [PATCH 5/5] 9p/xen: order the control reply buffer before the pending flag xen_9pfs_front_ctl() stores the reply buffer and its capacity, then sets ctl_pending with a plain WRITE_ONCE(), and the response worker tests the flag with READ_ONCE() before copying into the buffer. Nothing orders the two, so on a weakly ordered machine the worker can see ctl_pending set while ctl_reply still names the previous call's buffer, a stack array or kvmalloc()ed memory that has been freed, and copy the reply into that. Publish the buffer with smp_store_release() and test the flag with smp_load_acquire(). Signed-off-by: Ariadne Conill (cherry picked from commit 193a37988d456218f8b4adfa6556b1944f2b1f3a) --- net/9p/trans_xen.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index f5e35eb8ea3715..317936956b128b 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -321,7 +321,7 @@ static void p9_xen_response(struct work_struct *work) * ring 0; see xen_9pfs_front_ctl(). */ if (ring == priv->rings && h.tag == P9_NOTAG && - READ_ONCE(priv->ctl_pending)) { + smp_load_acquire(&priv->ctl_pending)) { if (h.size <= priv->ctl_reply_cap) { u32 size = XEN_9PFS_RING_SIZE(ring); @@ -561,7 +561,8 @@ static int xen_9pfs_front_ctl(struct xen_9pfs_front_priv *priv, priv->ctl_reply = reply; priv->ctl_reply_cap = cap; priv->ctl_reply_len = 0; - WRITE_ONCE(priv->ctl_pending, true); + /* Publish the buffer before the flag the response worker tests. */ + smp_store_release(&priv->ctl_pending, true); spin_lock_irqsave(&ring->lock, flags); cons = ring->intf->out_cons;