p9fs: hold an O_PATH fd per fid in File::Walk#40542
Draft
benhillis wants to merge 1 commit into
Draft
Conversation
File::Walk previously validated a child entry via fstatat by path and then re-resolved the same path for every subsequent operation on the fid. A guest could swap the validated entry with a symlink between the validation and the use to redirect later access. Hold a wil::unique_fd (O_PATH | O_NOFOLLOW) per fid that pins the validated inode. Walk opens the child via openat against the parent's held fd, stats it, runs the mount-boundary check, and only then commits the new fd. Stat/GetAttr fstatat through the held fd with AT_EMPTY_PATH. Open/OpenFile refuse symlink qids and Reopen through /proc/self/fd/N with O_NOFOLLOW stripped (the O_NOFOLLOW branch of util::Reopen is path- based and would defeat the fix). Create issues openat with O_NOFOLLOW against the parent's held fd and dups the result to populate the new fid's path fd. The copy constructor dups m_PathFd and also propagates m_Device. Path-based child ops (chmod/chown/utimensat/unlinkat/rename/mkdir/mknod/ symlink/link/readlink/xattr/access) are intentionally left untouched - same trust model. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Linux Plan9 (p9fs) server against TOCTOU path replacement by pinning each fid’s validated inode using an O_PATH | O_NOFOLLOW file descriptor, and then performing self-targeted operations (stat/open/reopen) via that held fd instead of re-resolving a stored path string.
Changes:
- Add per-fid
m_PathFdand aPathFd()helper to resolve “self” operations through the pinned inode. - Update
Walk/Stat/GetAttrto useopenat(...O_PATH|O_NOFOLLOW...)+fstatat(...AT_EMPTY_PATH...)for inode-stable behavior. - Update
Open/OpenFile/Createto reopen/create relative to the held fd and avoid symlink-following on the reopen path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/linux/plan9/p9file.h | Adds m_PathFd and PathFd() API to support inode-pinned fid operations. |
| src/linux/plan9/p9file.cpp | Switches stat/open/walk/create flows to operate via held fds to prevent path re-resolution TOCTOU. |
| // Returns the fd backing this fid for self-targeted ops (fstatat with | ||
| // AT_EMPTY_PATH, /proc/self/fd reopens, openat using this fid as dirfd). | ||
| // Non-root fids return the O_PATH fd pinned by Walk/Create; the root fid | ||
| // returns m_Root->RootFd. Caller must hold m_Lock. |
Comment on lines
+558
to
+570
| // Dup the new fd to use as the path fd, pinning the created inode. | ||
| int dupedFd = fcntl(file.get(), F_DUPFD_CLOEXEC, 3); | ||
| if (dupedFd < 0) | ||
| { | ||
| return LxError{-errno}; | ||
| } | ||
| wil::unique_fd newPathFd{dupedFd}; | ||
|
|
||
| auto newFileName = ChildPathWithLockHeld(name); | ||
| m_FileName = std::move(newFileName); | ||
| m_Io = CoroutineIoIssuer(file->get()); | ||
| m_File = std::move(file.Get()); | ||
| m_Io = CoroutineIoIssuer(file.get()); | ||
| m_File = std::move(file); | ||
| m_PathFd = std::move(newPathFd); |
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.
The Plan9 server's
File::Walkstored each fid's location as a path stringand revalidated it by path on every subsequent operation
(
fstatat(RootFd, m_FileName, ...), etc.). Between the Walk-time validationand any later use, the named entry on disk could be replaced (e.g. swapped
for a symlink), and subsequent ops would re-resolve the path against the new
target.
Pin the validated inode per fid with an
O_PATH | O_NOFOLLOWfd:wil::unique_fd m_PathFdand aPathFd()helper that falls back tom_Root->RootFdfor the share-root fid.Walkopens the child viaopenat(parent_PathFd, leaf, O_PATH | O_NOFOLLOW | O_CLOEXEC),stats it with
AT_EMPTY_PATH, runs the existing mount-boundary check,then commits
m_PathFd.Stat/GetAttrfstatatthrough the held fd.Open/OpenFilereject symlink qids withELOOPandReopenvia/proc/self/fd/N(O_NOFOLLOWstripped becauseutil::Reopen'sO_NOFOLLOWbranch is path-based and would defeat the fix).Createissuesopenatagainst the parent's held fd withO_NOFOLLOWand dups the resulting fd to populate the new fid's
m_PathFd.m_PathFdand propagatesm_Device.The mount-boundary check (drvfs/9p/virtiofs) is preserved. Path-based child
ops (chmod, chown, utimensat, unlinkat, rename, mkdir, mknod, symlink, link,
readlink, xattr, access) are intentionally left untouched.
Test
Plan9Tests::*pass.