Repository navigation
Conversation
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 <ariadne@ariadne.space>
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 <ariadne@ariadne.space>
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 <ariadne@ariadne.space>
… 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 <ariadne@ariadne.space>
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 <ariadne@ariadne.space>
kaniini
requested review from
antoineco,
azenla,
bleggett,
sveith and
tycho
as code owners
November 13, 2026 21:56
bleggett
approved these changes
Nov 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five fixes to the Edera session-transfer extension of the 9P Xen transport, from the Mythos audit.
Rlerror ecode taken for a reply length (ENG-218).
xen_9pfs_front_ctl()answered a backendRlerrorwith-(int)ecode. An ecode of 0 came back as 0 and anything aboveINT_MAXcame back positive, and callers read that as "reply of this length".xen_9pfs_front_suspend()then readtotalandnextfrom beyond the short error reply and sized a session chunk from the bogus length: one ecode copies about 1 MiB of uninitialised buffer into a chunk that is later sent to the backend, another hasmemcpy()read some 2 GiB past the reply buffer. The function now takes only1..MAX_ERRNOfrom anRlerrorand answers anything else with-EPROTO, so it can only return a negative errno or a reply with the id and body it asked for.priv->clientdereferenced across the restore loop (ENG-219). The Connected handler replays saved fids on xenwatch after processes are thawed, readingpriv->client->msizeper chunk with each chunk waiting up to 5 s on the backend. A killed umount or mount in that window runsp9_xen_close(), which clearspriv->clientand frees the client, and the next chunk dereferences NULL or freed memory. The owning client and its msize are now recorded at suspend, with userspace frozen; the replay uses those and only comparespriv->clientby address, before starting and per chunk, giving up if the owner is gone.Lower-severity items from the ENG-218 notes, each its own commit:
xen_9pfs_front_free_rings()tookring_orderand the data grant refs from the intf page, which the backend can rewrite, and the restore path calls it on every resume while the old backend may still run. The refs are now kept in the ring as granted and the frontend's own order is used.WRITE_ONCE()ofctl_pending; the response worker could see the flag set with the previous call's buffer. Nowsmp_store_release()/smp_load_acquire().Built with the latest
linux-kernel-ocix86_64zone.config(NET_9P_XEN=y): full vmlinux, no warnings;sparseon the transport is clean. Not runtime-tested; the save/restore paths need a session-transfer backend and a suspend/resume cycle.Not changed:
XEN_9PFS_RING_SIZE()still readsring_orderfrom the shared intf page on every ring access, as upstream does. That is an upstream matter and a candidate for a separate report.ENG-218
ENG-219