From a708fc683934d7d7ea7560354968fedd2be28581 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Thu, 24 Sep 2026 23:15:05 +0000 Subject: [PATCH] Handle Btrfs device mismatches when masking daemon sockets (#47968) ## Why Btrfs subvolumes can report an `st_dev` that differs from the device in `/proc/self/mountinfo`, causing daemon socket mount validation to reject valid mounts. ## What changed Allow the mismatch only when the opened directory and its uniquely identified mount are both verified as Btrfs. Discover socket directory aliases using both device identities while retaining checks that reject unsafe directory and socket bind mounts. ## Testing Add regression tests for aliases under both device numbers, unverified or inconsistent filesystems, unavailable or missing mount IDs, duplicate mount IDs, and unsafe bind mounts. GitOrigin-RevId: 921d6465b7a97022573a42412606a26560cc086a --- codex-rs/linux-sandbox/src/daemon_mounts.rs | 88 ++++++++++++--- .../linux-sandbox/src/daemon_mounts_tests.rs | 103 +++++++++++++++++- 2 files changed, 170 insertions(+), 21 deletions(-) diff --git a/codex-rs/linux-sandbox/src/daemon_mounts.rs b/codex-rs/linux-sandbox/src/daemon_mounts.rs index e0f9d66ea6..1ad026f1b5 100644 --- a/codex-rs/linux-sandbox/src/daemon_mounts.rs +++ b/codex-rs/linux-sandbox/src/daemon_mounts.rs @@ -1,8 +1,10 @@ //! Find host mount aliases that need the privileged socket directory mask. //! Mount roots describe filesystem identity; canonical paths alone miss bind mounts. +//! Btrfs subvolume device numbers can differ from the containing mount's device. use rustix::fs::AtFlags; use rustix::fs::StatxFlags; +use rustix::fs::fstatfs; use rustix::fs::statx; use std::collections::BTreeSet; use std::fs; @@ -13,12 +15,24 @@ use std::os::unix::fs::MetadataExt; use std::path::Path; use std::path::PathBuf; +#[derive(Clone, Copy, PartialEq, Eq)] +enum SocketFilesystem { + Btrfs, + Other, +} + pub(crate) fn daemon_socket_mask_paths( directory: &Path, masked_root: Option<&Path>, ) -> io::Result> { let directory_file = fs::File::open(directory)?; let device = directory_file.metadata()?.dev(); + let filesystem = + if fstatfs(&directory_file).is_ok_and(|stat| stat.f_type == libc::BTRFS_SUPER_MAGIC) { + SocketFilesystem::Btrfs + } else { + SocketFilesystem::Other + }; let mount_id = fs::read_to_string(format!("/proc/self/fdinfo/{}", directory_file.as_raw_fd())) .ok() .and_then(|fdinfo| { @@ -39,6 +53,7 @@ pub(crate) fn daemon_socket_mask_paths( check_mounts( directory, &format!("{}:{}", libc::major(device), libc::minor(device)), + filesystem, mount_id.as_deref(), &fs::read("/proc/self/mountinfo")?, masked_root, @@ -48,6 +63,7 @@ pub(crate) fn daemon_socket_mask_paths( fn check_mounts( directory: &Path, device: &str, + filesystem: SocketFilesystem, mount_id: Option<&str>, mountinfo: &[u8], masked_root: Option<&Path>, @@ -58,30 +74,66 @@ fn check_mounts( .split(|byte| *byte == b'\n') .filter(|line| !line.is_empty()) { - let fields: Vec<_> = line.split(|byte| *byte == b' ').take(5).collect(); - let [id, parent, mount_device, root, destination] = fields.as_slice() else { + let mut fields = line.split(|byte| *byte == b' '); + let prefix: Vec<_> = fields.by_ref().take(5).collect(); + let [id, parent, mount_device, root, destination] = prefix.as_slice() else { return Err(invalid()); }; let destination = mount_path(destination)?; - // Only roots on the socket filesystem can identify aliases. Other - // filesystems can use non-path roots such as nsfs `mnt:[inode]`, but - // their destinations still matter for ancestry and nested-mount checks. - let root = (*mount_device == device.as_bytes()) - .then(|| mount_path(root)) - .transpose()?; - mounts.push((*id, *parent, *mount_device, root, destination)); + let mount_filesystem = fields + .skip_while(|field| *field != b"-") + .nth(1) + .ok_or_else(invalid)?; + mounts.push(( + *id, + *parent, + *mount_device, + *root, + destination, + mount_filesystem, + )); } - let (location, containing_mount) = if let Some(mount_id) = mount_id { + let selected_index = mount_id + .map(|mount_id| { + let mut matching = mounts + .iter() + .enumerate() + .filter(|(_, (id, ..))| *id == mount_id.as_bytes()); + let (index, (_, _, mount_device, _, _, mount_filesystem)) = + matching.next().ok_or_else(invalid)?; + if matching.next().is_some() + || (*mount_device != device.as_bytes() + && (filesystem != SocketFilesystem::Btrfs || *mount_filesystem != b"btrfs")) + { + return Err(invalid()); + } + Ok(index) + }) + .transpose()?; + // Btrfs reports a per-subvolume st_dev, while mountinfo uses the superblock + // device. Only the verified descriptor's mount ID can authorize that mismatch. + // Retain both device identities when looking for aliases. + let mount_device = selected_index + .map(|index| mounts[index].2) + .unwrap_or(device.as_bytes()); + let mounts = mounts + .into_iter() + .map(|(id, parent, candidate_device, root, destination, _)| { + // Only roots on the socket filesystem can identify aliases. Other + // filesystems can use non-path roots such as nsfs `mnt:[inode]`, but + // their destinations still matter for ancestry and nested-mount checks. + let root = (candidate_device == device.as_bytes() || candidate_device == mount_device) + .then(|| mount_path(root)) + .transpose()?; + Ok((id, parent, candidate_device, root, destination)) + }) + .collect::>>()?; + let (location, containing_mount) = if let Some((mount_id, index)) = mount_id.zip(selected_index) + { // fdinfo/statx identifies the opened mount, which may have been covered // by another mount before we read mountinfo. - let selected = mounts - .iter() - .find(|(id, ..)| *id == mount_id.as_bytes()) - .ok_or_else(invalid)?; - let (_, _, mount_device, root, destination) = selected; - if *mount_device != device.as_bytes() { - return Err(invalid()); - } + let selected = &mounts[index]; + let (_, _, _, root, destination) = selected; let root = root.as_ref().ok_or_else(invalid)?; let relative = directory.strip_prefix(destination).map_err(|_| invalid())?; let mut current = Some(selected); diff --git a/codex-rs/linux-sandbox/src/daemon_mounts_tests.rs b/codex-rs/linux-sandbox/src/daemon_mounts_tests.rs index 55fb1f6a4c..c332cad2f5 100644 --- a/codex-rs/linux-sandbox/src/daemon_mounts_tests.rs +++ b/codex-rs/linux-sandbox/src/daemon_mounts_tests.rs @@ -10,10 +10,91 @@ fn check_mounts( mountinfo: &[u8], ) -> io::Result> { super::check_mounts( - directory, device, mount_id, mountinfo, /*masked_root*/ None, + directory, + device, + SocketFilesystem::Other, + mount_id, + mountinfo, + /*masked_root*/ None, ) } +fn check_btrfs_mounts(mount_id: &str, mountinfo: &[u8]) -> io::Result> { + super::check_mounts( + Path::new("/tmp/codex-daemon-1000"), + "0:2", + SocketFilesystem::Btrfs, + Some(mount_id), + mountinfo, + /*masked_root*/ None, + ) +} + +#[test] +fn btrfs_bind_mount_masks_aliases_for_both_device_numbers() { + let mounts = b"1 0 0:1 / / rw - btrfs disk rw\n\ + 2 1 0:1 /var/lib/system-tmp /tmp rw shared:1 - btrfs disk rw\n\ + 3 1 0:1 /var/lib/system-tmp /mount-device-alias rw - btrfs disk rw\n\ + 4 1 0:2 /var/lib/system-tmp /stat-device-alias rw - btrfs disk rw\n\ + 5 1 0:3 mnt:[1234] /run/example.mnt rw - nsfs nsfs rw\n"; + assert_eq!( + check_btrfs_mounts("2", mounts).unwrap(), + BTreeSet::from([ + PathBuf::from("/tmp/codex-daemon-1000"), + PathBuf::from("/var/lib/system-tmp/codex-daemon-1000"), + PathBuf::from("/mount-device-alias/codex-daemon-1000"), + PathBuf::from("/stat-device-alias/codex-daemon-1000"), + ]), + ); +} + +#[test_case(SocketFilesystem::Other, "btrfs", Some("1"); "unverified descriptor filesystem")] +#[test_case(SocketFilesystem::Btrfs, "ext4", Some("1"); "inconsistent mount filesystem")] +#[test_case(SocketFilesystem::Btrfs, "btrfs", None; "unavailable mount id")] +#[test_case(SocketFilesystem::Btrfs, "btrfs", Some("missing"); "missing mount id")] +fn device_mismatch_requires_verified_btrfs_mount( + filesystem: SocketFilesystem, + mount_filesystem: &str, + mount_id: Option<&str>, +) { + let mounts = format!("1 0 0:1 / / rw - {mount_filesystem} disk rw\n"); + assert_eq!( + super::check_mounts( + Path::new("/tmp/codex-daemon-1000"), + "0:2", + filesystem, + mount_id, + mounts.as_bytes(), + /*masked_root*/ None, + ) + .map_err(|error| error.kind()), + Err(io::ErrorKind::Other), + ); +} + +#[test_case("0:1", "/@/tmp/codex-daemon-1000", "/alias"; "mount device directory alias")] +#[test_case("0:2", "/@/tmp/codex-daemon-1000/rpc.sock", "/alias.sock"; "stat device socket alias")] +fn btrfs_device_mismatch_still_rejects_unsafe_mounts(device: &str, root: &str, destination: &str) { + let mounts = format!( + "1 0 0:1 /@ / rw - btrfs disk rw\n\ + 2 1 {device} {root} {destination} rw - btrfs disk rw\n" + ); + assert_eq!( + check_btrfs_mounts("1", mounts.as_bytes()).map_err(|error| error.kind()), + Err(io::ErrorKind::PermissionDenied), + ); +} + +#[test] +fn btrfs_device_mismatch_rejects_duplicate_mount_id() { + let mounts = b"1 0 0:1 /@ / rw - btrfs disk rw\n\ + 1 0 0:1 /@ / rw - btrfs disk rw\n"; + assert_eq!( + check_btrfs_mounts("1", mounts).map_err(|error| error.kind()), + Err(io::ErrorKind::Other), + ); +} + #[test_case("/tmp", "/host-tmp", true; "ancestor alias")] #[test_case("/tmp/codex-daemon-1000", "/alias", false; "directory alias")] #[test_case("/tmp/codex-daemon-1000/rpc.sock", "/alias.sock", false; "socket alias")] @@ -213,11 +294,27 @@ fn masked_wslg_alias_does_not_skip_other_socket_masks() { ]) ); assert_eq!( - super::check_mounts(directory, "0:1", mount_id, mounts.as_bytes(), mask).unwrap(), + super::check_mounts( + directory, + "0:1", + SocketFilesystem::Other, + mount_id, + mounts.as_bytes(), + mask + ) + .unwrap(), BTreeSet::from([directory.to_path_buf()]) ); assert_eq!( - super::check_mounts(directory, "0:1", mount_id, exposed.as_bytes(), mask).unwrap(), + super::check_mounts( + directory, + "0:1", + SocketFilesystem::Other, + mount_id, + exposed.as_bytes(), + mask + ) + .unwrap(), BTreeSet::from([ directory.to_path_buf(), PathBuf::from("/host-tmp/codex-daemon-1000"),