From 6e5c68a3830b636eb015e507e83ea97a27bf8a06 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sat, 31 Oct 2026 10:12:42 -0700 Subject: [PATCH 1/8] network: resolve chroot named unix sockets on-behalf Chroot mode never moves the child's real root or cwd, yet the named unix gate checked the raw sun_path lexically and returned Continue, leaving the kernel to resolve it on the host. An absolute virtual path therefore reached the host's socket of that name (an escape where Landlock cannot gate unix connect), and a relative one was checked as if anchored at / but resolved against the cwd the child was exec'ed in, so ./svc.sock from a granted cwd was refused while tmp/x.sock from an ungranted one passed. Serve chroot connect/sendto/sendmsg/sendmmsg on-behalf like non-chroot: build the virtual path against the tracked virtual cwd, pin the inode inside the root (mount aware), and check write access on where it really sits. The trap now stays on under chroot even with Landlock ABI v9. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 33 ++++++ crates/sandlock-core/src/network/connect.rs | 27 +---- crates/sandlock-core/src/network/mod.rs | 13 +-- crates/sandlock-core/src/network/send.rs | 53 ++------- crates/sandlock-core/src/network/unix.rs | 83 ++++++-------- crates/sandlock-core/src/network/verdict.rs | 10 -- crates/sandlock-core/src/seccomp/notif.rs | 6 +- crates/sandlock-core/src/seccomp_plan.rs | 5 +- .../tests/integration/test_chroot.rs | 107 ++++++++++++++++++ crates/sandlock-fixtures/rootfs-helper.c | 33 ++++++ 10 files changed, 235 insertions(+), 135 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 40af237e..ac970d7b 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -475,6 +475,39 @@ fn resolve_chroot_path_existing( resolve_existing_in_root(ctx.root, &full_path) } +/// Pin the socket inode a named unix address reaches in the sandbox's view +/// and require write access to it, as the kernel's connect does. +/// +/// The child's real root and cwd are the host's, so the kernel cannot be left +/// to resolve the address: the grant is checked on where the pinned inode +/// actually sits, after symlinks inside the root are followed. +pub(crate) fn pin_writable_unix_target( + notif: &SeccompNotif, + ctx: &ChrootCtx<'_>, + sun_path: &Path, +) -> Result { + let vpath = build_virtual_path( + notif, + libc::AT_FDCWD as i64, + sun_path.to_str().ok_or(libc::ECONNREFUSED)?, + ctx, + ) + .ok_or(libc::ECONNREFUSED)?; + let confined = confine(&vpath); + let (root, sub) = match ctx.mount_target(&confined) { + Some((mount, sub)) => (mount, sub), + None => (ctx.root, confined.to_string_lossy().into_owned()), + }; + let fd = openat2_in_root(root, &sub, libc::O_PATH | libc::O_CLOEXEC, 0) + .map_err(|_| libc::ECONNREFUSED)?; + let pinned = unsafe { OwnedFd::from_raw_fd(fd) }; + let host = std::fs::read_link(format!("/proc/self/fd/{}", fd)).map_err(|_| libc::EACCES)?; + match ctx.host_to_virtual(&host) { + Some(virtual_path) if ctx.can_write(&virtual_path) => Ok(pinned), + _ => Err(libc::EACCES), + } +} + /// Convert a Path to CString, returning Errno on failure. fn path_cstr(path: &Path, err: i32) -> Result { CString::new(path.to_str().unwrap_or("")).map_err(|_| NotifAction::Errno(err)) diff --git a/crates/sandlock-core/src/network/connect.rs b/crates/sandlock-core/src/network/connect.rs index 2dfad96d..f8110fb0 100644 --- a/crates/sandlock-core/src/network/connect.rs +++ b/crates/sandlock-core/src/network/connect.rs @@ -17,7 +17,7 @@ use super::materialize::{ }; use super::query_socket_protocol; use super::unix::connect_named_unix_on_behalf; -use super::verdict::{classify_destination, path_under_any, Destination}; +use super::verdict::{classify_destination, Destination}; // ============================================================ // connect_on_behalf — perform connect() on behalf of the child (TOCTOU-safe) @@ -126,28 +126,11 @@ pub(super) async fn connect_on_behalf( // never return Continue on the deny path, so it is TOCTOU-safe. // Abstract sockets (no path) are handled by the Landlock abstract scope. match named_unix_socket_path(&addr_bytes) { + // Resolve the symlink-followed target and connect on-behalf to + // the pinned inode, so a symlink inside a granted dir cannot + // redirect to an ungranted socket. Some(path) if ctx.policy.supervises_unix_connect() => { - if ctx.policy.chroot_root.is_some() { - // Chroot mode: the child's paths are virtual, so a lexical - // check against the (virtual) write grants is consistent, - // and host socket paths are absent from the chroot view - // anyway. Deny unless under a write grant. - if path_under_any(&path, &ctx.policy.chroot_writable) { - NotifAction::Continue - } else { - NotifAction::Errno(libc::EACCES) - } - } else { - // Non-chroot: resolve the symlink-followed real target and - // connect on-behalf to the pinned inode, so a symlink inside - // a granted dir cannot redirect to an ungranted socket. - connect_named_unix_on_behalf( - notif.pid, - sockfd, - &path, - &ctx.policy.chroot_writable, - ) - } + connect_named_unix_on_behalf(notif, ctx, sockfd, &path) } // Abstract/unnamed socket, non-AF_UNIX family, or gate disabled. _ => NotifAction::Continue, diff --git a/crates/sandlock-core/src/network/mod.rs b/crates/sandlock-core/src/network/mod.rs index 56604440..be821724 100644 --- a/crates/sandlock-core/src/network/mod.rs +++ b/crates/sandlock-core/src/network/mod.rs @@ -154,19 +154,18 @@ fn socket_is_unix(fd: RawFd) -> bool { /// 1. Non-IP families (AF_UNIX etc.): the IP policy doesn't apply; /// Landlock IPC scoping is the enforcement boundary. One sub-case is /// narrower: `sendto`'s non-IP fall-through does NOT Continue once a -/// destination policy is active. That fall-through has no chroot check — -/// it is whatever misses the named-path gate, i.e. a pathless AF_UNIX or a -/// non-AF_UNIX destination — so it fails closed under `--chroot` too. +/// destination policy is active. That fall-through is whatever misses the +/// named-path gate, i.e. a pathless AF_UNIX or a non-AF_UNIX destination, +/// so it fails closed under `--chroot` too. /// There the send goes on-behalf on a /// pinned fd — a named unix target resolved in the child's root view, or a /// non-unix destination on a unix socket (the NETLINK_ROUTE /// virtualization) — or fails closed with EAFNOSUPPORT for an AF_UNIX /// address with no pathname to pin, notably an abstract one: the /// supervisor has no Landlock domain, so an on-behalf abstract send would -/// escape the child's scope. The other non-IP arms still Continue under a -/// destination policy — `connect`'s abstract/unnamed/non-AF_UNIX arm and -/// both chroot named-unix arms (`connect`, `sendto`/`sendmsg`), which is -/// the residue tracked by issues #27 / #143. +/// escape the child's scope. `connect`'s abstract/unnamed/non-AF_UNIX +/// arm still Continues under a destination policy, which is the residue +/// tracked by issues #27 / #143. /// 2. Connected sockets with addr_ptr == 0 — the address was already /// validated at connect time, so the kernel re-read of (nothing) is /// moot. diff --git a/crates/sandlock-core/src/network/send.rs b/crates/sandlock-core/src/network/send.rs index 312c738e..59d5b133 100644 --- a/crates/sandlock-core/src/network/send.rs +++ b/crates/sandlock-core/src/network/send.rs @@ -20,9 +20,7 @@ use super::unix::{ mmsg_entry_named_unix_path, sendmmsg_named_unix_on_behalf, sendto_named_unix_on_behalf, sendto_pinned_unix_on_behalf, unix_sendmsg_gate, }; -use super::verdict::{ - check_ip_destination, classify_send_path, path_under_any, DestShape, Destination, SendPath, -}; +use super::verdict::{check_ip_destination, classify_send_path, DestShape, Destination, SendPath}; use super::{query_socket_protocol, socket_is_unix, Protocol}; // ============================================================ @@ -110,26 +108,9 @@ pub(super) async fn sendto_on_behalf( // sendto to a named socket is a WRITE on its inode, so deny unless the // resolved real target is under an fs-write grant. match named_unix_socket_path(&addr_bytes) { - Some(path) if ctx.policy.supervises_unix_send() => { - if ctx.policy.chroot_root.is_some() { - if path_under_any(&path, &ctx.policy.chroot_writable) { - NotifAction::Continue - } else { - NotifAction::Errno(libc::EACCES) - } - } else { - sendto_named_unix_on_behalf( - notif, - notif_fd, - sockfd, - buf_ptr, - buf_len, - flags, - &path, - &ctx.policy.chroot_writable, - ) - } - } + Some(path) if ctx.policy.supervises_unix_send() => sendto_named_unix_on_behalf( + notif, ctx, notif_fd, sockfd, buf_ptr, buf_len, flags, &path, + ), _ => { // Non-IP destination with no fs-path gate: an abstract unix // address, a named unix address with the fs-gate off, a non-unix @@ -475,29 +456,9 @@ pub(super) async fn sendmmsg_on_behalf( } } if named_unix { - if ctx.policy.chroot_root.is_some() { - // Chroot: lexical check; deny the whole call if any named-unix - // entry is outside the (virtual) write grants. - for i in 0..vlen { - let entry_ptr = mmsg_entry_ptr(msgvec_ptr, i); - if let Some(path) = mmsg_entry_named_unix_path(notif, notif_fd, entry_ptr) { - if !path_under_any(&path, &ctx.policy.chroot_writable) { - return NotifAction::Errno(libc::EACCES); - } - } - } - // All granted: fall through to the existing path. - } else { - return sendmmsg_named_unix_on_behalf( - notif, - notif_fd, - sockfd, - msgvec_ptr, - vlen, - flags, - &ctx.policy.chroot_writable, - ); - } + return sendmmsg_named_unix_on_behalf( + notif, ctx, notif_fd, sockfd, msgvec_ptr, vlen, flags, + ); } } diff --git a/crates/sandlock-core/src/network/unix.rs b/crates/sandlock-core/src/network/unix.rs index ed8ea359..b645c1c6 100644 --- a/crates/sandlock-core/src/network/unix.rs +++ b/crates/sandlock-core/src/network/unix.rs @@ -18,7 +18,7 @@ use super::materialize::{ MaterializedMsg, }; use super::send_engine::{prepare_batch, prepare_send, wants_blocking}; -use super::verdict::{path_under_any, real_path_under_any}; +use super::verdict::real_path_under_any; /// Address `sun_path` in child's context. Absolute paths use `/proc//root`, /// relative paths use `/proc//cwd`. @@ -34,7 +34,7 @@ fn child_lookup_path(child_pid: u32, sun_path: &std::path::Path) -> String { } /// Pin the inode a named unix socket `sun_path` resolves to **in the child's -/// root view** (see [`child_root_path`]), following the child's symlinks rather +/// root view** (see [`child_lookup_path`]), following the child's symlinks rather /// than the supervisor's. Returns an `O_PATH` fd to that exact inode; callers /// address it through `/proc/self/fd` so the resolved inode is the one acted on, /// immune to a path swap after the check (TOCTOU- and symlink-safe). @@ -55,20 +55,25 @@ fn pin_child_unix_target( } /// Resolve a named unix socket `sun_path` to its real, symlink-followed inode -/// in the child's root view (see [`pin_child_unix_target`]) and verify that -/// inode is under an fs-write grant. On success returns the pinned `O_PATH` fd; -/// on failure returns the deny/refuse `NotifAction`. +/// in the child's view and verify that inode is under an fs-write grant. On +/// success returns the pinned `O_PATH` fd; on failure returns the deny/refuse +/// `NotifAction`. fn resolve_named_unix_target( - child_pid: u32, + notif: &SeccompNotif, + ctx: &SupervisorCtx, sun_path: &std::path::Path, - writable: &[crate::grant::Grant], ) -> Result { - let pinned = pin_child_unix_target(child_pid, sun_path)?; + if ctx.policy.chroot_root.is_some() { + let chroot = crate::chroot::dispatch::ChrootCtx::new(&ctx.policy, &ctx.processes); + return crate::chroot::dispatch::pin_writable_unix_target(notif, &chroot, sun_path) + .map_err(NotifAction::Errno); + } + let pinned = pin_child_unix_target(notif.pid, sun_path)?; // Canonical path of the pinned inode in our mount namespace. let real = std::fs::read_link(format!("/proc/self/fd/{}", pinned.as_raw_fd())) .map_err(|_| NotifAction::Errno(libc::EACCES))?; - if real_path_under_any(&real, writable) { + if real_path_under_any(&real, &ctx.policy.chroot_writable) { Ok(pinned) } else { Err(NotifAction::Errno(libc::EACCES)) @@ -104,16 +109,17 @@ fn sockaddr_un_bytes(sun: &libc::sockaddr_un, len: libc::socklen_t) -> Vec { .to_vec() } -/// On-behalf `connect()` for a NAMED `AF_UNIX` socket in non-chroot mode: -/// resolve+verify the target, then connect the child's socket to the pinned -/// inode through `/proc/self/fd`. +/// On-behalf `connect()` for a NAMED `AF_UNIX` socket: resolve+verify the +/// target, then connect the child's socket to the pinned inode through +/// `/proc/self/fd`. pub(super) fn connect_named_unix_on_behalf( - child_pid: u32, + notif: &SeccompNotif, + ctx: &SupervisorCtx, sockfd: i32, sun_path: &std::path::Path, - writable: &[crate::grant::Grant], ) -> NotifAction { - let pinned = match resolve_named_unix_target(child_pid, sun_path, writable) { + let child_pid = notif.pid; + let pinned = match resolve_named_unix_target(notif, ctx, sun_path) { Ok(fd) => fd, Err(action) => return action, }; @@ -143,20 +149,20 @@ pub(super) fn connect_named_unix_on_behalf( }) } -/// On-behalf `sendto()` for a NAMED `AF_UNIX` datagram in non-chroot mode: -/// resolve+verify the target, copy the child's data, then send to the pinned -/// inode through `/proc/self/fd`. +/// On-behalf `sendto()` for a NAMED `AF_UNIX` datagram: resolve+verify the +/// target, copy the child's data, then send to the pinned inode through +/// `/proc/self/fd`. pub(super) fn sendto_named_unix_on_behalf( notif: &SeccompNotif, + ctx: &SupervisorCtx, notif_fd: RawFd, sockfd: i32, buf_ptr: u64, buf_len: usize, flags: i32, sun_path: &std::path::Path, - writable: &[crate::grant::Grant], ) -> NotifAction { - let pinned = match resolve_named_unix_target(notif.pid, sun_path, writable) { + let pinned = match resolve_named_unix_target(notif, ctx, sun_path) { Ok(fd) => fd, Err(action) => return action, }; @@ -274,38 +280,24 @@ pub(super) fn unix_sendmsg_gate( .ok()?; // None unless this is a NAMED AF_UNIX target; IP/abstract fall through. let path = named_unix_socket_path(&addr_bytes)?; - - if ctx.policy.chroot_root.is_some() { - return Some(if path_under_any(&path, &ctx.policy.chroot_writable) { - NotifAction::Continue - } else { - NotifAction::Errno(libc::EACCES) - }); - } Some(sendmsg_named_unix_on_behalf( - notif, - notif_fd, - sockfd, - msghdr_ptr, - flags, - &path, - &ctx.policy.chroot_writable, + notif, ctx, notif_fd, sockfd, msghdr_ptr, flags, &path, )) } -/// On-behalf `sendmsg()` for a NAMED `AF_UNIX` datagram in non-chroot mode: -/// resolve+verify the target, copy the message's iovecs and control data, then -/// send to the pinned inode through `/proc/self/fd`. +/// On-behalf `sendmsg()` for a NAMED `AF_UNIX` datagram: resolve+verify the +/// target, copy the message's iovecs and control data, then send to the pinned +/// inode through `/proc/self/fd`. fn sendmsg_named_unix_on_behalf( notif: &SeccompNotif, + ctx: &SupervisorCtx, notif_fd: RawFd, sockfd: i32, msghdr_ptr: u64, flags: i32, sun_path: &std::path::Path, - writable: &[crate::grant::Grant], ) -> NotifAction { - match send_named_unix_msghdr(notif, notif_fd, sockfd, msghdr_ptr, sun_path, writable) { + match send_named_unix_msghdr(notif, ctx, notif_fd, sockfd, msghdr_ptr, sun_path) { Ok((dup_fd, m)) => { let blocking = wants_blocking(dup_fd.as_raw_fd(), flags); prepare_send(dup_fd, m, flags, blocking) @@ -322,13 +314,13 @@ fn sendmsg_named_unix_on_behalf( /// the per-entry `sendmmsg` path. fn send_named_unix_msghdr( notif: &SeccompNotif, + ctx: &SupervisorCtx, notif_fd: RawFd, sockfd: i32, msghdr_ptr: u64, sun_path: &std::path::Path, - writable: &[crate::grant::Grant], ) -> Result<(OwnedFd, MaterializedMsg), i32> { - let pinned = match resolve_named_unix_target(notif.pid, sun_path, writable) { + let pinned = match resolve_named_unix_target(notif, ctx, sun_path) { Ok(fd) => fd, Err(NotifAction::Errno(e)) => return Err(e), Err(_) => return Err(libc::EACCES), @@ -374,20 +366,19 @@ pub(super) fn mmsg_entry_named_unix_path( named_unix_socket_path(&addr_bytes) } -/// On-behalf `sendmmsg` for a batch containing NAMED `AF_UNIX` entries -/// (non-chroot). Each named-unix entry is resolved, verified, and sent to its +/// On-behalf `sendmmsg` for a batch containing NAMED `AF_UNIX` entries. Each named-unix entry is resolved, verified, and sent to its /// pinned inode; the loop stops at the first entry it cannot gate on-behalf /// (connected/abstract) or that is denied, returning the count sent so far /// (standard short-`sendmmsg` semantics). Never returns `Continue`, so a unix /// entry cannot ride out via the binary whole-call passthrough. pub(super) fn sendmmsg_named_unix_on_behalf( notif: &SeccompNotif, + ctx: &SupervisorCtx, notif_fd: RawFd, sockfd: i32, msgvec_ptr: u64, vlen: usize, flags: i32, - writable: &[crate::grant::Grant], ) -> NotifAction { let mut entries = Vec::new(); let mut batch_bytes = 0usize; @@ -401,7 +392,7 @@ pub(super) fn sendmmsg_named_unix_on_behalf( None => break, }; let (dup_fd, m) = - match send_named_unix_msghdr(notif, notif_fd, sockfd, entry_ptr, &path, writable) { + match send_named_unix_msghdr(notif, ctx, notif_fd, sockfd, entry_ptr, &path) { Ok(pair) => pair, Err(errno) => { first_errno = Some(errno); diff --git a/crates/sandlock-core/src/network/verdict.rs b/crates/sandlock-core/src/network/verdict.rs index 9175b0c4..102339ac 100644 --- a/crates/sandlock-core/src/network/verdict.rs +++ b/crates/sandlock-core/src/network/verdict.rs @@ -140,16 +140,6 @@ pub(crate) fn real_path_under_any(real: &std::path::Path, grants: &[crate::grant }) } -/// True if `path`, lexically normalized (`.`/`..` resolved without touching the -/// filesystem), is at or under any of the granted `prefixes`. Mirrors the -/// prefix matching the chroot fs enforcement uses. -pub(crate) fn path_under_any(path: &std::path::Path, grants: &[crate::grant::Grant]) -> bool { - let norm = crate::chroot::resolve::confine(&path.to_string_lossy()); - grants - .iter() - .any(|grant| grant.covers(std::path::Path::new(&norm))) -} - /// The shape of a non-IP destination sockaddr, keyed on its ADDRESS FAMILY /// first. Produced by `materialize::classify_dest_shape` from the copied /// address bytes. diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index a0c604ee..c88250d8 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -1366,15 +1366,17 @@ pub struct NotifPolicy { impl NotifPolicy { /// Whether a named unix `connect()` is decided by the supervisor. It then /// connects on-behalf, so the peer's `SO_PEERCRED` names the supervisor. + /// Under chroot the kernel would resolve the address against the host's + /// root and the child's frozen cwd, so Landlock cannot take it over. pub(crate) fn supervises_unix_connect(&self) -> bool { - self.has_unix_fs_gate && !self.landlock_unix_gate + self.has_unix_fs_gate && (!self.landlock_unix_gate || self.chroot_root.is_some()) } /// Whether a named unix send is decided by the supervisor. A destination /// policy keeps sends on-behalf even under Landlock: a `Continue` would let /// a racing `dup2` move the send onto an IP socket. pub(crate) fn supervises_unix_send(&self) -> bool { - self.has_unix_fs_gate && (!self.landlock_unix_gate || self.has_net_destination_policy) + self.supervises_unix_connect() || (self.has_unix_fs_gate && self.has_net_destination_policy) } /// Whether an open may land on something the supervisor answers for diff --git a/crates/sandlock-core/src/seccomp_plan.rs b/crates/sandlock-core/src/seccomp_plan.rs index f1c37d9e..20465940 100644 --- a/crates/sandlock-core/src/seccomp_plan.rs +++ b/crates/sandlock-core/src/seccomp_plan.rs @@ -393,11 +393,12 @@ pub(crate) fn notif_syscalls_resolved(resolved: &ResolvedSandbox) -> Vec { if features.network_supervision { nrs.extend(NETWORK_POLICY_SYSCALLS); - } else if features.unix_fs_gate && !features.landlock_unix_gate { + } else if features.unix_fs_gate && (!features.landlock_unix_gate || features.chroot) { // Named-unix gate: trap connect() (stream) and sendto()/sendmsg()/ // sendmmsg() (datagram) so reaching a unix socket outside the fs-write // grants is denied, even when no IP network rules are present. Landlock - // before ABI v9 cannot gate this. Handlers bail cheaply on addr-less + // before ABI v9 cannot gate this, and under chroot no Landlock can, + // since the kernel would resolve the address on the host. Handlers bail cheaply on addr-less // (connected) sends. nrs.push(libc::SYS_connect); nrs.push(libc::SYS_sendto); diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index 2486510e..3d7d08d8 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -2646,3 +2646,110 @@ async fn test_chroot_shebang_loop_is_eloop() { ); cleanup_rootfs(&rootfs); } + +/// Run `rootfs-helper unix-send` inside a chroot granting write on virtual +/// `/tmp`, returning the child's result. +async fn chroot_unix_send( + rootfs: &PathBuf, + kind: &str, + dir: &str, + path: &str, + payload: &str, +) -> sandlock_core::RunResult { + minimal_exec_policy(rootfs) + .fs_write("/tmp") + .build() + .unwrap() + .run(&["rootfs-helper", "unix-send", kind, dir, path, payload]) + .await + .unwrap() +} + +/// Named unix sockets are virtual paths in chroot mode: an absolute target +/// reaches the socket inside the rootfs, and a relative one resolves against +/// the child's virtual cwd. +#[tokio::test] +async fn test_chroot_unix_connect_resolves_in_root_and_cwd() { + use std::io::Read; + use std::os::unix::net::UnixListener; + + let rootfs = build_test_rootfs("unix-connect"); + let listener = UnixListener::bind(rootfs.join("tmp/svc.sock")).unwrap(); + listener.set_nonblocking(true).unwrap(); + + for (dir, path) in [ + ("/", "/tmp/svc.sock"), + ("/tmp", "svc.sock"), + ("/tmp", "./svc.sock"), + ] { + let r = chroot_unix_send(&rootfs, "stream", dir, path, "hello").await; + assert!( + r.success(), + "connect {path} from {dir}: {}", + r.stderr_str().unwrap_or("") + ); + let (mut conn, _) = listener + .accept() + .expect("connect must reach the rootfs socket"); + let mut buf = [0u8; 5]; + conn.read_exact(&mut buf).unwrap(); + assert_eq!(&buf, b"hello"); + } + + cleanup_rootfs(&rootfs); +} + +/// A relative datagram target resolves against the virtual cwd too. +#[tokio::test] +async fn test_chroot_unix_sendto_relative_after_chdir() { + use std::os::unix::net::UnixDatagram; + + let rootfs = build_test_rootfs("unix-sendto"); + let listener = UnixDatagram::bind(rootfs.join("tmp/svc.dgram")).unwrap(); + listener.set_nonblocking(true).unwrap(); + + let r = chroot_unix_send(&rootfs, "dgram", "/tmp", "svc.dgram", "datagram").await; + assert!(r.success(), "sendto: {}", r.stderr_str().unwrap_or("")); + let mut buf = [0u8; 16]; + let n = listener + .recv(&mut buf) + .expect("datagram must reach the rootfs socket"); + assert_eq!(&buf[..n], b"datagram"); + + cleanup_rootfs(&rootfs); +} + +/// The grant is checked on the virtual path the relative target resolves to: +/// `tmp/svc.sock` from cwd `/etc` is `/etc/tmp/svc.sock`, outside the `/tmp` +/// write grant, and must not be mistaken for `/tmp/svc.sock`. +#[tokio::test] +async fn test_chroot_unix_connect_relative_outside_grant_denied() { + use std::os::unix::net::UnixListener; + + let rootfs = build_test_rootfs("unix-deny"); + fs::create_dir_all(rootfs.join("etc/tmp")).unwrap(); + let outside = UnixListener::bind(rootfs.join("etc/tmp/svc.sock")).unwrap(); + outside.set_nonblocking(true).unwrap(); + let _granted = UnixListener::bind(rootfs.join("tmp/svc.sock")).unwrap(); + + let r = chroot_unix_send(&rootfs, "stream", "/etc", "tmp/svc.sock", "hello").await; + assert!(!r.success(), "connect outside the write grant must fail"); + assert!( + outside.accept().is_err(), + "the ungranted socket must not be reached" + ); + + // A symlink inside the grant is judged by the socket it reaches. + std::os::unix::fs::symlink("/etc/tmp/svc.sock", rootfs.join("tmp/link.sock")).unwrap(); + let r = chroot_unix_send(&rootfs, "stream", "/tmp", "link.sock", "hello").await; + assert!( + !r.success(), + "a symlink out of the write grant must be denied" + ); + assert!( + outside.accept().is_err(), + "the ungranted socket must not be reached" + ); + + cleanup_rootfs(&rootfs); +} diff --git a/crates/sandlock-fixtures/rootfs-helper.c b/crates/sandlock-fixtures/rootfs-helper.c index abe0659c..ad46e694 100644 --- a/crates/sandlock-fixtures/rootfs-helper.c +++ b/crates/sandlock-fixtures/rootfs-helper.c @@ -19,8 +19,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -877,10 +879,41 @@ static int cmd_fexecve(int argc, char **argv) { return 1; } +/* ── unix-send (deliver a payload to a named unix socket) ───── */ +/* + * unix-send stream|dgram : chdir() so a relative + * has a cwd to resolve against, then connect+send (stream) or sendto + * (dgram) . + */ +static int cmd_unix_send(int argc, char **argv) { + if (argc < 4) { fprintf(stderr, "unix-send: need stream|dgram \n"); return 1; } + int dgram = strcmp(argv[0], "dgram") == 0; + if (chdir(argv[1]) != 0) { fprintf(stderr, "unix-send: chdir %s: %s\n", argv[1], strerror(errno)); return 1; } + struct sockaddr_un sun = { .sun_family = AF_UNIX }; + strncpy(sun.sun_path, argv[2], sizeof(sun.sun_path) - 1); + int s = socket(AF_UNIX, dgram ? SOCK_DGRAM : SOCK_STREAM, 0); + if (s < 0) { fprintf(stderr, "unix-send: socket: %s\n", strerror(errno)); return 1; } + size_t len = strlen(argv[3]); + ssize_t n; + if (dgram) { + n = sendto(s, argv[3], len, 0, (struct sockaddr *)&sun, sizeof(sun)); + } else { + if (connect(s, (struct sockaddr *)&sun, sizeof(sun)) != 0) { + fprintf(stderr, "unix-send: connect %s: %s\n", argv[2], strerror(errno)); + return 1; + } + n = send(s, argv[3], len, 0); + } + if (n != (ssize_t)len) { fprintf(stderr, "unix-send: send %s: %s\n", argv[2], strerror(errno)); return 1; } + close(s); + return 0; +} + /* ── dispatch ───────────────────────────────────────────────── */ static int dispatch(const char *cmd, int argc, char **argv) { if (strcmp(cmd, "chdir") == 0) return cmd_chdir(argc, argv); + if (strcmp(cmd, "unix-send") == 0) return cmd_unix_send(argc, argv); if (strcmp(cmd, "fchdir") == 0) return cmd_fchdir(argc, argv); if (strcmp(cmd, "openat2") == 0) return cmd_openat2(argc, argv); if (strcmp(cmd, "chdir-self") == 0) return cmd_chdir_self(argc, argv); From a8cbd957a595beaf3fe0408dba7de09097952694 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 15:54:05 -0800 Subject: [PATCH 2/8] seccomp: resume the handler chain after a deferral A deferred handler made a terminal decision: its Continue skipped every later handler, and the supervisor refused deferral on fork and exec because answering later would bypass fork accounting and the argv freeze. Deferrals beyond a fixed cap failed with EAGAIN even though each trapped task can only have one notification outstanding. Let a deferral resume where it was returned. A deferred handler's Continue runs the next handler, any other result ends the chain, and the outcome then takes the same path as an inline decision. Fork accounting moves out of the chain to run once it settles on Continue, so no handler can defer a fork after it was counted. All deferrals of a notification share one deadline, admission no longer fails, a panicked deferral still answers its caller, and settled deferrals are drained while notifications keep arriving. Signed-off-by: Cong Wang --- crates/sandlock-core/src/seccomp/dispatch.rs | 124 +++--- crates/sandlock-core/src/seccomp/notif.rs | 400 ++++++++++--------- docs/extension-handlers.md | 60 ++- 3 files changed, 304 insertions(+), 280 deletions(-) diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 57bc4d19..10dea7f5 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -246,25 +246,54 @@ impl DispatchTable { .push(handler); } - /// Dispatch a notification through the handler chain for its syscall number. - pub(crate) async fn dispatch( + /// The handler chain for a notification, not yet started. + pub(crate) fn chain( &self, notif: SeccompNotif, notif_fd: RawFd, open: Option>, - ) -> NotifAction { - let nr = notif.data.nr as i64; - if let Some(chain) = self.chains.get(&nr) { - let handler_ctx = HandlerCtx { + ) -> Chain { + let handlers = self + .chains + .get(&(notif.data.nr as i64)) + .map(|chain| chain.handlers.clone()) + .unwrap_or_default(); + Chain { + handlers: handlers.into_iter(), + cx: HandlerCtx { notif, notif_fd, open, - }; - for handler in &chain.handlers { - let action = handler.handle(&handler_ctx).await; - if !matches!(action, NotifAction::Continue) { - return action; - } + }, + } + } + + /// Dispatch a notification through the handler chain for its syscall number. + #[cfg(test)] + pub(crate) async fn dispatch( + &self, + notif: SeccompNotif, + notif_fd: RawFd, + open: Option>, + ) -> NotifAction { + self.chain(notif, notif_fd, open).next().await + } +} + +/// A handler chain that can stop at a deferral and pick up after it. +pub(crate) struct Chain { + handlers: std::vec::IntoIter>, + cx: HandlerCtx, +} + +impl Chain { + /// Run the remaining handlers until one decides or defers; `Continue` + /// means none intervened. + pub(crate) async fn next(&mut self) -> NotifAction { + for handler in self.handlers.by_ref() { + let action = handler.handle(&self.cx).await; + if !matches!(action, NotifAction::Continue) { + return action; } } NotifAction::Continue @@ -291,20 +320,6 @@ pub(crate) fn build_dispatch_table( ) -> DispatchTable { let mut table = DispatchTable::new(); - // ------------------------------------------------------------------ - // Fork/clone family (always on) - // ------------------------------------------------------------------ - for nr in arch::fork_like_syscalls() { - let policy_for_fork = Arc::clone(policy); - let ctx_for_fork = Arc::clone(ctx); - table.register(nr, move |cx: &HandlerCtx| { - let notif = cx.notif; - let policy = Arc::clone(&policy_for_fork); - let ctx = Arc::clone(&ctx_for_fork); - async move { crate::resource::handle_fork(¬if, &ctx, &policy).await } - }); - } - // ------------------------------------------------------------------ // Process groups (always on) // ------------------------------------------------------------------ @@ -1495,41 +1510,48 @@ mod handler_tests { ); } - /// A handler returning `Defer` is non-`Continue`, so it must short-circuit - /// the chain exactly like `Errno`/`ReturnValue`: later handlers on the same - /// syscall do not run. Deferral is therefore a terminal decision. + /// A deferral stops the chain; a `Continue` result picks it up at the next + /// handler, and any other result ends it. #[tokio::test] - async fn dispatch_short_circuits_on_defer() { + async fn chain_resumes_after_a_deferral_that_continues() { let mut table = DispatchTable::new(); let later_ran = Arc::new(AtomicUsize::new(0)); - table.register(libc::SYS_openat, |_cx: &HandlerCtx| async { - NotifAction::defer(async { NotifAction::ReturnValue(1) }) + NotifAction::defer(async { NotifAction::Continue }) }); - let later = Arc::clone(&later_ran); table.register(libc::SYS_openat, move |_cx: &HandlerCtx| { - let later = Arc::clone(&later); - async move { - later.fetch_add(1, Ordering::SeqCst); - NotifAction::Continue - } + later.fetch_add(1, Ordering::SeqCst); + async { NotifAction::ReturnValue(42) } }); - let _ctx = fake_supervisor_ctx(); - let action = table - .dispatch(fake_notif(libc::SYS_openat as i32), -1, None) - .await; + let mut chain = table.chain(fake_notif(libc::SYS_openat as i32), -1, None); + let NotifAction::Defer(deferred) = chain.next().await else { + panic!("the first handler defers") + }; + assert_eq!(later_ran.load(Ordering::SeqCst), 0); + assert!(matches!(deferred.run().await, NotifAction::Continue)); + assert!(matches!(chain.next().await, NotifAction::ReturnValue(42))); + assert_eq!(later_ran.load(Ordering::SeqCst), 1); + } - assert!( - matches!(action, NotifAction::Defer(_)), - "dispatch must return the Defer produced by the first handler" - ); - assert_eq!( - later_ran.load(Ordering::SeqCst), - 0, - "Defer must short-circuit the chain like any non-Continue action" - ); + #[tokio::test] + async fn chain_ends_at_a_deferral_that_decides() { + let mut table = DispatchTable::new(); + table.register(libc::SYS_openat, |_cx: &HandlerCtx| async { + NotifAction::defer(async { NotifAction::Errno(libc::EACCES) }) + }); + table.register(libc::SYS_openat, |_cx: &HandlerCtx| async { + panic!("a decided deferral must end the chain") + }); + let mut chain = table.chain(fake_notif(libc::SYS_openat as i32), -1, None); + let NotifAction::Defer(deferred) = chain.next().await else { + panic!("the first handler defers") + }; + assert!(matches!( + deferred.run().await, + NotifAction::Errno(libc::EACCES) + )); } /// `validate_handler_syscalls_against_policy` must reject handlers whose diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index c88250d8..68c97539 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -43,36 +43,26 @@ impl OnInjectSuccess { } } -/// A deferred decision: an owned, `'static` future that produces the real -/// [`NotifAction`] off the supervisor's notification loop. +/// Work that decides a notification off the supervisor's notification loop. /// /// A handler returns [`NotifAction::Defer`] when computing the response is /// slow (a network round-trip, a blocking syscall) and must not stall the -/// single supervisor task that gates every other trapped syscall. The -/// supervisor moves the future onto a worker, lets the loop proceed, and -/// sends the response (via the still-valid `notif.id`) when the future -/// resolves. The future is `'static` because it outlives the borrowed -/// `HandlerCtx` — capture what you need (`notif` is `Copy`, `notif_fd` is a -/// `RawFd`) by value rather than borrowing `&self`. +/// loop that gates every other trapped syscall. The child stays blocked in +/// its syscall meanwhile. A deferral resumes where it was returned: when a handler +/// defers, a `Continue` result runs the next handler in the chain and any other +/// result ends it, so a deferred decision still passes every later check. /// -/// The deferred future need only be `Send` (not `Sync`): the supervisor -/// moves it onto a worker task and never shares it by reference. Requiring -/// `Sync` of user futures would be a leaky bound (it would reject a future -/// capturing, say, a `Cell`), so it is not required. +/// The future outlives the borrowed `HandlerCtx`; capture what it needs by +/// value. It need only be `Send`: the supervisor moves it onto a task and +/// never shares it. pub struct Deferred { future: Pin + Send + 'static>>, patient: bool, } -// Safety: `NotifAction` must stay `Sync` so it can live in `Sync` contexts -// (handler `&self` state, etc.; the `Handler` trait is `Send + Sync`), which -// requires `Deferred: Sync`. A `Send`-only future is not `Sync`, but the -// boxed future is unreachable through a shared `&Deferred`: the field is -// private, `Debug` touches only a static string, and `run(self)` consumes -// the value (it is never callable through `&self`). With no path to poll or -// read the future via a shared reference, sharing `&Deferred` across threads -// cannot race, so asserting `Sync` is sound while keeping user futures -// `Send`-only. +// Safety: `NotifAction` must stay `Sync` because handlers keep it in `Sync` +// state. The boxed future is private and only reachable through `run(self)`, +// so a shared `&Deferred` can never poll it. unsafe impl Sync for Deferred {} impl std::fmt::Debug for Deferred { @@ -98,11 +88,29 @@ impl Deferred { } } - /// Drive the deferred future to its terminal action. Consumes `self` - /// because the future is run exactly once, on a worker task. pub async fn run(self) -> NotifAction { self.future.await } + + /// Run within `deadline` unless patient; a deferral never resolves to + /// another unexecuted action. + async fn run_until(self, deadline: tokio::time::Instant) -> NotifAction { + let action = if self.patient { + self.run().await + } else { + match tokio::time::timeout_at(deadline, self.run()).await { + Ok(action) => action, + Err(_) => { + log::warn!("parked notification exceeded its deadline; failing with EIO"); + NotifAction::Errno(libc::EIO) + } + } + }; + match action { + NotifAction::Defer(_) | NotifAction::Prepared(_) => NotifAction::Errno(libc::EIO), + other => other, + } + } } /// An owned operation whose side effects require callback approval. @@ -165,10 +173,7 @@ pub enum NotifAction { /// Signal one process, named by any pid in it. `SIGKILL` here ends /// the whole thread group, as it does for any task. KillTask { sig: i32, pid: i32 }, - /// Defer the response: run the carried future on a worker task and - /// send its terminal action later, keyed by `notif.id`. Non-`Continue`, - /// so it short-circuits the handler chain — a deferring handler makes a - /// terminal decision. See [`Deferred`]. + /// Decide off the notification loop. See [`Deferred`]. Defer(Deferred), /// Execute only after the policy callback permits the operation. Prepared(PreparedOperation), @@ -206,8 +211,6 @@ impl NotifAction { self } - /// Construct a [`NotifAction::Defer`] from a `'static` future. Ergonomic - /// shorthand for `NotifAction::Defer(Deferred::new(fut))`. pub fn defer + Send + 'static>(fut: F) -> Self { NotifAction::Defer(Deferred::new(fut)) } @@ -292,17 +295,6 @@ pub(crate) fn named_content_memfd(content: &[u8], seal: bool, name: &str) -> io: Ok(memfd) } -/// Collapse a deferred future's resolved action into a sendable terminal -/// action. A deferred future that itself resolves to `Defer` is a bug -/// (no nested deferral); collapse it to `EIO` so the trapped child gets a -/// definite response instead of wedging forever waiting for one. -fn finalize_deferred(action: NotifAction) -> NotifAction { - match action { - NotifAction::Defer(_) | NotifAction::Prepared(_) => NotifAction::Errno(libc::EIO), - other => other, - } -} - // ============================================================ // NetworkPolicy — network access policy enum // ============================================================ @@ -2854,43 +2846,27 @@ async fn emit_policy_event( // Per-notification handler (runs in a spawned task) // ============================================================ -/// Process a single seccomp notification: vDSO re-patch, path denial check, -/// dispatch, policy event emission, and response. -/// Maximum number of deferred handler futures running concurrently. Caps -/// the worker fan-out (and any resources those workers hold, e.g. memfds or -/// sockets) so a burst of deferrals cannot exhaust the supervisor process. -const DEFER_MAX_INFLIGHT: usize = 64; - -/// Maximum time a deferred handler future may run before the supervisor gives -/// up and fails the trapped syscall closed. Bounds the worst case so a hung -/// future (e.g. a stalled network fetch in a token-injection handler) cannot -/// park the child forever or permanently leak its `DEFER_MAX_INFLIGHT` slot. -const DEFER_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); - -/// Drive a deferred future to its terminal action, bounded by `limit`. -/// -/// On timeout, fail closed with `EIO` so the trapped child gets a definite -/// response instead of parking forever; `finalize_deferred` still guards a -/// future that resolves to a nested `Defer`. -async fn run_deferred_within(deferred: Deferred, limit: std::time::Duration) -> NotifAction { - match tokio::time::timeout(limit, deferred.run()).await { - Ok(action) => finalize_deferred(action), - Err(_) => { - log::warn!( - "deferred handler exceeded {:?}; failing syscall with EIO", - limit - ); - NotifAction::Errno(libc::EIO) - } - } -} +/// How long a notification may stay parked before it fails closed with EIO, +/// so a hung future cannot hold its caller forever. +const PARK_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); /// A notification whose response is decided off the loop. The loop sends /// it: a response sent while an exec freeze is held would release a task /// the freeze skipped because it was parked in seccomp. enum Parked { + /// A handler deferred; the chain picks up after it. + Chain(Box, NotifAction), Approval(Box, crate::policy_fn::Verdict), - Deferred(u64, NotifAction), + Response(u64, NotifAction), +} + +struct ParkedChain { + notif: SeccompNotif, + chain: super::dispatch::Chain, + open: Option>, + has_denied: bool, + /// Shared by every deferral of this notification's chain. + deadline: tokio::time::Instant, } /// Parked notifications and the tids waiting on them, which the exec freeze @@ -2915,17 +2891,29 @@ impl ParkedSet { } async fn join_next(&mut self) -> Option { - loop { - match self.tasks.join_next_with_id().await? { - Ok((id, parked)) => { - self.callers.remove(&id); - return Some(parked); - } - Err(e) => { - self.callers.remove(&e.id()); - } - } - } + let joined = self.tasks.join_next_with_id().await?; + Some(self.settled(joined)) + } + + fn try_join_next(&mut self) -> Option { + let joined = self.tasks.try_join_next_with_id()?; + Some(self.settled(joined)) + } + + fn settled( + &mut self, + joined: Result<(tokio::task::Id, Parked), tokio::task::JoinError>, + ) -> Parked { + let (id, outcome) = match joined { + Ok((id, parked)) => (id, Ok(parked)), + Err(e) => (e.id(), Err(())), + }; + let caller = self.callers.remove(&id); + // A panicked task still owes its caller a response. + outcome.unwrap_or_else(|()| { + let notif_id = caller.map_or(0, |(_, notif_id)| notif_id); + Parked::Response(notif_id, NotifAction::Errno(libc::EIO)) + }) } /// The callers still parked: one that died since leaves its tid free for @@ -2947,30 +2935,12 @@ struct Prepared { exec_image: Option, } -fn park_deferred( - parked: &mut ParkedSet, - notif: &SeccompNotif, - deferred: Deferred, - permit: tokio::sync::OwnedSemaphorePermit, -) { - let id = notif.id; - parked.spawn(notif, async move { - let _permit = permit; - let action = match deferred.patient { - true => finalize_deferred(deferred.run().await), - false => run_deferred_within(deferred, DEFER_TIMEOUT).await, - }; - Parked::Deferred(id, action) - }); -} - async fn handle_notification( notif: SeccompNotif, ctx: &Arc, dispatch_table: &super::dispatch::DispatchTable, fd: RawFd, parked: &mut ParkedSet, - defer_sem: &Arc, ) { let policy = &ctx.policy; @@ -2999,38 +2969,91 @@ async fn handle_notification( .then(|| OpenRequest::decode(¬if, fd, policy, &ctx.processes)) .flatten() .map(Arc::new); - let mut action = { - let denied = has_denied - && crate::seccomp_plan::fs_denied_path_syscalls().contains(&nr) - && is_path_denied_for_notif(&*ctx.policy_fn.lock().await, ¬if, fd); - if denied { - NotifAction::Errno(libc::EACCES) - } else { - // Dispatch runs first so /proc virtualization and the other - // handlers still win for their paths. - let action = dispatch_table.dispatch(notif, fd, open.clone()).await; - // A bare Continue for an open is the racy window: the kernel - // re-resolves the path after it, and a racing thread can swap a - // link to reach a denied carve-out inside a granted tree (issue - // #111) or a procfs entry the handlers hide by name (issue - // #236). The open is done against the pinned inode instead. - let resolves = policy.chroot_root.is_none() && (has_denied || policy.resolves_opens()); - match (&action, &open) { - (NotifAction::Continue, Some(open)) if resolves => { - let pfs = ctx.policy_fn.lock().await; - on_behalf_open(¬if, open, ctx, &pfs, fd, has_denied).await - } - _ => action, - } - } + let denied = has_denied + && crate::seccomp_plan::fs_denied_path_syscalls().contains(&nr) + && is_path_denied_for_notif(&*ctx.policy_fn.lock().await, ¬if, fd); + // Dispatch runs first so /proc virtualization and the other handlers + // still win for their paths. + let mut chain = dispatch_table.chain(notif, fd, open.clone()); + let action = match denied { + true => NotifAction::Errno(libc::EACCES), + false => chain.next().await, }; + let state = ParkedChain { + notif, + chain, + open, + has_denied, + deadline: tokio::time::Instant::now() + PARK_TIMEOUT, + }; + settle_chain(state, action, ctx, fd, parked).await; +} + +async fn settle_chain( + state: ParkedChain, + action: NotifAction, + ctx: &Arc, + fd: RawFd, + parked: &mut ParkedSet, +) { + match action { + NotifAction::Defer(deferred) => { + let notif = state.notif; + parked.spawn(¬if, async move { + let action = deferred.run_until(state.deadline).await; + Parked::Chain(Box::new(state), action) + }); + } + action => { + let ParkedChain { + notif, + open, + has_denied, + .. + } = state; + decide(notif, action, open, has_denied, ctx, fd, parked).await + } + } +} + +/// Everything after the handler chain settles: pinned opens, fork +/// accounting, the policy event and its verdict, and the response. +async fn decide( + notif: SeccompNotif, + mut action: NotifAction, + open: Option>, + has_denied: bool, + ctx: &Arc, + fd: RawFd, + parked: &mut ParkedSet, +) { + let policy = &ctx.policy; + let nr = notif.data.nr as i64; + + // A bare Continue for an open is the racy window: the kernel re-resolves + // the path after it, and a racing thread can swap a link to reach a + // denied carve-out inside a granted tree (issue #111) or a procfs entry + // the handlers hide by name (issue #236). The open is done against the + // pinned inode instead. + let resolves = policy.chroot_root.is_none() && (has_denied || policy.resolves_opens()); + if let (NotifAction::Continue, Some(open)) = (&action, &open) { + if resolves { + let pfs = ctx.policy_fn.lock().await; + action = on_behalf_open(¬if, open, ctx, &pfs, fd, has_denied).await; + } + } if let Some(open) = &open { action = crate::procfs::net_dispatch::guard_injection(action, ¬if, open, ctx).await; } - let fork_counted = matches!(action, NotifAction::Continue) - && crate::resource::fork_counted_on_continue(¬if); + // Counted only once no handler can still refuse or defer the fork. + let mut fork_counted = false; + if matches!(action, NotifAction::Continue) && arch::fork_like_syscalls().contains(&nr) { + action = crate::resource::handle_fork(¬if, ctx, policy).await; + fork_counted = matches!(action, NotifAction::Continue) + && crate::resource::fork_counted_on_continue(¬if); + } // Read without freezing so the sandbox keeps running while policy_fn // decides; `freeze_exec` re-reads it under the freeze before Continue. @@ -3054,7 +3077,7 @@ async fn handle_notification( Parked::Approval(Box::new(prepared), verdict) }); } - None => finish_notification(prepared, None, ctx, fd, parked, defer_sem).await, + None => finish_notification(prepared, None, ctx, fd, parked).await, } } @@ -3106,7 +3129,6 @@ async fn finish_notification( ctx: &Arc, fd: RawFd, parked: &mut ParkedSet, - defer_sem: &Arc, ) { let policy = &ctx.policy; let Prepared { @@ -3164,13 +3186,9 @@ async fn finish_notification( } } - // Deferred response: run the handler's future off the loop so slow work - // (a network round-trip, a blocking syscall) does not stall it. The - // trapped child stays parked; the loop sends the result when it lands. - // - // Deferral is refused on syscalls whose Continue path requires the - // execve argv-safety freeze or fork creation-tracking: answering later - // would skip that TOCTOU-closing work. + // Supervisor work decided after the verdict, such as an approved connect + // or a FIFO open, answers with its result. Answering a fork or exec later + // would skip its TOCTOU-closing work; neither is ever parked here. if let NotifAction::Defer(deferred) = action { if crate::freeze::requires_freeze_on_continue(nr) || crate::resource::requires_process_creation_tracking(¬if, policy) @@ -3178,14 +3196,11 @@ async fn finish_notification( let _ = send_response(fd, notif.id, NotifAction::Errno(libc::EPERM)); return; } - match Arc::clone(defer_sem).try_acquire_owned() { - Ok(permit) => park_deferred(parked, ¬if, deferred, permit), - // Too many deferrals in flight: fail fast with EAGAIN rather than - // blocking the loop or letting unbounded workers accrete. - Err(_) => { - let _ = send_response(fd, notif.id, NotifAction::Errno(libc::EAGAIN)); - } - } + let id = notif.id; + let deadline = tokio::time::Instant::now() + PARK_TIMEOUT; + parked.spawn(¬if, async move { + Parked::Response(id, deferred.run_until(deadline).await) + }); return; } @@ -3214,6 +3229,9 @@ async fn finish_notification( crate::freeze::reap_pending(&freeze.pending_tids); } + if fork_counted && creation_trace.is_none() && send_result.is_err() { + crate::resource::rollback_fork_count(&ctx.resource).await; + } if let Some(trace) = creation_trace { if send_result.is_ok() { match crate::resource::finish_process_creation_tracking(trace).await { @@ -3242,13 +3260,22 @@ async fn settle_parked( ctx: &Arc, fd: RawFd, set: &mut ParkedSet, - defer_sem: &Arc, ) { match parked { + Parked::Chain(mut state, action) => { + if id_valid(fd, state.notif.id).is_err() { + return; + } + let action = match action { + NotifAction::Continue => state.chain.next().await, + decided => decided, + }; + settle_chain(*state, action, ctx, fd, set).await + } Parked::Approval(prepared, verdict) => { - finish_notification(*prepared, Some(verdict), ctx, fd, set, defer_sem).await + finish_notification(*prepared, Some(verdict), ctx, fd, set).await } - Parked::Deferred(id, action) => { + Parked::Response(id, action) => { let _ = send_response(fd, id, action); } } @@ -3306,10 +3333,6 @@ pub async fn supervisor( // still per-child pidfd readiness in `spawn_pid_watcher`. let gc = tokio::spawn(process_index_gc(Arc::clone(&ctx.processes))); - // Bounds the number of in-flight deferred handler futures (see - // `DEFER_MAX_INFLIGHT`). Shared across all notifications this supervisor - // processes. - let defer_sem = Arc::new(tokio::sync::Semaphore::new(DEFER_MAX_INFLIGHT)); let mut parked = ParkedSet::new(); // Edge-triggered drain: each `readable().await` returns once per @@ -3321,7 +3344,7 @@ pub async fn supervisor( // // Notifications are processed sequentially (not spawned) to avoid // mutex contention between concurrent handlers. Policy verdicts and - // deferred handlers resolve in `parked` and are answered here too. + // parked handlers resolve in `parked` and are answered here too. 'outer: loop { let mut ready = tokio::select! { ready = async_fd.readable() => match ready { @@ -3329,7 +3352,7 @@ pub async fn supervisor( Err(_) => break 'outer, }, Some(done) = parked.join_next() => { - settle_parked(done, &ctx, fd, &mut parked, &defer_sem).await; + settle_parked(done, &ctx, fd, &mut parked).await; continue; } }; @@ -3337,6 +3360,10 @@ pub async fn supervisor( drop(ready); loop { + // A child that keeps the queue full must not postpone settled parks. + if let Some(done) = parked.try_join_next() { + settle_parked(done, &ctx, fd, &mut parked).await; + } match probe_notif_fd(fd) { NotifFdState::Pending => { let notif = match recv_notif(fd) { @@ -3353,8 +3380,7 @@ pub async fn supervisor( Err(e) if e.raw_os_error() == Some(libc::ENOENT) => continue, Err(_) => break 'outer, }; - handle_notification(notif, &ctx, &dispatch_table, fd, &mut parked, &defer_sem) - .await; + handle_notification(notif, &ctx, &dispatch_table, fd, &mut parked).await; } NotifFdState::Empty => break, NotifFdState::Terminal => break 'outer, @@ -3787,68 +3813,52 @@ mod tests { #[tokio::test] async fn deferred_future_need_not_be_sync() { - // A deferred future may capture Send-but-not-Sync state across an - // await. `Cell` is Send but never Sync; holding it across `.await` - // makes the future !Sync. Only `Send` is required (the supervisor - // moves the future to a worker, never shares it by reference). use std::cell::Cell; let action = NotifAction::defer(async move { let counter = Cell::new(0); counter.set(counter.get() + 41); - tokio::task::yield_now().await; // hold the !Sync Cell across await + tokio::task::yield_now().await; NotifAction::ReturnValue(counter.get() + 1) }); - let NotifAction::Defer(d) = action else { - panic!("expected Defer") - }; - assert!(matches!(d.run().await, NotifAction::ReturnValue(42))); - } - - #[tokio::test] - async fn deferred_runs_to_its_terminal_action() { - // A Defer carries a future; running it yields the deferred decision. - let action = NotifAction::defer(async { NotifAction::ReturnValue(7) }); let NotifAction::Defer(deferred) = action else { - panic!("defer() must construct a NotifAction::Defer"); + panic!("expected Defer") }; - assert!(matches!(deferred.run().await, NotifAction::ReturnValue(7))); + assert!(matches!(deferred.run().await, NotifAction::ReturnValue(42))); } #[tokio::test(start_paused = true)] - async fn deferred_times_out_to_eio() { - // A deferred future that exceeds its limit must fail closed (EIO) so - // the trapped child gets a definite response instead of parking - // forever (and leaking its DEFER_MAX_INFLIGHT slot). + async fn deferred_past_deadline_fails_with_eio() { let slow = Deferred::new(async { tokio::time::sleep(std::time::Duration::from_secs(60)).await; NotifAction::ReturnValue(7) }); - let action = run_deferred_within(slow, std::time::Duration::from_secs(1)).await; - assert!(matches!(action, NotifAction::Errno(e) if e == libc::EIO)); + let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1); + assert!(matches!( + slow.run_until(deadline).await, + NotifAction::Errno(libc::EIO) + )); } #[tokio::test(start_paused = true)] - async fn deferred_within_limit_passes_through() { - // A future that resolves within the limit returns its terminal action. - let fast = Deferred::new(async { NotifAction::ReturnValue(7) }); - let action = run_deferred_within(fast, std::time::Duration::from_secs(1)).await; - assert!(matches!(action, NotifAction::ReturnValue(7))); - } - - #[test] - fn finalize_deferred_collapses_nested_defer_to_eio() { - // A deferred future that itself resolves to Defer is a bug: collapse - // to EIO so the trapped child is never wedged waiting for a response. - let nested = NotifAction::defer(async { NotifAction::Continue }); - assert!(matches!(finalize_deferred(nested), NotifAction::Errno(e) if e == libc::EIO)); - // Non-nested terminal actions pass through unchanged. + async fn patient_deferral_ignores_deadline() { + let slow = Deferred::patient(async { + tokio::time::sleep(std::time::Duration::from_secs(60)).await; + NotifAction::ReturnValue(7) + }); + let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1); assert!(matches!( - finalize_deferred(NotifAction::Continue), - NotifAction::Continue + slow.run_until(deadline).await, + NotifAction::ReturnValue(7) )); + } + + #[tokio::test] + async fn deferred_resolving_to_unexecuted_work_fails_with_eio() { + let nested = Deferred::new(async { NotifAction::defer(async { NotifAction::Continue }) }); + let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1); assert!(matches!( - finalize_deferred(NotifAction::ReturnValue(3)), - NotifAction::ReturnValue(3) + nested.run_until(deadline).await, + NotifAction::Errno(libc::EIO) )); } diff --git a/docs/extension-handlers.md b/docs/extension-handlers.md index cb207241..3a212762 100644 --- a/docs/extension-handlers.md +++ b/docs/extension-handlers.md @@ -377,7 +377,7 @@ The contract is exercised at two layers: | `InjectFd { srcfd, targetfd }` | Inject `srcfd` into the guest at slot `targetfd`, then continue. | | `InjectFdSendTracked { srcfd, newfd_flags, on_success }` | Inject `srcfd`; `on_success` callback runs synchronously when the kernel returns the slot, so downstream tracking cannot race with the guest seeing the new fd. | | `Kill { sig, pgid }` | Send `sig` to the guest's process group. | -| `Defer(Deferred)` | Run the carried future off the supervisor loop; its terminal action is sent later, keyed by `notif.id`. Non-`Continue`, so it ends the chain. See [Deferred handlers](#deferred-handlers). | +| `Defer(Deferred)` | Run the carried future off the supervisor loop. A `Continue` result resumes the chain at the next handler; any other result is the decision. See [Deferred handlers](#deferred-handlers). | `InjectFd`/`InjectFdSend`/`InjectFdSendTracked` carry a file descriptor. To synthesise *content* (rather than inject an fd you already hold), construct the action with @@ -464,13 +464,13 @@ user handlers. ### Deferred handlers -Continue-site safety exists because the supervisor processes notifications sequentially: a handler -that blocks (a network round-trip, a blocking syscall, a slow lock) stalls every other trapped -syscall until it returns. `NotifAction::Defer` is the escape hatch. A handler that returns `Defer` -hands the supervisor an owned, `'static` future; the supervisor moves it onto a worker task, lets -the notification loop proceed immediately, and sends the response (keyed by `notif.id`) when the -future resolves. The trapped child stays parked in the syscall until then, so `notif.id` stays -valid and the child-memory helpers keep working inside the deferred future. +The supervisor processes notifications sequentially: a handler that blocks (a network round-trip, +a blocking syscall, a slow lock) stalls every other trapped syscall until it returns. +`NotifAction::Defer` is the escape hatch. A handler that returns `Defer` hands the supervisor an +owned, `'static` future; the supervisor runs it on a task, lets the notification loop proceed, and +picks the notification up again when the future resolves. The trapped child stays blocked in the +syscall until then, so `notif.id` stays valid and the child-memory helpers keep working inside the +deferred future. ```rust use std::future::Future; @@ -482,17 +482,14 @@ fn handle<'a>( &'a self, cx: &'a HandlerCtx, ) -> Pin + Send + 'a>> { - // Copy out what the deferred future needs: `notif` is `Copy`, `notif_fd` - // is a `RawFd`, and `Arc` state is cloned. Never borrow `&self`/`cx`: - // the deferred future is `'static` and outlives this call. + // The deferred future is `'static` and outlives this call: copy `notif`, + // clone `Arc` state, never borrow `&self`/`cx`. let (fd, id, pid) = (cx.notif_fd, cx.notif.id, cx.notif.pid); let key = read_child_cstr(fd, id, pid, cx.notif.data.args[1], 4096); let backend = self.backend.clone(); Box::pin(async move { let Some(key) = key else { return NotifAction::Continue }; NotifAction::defer(async move { - // Runs on a worker, off the supervisor loop. The child is still - // parked, so child-memory helpers and `id` are valid here. match backend.get(&key).await { Ok(_data) => NotifAction::ReturnValue(0), // e.g. inject a memfd of `_data` Err(_) => NotifAction::Errno(libc::EIO), @@ -502,30 +499,25 @@ fn handle<'a>( } ``` -The deferred future must be `Send + 'static`: `Send` so the supervisor can move it onto a worker, -and `'static` so it can outlive the borrowed `HandlerCtx`. It does **not** need to be `Sync` (the -supervisor moves it, never shares it by reference). Capture owned data only: copy `notif` (it is -`Copy`), clone an `Arc` for shared state, and never borrow `&self`/`cx`. +The deferred future must be `Send + 'static`; it does **not** need to be `Sync` (the supervisor +moves it, never shares it by reference). Contract: -- **Terminal decision.** `Defer` is non-`Continue`, so it short-circuits the handler chain exactly - like `Errno`/`ReturnValue`: later handlers on the same syscall do not run. A deferring handler - decides the outcome. -- **No deferral on freeze/fork syscalls.** Deferral is refused (with `EPERM`) on - `execve`/`execveat` and fork-creating syscalls, because moving the response off-loop would skip - the argv-safety freeze (see [issue #27][i27]) and process creation-tracking that those paths - require before `Continue`. -- **Bounded fan-out.** At most `DEFER_MAX_INFLIGHT` deferred futures run concurrently; beyond that, - further deferrals fail fast with `EAGAIN` rather than queuing. The cap also bounds the resources - workers hold (memfds, sockets). -- **No nesting.** A deferred future that itself resolves to `Defer` is a bug; the supervisor - collapses it to `EIO` so the child is never left wedged. -- **Stale id.** If the child exits mid-defer, the eventual `send_response` is a no-op and the - child-memory helpers fail safe (they are `id_valid`-bracketed), matching the inline path's - "child may have exited" tolerance. - -Do not defer trivial fast handlers: the worker hop adds latency. Defer only when the work would +- **Resumes the chain.** A `Continue` result runs the next handler on the same syscall; any + other result ends the chain. Either way the outcome then passes every check a handler's inline + result would: pinned opens, fork accounting, the policy callback and the execve argv-safety + freeze (see [issue #27][i27]). Deferring on `execve` or `fork` is therefore safe. +- **Deadline.** All deferred futures of one notification share a 30 second deadline; past it the syscall + fails with `EIO`. +- **No admission limit.** Each trapped task has at most one outstanding notification, so deferred + work is already bounded by the number of tasks in the sandbox. +- **No nesting.** A deferred future that resolves to another `Defer` is a bug; the supervisor turns it + into `EIO` so the child is never left wedged. +- **Stale id.** If the child exits mid-defer, the chain is dropped and the child-memory helpers + fail safe (they are `id_valid`-bracketed). + +Do not defer trivial fast handlers: the task hop adds latency. Defer only when the work would otherwise block the loop. ## Security boundary From db42d972dcb8dd52e32daa5befc1a3f0c1ba7246 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 15:54:06 -0800 Subject: [PATCH 3/8] seccomp: run synchronous deferred work on native threads Supervised connects ran on the notification loop after approval, so a slow peer or a full listener backlog stalled every other trapped syscall in the sandbox. Let a deferral carry a synchronous closure. It runs on a process-wide runtime kept apart from the embedder's, whose shutdown would otherwise wait for a closure stuck on a peer. A closure cannot be cancelled, so it has no deadline: failing the syscall early would not stop its side effects. The syscall waits for it as it would for the kernel, and since a trapped task has at most one notification outstanding, the sandbox's own task count bounds its closures. Both supervised connects now run there. Signed-off-by: Cong Wang --- crates/sandlock-core/src/network/connect.rs | 22 +++++--- crates/sandlock-core/src/network/unix.rs | 29 +++++----- crates/sandlock-core/src/seccomp/mod.rs | 1 + crates/sandlock-core/src/seccomp/native.rs | 61 +++++++++++++++++++++ crates/sandlock-core/src/seccomp/notif.rs | 51 ++++++++++++++--- docs/extension-handlers.md | 10 +++- 6 files changed, 145 insertions(+), 29 deletions(-) create mode 100644 crates/sandlock-core/src/seccomp/native.rs diff --git a/crates/sandlock-core/src/network/connect.rs b/crates/sandlock-core/src/network/connect.rs index f8110fb0..e0533455 100644 --- a/crates/sandlock-core/src/network/connect.rs +++ b/crates/sandlock-core/src/network/connect.rs @@ -102,14 +102,22 @@ pub(super) async fn connect_on_behalf( let metadata_addr = addr_bytes.clone(); NotifAction::prepare(&metadata_addr, dup_fd.as_raw_fd(), move || { - // Record the original destination before connect so the proxy - // cannot receive the connection before its mapping is available. - if plan.record_orig_dest { - if let Some(ref map) = orig_dest_map { - record_orig_dest(map, dup_fd.as_raw_fd(), sockaddr_is_ipv6(&addr_bytes), ip); + // A blocking connect waits on the peer, which must not stall the loop. + NotifAction::native(move || { + // Record the original destination before connect so the proxy + // cannot receive the connection before its mapping is available. + if plan.record_orig_dest { + if let Some(ref map) = orig_dest_map { + record_orig_dest( + map, + dup_fd.as_raw_fd(), + sockaddr_is_ipv6(&addr_bytes), + ip, + ); + } } - } - connect_dup(dup_fd.as_raw_fd(), &plan.addr) + connect_dup(dup_fd.as_raw_fd(), &plan.addr) + }) }) .held_if(held) } else { diff --git a/crates/sandlock-core/src/network/unix.rs b/crates/sandlock-core/src/network/unix.rs index b645c1c6..ab320428 100644 --- a/crates/sandlock-core/src/network/unix.rs +++ b/crates/sandlock-core/src/network/unix.rs @@ -133,19 +133,22 @@ pub(super) fn connect_named_unix_on_behalf( }; let addr = sockaddr_un_bytes(&sun, len); NotifAction::prepare(&addr.clone(), dup_fd.as_raw_fd(), move || { - let _pinned = pinned; - let ret = unsafe { - libc::connect( - dup_fd.as_raw_fd(), - addr.as_ptr() as *const libc::sockaddr, - addr.len() as libc::socklen_t, - ) - }; - if ret == 0 { - NotifAction::ReturnValue(0) - } else { - NotifAction::Errno(unsafe { *libc::__errno_location() }) - } + // A listener with a full backlog blocks the connect. + NotifAction::native(move || { + let _pinned = pinned; + let ret = unsafe { + libc::connect( + dup_fd.as_raw_fd(), + addr.as_ptr() as *const libc::sockaddr, + addr.len() as libc::socklen_t, + ) + }; + if ret == 0 { + NotifAction::ReturnValue(0) + } else { + NotifAction::Errno(unsafe { *libc::__errno_location() }) + } + }) }) } diff --git a/crates/sandlock-core/src/seccomp/mod.rs b/crates/sandlock-core/src/seccomp/mod.rs index 231fc867..673f05b7 100644 --- a/crates/sandlock-core/src/seccomp/mod.rs +++ b/crates/sandlock-core/src/seccomp/mod.rs @@ -1,6 +1,7 @@ pub mod bpf; pub(crate) mod ctx; pub mod dispatch; +pub(crate) mod native; pub mod notif; pub(crate) mod state; pub mod syscall; diff --git a/crates/sandlock-core/src/seccomp/native.rs b/crates/sandlock-core/src/seccomp/native.rs new file mode 100644 index 00000000..d397fc8c --- /dev/null +++ b/crates/sandlock-core/src/seccomp/native.rs @@ -0,0 +1,61 @@ +//! Synchronous work run on behalf of a parked notification. +//! +//! A native closure cannot be cancelled, so it has no deadline: its caller +//! waits for it as for the syscall itself. A trapped task has at most one +//! notification outstanding, which bounds how many a sandbox can run. + +use std::sync::OnceLock; + +use tokio::runtime::Runtime; + +use super::notif::NotifAction; + +const MAX_THREADS: usize = 512; + +// The embedder's runtime waits for its blocking tasks on shutdown, and a +// closure stuck on a peer would hold that shutdown hostage. +fn runtime() -> &'static Runtime { + static RUNTIME: OnceLock = OnceLock::new(); + RUNTIME.get_or_init(|| { + tokio::runtime::Builder::new_current_thread() + .max_blocking_threads(MAX_THREADS) + .thread_name("sandlock-native") + .build() + .expect("building a runtime without drivers cannot fail") + }) +} + +/// Run `work` on a native thread. +pub(crate) async fn run(work: Box NotifAction + Send>) -> NotifAction { + let job = runtime().spawn_blocking(work); + job.await.unwrap_or(NotifAction::Errno(libc::EIO)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn abandoned_work_runs_to_completion() { + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel::<()>(); + let (done_tx, done_rx) = tokio::sync::oneshot::channel(); + let waiter = tokio::spawn(run(Box::new(move || { + let _ = started_tx.send(()); + let _ = release_rx.recv(); + let _ = done_tx.send(()); + NotifAction::Continue + }))); + started_rx.await.unwrap(); + waiter.abort(); + let _ = waiter.await; + release_tx.send(()).unwrap(); + done_rx.await.unwrap(); + } + + #[tokio::test] + async fn panicking_work_fails_closed() { + let action = run(Box::new(|| panic!("native panic"))).await; + assert!(matches!(action, NotifAction::Errno(libc::EIO))); + } +} diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index 68c97539..f0146f92 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -56,40 +56,60 @@ impl OnInjectSuccess { /// value. It need only be `Send`: the supervisor moves it onto a task and /// never shares it. pub struct Deferred { - future: Pin + Send + 'static>>, + work: Work, patient: bool, } +enum Work { + Future(Pin + Send + 'static>>), + Native(Box NotifAction + Send + 'static>), +} + // Safety: `NotifAction` must stay `Sync` because handlers keep it in `Sync` -// state. The boxed future is private and only reachable through `run(self)`, -// so a shared `&Deferred` can never poll it. +// state. The work is private and only reachable by consuming the `Deferred`, so +// a shared `&Deferred` can never touch it. unsafe impl Sync for Deferred {} impl std::fmt::Debug for Deferred { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str("Deferred()") + match &self.work { + Work::Future(_) => f.write_str("Deferred()"), + Work::Native(_) => f.write_str("Deferred()"), + } } } impl Deferred { pub fn new + Send + 'static>(f: F) -> Self { Self { - future: Box::pin(f), + work: Work::Future(Box::pin(f)), patient: false, } } + /// Synchronous work run on a native thread. It cannot be cancelled, so + /// it is patient: failing the syscall would not stop its side effects. + pub fn native(work: impl FnOnce() -> NotifAction + Send + 'static) -> Self { + Self { + work: Work::Native(Box::new(work)), + patient: true, + } + } + /// A deferral with no deadline, for a syscall that natively waits as long /// as it takes. The future has to end by itself once the child is gone. pub(crate) fn patient + Send + 'static>(f: F) -> Self { Self { - future: Box::pin(f), + work: Work::Future(Box::pin(f)), patient: true, } } pub async fn run(self) -> NotifAction { - self.future.await + match self.work { + Work::Future(future) => future.await, + Work::Native(work) => super::native::run(work).await, + } } /// Run within `deadline` unless patient; a deferral never resolves to @@ -215,6 +235,10 @@ impl NotifAction { NotifAction::Defer(Deferred::new(fut)) } + pub fn native(work: impl FnOnce() -> NotifAction + Send + 'static) -> Self { + NotifAction::Defer(Deferred::native(work)) + } + /// Inject `content` into the child as the syscall's returned fd, backed by /// a sealed (read-only, fixed-size), `O_CLOEXEC` in-memory file. /// @@ -3852,6 +3876,19 @@ mod tests { )); } + #[tokio::test(start_paused = true)] + async fn native_deferral_ignores_deadline() { + let slow = Deferred::native(|| { + std::thread::sleep(std::time::Duration::from_millis(50)); + NotifAction::ReturnValue(7) + }); + let deadline = tokio::time::Instant::now() + std::time::Duration::from_millis(1); + assert!(matches!( + slow.run_until(deadline).await, + NotifAction::ReturnValue(7) + )); + } + #[tokio::test] async fn deferred_resolving_to_unexecuted_work_fails_with_eio() { let nested = Deferred::new(async { NotifAction::defer(async { NotifAction::Continue }) }); diff --git a/docs/extension-handlers.md b/docs/extension-handlers.md index 3a212762..c5dd1bbc 100644 --- a/docs/extension-handlers.md +++ b/docs/extension-handlers.md @@ -502,14 +502,20 @@ fn handle<'a>( The deferred future must be `Send + 'static`; it does **not** need to be `Sync` (the supervisor moves it, never shares it by reference). +For synchronous work (a blocking syscall, a C library call), return +`NotifAction::native(move || ...)` instead of wrapping it in a future. The closure runs on a +native thread and cannot be cancelled, so it has no deadline: the syscall waits for it as it would +for the kernel. It may still outlive its syscall if the child dies, so capture owned resources +rather than raw fd numbers or pointers. + Contract: - **Resumes the chain.** A `Continue` result runs the next handler on the same syscall; any other result ends the chain. Either way the outcome then passes every check a handler's inline result would: pinned opens, fork accounting, the policy callback and the execve argv-safety freeze (see [issue #27][i27]). Deferring on `execve` or `fork` is therefore safe. -- **Deadline.** All deferred futures of one notification share a 30 second deadline; past it the syscall - fails with `EIO`. +- **Deadline.** All deferred futures of one notification share a 30 second deadline; past it the + syscall fails with `EIO`. Native closures have none. - **No admission limit.** Each trapped task has at most one outstanding notification, so deferred work is already bounded by the number of tasks in the sandbox. - **No nesting.** A deferred future that resolves to another `Defer` is a bug; the supervisor turns it From 5409a64edb92c09b5ebc90892459da418f88f826 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 15:54:08 -0800 Subject: [PATCH 4/8] seccomp: order deferred child-memory writes against exec Deferred work keeps running while the loop validates an execve's argv under the freeze. A write it makes into a CLONE_VM peer's memory can land on that argv after validation and before the kernel copies it, and the deferred sendmmsg completion already wrote msg_len that way. Give each sandbox a gate. Deferred futures and native closures run with it installed, write_child_mem and write_child_mem_force hold its shared side for each single write, and the exec freeze holds the exclusive side until the freeze is released. Writes are the only access that can change argv and each holds the gate for one copy, so an exec waits microseconds and never fails because unrelated work is still running. Signed-off-by: Cong Wang --- crates/sandlock-core/src/seccomp/gate.rs | 101 +++++++++++++++++++ crates/sandlock-core/src/seccomp/mod.rs | 1 + crates/sandlock-core/src/seccomp/native.rs | 30 ++++-- crates/sandlock-core/src/seccomp/notif.rs | 108 ++++++++++++++------- docs/extension-handlers.md | 3 + 5 files changed, 200 insertions(+), 43 deletions(-) create mode 100644 crates/sandlock-core/src/seccomp/gate.rs diff --git a/crates/sandlock-core/src/seccomp/gate.rs b/crates/sandlock-core/src/seccomp/gate.rs new file mode 100644 index 00000000..e5a1fba8 --- /dev/null +++ b/crates/sandlock-core/src/seccomp/gate.rs @@ -0,0 +1,101 @@ +//! Orders off-loop writes to child memory against execve argv validation. +//! +//! Parked work runs with its sandbox's gate installed, and every write to +//! child memory holds the shared side for that one write. The exec freeze +//! holds the exclusive side, so no parked write can land on a peer's argv +//! between its validation and the kernel copying it. + +use std::cell::RefCell; +use std::future::Future; +use std::sync::{Arc, RwLock, RwLockWriteGuard}; + +#[derive(Clone, Default)] +pub(crate) struct MemGate(Arc>); + +tokio::task_local! { + static TASK_GATE: MemGate; +} + +thread_local! { + static THREAD_GATE: RefCell> = const { RefCell::new(None) }; +} + +impl MemGate { + /// Writes only hold the shared side for a single copy, so this waits + /// microseconds, never for parked work to finish. + pub(crate) fn exclusive(&self) -> RwLockWriteGuard<'_, ()> { + self.0.write().unwrap_or_else(|e| e.into_inner()) + } + + pub(crate) async fn scope(self, future: F) -> F::Output { + TASK_GATE.scope(self, future).await + } + + pub(crate) fn enter(self, work: impl FnOnce() -> T) -> T { + let previous = THREAD_GATE.with(|gate| gate.replace(Some(self))); + let result = work(); + THREAD_GATE.with(|gate| *gate.borrow_mut() = previous); + result + } +} + +/// Run one write to child memory under the installed gate, if any. The +/// notification loop installs none: nothing else runs on it during a freeze. +pub(crate) fn write(work: impl FnOnce() -> T) -> T { + let gate = TASK_GATE + .try_with(MemGate::clone) + .ok() + .or_else(|| THREAD_GATE.with(|gate| gate.borrow().clone())); + match gate { + Some(gate) => { + let _shared = gate.0.read().unwrap_or_else(|e| e.into_inner()); + work() + } + None => work(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn exclusive_waits_for_a_write_in_progress() { + let gate = MemGate::default(); + let (writing_tx, writing_rx) = std::sync::mpsc::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel::<()>(); + let writer = std::thread::spawn({ + let gate = gate.clone(); + move || { + gate.enter(|| { + write(|| { + writing_tx.send(()).unwrap(); + release_rx.recv().unwrap(); + }) + }) + } + }); + writing_rx.recv().unwrap(); + assert!(gate.0.try_write().is_err()); + release_tx.send(()).unwrap(); + writer.join().unwrap(); + drop(gate.exclusive()); + } + + #[tokio::test] + async fn parked_futures_write_under_the_gate() { + let gate = MemGate::default(); + let held = gate.clone(); + gate.scope(async move { + write(|| assert!(held.0.try_write().is_err())); + }) + .await; + } + + #[test] + fn writes_outside_parked_work_are_ungated() { + let gate = MemGate::default(); + let _exclusive = gate.exclusive(); + write(|| ()); + } +} diff --git a/crates/sandlock-core/src/seccomp/mod.rs b/crates/sandlock-core/src/seccomp/mod.rs index 673f05b7..b7b59e1b 100644 --- a/crates/sandlock-core/src/seccomp/mod.rs +++ b/crates/sandlock-core/src/seccomp/mod.rs @@ -1,6 +1,7 @@ pub mod bpf; pub(crate) mod ctx; pub mod dispatch; +pub(crate) mod gate; pub(crate) mod native; pub mod notif; pub(crate) mod state; diff --git a/crates/sandlock-core/src/seccomp/native.rs b/crates/sandlock-core/src/seccomp/native.rs index d397fc8c..5b308819 100644 --- a/crates/sandlock-core/src/seccomp/native.rs +++ b/crates/sandlock-core/src/seccomp/native.rs @@ -8,6 +8,7 @@ use std::sync::OnceLock; use tokio::runtime::Runtime; +use super::gate::MemGate; use super::notif::NotifAction; const MAX_THREADS: usize = 512; @@ -25,9 +26,15 @@ fn runtime() -> &'static Runtime { }) } -/// Run `work` on a native thread. -pub(crate) async fn run(work: Box NotifAction + Send>) -> NotifAction { - let job = runtime().spawn_blocking(work); +/// Run `work` on a native thread under `gate`, or ungated outside a supervisor. +pub(crate) async fn run( + gate: Option, + work: Box NotifAction + Send>, +) -> NotifAction { + let job = runtime().spawn_blocking(move || match gate { + Some(gate) => gate.enter(work), + None => work(), + }); job.await.unwrap_or(NotifAction::Errno(libc::EIO)) } @@ -40,12 +47,15 @@ mod tests { let (started_tx, started_rx) = tokio::sync::oneshot::channel(); let (release_tx, release_rx) = std::sync::mpsc::channel::<()>(); let (done_tx, done_rx) = tokio::sync::oneshot::channel(); - let waiter = tokio::spawn(run(Box::new(move || { - let _ = started_tx.send(()); - let _ = release_rx.recv(); - let _ = done_tx.send(()); - NotifAction::Continue - }))); + let waiter = tokio::spawn(run( + None, + Box::new(move || { + let _ = started_tx.send(()); + let _ = release_rx.recv(); + let _ = done_tx.send(()); + NotifAction::Continue + }), + )); started_rx.await.unwrap(); waiter.abort(); let _ = waiter.await; @@ -55,7 +65,7 @@ mod tests { #[tokio::test] async fn panicking_work_fails_closed() { - let action = run(Box::new(|| panic!("native panic"))).await; + let action = run(None, Box::new(|| panic!("native panic"))).await; assert!(matches!(action, NotifAction::Errno(libc::EIO))); } } diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index f0146f92..efa9872d 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -105,20 +105,29 @@ impl Deferred { } } + /// Run outside a supervisor, where native work is not ordered against exec. pub async fn run(self) -> NotifAction { match self.work { Work::Future(future) => future.await, - Work::Native(work) => super::native::run(work).await, + Work::Native(work) => super::native::run(None, work).await, + } + } + + async fn run_in(self, offload: &Offload) -> NotifAction { + let gate = offload.gate.clone(); + match self.work { + Work::Future(future) => gate.scope(future).await, + Work::Native(work) => super::native::run(Some(gate), work).await, } } /// Run within `deadline` unless patient; a deferral never resolves to /// another unexecuted action. - async fn run_until(self, deadline: tokio::time::Instant) -> NotifAction { + async fn run_until(self, offload: &Offload, deadline: tokio::time::Instant) -> NotifAction { let action = if self.patient { - self.run().await + self.run_in(offload).await } else { - match tokio::time::timeout_at(deadline, self.run()).await { + match tokio::time::timeout_at(deadline, self.run_in(offload)).await { Ok(action) => action, Err(_) => { log::warn!("parked notification exceeded its deadline; failing with EIO"); @@ -133,6 +142,13 @@ impl Deferred { } } +/// What one sandbox's parked work shares: the gate ordering its child-memory +/// writes against exec. +#[derive(Clone)] +struct Offload { + gate: super::gate::MemGate, +} + /// An owned operation whose side effects require callback approval. pub struct PreparedOperation { pub(crate) host: Option, @@ -1777,8 +1793,12 @@ pub fn write_child_mem( addr: u64, data: &[u8], ) -> Result<(), NotifError> { - id_valid(notif_fd, id).map_err(NotifError::Ioctl)?; - write_child_mem_vm(pid, addr, data)?; + // Checked under the gate: a write that waited out an exec freeze must not + // follow the leader's pid into the image that exec installed. + super::gate::write(|| { + id_valid(notif_fd, id).map_err(NotifError::Ioctl)?; + write_child_mem_vm(pid, addr, data) + })?; id_valid(notif_fd, id).map_err(NotifError::Ioctl)?; Ok(()) } @@ -1816,8 +1836,10 @@ pub fn write_child_mem_force( addr: u64, data: &[u8], ) -> Result<(), NotifError> { - id_valid(notif_fd, id).map_err(NotifError::Ioctl)?; - write_child_mem_proc(pid, addr, data)?; + super::gate::write(|| { + id_valid(notif_fd, id).map_err(NotifError::Ioctl)?; + write_child_mem_proc(pid, addr, data) + })?; id_valid(notif_fd, id).map_err(NotifError::Ioctl)?; Ok(()) } @@ -2898,6 +2920,7 @@ struct ParkedChain { struct ParkedSet { tasks: tokio::task::JoinSet, callers: HashMap, + offload: Offload, } impl ParkedSet { @@ -2905,6 +2928,9 @@ impl ParkedSet { Self { tasks: tokio::task::JoinSet::new(), callers: HashMap::new(), + offload: Offload { + gate: super::gate::MemGate::default(), + }, } } @@ -3023,8 +3049,9 @@ async fn settle_chain( match action { NotifAction::Defer(deferred) => { let notif = state.notif; + let offload = parked.offload.clone(); parked.spawn(¬if, async move { - let action = deferred.run_until(state.deadline).await; + let action = deferred.run_until(&offload, state.deadline).await; Parked::Chain(Box::new(state), action) }); } @@ -3222,36 +3249,45 @@ async fn finish_notification( } let id = notif.id; let deadline = tokio::time::Instant::now() + PARK_TIMEOUT; + let offload = parked.offload.clone(); parked.spawn(¬if, async move { - Parked::Response(id, deferred.run_until(deadline).await) + Parked::Response(id, deferred.run_until(&offload, deadline).await) }); return; } // From the freeze to its release nothing may await: the tracer is this // thread, and the loop must not release any parked task meanwhile. - let mut exec_freeze = None; - if let (NotifAction::Continue, Some(judged)) = (&action, &exec_image) { - let (frozen_action, freeze) = freeze_exec(¬if, judged, ctx, fd, parked); - action = frozen_action; - exec_freeze = Some(freeze); - } + // Parked work keeps running, so its child-memory writes wait on the gate + // until the kernel has copied argv. + let gate = parked.offload.gate.clone(); + let send_result = { + let mut exec_freeze = None; + let mut _writes_held = None; + if let (NotifAction::Continue, Some(judged)) = (&action, &exec_image) { + _writes_held = Some(gate.exclusive()); + let (frozen_action, freeze) = freeze_exec(¬if, judged, ctx, fd, parked); + action = frozen_action; + exec_freeze = Some(freeze); + } - // Ignore error — child may have exited between recv and response. - let exec_continued = exec_freeze.is_some() && matches!(action, NotifAction::Continue); - let send_result = send_response(fd, notif.id, action); + // Ignore error — child may have exited between recv and response. + let exec_continued = exec_freeze.is_some() && matches!(action, NotifAction::Continue); + let send_result = send_response(fd, notif.id, action); - if let Some(freeze) = exec_freeze { - if exec_continued && send_result.is_ok() { - crate::freeze::release_after_exec(&freeze); - } else { - crate::freeze::detach_all(&freeze); + if let Some(freeze) = exec_freeze { + if exec_continued && send_result.is_ok() { + crate::freeze::release_after_exec(&freeze); + } else { + crate::freeze::detach_all(&freeze); + } + // Now that the response is out, the kernel wait holding any pending + // task (the vfork parent waiting on this very execve) can clear; + // reap the queued interrupts and detach. + crate::freeze::reap_pending(&freeze.pending_tids); } - // Now that the response is out, the kernel wait holding any pending - // task (the vfork parent waiting on this very execve) can clear; - // reap the queued interrupts and detach. - crate::freeze::reap_pending(&freeze.pending_tids); - } + send_result + }; if fork_counted && creation_trace.is_none() && send_result.is_err() { crate::resource::rollback_fork_count(&ctx.resource).await; @@ -3496,6 +3532,12 @@ pub(crate) async fn cleanup_pid(ctx: &super::ctx::SupervisorCtx, key: super::sta mod tests { use super::*; + fn test_offload() -> Offload { + Offload { + gate: super::super::gate::MemGate::default(), + } + } + #[test] fn openat_how_matches_what_openat2_accepts() { let o_path = (libc::O_PATH | libc::O_DIRECTORY) as u64; @@ -3858,7 +3900,7 @@ mod tests { }); let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1); assert!(matches!( - slow.run_until(deadline).await, + slow.run_until(&test_offload(), deadline).await, NotifAction::Errno(libc::EIO) )); } @@ -3871,7 +3913,7 @@ mod tests { }); let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1); assert!(matches!( - slow.run_until(deadline).await, + slow.run_until(&test_offload(), deadline).await, NotifAction::ReturnValue(7) )); } @@ -3884,7 +3926,7 @@ mod tests { }); let deadline = tokio::time::Instant::now() + std::time::Duration::from_millis(1); assert!(matches!( - slow.run_until(deadline).await, + slow.run_until(&test_offload(), deadline).await, NotifAction::ReturnValue(7) )); } @@ -3894,7 +3936,7 @@ mod tests { let nested = Deferred::new(async { NotifAction::defer(async { NotifAction::Continue }) }); let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1); assert!(matches!( - nested.run_until(deadline).await, + nested.run_until(&test_offload(), deadline).await, NotifAction::Errno(libc::EIO) )); } diff --git a/docs/extension-handlers.md b/docs/extension-handlers.md index c5dd1bbc..54a931d3 100644 --- a/docs/extension-handlers.md +++ b/docs/extension-handlers.md @@ -514,6 +514,9 @@ Contract: other result ends the chain. Either way the outcome then passes every check a handler's inline result would: pinned opens, fork accounting, the policy callback and the execve argv-safety freeze (see [issue #27][i27]). Deferring on `execve` or `fork` is therefore safe. +- **Writes wait for exec.** Child-memory writes from a deferred future or native closure are ordered + against the execve argv-safety freeze: an exec holds them off until the kernel has copied its + argv. Write from the deferred future or closure itself; a task you spawn from it is not ordered. - **Deadline.** All deferred futures of one notification share a 30 second deadline; past it the syscall fails with `EIO`. Native closures have none. - **No admission limit.** Each trapped task has at most one outstanding notification, so deferred From ce840691e3b6820ffc2974d0d8236efe0a9ed3fa Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 15:54:11 -0800 Subject: [PATCH 5/8] seccomp: keep the listener open while native work runs A native closure runs to completion even after its child is gone, so it can outlive the supervisor and still use the listener's raw fd number for child-memory access. Once the supervisor closed the listener that number could be reused by another sandbox's listener. Share the listener between the supervisor and the native closures it defers, so its fd stays open until the last closure returns. Signed-off-by: Cong Wang --- crates/sandlock-core/src/seccomp/notif.rs | 38 ++++++++++++++++------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index efa9872d..8518efd6 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -117,7 +117,16 @@ impl Deferred { let gate = offload.gate.clone(); match self.work { Work::Future(future) => gate.scope(future).await, - Work::Native(work) => super::native::run(Some(gate), work).await, + Work::Native(work) => { + // The closure may outlive the supervisor and still use the + // listener's fd number, which must not be reused meanwhile. + let listener = Arc::clone(&offload.listener); + let work = Box::new(move || { + let _listener = listener; + work() + }); + super::native::run(Some(gate), work).await + } } } @@ -143,10 +152,11 @@ impl Deferred { } /// What one sandbox's parked work shares: the gate ordering its child-memory -/// writes against exec. +/// writes against exec, and the listener its native closures may outlive. #[derive(Clone)] struct Offload { gate: super::gate::MemGate, + listener: Arc, } /// An owned operation whose side effects require callback approval. @@ -2924,12 +2934,13 @@ struct ParkedSet { } impl ParkedSet { - fn new() -> Self { + fn new(listener: Arc) -> Self { Self { tasks: tokio::task::JoinSet::new(), callers: HashMap::new(), offload: Offload { gate: super::gate::MemGate::default(), + listener, }, } } @@ -3360,14 +3371,16 @@ pub async fn supervisor( ) { // Register the notif fd with the Tokio IO driver so we can wait for // readiness via epoll instead of a dedicated blocking thread. - let async_fd = - match tokio::io::unix::AsyncFd::with_interest(notif_fd, tokio::io::Interest::READABLE) { - Ok(fd) => fd, - Err(err) => { - let _ = startup.send(Err(err)); - return; - } - }; + let async_fd = match tokio::io::unix::AsyncFd::with_interest( + Arc::new(notif_fd), + tokio::io::Interest::READABLE, + ) { + Ok(fd) => fd, + Err(err) => { + let _ = startup.send(Err(err)); + return; + } + }; let fd = async_fd.get_ref().as_raw_fd(); // Build the dispatch table once at startup. @@ -3393,7 +3406,7 @@ pub async fn supervisor( // still per-child pidfd readiness in `spawn_pid_watcher`. let gc = tokio::spawn(process_index_gc(Arc::clone(&ctx.processes))); - let mut parked = ParkedSet::new(); + let mut parked = ParkedSet::new(Arc::clone(async_fd.get_ref())); // Edge-triggered drain: each `readable().await` returns once per // epoll edge, then we drain the kernel queue via `probe_notif_fd` @@ -3535,6 +3548,7 @@ mod tests { fn test_offload() -> Offload { Offload { gate: super::super::gate::MemGate::default(), + listener: Arc::new(std::fs::File::open("/dev/null").unwrap().into()), } } From 606fea88ccc86350460822303fb72fdaaf245516 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 15:54:12 -0800 Subject: [PATCH 6/8] ffi: run every handler callback as native work A C callback was awaited on the notification loop unless the caller flagged it deferred, and a deferred callback made a terminal decision that skipped later handlers and was refused on fork and exec. Callers had to choose between stalling every trapped syscall and changing what their Continue meant. Defer every callback as native work, where Continue resumes the chain like any other deferral, and remove sandlock_handler_set_deferred. Like any native work a callback has no deadline, and it runs to completion even after its child is gone, but the caller's userdata may only be borrowed until the run returns, so each run holds a guard per handler: dropping it waits for running callbacks and releases the container, and callbacks still queued fail with EIO instead of reaching freed userdata. Python async handlers no longer need a flag, and a sync handler no longer stalls the supervisor. Signed-off-by: Cong Wang --- crates/sandlock-ffi/include/sandlock.h | 23 +- crates/sandlock-ffi/src/handler/abi.rs | 38 +--- crates/sandlock-ffi/src/handler/adapter.rs | 232 ++++++++++++++------- crates/sandlock-ffi/src/handler/run.rs | 1 + crates/sandlock-ffi/tests/handler_smoke.rs | 72 +++---- docs/python-handlers.md | 61 ++---- python/src/sandlock/_handler_ffi.py | 8 +- python/src/sandlock/_sdk.py | 3 - python/src/sandlock/handler.py | 24 +-- python/src/sandlock/sandbox.py | 15 -- python/tests/test_handler_smoke.py | 44 ++-- 11 files changed, 247 insertions(+), 274 deletions(-) diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index a172cadb..77cff780 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -1836,6 +1836,11 @@ void sandlock_action_set_kill(sandlock_action_out_t *out, int32_t sig, int32_t p * Allocate a handler container. `handler_fn` must be non-null; passing * `ud_drop = None` is legal when `ud` does not require cleanup. * + * Callbacks run on worker threads, concurrently and off the notification + * loop. A syscall waits for its callback however long it takes, and the + * run waits for every callback before returning, so one that never + * returns prevents the run from completing. + * * # Safety * `ud` is opaque to Rust — the caller guarantees that the pointer * remains valid until either (a) `sandlock_handler_free` is called or @@ -1852,24 +1857,6 @@ sandlock_handler_t *sandlock_handler_new(int32_t (*handler_fn)(void *ud, void (*ud_drop)(void *ud), uint32_t on_exception); -/** - * Mark a handler as deferred: the supervisor will run its callback off the - * notification loop so a slow callback does not block other trapped - * syscalls. Must be called before the handler is registered with a run. - * - * A deferred handler makes a *terminal* decision: deferral short-circuits - * the handler chain, so if a deferred handler would `Continue` and other - * user handlers are registered on the same syscall, those later handlers do - * not run. Builtins always run first regardless. Deferral is refused at - * dispatch time for syscalls whose Continue path needs the execve - * argv-safety freeze (`execve`/`execveat`) or fork creation-tracking. - * - * # Safety - * `h` must be a non-null pointer returned by `sandlock_handler_new` that has - * not yet been registered with a run or freed. - */ -void sandlock_handler_set_deferred(sandlock_handler_t *h, bool deferred); - /** * Free a handler container that has *not* been registered with a * sandbox. After successful registration the supervisor owns the diff --git a/crates/sandlock-ffi/src/handler/abi.rs b/crates/sandlock-ffi/src/handler/abi.rs index 53016875..c7ab4eac 100644 --- a/crates/sandlock-ffi/src/handler/abi.rs +++ b/crates/sandlock-ffi/src/handler/abi.rs @@ -458,18 +458,12 @@ pub struct sandlock_handler_t { pub(super) ud: *mut std::ffi::c_void, pub(super) ud_drop: Option, pub(super) on_exception: sandlock_exception_policy_t, - /// When true, the supervisor runs this handler's callback off the - /// notification loop (as a deferred future) instead of inline, so a - /// slow callback (network round-trip, blocking syscall) does not stall - /// every other trapped syscall. The container is an opaque box — C - /// never sees this field's layout — so adding it is not a C ABI break. - pub(super) deferred: bool, } // Safety: // -// `Send`: required so the supervisor can move the handler container into -// a `tokio::task::spawn_blocking` closure. The struct contains only +// `Send`: required so the supervisor can run the callback on a native +// worker thread. The struct contains only // pointers (function pointer + `void*` user-data) and a `#[repr(u32)]` // enum, all of which are `Send`-safe to move across threads. // @@ -504,6 +498,11 @@ impl Drop for sandlock_handler_t { /// Allocate a handler container. `handler_fn` must be non-null; passing /// `ud_drop = None` is legal when `ud` does not require cleanup. /// +/// Callbacks run on worker threads, concurrently and off the notification +/// loop. A syscall waits for its callback however long it takes, and the +/// run waits for every callback before returning, so one that never +/// returns prevents the run from completing. +/// /// # Safety /// `ud` is opaque to Rust — the caller guarantees that the pointer /// remains valid until either (a) `sandlock_handler_free` is called or @@ -550,33 +549,10 @@ pub unsafe extern "C" fn sandlock_handler_new( ud, ud_drop, on_exception, - deferred: false, }); Box::into_raw(h) } -/// Mark a handler as deferred: the supervisor will run its callback off the -/// notification loop so a slow callback does not block other trapped -/// syscalls. Must be called before the handler is registered with a run. -/// -/// A deferred handler makes a *terminal* decision: deferral short-circuits -/// the handler chain, so if a deferred handler would `Continue` and other -/// user handlers are registered on the same syscall, those later handlers do -/// not run. Builtins always run first regardless. Deferral is refused at -/// dispatch time for syscalls whose Continue path needs the execve -/// argv-safety freeze (`execve`/`execveat`) or fork creation-tracking. -/// -/// # Safety -/// `h` must be a non-null pointer returned by `sandlock_handler_new` that has -/// not yet been registered with a run or freed. -#[no_mangle] -pub unsafe extern "C" fn sandlock_handler_set_deferred(h: *mut sandlock_handler_t, deferred: bool) { - if h.is_null() { - return; - } - (*h).deferred = deferred; -} - /// Free a handler container that has *not* been registered with a /// sandbox. After successful registration the supervisor owns the /// handler; calling this on a registered handler is undefined behaviour diff --git a/crates/sandlock-ffi/src/handler/adapter.rs b/crates/sandlock-ffi/src/handler/adapter.rs index 5131e5cd..7cabd96f 100644 --- a/crates/sandlock-ffi/src/handler/adapter.rs +++ b/crates/sandlock-ffi/src/handler/adapter.rs @@ -7,6 +7,7 @@ use std::future::Future; use std::os::unix::io::FromRawFd; use std::pin::Pin; +use std::sync::{Arc, RwLock}; use sandlock_core::seccomp::dispatch::{Handler, HandlerCtx}; use sandlock_core::seccomp::notif::NotifAction; @@ -44,7 +45,23 @@ pub(crate) const UNSAFE_PGID: i32 = i32::MIN; /// `Handler`. Constructed when the supervisor accepts handlers passed /// through `sandlock_run_with_handlers`. pub struct FfiHandler { - inner: Box, + callback: Arc, + on_exception: sandlock_exception_policy_t, +} + +/// A callback can outlive the notification that started it, but not the +/// FFI run: the caller's userdata may be borrowed only until the run returns. +struct Callback(RwLock>>); + +/// Held for an FFI run. Dropping it waits for running callbacks, then +/// releases the container so queued callbacks find nothing to call. +pub(super) struct RunGuard(Arc); + +impl Drop for RunGuard { + fn drop(&mut self) { + let handler = self.0 .0.write().unwrap_or_else(|e| e.into_inner()).take(); + drop(handler); + } } impl FfiHandler { @@ -57,13 +74,19 @@ impl FfiHandler { /// supervisor owns the container. pub unsafe fn from_raw(raw: *mut sandlock_handler_t) -> Self { assert!(!raw.is_null(), "FfiHandler::from_raw on null pointer"); + let handler = Box::from_raw(raw); Self { - inner: Box::from_raw(raw), + on_exception: handler.on_exception, + callback: Arc::new(Callback(RwLock::new(Some(handler)))), } } + pub(super) fn run_guard(&self) -> RunGuard { + RunGuard(Arc::clone(&self.callback)) + } + fn exception_action(&self, child_pgid: i32) -> NotifAction { - match self.inner.on_exception { + match self.on_exception { sandlock_exception_policy_t::Kill => { if child_pgid == UNSAFE_PGID { // No safe pgid is available (nested PID namespace, @@ -88,23 +111,11 @@ impl FfiHandler { } } -/// `Send`-only wrapper around the C user-data pointer so it can travel -/// into `spawn_blocking`. Only the move (not sharing across threads) is -/// required; the deeper Send/Sync rationale for the underlying handler -/// container lives on `sandlock_handler_t`. -struct UdPtr(*mut std::ffi::c_void); -// Safety: ud is opaque to Rust; the spawn_blocking pipeline only moves -// (not shares) the wrapper. See `sandlock_handler_t` for the deeper -// Send/Sync rationale that justifies the underlying handler container. -unsafe impl Send for UdPtr {} - impl Handler for FfiHandler { fn handle<'a>( &'a self, cx: &'a HandlerCtx, ) -> Pin + Send + 'a>> { - // Capture the pieces we need by value so spawn_blocking can run - // the C callback on a worker thread without &self lifetime games. let notif_snap = crate::notif_repr::sandlock_notif_data_t::from(&cx.notif); let notif_fd = cx.notif_fd; let notif_id = cx.notif.id; @@ -156,82 +167,47 @@ impl Handler for FfiHandler { } } }; - let handler_fn = self.inner.handler_fn; - let ud = UdPtr(self.inner.ud); + let callback = Arc::clone(&self.callback); let on_exception_fallback = self.exception_action(child_pgid); - let deferred = self.inner.deferred; - // The whole callback (including its slow work) runs on a blocking - // worker. When `deferred` is set we hand this future to the - // supervisor as `NotifAction::Defer` so it runs off the notification - // loop; otherwise the supervisor awaits it inline as before. The - // future is fully owned (`'static`) either way — nothing borrows - // `self`/`cx`. - let run = async move { - let join = tokio::task::spawn_blocking(move || { - // Rust 2021 disjoint closure captures (RFC 2229) would - // otherwise capture `ud.0` (a bare `*mut c_void`, not - // `Send`) rather than the whole `UdPtr`. Binding `ud` to - // a fresh local at the top of the closure forces a - // whole-struct capture so the `Send` impl on `UdPtr` - // applies to the outer closure. - let ud = ud; - let UdPtr(ud_raw) = ud; - let mut mem = sandlock_mem_handle_t::new(notif_fd, notif_id, pid); - let mut out = sandlock_action_out_t::zeroed(); - let rc = match handler_fn { - Some(f) => std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - f(ud_raw, ¬if_snap, &mut mem, &mut out) - })), - None => Ok(-1), - }; - (rc, out) - }) - .await; - - let (rc_or_panic, out) = match join { - Ok(pair) => pair, - Err(_join_err) => return on_exception_fallback, + // A callback may wait on anything, so it never runs on the loop. + let action = NotifAction::native(move || { + let handler = callback.0.read().unwrap_or_else(|e| e.into_inner()); + let Some(handler) = handler.as_ref() else { + return NotifAction::Errno(libc::EIO); }; - - match rc_or_panic { + let mut mem = sandlock_mem_handle_t::new(notif_fd, notif_id, pid); + let mut out = sandlock_action_out_t::zeroed(); + let rc = match handler.handler_fn { + Some(f) => std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + f(handler.ud, ¬if_snap, &mut mem, &mut out) + })), + None => Ok(-1), + }; + match rc { Ok(0) => match translate_action(&out, child_pgid) { Some(action) => action, None => { // Action kind ended up Unset, unknown, or the - // reserved InjectFdSendTracked discriminant. - // Drain any inject-fd payload before falling - // back to the exception policy — otherwise the - // supervisor leaks the srcfd that was armed by - // the (failed) callback. + // reserved InjectFdSendTracked discriminant. Drain + // any inject-fd payload the callback armed before + // falling back, or its srcfd leaks. // SAFETY: `drain_pending_inject_fd` inspects - // `out.kind` itself before touching the union, - // and `out.kind` matches the union variant per - // the action setters' contract. + // `out.kind` itself before touching the union. unsafe { drain_pending_inject_fd(&out) }; on_exception_fallback } }, _ => { - // Either the callback returned a non-zero rc OR - // `catch_unwind` caught a panic. The callback may - // have armed an InjectFdSend{,Tracked} payload - // before failing; drain it so its srcfd doesn't - // leak in the supervisor. + // A non-zero rc or a caught panic may still have armed + // an inject-fd payload; drain it so its srcfd doesn't leak. // SAFETY: see the `Ok(0) -> None` branch above. unsafe { drain_pending_inject_fd(&out) }; on_exception_fallback } } - }; - - if deferred { - // Resolves immediately to Defer; the supervisor moves `run` onto - // a worker task and sends the response when it completes. - Box::pin(async move { NotifAction::defer(run) }) - } else { - Box::pin(run) - } + }); + Box::pin(async move { action }) } } @@ -358,3 +334,113 @@ fn translate_action(out: &sandlock_action_out_t, child_pgid: i32) -> Option>>, + release: Mutex>, + } + + extern "C-unwind" fn blocking_callback( + ud: *mut std::ffi::c_void, + _: *const crate::notif_repr::sandlock_notif_data_t, + _: *mut sandlock_mem_handle_t, + out: *mut sandlock_action_out_t, + ) -> i32 { + let gate = unsafe { &*ud.cast::() }; + if let Some(started) = gate.started.lock().unwrap().take() { + let _ = started.send(()); + } + let _ = gate.release.lock().unwrap().recv(); + unsafe { sandlock_action_set_return_value(out, 42) }; + 0 + } + + fn handler(gate: &Gate) -> FfiHandler { + let raw = unsafe { + sandlock_handler_new( + Some(blocking_callback), + (gate as *const Gate).cast_mut().cast(), + None, + sandlock_exception_policy_t::DenyEio as u32, + ) + }; + unsafe { FfiHandler::from_raw(raw) } + } + + fn context() -> HandlerCtx { + let notif = SeccompNotif { + id: 1, + pid: 0, + flags: 0, + data: SeccompData { + nr: libc::SYS_getpid as i32, + arch: 0, + instruction_pointer: 0, + args: [0; 6], + }, + }; + HandlerCtx::new(notif, -1) + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn run_guard_waits_for_an_abandoned_callback() { + let (started_tx, started_rx) = mpsc::channel(); + let (release_tx, release_rx) = mpsc::channel(); + let gate = Gate { + started: Mutex::new(Some(started_tx)), + release: Mutex::new(release_rx), + }; + let h = handler(&gate); + let guard = h.run_guard(); + let NotifAction::Defer(deferred) = h.handle(&context()).await else { + panic!("expected a deferral"); + }; + let waiter = tokio::spawn(deferred.run()); + started_rx.recv().unwrap(); + waiter.abort(); + let _ = waiter.await; + drop(h); + + let (done_tx, done_rx) = mpsc::channel(); + let drain = std::thread::spawn(move || { + drop(guard); + done_tx.send(()).unwrap(); + }); + let returned_early = done_rx + .recv_timeout(std::time::Duration::from_millis(50)) + .is_ok(); + release_tx.send(()).unwrap(); + drain.join().unwrap(); + assert!( + !returned_early, + "the run returned while userdata was in use" + ); + } + + #[tokio::test] + async fn callbacks_queued_past_the_run_do_not_touch_userdata() { + let (_release_tx, release_rx) = mpsc::channel(); + let gate = Gate { + started: Mutex::new(None), + release: Mutex::new(release_rx), + }; + let h = handler(&gate); + let NotifAction::Defer(deferred) = h.handle(&context()).await else { + panic!("expected a deferral"); + }; + drop(h.run_guard()); + assert!(matches!( + deferred.run().await, + NotifAction::Errno(libc::EIO) + )); + } +} diff --git a/crates/sandlock-ffi/src/handler/run.rs b/crates/sandlock-ffi/src/handler/run.rs index 0ed5a4e2..3a145655 100644 --- a/crates/sandlock-ffi/src/handler/run.rs +++ b/crates/sandlock-ffi/src/handler/run.rs @@ -116,6 +116,7 @@ fn block_on_run( // `crate::runtime` for why this is `current_thread`. This path is // reached from `extern "C-unwind"` entry points, so user callback // panics are intentionally allowed to propagate. + let _callbacks: Vec<_> = handlers.iter().map(|(_, h)| h.run_guard()).collect(); crate::runtime::with_runtime_unwind(|rt| { rt.block_on(async move { if interactive { diff --git a/crates/sandlock-ffi/tests/handler_smoke.rs b/crates/sandlock-ffi/tests/handler_smoke.rs index ee6498c4..7dfb94cf 100644 --- a/crates/sandlock-ffi/tests/handler_smoke.rs +++ b/crates/sandlock-ffi/tests/handler_smoke.rs @@ -280,6 +280,14 @@ use sandlock_core::seccomp::notif::NotifAction; use sandlock_core::{SeccompData, SeccompNotif}; use sandlock_ffi::handler::FfiHandler; +/// What the supervisor would answer once the handler's deferral resolves. +async fn resolve(h: &FfiHandler, cx: &HandlerCtx) -> NotifAction { + match h.handle(cx).await { + NotifAction::Defer(deferred) => deferred.run().await, + decided => decided, + } +} + fn fake_ctx() -> HandlerCtx { HandlerCtx::new( SeccompNotif { @@ -394,15 +402,12 @@ async fn ffi_handler_translates_return_value() { }; let h = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx(); - let action = h.handle(&cx).await; + let action = resolve(&h, &cx).await; assert!(matches!(action, NotifAction::ReturnValue(42))); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn deferred_ffi_handler_returns_defer_that_resolves_to_callback() { - // A handler flagged deferred must return NotifAction::Defer (so the - // supervisor runs it off-loop), and driving that future must yield the - // same action the C callback would have produced inline. +async fn ffi_handler_defers_its_callback_off_the_loop() { let raw = unsafe { sandlock_ffi::handler::sandlock_handler_new( Some(return_value_42), @@ -411,33 +416,14 @@ async fn deferred_ffi_handler_returns_defer_that_resolves_to_callback() { sandlock_exception_policy_t::Kill as u32, ) }; - unsafe { sandlock_ffi::handler::sandlock_handler_set_deferred(raw, true) }; let h = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx(); - let action = h.handle(&cx).await; - let deferred = match action { - NotifAction::Defer(d) => d, - other => panic!("deferred handler must return Defer, got {other:?}"), + let NotifAction::Defer(deferred) = h.handle(&cx).await else { + panic!("a C callback must not run on the notification loop"); }; assert!(matches!(deferred.run().await, NotifAction::ReturnValue(42))); } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn non_deferred_ffi_handler_stays_inline() { - // Without the flag, handle() resolves the action inline (no Defer). - let raw = unsafe { - sandlock_ffi::handler::sandlock_handler_new( - Some(return_value_42), - std::ptr::null_mut(), - None, - sandlock_exception_policy_t::Kill as u32, - ) - }; - let h = unsafe { FfiHandler::from_raw(raw) }; - let cx = fake_ctx(); - assert!(matches!(h.handle(&cx).await, NotifAction::ReturnValue(42))); -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn ffi_handler_applies_exception_policy_on_failure() { let raw = unsafe { @@ -450,7 +436,7 @@ async fn ffi_handler_applies_exception_policy_on_failure() { }; let h = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx(); - let action = h.handle(&cx).await; + let action = resolve(&h, &cx).await; assert!(matches!(action, NotifAction::Errno(e) if e == libc::EPERM)); } @@ -709,7 +695,7 @@ async fn ffi_handler_translates_continue() { // Safety: `raw` was just produced by `sandlock_handler_new` and is // non-null; ownership transfers into the adapter. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Continue)); } @@ -725,7 +711,7 @@ async fn ffi_handler_translates_errno() { }; // Safety: see `ffi_handler_translates_continue`. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Errno(e) if e == 13)); } @@ -741,7 +727,7 @@ async fn ffi_handler_translates_hold() { }; // Safety: see `ffi_handler_translates_continue`. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Hold)); } @@ -757,7 +743,7 @@ async fn ffi_handler_translates_kill_with_explicit_pgid() { }; // Safety: see `ffi_handler_translates_continue`. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!(matches!( action, NotifAction::KillGroup { sig, pgid } if sig == libc::SIGTERM && pgid == 1234 @@ -839,7 +825,7 @@ async fn ffi_handler_translates_kill_zero_pgid_substitutes_child_pgid() { }, -1, ); - let action = h.handle(&cx).await; + let action = resolve(&h, &cx).await; // Reap the child regardless of assertion outcome. let _ = child.kill(); @@ -911,7 +897,7 @@ async fn k1_pgid_resolution_rejects_pid_zero() { }; let handler = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx_with_pid(0); - let action = handler.handle(&cx).await; + let action = resolve(&handler, &cx).await; assert!( matches!(action, NotifAction::Continue), "expected exception-policy fallback (Continue) when no safe pgid available, got {action:?}", @@ -965,7 +951,7 @@ async fn k1_no_supervisor_signal_on_pid_zero_kill() { }; let handler = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx_with_pid(0); // pid=0 -> UNSAFE_PGID - let action = handler.handle(&cx).await; + let action = resolve(&handler, &cx).await; // The action must be Continue (exception-policy fallback), NOT a // Kill that send_response would forward to killpg(0). @@ -1019,7 +1005,7 @@ async fn k2_pgid_resolution_rejects_supervisor_pgid_match() { }; let handler = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx_with_pid(child_pid as u32); - let action = handler.handle(&cx).await; + let action = resolve(&handler, &cx).await; // Reap the child regardless of assertion outcome. let _ = child.kill(); @@ -1054,7 +1040,7 @@ async fn k3_pgid_resolution_falls_back_on_esrch() { }; let handler = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx_with_pid(dead_pid); - let action = handler.handle(&cx).await; + let action = resolve(&handler, &cx).await; assert!( matches!(action, NotifAction::Continue), "expected exception-policy fallback (Continue) on ESRCH, got {action:?}", @@ -1081,7 +1067,7 @@ async fn ffi_handler_kill_policy_on_callback_rc_nonzero() { // suicide, and the assertion below would not exercise the // Kill-path the test exists to cover. let (cx, mut child) = fake_ctx_with_isolated_child(); - let action = h.handle(&cx).await; + let action = resolve(&h, &cx).await; let _ = child.kill(); let _ = child.wait(); assert!(matches!(action, NotifAction::KillGroup { sig, .. } if sig == libc::SIGKILL)); @@ -1099,7 +1085,7 @@ async fn ffi_handler_continue_policy_on_callback_rc_nonzero() { }; // Safety: see `ffi_handler_translates_continue`. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Continue)); } @@ -1136,7 +1122,7 @@ async fn ffi_handler_recovers_from_callback_panic() { // Use an isolated child so the Kill exception policy is observable // (rationale identical to `ffi_handler_kill_policy_on_callback_rc_nonzero`). let (cx, mut child) = fake_ctx_with_isolated_child(); - let action = h.handle(&cx).await; + let action = resolve(&h, &cx).await; let _ = child.kill(); let _ = child.wait(); // The `catch_unwind` inside `spawn_blocking` swallows the panic and @@ -1167,7 +1153,7 @@ async fn ffi_handler_callback_returns_zero_but_never_sets_action_triggers_fallba }; // Safety: see `ffi_handler_translates_continue`. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; // `translate_action` returns `None` for `Unset`, which routes the // dispatcher onto the exception policy fallback. assert!(matches!(action, NotifAction::Errno(e) if e == libc::EPERM)); @@ -2028,7 +2014,7 @@ async fn a1_ffi_handler_drains_inject_fd_on_panic() { }; // SAFETY: `raw` was just produced and is non-null. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!( matches!(action, NotifAction::Errno(e) if e == libc::EPERM), "panic must route to the exception-policy fallback (Kill degraded to EPERM under UNSAFE_PGID), got {action:?}", @@ -2103,7 +2089,7 @@ async fn a2_ffi_handler_drains_inject_fd_tracked_discriminant() { }; // SAFETY: `raw` was just produced and is non-null. let h = unsafe { FfiHandler::from_raw(raw) }; - let action = h.handle(&fake_ctx()).await; + let action = resolve(&h, &fake_ctx()).await; assert!( matches!(action, NotifAction::Errno(e) if e == libc::EPERM), "unsupported tracked discriminant must route to the exception-policy fallback (Kill degraded to EPERM under UNSAFE_PGID), got {action:?}", @@ -2318,7 +2304,7 @@ async fn ffi_handler_deny_eio_policy_on_callback_rc_nonzero() { }; let h = unsafe { sandlock_ffi::handler::FfiHandler::from_raw(raw) }; let cx = fake_ctx(); - let action = h.handle(&cx).await; + let action = resolve(&h, &cx).await; assert!( matches!(action, NotifAction::Errno(e) if e == libc::EIO), "expected Errno(EIO), got {:?}", diff --git a/docs/python-handlers.md b/docs/python-handlers.md index fc46c336..2a4ff96a 100644 --- a/docs/python-handlers.md +++ b/docs/python-handlers.md @@ -22,9 +22,8 @@ sb.run_with_handlers( ## Core types - `Handler` — base class. Subclass and override `handle(ctx) -> NotifAction`. - Define `handle` as `async def` to run it off the supervisor loop so it can - `await` slow work without stalling other trapped syscalls; see - [Deferred handlers](#deferred-handlers). Set the class attribute + `handle` runs on a worker thread off the supervisor loop and may be + `async def`; see [Deferred handlers](#deferred-handlers). Set the class attribute `on_exception` (default `ExceptionPolicy.KILL`) to choose what the supervisor does when the handler errors. - `HandlerCtx` — frozen dataclass with the notification fields (`id`, `pid`, @@ -265,13 +264,10 @@ syscall list rather than against an empty dispatch table. - **GIL contention.** Each handler dispatch holds the GIL for the duration of `handle()`. The supervisor may dispatch handler - callbacks concurrently across different notifications, so design - `handle()` to be fast (sub-millisecond) and to protect any mutable - handler state with your own synchronization. High-frequency + callbacks concurrently across different notifications, so protect any + mutable handler state with your own synchronization. High-frequency interception (e.g. per-`SYS_openat` audit on a busy workload) will - serialize on the GIL and can stall the supervisor. For a `handle()` - that must do slow work (a network call, a blocking read), define it as - `async def` so it runs off the loop instead; see + serialize on the GIL and slow down the sandbox's trapped syscalls. See [Deferred handlers](#deferred-handlers). - **Interpreter finalization.** If `Py_FinalizeEx` runs while the @@ -294,12 +290,10 @@ syscall list rather than against an empty dispatch table. ## Deferred handlers -By default `handle()` runs synchronously inside the supervisor's -notification loop, so a slow callback stalls every other trapped syscall -until it returns. Define `handle` as `async def` to run it off that loop -instead: the coroutine is driven to completion on a worker thread, so it -can `await` slow work without blocking the supervisor. That is the only -change; you do not set any flag. +Every `handle()` runs on a worker thread, off the supervisor's notification +loop, so a slow callback delays only its own syscall. Define `handle` as +`async def` to `await` slow work; the coroutine is driven to completion on +that worker thread. You do not set any flag. ```python class FetchHandler(Handler): @@ -310,39 +304,30 @@ class FetchHandler(Handler): key = ctx.read_path() # read the path before the slow work if key is None: return NotifAction.continue_() - data = await self.backend.get(key) # slow network GET, awaited off-loop + data = await self.backend.get(key) # slow network GET # inject_bytes builds the sealed memfd and transfers fd ownership to # the supervisor (see "Inject synthetic content" above). return NotifAction.inject_bytes(data) ``` -Why it helps Python specifically: the coroutine runs on a worker thread, -and CPython releases the GIL while it `await`s I/O, so multiple async -handlers doing network work genuinely overlap while the supervisor loop -stays free. It does not parallelize CPU-bound Python work (the GIL still -serializes that); for that, push the hot path into a C extension that -releases the GIL. +CPython releases the GIL while a coroutine `await`s I/O, so multiple async +handlers doing network work overlap. It does not parallelize CPU-bound +Python work (the GIL still serializes that); for that, push the hot path +into a C extension that releases the GIL. `ctx` and its child-memory accessors (`read_cstr`, `read`, `write`) stay -valid for the whole coroutine, so you can read paths and write results -across `await` points. +valid for the whole call, so you can read paths and write results across +`await` points. Contract (enforced by the supervisor): -- **Terminal decision.** Deferral short-circuits the handler chain, so if - an async handler returns `continue_()` and other handlers are registered - on the same syscall, those later handlers do not run. Built-ins always - run first regardless. -- **Refused on `execve`/`execveat` and fork-creating syscalls.** The - response cannot be moved off-loop there without skipping argv-safety - work, so such a call is denied with `EPERM`. -- **Bounded.** Beyond an internal in-flight cap, further deferrals fail - fast with `EAGAIN` rather than queuing. - -Make `handle` async only when it must do slow work. For fast handlers -(audit counters, path checks) a synchronous `handle` runs inline at lower -latency. See -[`extension-handlers.md`](extension-handlers.md#deferred-handlers) for the +- **Same chain as any handler.** Returning `continue_()` hands the syscall + to the next handler registered on it, and the result still passes the + execve argv-safety checks and fork accounting. +- **No deadline.** A syscall waits for its handler to return, however + long that takes, and the run waits for every handler before completing. + +See [`extension-handlers.md`](extension-handlers.md#deferred-handlers) for the underlying Rust/C contract. ## Ownership rules diff --git a/python/src/sandlock/_handler_ffi.py b/python/src/sandlock/_handler_ffi.py index 42e83519..beda761f 100644 --- a/python/src/sandlock/_handler_ffi.py +++ b/python/src/sandlock/_handler_ffi.py @@ -129,11 +129,9 @@ def _trampoline_impl(ud, notif_ptr, mem_ptr, out_ptr) -> int: action = handler.handle(ctx) if inspect.iscoroutine(action): # `async def handle`: drive the coroutine to completion on - # this worker thread. Async handlers are always deferred (see - # sandbox._is_deferred_handler), so this runs off the - # supervisor loop and `await` does not stall other syscalls. - # The mem handle stays valid until the finally below, so - # child-memory access inside the coroutine works; awaits + # this worker thread, which is already off the supervisor + # loop. The mem handle stays valid until the finally below, + # so child-memory access inside the coroutine works; awaits # release the GIL, so concurrent async handlers overlap. action = asyncio.run(action) except BaseException: diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index ad3d6d5a..2dfcf3d4 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -655,9 +655,6 @@ class _SandlockHandlerRegistration(ctypes.Structure): _lib.sandlock_handler_free.restype = None _lib.sandlock_handler_free.argtypes = [_c_handler_p] -_lib.sandlock_handler_set_deferred.restype = None -_lib.sandlock_handler_set_deferred.argtypes = [_c_handler_p, ctypes.c_bool] - _lib.sandlock_run_with_handlers.restype = _c_result_p _lib.sandlock_run_with_handlers.argtypes = [ _c_policy_p, ctypes.c_char_p, diff --git a/python/src/sandlock/handler.py b/python/src/sandlock/handler.py index 371f7aaa..f2dd07ac 100644 --- a/python/src/sandlock/handler.py +++ b/python/src/sandlock/handler.py @@ -179,23 +179,13 @@ class Handler: guard it with your own synchronization — the wrapper does not serialize handler dispatch. - Promptness: ``handle()`` must return quickly. It runs synchronously - inside the supervisor's dispatch path while holding the GIL; a - handler that blocks (a long sleep, a blocking I/O call, an infinite - loop) stalls the supervisor and can wedge the entire run. - - Deferral: define ``handle`` as ``async def`` to run it off the - supervisor's notification loop, so a slow handler (a network round-trip, - a blocking call) does not stall every other trapped syscall. The - coroutine is driven to completion on a worker thread, and for I/O-bound - work multiple async handlers overlap (the GIL is released during - blocking I/O). Trade-offs: an async handler makes a *terminal* - decision: deferral short-circuits the handler chain, so if it returns - ``continue_()`` and other handlers are registered on the same syscall, - those later handlers do not run. Deferral is refused for - ``execve``/``execveat`` and fork-creating syscalls (the response cannot - be moved off-loop without skipping argv-safety work); such a call is - denied with EPERM. + Deferral: every ``handle()`` runs on a worker thread, off the + supervisor's notification loop, so a slow handler (a network + round-trip, a blocking call) delays only its own syscall. ``handle`` + may also be ``async def``; the coroutine is driven to completion on + that worker thread. Returning ``continue_()`` hands the syscall to the + next handler registered on it. The syscall waits for the handler + however long it takes, and the run waits for it before completing. """ on_exception: ExceptionPolicy = ExceptionPolicy.KILL diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index dd939566..944f2c62 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -8,7 +8,6 @@ from __future__ import annotations -import inspect import itertools import os import re @@ -954,10 +953,6 @@ def run_with_handlers( "sandlock_handler_new returned NULL for syscall " f"{syscall_nr}" ) - # An async `handle` runs off the supervisor loop. Flag the - # container before it is handed to the run. - if _is_deferred_handler(handler): - _lib.sandlock_handler_set_deferred(container, True) regs[i].syscall_nr = int(syscall_nr) regs[i].handler = container # Ownership now lives in regs[i]; clear the pending ref so @@ -1542,16 +1537,6 @@ def _encode(s: str) -> bytes: return result -def _is_deferred_handler(handler) -> bool: - """Whether a handler runs off the supervisor loop. - - True when its ``handle`` is an ``async def`` (a coroutine function): the - coroutine is driven to completion on a worker thread, which would block - the supervisor loop if run inline, so it must be deferred. - """ - return inspect.iscoroutinefunction(getattr(handler, "handle", None)) - - def _resolve_syscall(key) -> int: """Resolve a handler-registration key to a kernel syscall number. diff --git a/python/tests/test_handler_smoke.py b/python/tests/test_handler_smoke.py index d76f7f54..401ec83f 100644 --- a/python/tests/test_handler_smoke.py +++ b/python/tests/test_handler_smoke.py @@ -728,27 +728,10 @@ def test_run_with_handlers_rejects_non_handler(): # ---------------------------------------------------------------- -# Deferred (off-loop) handler dispatch. +# Async handler dispatch. # ---------------------------------------------------------------- -def test_is_deferred_handler_detects_async(): - """Registration treats a handler as deferred iff its `handle` is an - `async def`. A synchronous handler runs inline. This pins the helper.""" - from sandlock.sandbox import _is_deferred_handler - - class Sync(Handler): - def handle(self, ctx): - return NotifAction.continue_() - - class Async(Handler): - async def handle(self, ctx): - return NotifAction.continue_() - - assert _is_deferred_handler(Sync()) is False - assert _is_deferred_handler(Async()) is True - - def test_async_handle_delivers_errno_e2e(tmp_dir): """An `async def handle` that awaits and then returns errno(EACCES) must make the child's openat fail with EACCES (exit 7). Proves the trampoline @@ -793,13 +776,11 @@ async def handle(self, ctx): assert result.exit_code == 7, (result.exit_code, result.stderr) -def test_async_handle_short_circuits_chain_e2e(tmp_dir): - """An `async def handle` is auto-deferred, hence terminal: when it - continues, a later handler on the same syscall must NOT run. First - handler is async and always continues; second denies the probe. The - child opening successfully proves the async handler auto-deferred (a - non-deferred async handler driven inline would Continue, letting the - second handler deny -> child fails).""" +def test_async_handle_continue_resumes_chain_e2e(tmp_dir): + """An `async def handle` that continues hands the syscall to the next + handler on the same syscall, like a synchronous one. First handler is + async and always continues; second denies the probe, so the child's + open must fail with EACCES.""" if not os.path.exists(_SYSTEM_PYTHON): pytest.skip(f"{_SYSTEM_PYTHON} not available") @@ -829,17 +810,18 @@ def handle(self, ctx): sb = sandlock.Sandbox(fs_readable=[*_PYTHON_READABLE, str(tmp_dir)]) script = ( "import os, sys\n" - "fd = os.open(%r, os.O_RDONLY)\n" - "os.close(fd)\n" % probe_path + "try:\n" + " os.close(os.open(%r, os.O_RDONLY))\n" + "except OSError as e:\n" + " sys.stderr.write('errno=%%d' %% e.errno)\n" + " sys.exit(3)\n" % probe_path ) result = sb.run_with_handlers( cmd=[_SYSTEM_PYTHON, "-c", script], handlers=[("openat", _AsyncContinue()), ("openat", _DenyProbe())], ) - assert result.success, ( - "async handler must auto-defer (terminal), so the deny handler never " - f"runs; got {result}" - ) + assert result.exit_code == 3, (result.exit_code, result.stderr) + assert b"errno=%d" % _errno.EACCES in result.stderr, result.stderr # ---------------------------------------------------------------- From 783b15103f0795f09729d42ca4dfba7c639eb219 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 16:34:08 -0800 Subject: [PATCH 7/8] cow: copy up off the loop through a staged file A copy-up of a large lower file ran under the COW lock on the supervisor loop, so one big write stalled every syscall of the sandbox (#242), in COW and chroot mode alike. Handlers now plan on the loop as before, and only the copy leaves it: the dispatcher copies what a write needs into the upper as native work, then runs the write under the lock that found every copy in place. The branch handlers no longer copy at all, so there is one copy path and no copier ever holds the lock across file I/O. A copy streams into a staging file in the branch storage and is installed with RENAME_NOREPLACE, so a half-written upper is never visible. A second caller for the same file waits for the first and plans again, a path deleted while it was copied is not resurrected, and a quota resync keeps the reservation of a copy still in flight. A directory rename walks the lower tree off the loop and plans the whole tree at once, all or nothing, so a refusal still stages nothing. A caller interrupted while its copy ran restarts the syscall, so the write is applied only if the notification is still waiting. Without a killable wait an exec freeze is such an interruption, and a rename or link done anyway would make the restarted call fail. Fixes #242 Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 293 +++--- crates/sandlock-core/src/cow/dispatch.rs | 663 +++++++------ crates/sandlock-core/src/cow/seccomp.rs | 972 +++++++++++++------- crates/sandlock-core/src/sys/fs.rs | 59 ++ 4 files changed, 1280 insertions(+), 707 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index ac970d7b..6b41513c 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -593,6 +593,35 @@ fn read_and_resolve_existing( } /// Perform a libc syscall on a host path; return ReturnValue(0) or Errno. +fn rename_on_host(old_host: &Path, new_host: &Path) -> NotifAction { + let c_old = match path_cstr(old_host, libc::EINVAL) { + Ok(c) => c, + Err(a) => return a, + }; + let c_new = match path_cstr(new_host, libc::EINVAL) { + Ok(c) => c, + Err(a) => return a, + }; + if unsafe { libc::rename(c_old.as_ptr(), c_new.as_ptr()) } < 0 { + NotifAction::Errno(last_errno(libc::EIO)) + } else { + NotifAction::ReturnValue(0) + } +} + +fn chown_on_host(host: &Path, uid: u32, gid: u32, nofollow: bool) -> NotifAction { + exec_on_host( + |p| unsafe { + if nofollow { + libc::lchown(p, uid, gid) + } else { + libc::chown(p, uid, gid) + } + }, + host, + ) +} + fn exec_on_host(f: impl FnOnce(*const libc::c_char) -> libc::c_int, host: &Path) -> NotifAction { let c = match path_cstr(host, libc::EINVAL) { Ok(c) => c, @@ -661,64 +690,10 @@ pub(crate) async fn handle_chroot_open( // COW path: the branch picks the layer, the kernel resolves the rest. // A symlink in the image is followed inside that layer's root, never on // the host, because the supervisor holds no chroot of its own. + #[cfg(feature = "cow")] + if let Some(action) = chroot_cow_open(notif, notif_fd, cow_state, &host_path, flags, mode).await { - #[cfg(feature = "cow")] - let mut cs = cow_state.lock().await; - #[cfg(feature = "cow")] - if let Some(cow) = cs.branch.as_mut() { - let host_str = host_path.to_string_lossy(); - if cow.matches(&host_str) { - match cow.handle_open(&host_str, flags) { - Ok(Some(real_path)) => { - let upper_root = cow.upper_dir().to_path_buf(); - let workdir_root = cow.workdir().to_path_buf(); - drop(cs); - let fd = match crate::cow::dispatch::open_confined( - &upper_root, - &workdir_root, - &real_path, - flags_i, - mode, - ) { - Ok(fd) => fd, - Err(errno) => return NotifAction::Errno(errno), - }; - let newfd_flags = if flags & libc::O_CLOEXEC as u64 != 0 { - libc::O_CLOEXEC as u32 - } else { - 0 - }; - let owned = unsafe { OwnedFd::from_raw_fd(fd) }; - return NotifAction::InjectFdSend { - srcfd: owned, - newfd_flags, - }; - } - Ok(None) => { - // Fall through to openat2_in_root below. This keeps - // directory opens and other non-COW / non-whiteout - // cases confined to the chroot instead of executing the - // original host syscall. (Whiteouts are handled by the - // BranchError::Deleted arm below, not here.) - } - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC); - } - Err(crate::error::BranchError::Exists) => { - return NotifAction::Errno(libc::EEXIST); - } - Err(crate::error::BranchError::Deleted) => { - // Whiteout read-open: the lower file still physically - // exists, but the branch deleted it. Return ENOENT - // rather than leaking pre-delete bytes — parity with the - // async cow open path (cow/dispatch.rs) and - // stat/statx/access/readlink. - return NotifAction::Errno(libc::ENOENT); - } - Err(_) => return NotifAction::Errno(libc::EIO), - } - } - } + return action; } // Resolve the path to an fd to hand the child: either a freshly opened tree @@ -735,6 +710,89 @@ pub(crate) async fn handle_chroot_open( } } +/// The answer for an open the COW branch owns, or `None` to open it in the +/// namespace. +#[cfg(feature = "cow")] +async fn chroot_cow_open( + notif: &SeccompNotif, + notif_fd: RawFd, + cow_state: &Arc>, + host_path: &Path, + flags: u64, + mode: u32, +) -> Option { + use crate::cow::dispatch::{Caller, CowOpen, Layers}; + + let host_str = host_path.to_string_lossy().into_owned(); + let mut cs = cow_state.lock().await; + let cow = cs.branch.as_mut()?; + if !cow.matches(&host_str) { + return None; + } + if flags & libc::O_DIRECTORY as u64 != 0 { + // The namespace open sees every layer but a whiteout: the lower + // directory survives one, where the merged answer is ENOENT. + let rel = cow.safe_rel(&host_str)?; + return cow + .is_deleted(&rel) + .then_some(NotifAction::Errno(libc::ENOENT)); + } + let open = CowOpen { + caller: Caller::of(notif, notif_fd), + layers: Layers::of(cow), + path: host_str, + flags, + open_flags: flags as i32, + mode, + may_continue: false, + }; + open.run(cow_state, cow) +} + +/// A branch handler's answer in chroot mode, where `host` is the syscall +/// done on the host path for what the branch leaves to the kernel. +#[cfg(feature = "cow")] +fn branch_answer( + r: Result, + host: impl FnOnce() -> NotifAction, +) -> NotifAction { + match r { + Ok(true) => NotifAction::ReturnValue(0), + Ok(false) | Err(crate::error::BranchError::NotOwned) => host(), + Err(e) => NotifAction::Errno(crate::cow::seccomp::branch_errno(e)), + } +} + +/// Answer a chroot write the branch owns: copy up what it needs off the +/// loop, then run `apply`, with `host` for what the branch leaves to the +/// kernel. +#[cfg(feature = "cow")] +fn chroot_cow_write( + cow_state: &Arc>, + caller: crate::cow::dispatch::Caller, + cow: &mut crate::cow::seccomp::SeccompCowBranch, + path: String, + host: impl Fn() -> NotifAction + Clone + Send + 'static, + apply: impl FnOnce( + &mut crate::cow::seccomp::SeccompCowBranch, + &str, + ) -> Result + + Send + + 'static, +) -> NotifAction { + use crate::cow::seccomp::CopyUp; + let need = cow.safe_rel(&path).map_or(CopyUp::Nothing, CopyUp::Entry); + let refuse_host = host.clone(); + crate::cow::dispatch::after_copy_up( + cow_state, + caller, + cow, + need, + move |e| branch_answer(Err(e), &refuse_host), + move |cow| branch_answer(apply(cow, &path), host), + ) +} + /// Open `virtual_path` within the child's virtual namespace, returning an fd to /// inject into the child. /// @@ -1480,30 +1538,35 @@ pub(crate) async fn handle_chroot_write( let mut cs = cow_state.lock().await; #[cfg(feature = "cow")] if let Some(cow) = cs.branch.as_mut() { - let old_str = old_host.to_string_lossy(); + let old_str = old_host.to_string_lossy().into_owned(); if cow.matches(&old_str) { - match cow.handle_rename(&old_str, &new_host.to_string_lossy()) { - Ok(true) => return NotifAction::ReturnValue(0), - Err(errno) => return NotifAction::Errno(errno), - Ok(false) => {} + let new_str = new_host.to_string_lossy().into_owned(); + // Refused before the copy, so a refused rename stages nothing. + if let Err(errno) = cow.check_rename(&old_str, &new_str) { + return NotifAction::Errno(errno); } + let need = cow + .safe_rel(&old_str) + .map_or(crate::cow::seccomp::CopyUp::Nothing, |r| { + cow.rename_copy_up(r) + }); + return crate::cow::dispatch::after_copy_up( + cow_state, + crate::cow::dispatch::Caller::of(notif, notif_fd), + cow, + need, + |e| NotifAction::Errno(crate::cow::seccomp::branch_errno(e)), + move |cow| match cow.handle_rename(&old_str, &new_str) { + Ok(true) => NotifAction::ReturnValue(0), + Err(errno) => NotifAction::Errno(errno), + Ok(false) => rename_on_host(&old_host, &new_host), + }, + ); } } } - let c_old = match path_cstr(&old_host, libc::EINVAL) { - Ok(c) => c, - Err(a) => return a, - }; - let c_new = match path_cstr(&new_host, libc::EINVAL) { - Ok(c) => c, - Err(a) => return a, - }; - return if unsafe { libc::rename(c_old.as_ptr(), c_new.as_ptr()) } < 0 { - NotifAction::Errno(last_errno(libc::EIO)) - } else { - NotifAction::ReturnValue(0) - }; + return rename_on_host(&old_host, &new_host); } if nr == libc::SYS_symlinkat { @@ -1637,7 +1700,20 @@ pub(crate) async fn handle_chroot_write( return NotifAction::Errno(libc::EXDEV); } if cow.matches(&new_s) { - return crate::cow::result::link_result(cow.handle_link(&old_s, &new_s)); + let (old_s, new_s) = (old_s.into_owned(), new_s.into_owned()); + let need = cow + .safe_rel(&old_s) + .map_or(crate::cow::seccomp::CopyUp::Nothing, |r| { + cow.link_copy_up(r) + }); + return crate::cow::dispatch::after_copy_up( + cow_state, + crate::cow::dispatch::Caller::of(notif, notif_fd), + cow, + need, + |e| crate::cow::result::link_result(Err(e)), + move |cow| crate::cow::result::link_result(cow.handle_link(&old_s, &new_s)), + ); } } } @@ -1702,15 +1778,17 @@ pub(crate) async fn handle_chroot_write( let mut cs = cow_state.lock().await; #[cfg(feature = "cow")] if let Some(cow) = cs.branch.as_mut() { - let s = host_path.to_string_lossy(); + let s = host_path.to_string_lossy().into_owned(); if cow.matches(&s) { - match cow.handle_chmod(&s, mode) { - Ok(true) => return NotifAction::ReturnValue(0), - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC) - } - _ => {} - } + let host = host_path.clone(); + return chroot_cow_write( + cow_state, + crate::cow::dispatch::Caller::of(notif, notif_fd), + cow, + s, + move || exec_on_host(|p| unsafe { libc::chmod(p, mode) }, &host), + move |cow, s| cow.handle_chmod(s, mode), + ); } } } @@ -1739,28 +1817,21 @@ pub(crate) async fn handle_chroot_write( let mut cs = cow_state.lock().await; #[cfg(feature = "cow")] if let Some(cow) = cs.branch.as_mut() { - let s = host_path.to_string_lossy(); + let s = host_path.to_string_lossy().into_owned(); if cow.matches(&s) { - match cow.handle_chown(&s, uid, gid) { - Ok(true) => return NotifAction::ReturnValue(0), - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC) - } - _ => {} - } + let host = host_path.clone(); + return chroot_cow_write( + cow_state, + crate::cow::dispatch::Caller::of(notif, notif_fd), + cow, + s, + move || chown_on_host(&host, uid, gid, nofollow), + move |cow, s| cow.handle_chown(s, uid, gid), + ); } } } - return exec_on_host( - |p| unsafe { - if nofollow { - libc::lchown(p, uid, gid) - } else { - libc::chown(p, uid, gid) - } - }, - &host_path, - ); + return chown_on_host(&host_path, uid, gid, nofollow); } if nr == libc::SYS_truncate { @@ -1782,15 +1853,17 @@ pub(crate) async fn handle_chroot_write( let mut cs = cow_state.lock().await; #[cfg(feature = "cow")] if let Some(cow) = cs.branch.as_mut() { - let s = host_path.to_string_lossy(); + let s = host_path.to_string_lossy().into_owned(); if cow.matches(&s) { - match cow.handle_truncate(&s, length) { - Ok(true) => return NotifAction::ReturnValue(0), - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC) - } - _ => {} - } + let host = host_path.clone(); + return chroot_cow_write( + cow_state, + crate::cow::dispatch::Caller::of(notif, notif_fd), + cow, + s, + move || exec_on_host(|p| unsafe { libc::truncate(p, length) }, &host), + move |cow, s| cow.handle_truncate(s, length), + ); } } } diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index b65a789e..d8f71fec 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -33,10 +33,11 @@ use tokio::sync::Mutex as AsyncMutex; use crate::arch; use crate::cow::result::link_result; -use crate::cow::seccomp::{branch_errno, SeccompCowBranch}; +use crate::cow::seccomp::{branch_errno, CopyUp, CowCopyPlan, CowOpenPlan, SeccompCowBranch}; +use crate::error::BranchError; use crate::procfs::{build_dirent64, DT_DIR, DT_LNK, DT_REG}; use crate::seccomp::notif::{ - decode_open_args, read_child_mem, write_child_mem, write_child_mem_force, NotifAction, + decode_open_args, id_valid, read_child_mem, write_child_mem, write_child_mem_force, NotifAction, }; use crate::seccomp::state::{CowState, PerProcessState, ProcessIndex}; use crate::sys::structs::SeccompNotif; @@ -197,6 +198,254 @@ pub(crate) fn open_confined( crate::sys::fs::openat2_in_root(root, &rel, flags, mode) } +/// The branch's directories, taken under the lock for work done without it. +#[derive(Clone)] +pub(crate) struct Layers { + upper: PathBuf, + workdir: PathBuf, + staging: PathBuf, +} + +impl Layers { + pub(crate) fn of(cow: &SeccompCowBranch) -> Self { + Self { + upper: cow.upper_dir().to_path_buf(), + workdir: cow.workdir().to_path_buf(), + staging: cow.staging_dir().to_path_buf(), + } + } +} + +/// The trapped syscall a copy-up is made for. +#[derive(Clone, Copy)] +pub(crate) struct Caller { + fd: RawFd, + id: u64, +} + +impl Caller { + pub(crate) fn of(notif: &SeccompNotif, fd: RawFd) -> Self { + Self { fd, id: notif.id } + } + + fn waiting(self) -> bool { + id_valid(self.fd, self.id).is_ok() + } +} + +/// Run `apply` under the branch lock once `need` is in the upper. Only the +/// copy is slow, so only it leaves the loop, and `apply` runs under the lock +/// that found every copy in place. +pub(crate) fn after_copy_up( + cow_state: &Arc>, + caller: Caller, + cow: &mut SeccompCowBranch, + need: CopyUp, + refuse: impl Fn(BranchError) -> NotifAction + Send + 'static, + apply: impl FnOnce(&mut SeccompCowBranch) -> NotifAction + Send + 'static, +) -> NotifAction { + let plans = match &need { + CopyUp::Nothing => return apply(cow), + CopyUp::Entry(rel) => match cow.prepare_copies(std::slice::from_ref(rel)) { + Ok(plans) if plans.is_empty() => return apply(cow), + Ok(plans) => plans, + Err(e) => return refuse(e), + }, + // Walking the lower tree is I/O too. + CopyUp::Tree(_) => Vec::new(), + }; + copy_up_off_loop( + cow_state, + caller, + Layers::of(cow), + need, + plans, + refuse, + apply, + ) +} + +/// `after_copy_up` for a caller holding `plans` it already made for `need`. +pub(crate) fn copy_up_off_loop( + cow_state: &Arc>, + caller: Caller, + layers: Layers, + need: CopyUp, + mut plans: Vec<(String, CowCopyPlan)>, + refuse: impl Fn(BranchError) -> NotifAction + Send + 'static, + apply: impl FnOnce(&mut SeccompCowBranch) -> NotifAction + Send + 'static, +) -> NotifAction { + let cow_state = Arc::clone(cow_state); + NotifAction::native(move || { + let rels = match need { + CopyUp::Nothing => Vec::new(), + CopyUp::Entry(rel) => vec![rel], + CopyUp::Tree(root) => match SeccompCowBranch::lower_tree(&layers.workdir, &root) { + Ok(tree) => tree, + Err(errno) => return NotifAction::Errno(errno), + }, + }; + loop { + if let Err(errno) = run_copies(&cow_state, &layers, plans) { + return NotifAction::Errno(errno); + } + let mut st = cow_state.blocking_lock(); + let Some(cow) = st.branch.as_mut() else { + return NotifAction::Errno(libc::EIO); + }; + plans = match cow.prepare_copies(&rels) { + Ok(plans) => plans, + Err(e) => return refuse(e), + }; + // A caller interrupted meanwhile (a signal, or an exec freeze + // without a killable wait) restarts its syscall, which must not + // find the write already done. + if plans.is_empty() { + return match caller.waiting() { + true => apply(cow), + false => NotifAction::Errno(libc::EINTR), + }; + } + } + }) +} + +/// Blocks. Copies first, waits after: a caller that waits holds no ticket, +/// so two of them never wait on each other. A failed copy fails the syscall, +/// since continuing would reach the lower file. +fn run_copies( + cow_state: &Mutex, + layers: &Layers, + plans: Vec<(String, CowCopyPlan)>, +) -> Result<(), i32> { + let (copies, waits): (Vec<_>, Vec<_>) = plans + .into_iter() + .partition(|(_, plan)| matches!(plan, CowCopyPlan::NeedsCopy { .. })); + let mut copies = copies.into_iter(); + while let Some((rel, plan)) = copies.next() { + let CowCopyPlan::NeedsCopy { + file_size, ticket, .. + } = plan + else { + continue; + }; + let staged = SeccompCowBranch::stage_copy(&layers.workdir, &layers.staging, &rel); + let mut st = cow_state.blocking_lock(); + let cow = st.branch.as_mut().ok_or(libc::EIO)?; + if let Err(e) = cow.finish_copy(&rel, file_size, ticket, staged) { + cow.abandon_copies(copies); + return Err(e.raw_os_error().unwrap_or(libc::EIO)); + } + } + for (_, plan) in waits { + if let CowCopyPlan::InFlight(done) = plan { + done.wait(); + } + } + Ok(()) +} + +/// Where an open plan leads. +pub(crate) enum OpenStep { + /// Outside the branch's say: the caller opens it the way it would anyway. + Skip, + Open(PathBuf), + Copy(String, CowCopyPlan), + Answer(NotifAction), +} + +impl OpenStep { + pub(crate) fn of(plan: Result) -> Self { + match plan { + Ok(CowOpenPlan::Skip) | Err(BranchError::NotOwned) => OpenStep::Skip, + // Deleted in this branch (whiteout): the lower file still exists, so + // opening it would read its pre-delete content. Return ENOENT, + // matching the stat/access handlers. + Ok(CowOpenPlan::Deleted) => OpenStep::Answer(NotifAction::Errno(libc::ENOENT)), + Ok(CowOpenPlan::Resolved(p)) | Ok(CowOpenPlan::UpperReady { upper: p }) => { + OpenStep::Open(p) + } + Ok(CowOpenPlan::Copy { rel_path, plan }) => OpenStep::Copy(rel_path, plan), + Err(e) => OpenStep::Answer(NotifAction::Errno(branch_errno(e))), + } + } +} + +/// An open the branch may redirect. +pub(crate) struct CowOpen { + pub(crate) caller: Caller, + pub(crate) layers: Layers, + pub(crate) path: String, + pub(crate) flags: u64, + pub(crate) open_flags: i32, + pub(crate) mode: u32, + /// Chroot mode has no kernel open to fall back to after a copy. + pub(crate) may_continue: bool, +} + +impl CowOpen { + /// The answer for this open, or `None` when the branch leaves it alone. + pub(crate) fn run( + self, + cow_state: &Arc>, + cow: &mut SeccompCowBranch, + ) -> Option { + Some( + match OpenStep::of(cow.prepare_open(&self.path, self.flags)) { + OpenStep::Skip => return None, + OpenStep::Answer(action) => action, + OpenStep::Open(p) => self.inject(&p), + OpenStep::Copy(rel, plan) => copy_up_off_loop( + cow_state, + self.caller, + self.layers.clone(), + CopyUp::Entry(rel.clone()), + vec![(rel, plan)], + |e| NotifAction::Errno(branch_errno(e)), + move |cow| self.reopen(cow), + ), + }, + ) + } + + fn reopen(self, cow: &mut SeccompCowBranch) -> NotifAction { + match OpenStep::of(cow.prepare_open(&self.path, self.flags)) { + OpenStep::Open(p) => self.inject(&p), + OpenStep::Answer(action) => action, + OpenStep::Skip if self.may_continue => NotifAction::Continue, + OpenStep::Skip => NotifAction::Errno(libc::EIO), + OpenStep::Copy(rel, plan) => { + cow.abandon_copies([(rel, plan)]); + NotifAction::Errno(libc::EIO) + } + } + } + + fn inject(&self, real_path: &Path) -> NotifAction { + let fd = match open_confined( + &self.layers.upper, + &self.layers.workdir, + real_path, + self.open_flags, + self.mode, + ) { + Ok(fd) => fd, + // The kernel cannot see the upper layer, so its answer would be wrong. + Err(errno) => return NotifAction::Errno(errno), + }; + // Preserve O_CLOEXEC from the original openat flags. + let newfd_flags = if self.flags & libc::O_CLOEXEC as u64 != 0 { + libc::O_CLOEXEC as u32 + } else { + 0 + }; + NotifAction::InjectFdSend { + srcfd: unsafe { OwnedFd::from_raw_fd(fd) }, + newfd_flags, + } + } +} + /// Handle open/openat/openat2 under workdir: redirect to COW upper/lower. pub(crate) async fn handle_cow_open( notif: &SeccompNotif, @@ -204,8 +453,6 @@ pub(crate) async fn handle_cow_open( processes: &Arc, notif_fd: RawFd, ) -> NotifAction { - use crate::cow::seccomp::CowOpenPlan; - let (dirfd, path_ptr, flags, mode) = match decode_open_args(notif, notif_fd) { Some(a) => (a.dirfd, a.path_ptr, a.flags, a.mode), None => return NotifAction::Continue, @@ -222,121 +469,42 @@ pub(crate) async fn handle_cow_open( }; let mut path = resolve_at_path_with_virtual(notif, dirfd, &rel_path, virtual_cwd.as_deref()); - // Phase 1: determine plan under lock (no heavy I/O) - let (plan, upper_root, workdir_root) = { - let mut st = cow_state.lock().await; - let cow = match st.branch.as_mut() { - Some(c) => c, - None => return NotifAction::Continue, - }; - let upper_root = cow.upper_dir().to_path_buf(); - let workdir_root = cow.workdir().to_path_buf(); - - path = map_cow_upper_path(cow, &path); - if !cow.matches(&path) { - return NotifAction::Continue; - } - - // Read-only opens don't need interception unless the file was - // modified or deleted in the COW layer. - const WRITE_FLAGS: u64 = - (libc::O_WRONLY | libc::O_RDWR | libc::O_CREAT | libc::O_TRUNC | libc::O_APPEND) as u64; - let is_write = flags & WRITE_FLAGS != 0; - if !is_write && !cow.needs_read_intercept(&path) { - return NotifAction::Continue; - } - - let plan = match cow.prepare_open(&path, flags) { - Ok(plan) => plan, - Err(crate::error::BranchError::NotOwned) => return NotifAction::Continue, - Err(e) => return NotifAction::Errno(branch_errno(e)), - }; - (plan, upper_root, workdir_root) - }; - // Lock is released here - - // Phase 2: execute I/O plan without holding the lock - let real_path = match plan { - CowOpenPlan::Skip => return NotifAction::Continue, - // Deleted in this branch (whiteout): the lower file still exists, so - // Continue would read its pre-delete content. Return ENOENT, matching - // the stat/access handlers. - CowOpenPlan::Deleted => return NotifAction::Errno(libc::ENOENT), - CowOpenPlan::Resolved(p) | CowOpenPlan::UpperReady { upper: p } => p, - CowOpenPlan::NeedsCopy { - upper, - lower: _lower, - file_size, - rel_path, - } => { - // Do the potentially-expensive copy on a blocking thread - let root = workdir_root.clone(); - let uroot = upper_root.clone(); - let rel = rel_path.clone(); - let copy_result = tokio::task::spawn_blocking(move || { - crate::cow::seccomp::SeccompCowBranch::execute_copy(&root, &uroot, &rel) - }) - .await; - - match copy_result { - Ok(Ok(digest)) => { - if let Some(d) = digest { - let mut st = cow_state.lock().await; - if let Some(cow) = st.branch.as_mut() { - cow.record_digest(&rel_path, d); - } - } - upper - } - Ok(Err(_)) | Err(_) => { - // Continuing would let the kernel open the lower file - // itself, so a write would land outside the branch. - let mut st = cow_state.lock().await; - if let Some(cow) = st.branch.as_mut() { - cow.rollback_copy(file_size); - } - let errno = match copy_result { - Ok(Err(e)) => e.raw_os_error().unwrap_or(libc::EIO), - _ => libc::EIO, - }; - return NotifAction::Errno(errno); - } - } - } + let mut st = cow_state.lock().await; + let cow = match st.branch.as_mut() { + Some(c) => c, + None => return NotifAction::Continue, }; - // Phase 3: open the resolved path and inject fd - // Strip O_EXCL — the COW layer already verified exclusivity. Keeping it - // would fail because we may have just copied the file into upper. - let open_flags = (flags & !(libc::O_EXCL as u64)) as i32; - // Honor the child's requested creation mode (masked to permission bits). - // Hardcoding 0o666 dropped the execute bits, so a binary copied into the - // workdir (e.g. `cp /bin/echo m`) landed in upper non-executable and - // `./m` failed with EACCES. The kernel ignores mode unless O_CREAT is set. - let create_mode = (mode & 0o7777) as libc::c_uint; - let fd = match open_confined( - &upper_root, - &workdir_root, - &real_path, - open_flags, - create_mode, - ) { - Ok(fd) => fd, - // The kernel cannot see the upper layer, so its answer would be wrong. - Err(errno) => return NotifAction::Errno(errno), - }; + path = map_cow_upper_path(cow, &path); + if !cow.matches(&path) { + return NotifAction::Continue; + } - // Preserve O_CLOEXEC from the original openat flags. - let newfd_flags = if flags & libc::O_CLOEXEC as u64 != 0 { - libc::O_CLOEXEC as u32 - } else { - 0 - }; - let owned = unsafe { OwnedFd::from_raw_fd(fd) }; - NotifAction::InjectFdSend { - srcfd: owned, - newfd_flags, + // Read-only opens don't need interception unless the file was + // modified or deleted in the COW layer. + const WRITE_FLAGS: u64 = + (libc::O_WRONLY | libc::O_RDWR | libc::O_CREAT | libc::O_TRUNC | libc::O_APPEND) as u64; + let is_write = flags & WRITE_FLAGS != 0; + if !is_write && !cow.needs_read_intercept(&path) { + return NotifAction::Continue; } + + let open = CowOpen { + caller: Caller::of(notif, notif_fd), + layers: Layers::of(cow), + path, + flags, + // Strip O_EXCL — the COW layer already verified exclusivity. Keeping it + // would fail because we may have just copied the file into upper. + open_flags: (flags & !(libc::O_EXCL as u64)) as i32, + // Honor the child's requested creation mode (masked to permission bits). + // Hardcoding 0o666 dropped the execute bits, so a binary copied into the + // workdir (e.g. `cp /bin/echo m`) landed in upper non-executable and + // `./m` failed with EACCES. The kernel ignores mode unless O_CREAT is set. + mode: (mode & 0o7777) as u32, + may_continue: true, + }; + open.run(cow_state, cow).unwrap_or(NotifAction::Continue) } // ============================================================ @@ -568,16 +736,13 @@ fn parse_cow_write( } /// Map a BranchError result to a NotifAction. -fn cow_result(r: Result) -> NotifAction { +fn cow_result(r: Result) -> NotifAction { match r { Ok(true) => NotifAction::ReturnValue(0), - Err(crate::error::BranchError::QuotaExceeded) => NotifAction::Errno(libc::ENOSPC), - Err(crate::error::BranchError::Denied) => NotifAction::Errno(libc::EPERM), - // Whiteouted source: Continue would let the kernel act on the lower - // entry, which still exists with its pre-delete content. - Err(crate::error::BranchError::Deleted) => NotifAction::Errno(libc::ENOENT), - Err(crate::error::BranchError::NotOwned) => NotifAction::Continue, - _ => NotifAction::Continue, + Ok(false) | Err(BranchError::NotOwned) => NotifAction::Continue, + // Continue would let the kernel act on the lower entry, which the + // branch promised to leave alone. + Err(e) => NotifAction::Errno(branch_errno(e)), } } @@ -590,76 +755,38 @@ fn unlink_result(r: Result) -> NotifAction { } } -/// Determine which relative path (if any) needs a COW copy for this operation. -/// Returns `(match_path, copy_rel)` where match_path is checked against -/// `cow.matches()` and copy_rel is the relative path to pre-copy. -fn cow_copy_rel<'a>( - op: &'a CowWriteOp, - cow: &crate::cow::seccomp::SeccompCowBranch, -) -> Option<(&'a str, String)> { - let (match_path, copy_path) = match op { - // These ops call ensure_cow_copy internally — pre-copy the target - CowWriteOp::Chmod { ref path, .. } - | CowWriteOp::Chown { ref path, .. } - | CowWriteOp::Truncate { ref path, .. } => (path.as_str(), path.as_str()), - CowWriteOp::Rename { ref old_path, .. } => (old_path.as_str(), old_path.as_str()), - CowWriteOp::Link { - ref old_path, - ref new_path, - .. - } => (new_path.as_str(), old_path.as_str()), - // These ops don't need a pre-copy - _ => return None, - }; - if !cow.matches(match_path) { - return None; +impl CowWriteOp { + /// What the branch must copy up before this write can run. + fn copy_up(&self, cow: &SeccompCowBranch) -> CopyUp { + let entry = |path: &str| cow.matches(path).then(|| cow.safe_rel(path)).flatten(); + let need = match self { + CowWriteOp::Chmod { path, .. } + | CowWriteOp::Chown { path, .. } + | CowWriteOp::Truncate { path, .. } => entry(path).map(CopyUp::Entry), + CowWriteOp::Rename { old_path, .. } => entry(old_path).map(|r| cow.rename_copy_up(r)), + CowWriteOp::Link { old_path, new_path } if cow.matches(new_path) => { + entry(old_path).map(|r| cow.link_copy_up(r)) + } + _ => None, + }; + need.unwrap_or(CopyUp::Nothing) } - cow.safe_rel(copy_path).map(|rel| (match_path, rel)) } -/// Execute a deferred `CowCopyPlan::NeedsCopy` on a blocking thread. -/// Returns the upper path on success, or rolls back quota on failure. -async fn execute_deferred_copy( - cow_state: &Arc>, - workdir_root: std::path::PathBuf, - upper_root: std::path::PathBuf, - rel: String, - upper: std::path::PathBuf, - file_size: u64, -) -> Option { - let rel_path = rel.clone(); - let copy_result = tokio::task::spawn_blocking(move || { - crate::cow::seccomp::SeccompCowBranch::execute_copy(&workdir_root, &upper_root, &rel) - }) - .await; - match copy_result { - Ok(Ok(digest)) => { - if let Some(d) = digest { - let mut st = cow_state.lock().await; - if let Some(cow) = st.branch.as_mut() { - cow.record_digest(&rel_path, d); - } - } - Some(upper) - } - _ => { - let mut st = cow_state.lock().await; - if let Some(cow) = st.branch.as_mut() { - cow.rollback_copy(file_size); - } - None - } +/// How a write answers a copy-up the branch refused. +fn refuse_write_copy(names_entry: bool, e: BranchError) -> NotifAction { + match e { + BranchError::QuotaExceeded => NotifAction::Errno(libc::ENOSPC), + // A kernel object: its metadata is the kernel's to change, but it + // cannot be staged under a new name. + BranchError::NotOwned if names_entry => NotifAction::Errno(libc::EXDEV), + BranchError::NotOwned => NotifAction::Continue, + e => NotifAction::Errno(branch_errno(e)), } } /// Handle all write-type syscalls: both *at variants (unlinkat, mkdirat, etc.) /// and legacy variants (unlink, rmdir, mkdir, etc.). -/// -/// For operations that modify existing files (chmod, chown, rename, link, -/// truncate), the handler uses a two-phase pattern: prepare the copy plan -/// under the lock, execute the potentially expensive file copy outside the -/// lock, then re-acquire the lock and run the actual operation (which finds -/// the file already in upper). pub(crate) async fn handle_cow_write( notif: &SeccompNotif, cow_state: &Arc>, @@ -672,79 +799,35 @@ pub(crate) async fn handle_cow_write( None => return NotifAction::Continue, }; - // Phase 1: check if we need to pre-copy a file (under lock, no heavy I/O). - // Capture both layer roots here so Phase 2 needs no second lock. - let (copy_plan, copy_workdir, copy_upper, copy_rel) = { - let mut st = cow_state.lock().await; - let cow = match st.branch.as_mut() { - Some(c) => c, - None => return NotifAction::Continue, - }; - - op.remap_upper_paths(cow); - match cow_copy_rel(&op, cow) { - Some((_match_path, ref rel)) => { - let workdir = cow.workdir().to_path_buf(); - let upper = cow.upper_dir().to_path_buf(); - match cow.prepare_copy(rel) { - Ok(plan) => (Some(plan), workdir, upper, rel.clone()), - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC) - } - // A kernel object: its metadata is the kernel's to change, - // but it cannot be staged under a new name. - Err(crate::error::BranchError::NotOwned) => { - return match op { - CowWriteOp::Rename { .. } | CowWriteOp::Link { .. } => { - NotifAction::Errno(libc::EXDEV) - } - _ => NotifAction::Continue, - } - } - Err(_) => return NotifAction::Continue, - } - } - None => ( - None, - std::path::PathBuf::new(), - std::path::PathBuf::new(), - String::new(), - ), - } - }; - // Lock is released here - - // Phase 2: execute the file copy outside the lock (if needed) - if let Some(crate::cow::seccomp::CowCopyPlan::NeedsCopy { - upper, - lower: _lower, - file_size, - }) = copy_plan - { - if execute_deferred_copy( - cow_state, - copy_workdir, - copy_upper, - copy_rel, - upper, - file_size, - ) - .await - .is_none() - { - return NotifAction::Continue; - } - } - - // Phase 3: execute the operation under lock (ensure_cow_copy is now a no-op - // for the pre-copied file since it's already in upper) let mut st = cow_state.lock().await; let cow = match st.branch.as_mut() { Some(c) => c, None => return NotifAction::Continue, }; + op.remap_upper_paths(cow); + if let CowWriteOp::Rename { old_path, new_path } = &op { + // Refused before the copy, so a refused rename stages nothing. + if cow.matches(old_path) { + if let Err(errno) = cow.check_rename(old_path, new_path) { + return NotifAction::Errno(errno); + } + } + } + let need = op.copy_up(cow); + let names_entry = matches!(op, CowWriteOp::Rename { .. } | CowWriteOp::Link { .. }); + after_copy_up( + cow_state, + Caller::of(notif, notif_fd), + cow, + need, + move |e| refuse_write_copy(names_entry, e), + move |cow| apply_write(cow, &op), + ) +} - match op { +/// Run a write operation once any file it changes is in the upper. +fn apply_write(cow: &mut SeccompCowBranch, op: &CowWriteOp) -> NotifAction { + match *op { CowWriteOp::Unlink { ref path, is_dir } => { if !cow.matches(path) { return NotifAction::Continue; @@ -918,36 +1001,8 @@ pub(crate) async fn handle_cow_utimensat( return NotifAction::Continue; } - let virtual_cwd = current_virtual_cwd(processes, notif.pid).await; - let path = match read_path(notif, path_ptr, notif_fd) { - Some(p) => resolve_at_path_with_virtual(notif, dirfd, &p, virtual_cwd.as_deref()), - None => return NotifAction::Continue, - }; - - let (upper_path, upper_root, workdir_root) = { - let mut st = cow_state.lock().await; - let cow = match st.branch.as_mut() { - Some(c) => c, - None => return NotifAction::Continue, - }; - let upper_root = cow.upper_dir().to_path_buf(); - let workdir_root = cow.workdir().to_path_buf(); - let path = map_cow_upper_path(cow, &path); - if !cow.matches(&path) { - return NotifAction::Continue; - } - let p = match cow.handle_utimensat(&path) { - Ok(Some(p)) => p, - Ok(None) => return NotifAction::Continue, - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC) - } - Err(_) => return NotifAction::Continue, - }; - (p, upper_root, workdir_root) - }; - - // Read times from child memory (2 x struct timespec = 32 bytes on x86_64) + // The kernel faults on the times before it looks at the path, and a plan + // given up afterwards would leave its copy reserved in the quota. let times = if times_ptr != 0 { match read_child_mem(notif_fd, notif.id, notif.pid, times_ptr, 32) { Ok(data) => { @@ -962,8 +1017,49 @@ pub(crate) async fn handle_cow_utimensat( } else { None }; + let follow = (flags & libc::AT_SYMLINK_NOFOLLOW) == 0; - let (root, rel) = match pick_root_rel(&upper_root, &workdir_root, &upper_path) { + let virtual_cwd = current_virtual_cwd(processes, notif.pid).await; + let path = match read_path(notif, path_ptr, notif_fd) { + Some(p) => resolve_at_path_with_virtual(notif, dirfd, &p, virtual_cwd.as_deref()), + None => return NotifAction::Continue, + }; + + let mut st = cow_state.lock().await; + let cow = match st.branch.as_mut() { + Some(c) => c, + None => return NotifAction::Continue, + }; + let path = map_cow_upper_path(cow, &path); + if !cow.matches(&path) { + return NotifAction::Continue; + } + let Some(rel) = cow.safe_rel(&path) else { + return NotifAction::Continue; + }; + let layers = Layers::of(cow); + let upper = layers.upper.join(&rel); + after_copy_up( + cow_state, + Caller::of(notif, notif_fd), + cow, + CopyUp::Entry(rel), + refuse_times_copy, + move |_| set_times(&layers, &upper, times, follow), + ) +} + +fn refuse_times_copy(e: BranchError) -> NotifAction { + refuse_write_copy(false, e) +} + +fn set_times( + layers: &Layers, + upper_path: &Path, + times: Option<[libc::timespec; 2]>, + follow: bool, +) -> NotifAction { + let (root, rel) = match pick_root_rel(&layers.upper, &layers.workdir, upper_path) { Ok(v) => v, Err(e) => return NotifAction::Errno(e), }; @@ -971,7 +1067,6 @@ pub(crate) async fn handle_cow_utimensat( .as_ref() .map(|t| t.as_ptr()) .unwrap_or(std::ptr::null()); - let follow = (flags & libc::AT_SYMLINK_NOFOLLOW) == 0; if let Err(e) = crate::sys::fs::utimensat_in_root(root, &rel, times_raw, follow) { return NotifAction::Errno(e); } diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index 81101d4d..9e720289 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -5,12 +5,13 @@ //! Works on any Linux 5.9+ kernel with seccomp user notification. use crate::result::{Entry, EntryKind}; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::fs; use std::os::unix::ffi::OsStringExt; -use std::os::unix::fs::{FileTypeExt, MetadataExt}; +use std::os::unix::fs::{FileTypeExt, MetadataExt, OpenOptionsExt}; use std::os::unix::io::FromRawFd; use std::path::{Path, PathBuf}; +use std::sync::{Arc, Condvar, Mutex}; use std::time::Duration; use crate::error::BranchError; @@ -32,7 +33,7 @@ fn parent_rel(rel: &str) -> Option<&str> { /// Errno for a branch failure, for handlers that answer the child directly /// (the `handle_unlink` convention) rather than returning a `BranchError`. -pub(super) fn branch_errno(e: BranchError) -> i32 { +pub(crate) fn branch_errno(e: BranchError) -> i32 { match e { BranchError::QuotaExceeded => libc::ENOSPC, BranchError::Denied => libc::EPERM, @@ -57,7 +58,83 @@ pub enum CowCopyPlan { upper: PathBuf, lower: PathBuf, file_size: u64, + ticket: CopyTicket, }, + /// Another caller is copying this file; plan again once it is done. + InFlight(CopyDone), +} + +#[derive(Debug, Default)] +struct CopySignal { + done: Mutex, + cv: Condvar, +} + +/// Held by whoever runs a copy-up `prepare_copy` handed out. Dropping it +/// wakes the waiters, so a copier that fails cannot strand them. +#[derive(Debug)] +pub struct CopyTicket(Arc); + +impl Drop for CopyTicket { + fn drop(&mut self) { + *self.0.done.lock().unwrap_or_else(|e| e.into_inner()) = true; + self.0.cv.notify_all(); + } +} + +/// Released when the copy-up it watches has been installed or given up. +#[derive(Debug, Clone)] +pub struct CopyDone(Arc); + +impl CopyDone { + /// Blocks, so only work off the notification loop may wait. + pub fn wait(&self) { + let mut done = self.0.done.lock().unwrap_or_else(|e| e.into_inner()); + while !*done { + done = self.0.cv.wait(done).unwrap_or_else(|e| e.into_inner()); + } + } + + fn is_done(&self) -> bool { + *self.0.done.lock().unwrap_or_else(|e| e.into_inner()) + } +} + +/// What `plan_copy` found at a path. +enum LowerCopy { + Ready(PathBuf), + Needed { upper: PathBuf, file_size: u64 }, +} + +/// A copy-up streamed to a private file beside the upper, not yet visible in +/// the merged view. Dropping it discards the file. +pub struct StagedCopy { + tmp: PathBuf, + digest: Option<[u8; 32]>, +} + +impl Drop for StagedCopy { + fn drop(&mut self) { + let _ = fs::remove_file(&self.tmp); + } +} + +/// What became of a staged copy-up. +#[derive(Debug, PartialEq, Eq)] +pub enum CopyInstall { + /// The upper holds the file, from this copy or one that got there first. + Ready, + /// The path was deleted meanwhile; the caller plans again. + Replan, +} + +/// What a write must find in the upper before it runs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CopyUp { + Nothing, + Entry(String), + /// A directory with everything below it in the lower. + Tree(String), } /// Plan returned by `prepare_open` — describes what I/O to do after releasing the lock. @@ -71,13 +148,9 @@ pub enum CowOpenPlan { Deleted, /// File already resolved (upper or lower) — open this path directly. Resolved(PathBuf), - /// Need to copy lower to upper, then open upper. - NeedsCopy { - upper: PathBuf, - lower: PathBuf, - file_size: u64, - rel_path: String, - }, + /// The file must be copied into the upper first, by this caller or by + /// the one already copying it. + Copy { rel_path: String, plan: CowCopyPlan }, /// Upper path ready (already exists in upper, or new file placeholder). UpperReady { upper: PathBuf }, } @@ -729,6 +802,8 @@ pub struct SeccompCowBranch { keep_if_abandoned: bool, max_disk_bytes: u64, disk_used: u64, + /// Copy-ups staged but not yet installed, with the bytes each reserved. + copying: HashMap, } /// The entry at `rel` under `root`, confined and without following @@ -982,9 +1057,16 @@ impl SeccompCowBranch { keep_if_abandoned: false, max_disk_bytes, disk_used: 0, + copying: HashMap::new(), }) } + /// Where copy-ups are staged: on the upper's filesystem, but outside the + /// merged view and anything a commit publishes. + pub fn staging_dir(&self) -> &Path { + &self.storage_dir + } + /// The upper directory where writes are stored. pub fn upper_dir(&self) -> &Path { &self.upper @@ -1135,27 +1217,56 @@ impl SeccompCowBranch { Ok(()) } - /// Recalculate `disk_used` by walking the upper directory. + /// Recalculate `disk_used` by walking the upper directory. Staged copies + /// are not in it yet, so their reservations are added back. fn recalc_disk_used(&mut self) { - self.disk_used = dir_size(&self.upper); + self.copying.retain(|_, (_, done)| !done.is_done()); + let staged: u64 = self.copying.values().map(|(bytes, _)| bytes).sum(); + self.disk_used = dir_size(&self.upper) + staged; } - /// Prepare a COW copy: update metadata (deleted set, quota reservation) - /// and handle small items (symlinks, dirs) immediately, but defer large - /// file copies to the caller. This is the shared core used by both - /// `ensure_cow_copy` (synchronous) and the async two-phase dispatch. + /// `plan_copy` for a caller that copies outside the branch lock. A file + /// someone is already copying is not copied twice: the caller waits and + /// plans again. The ticket goes back to `finish_copy`. pub fn prepare_copy(&mut self, rel_path: &str) -> Result { + if let Some((_, done)) = self.copying.get(rel_path) { + if !done.is_done() { + return Ok(CowCopyPlan::InFlight(done.clone())); + } + self.copying.remove(rel_path); + } + Ok(match self.plan_copy(rel_path)? { + LowerCopy::Ready(upper) => CowCopyPlan::Ready(upper), + LowerCopy::Needed { upper, file_size } => { + let signal = Arc::new(CopySignal::default()); + self.copying.insert( + rel_path.to_string(), + (file_size, CopyDone(Arc::clone(&signal))), + ); + CowCopyPlan::NeedsCopy { + upper, + lower: self.workdir.join(rel_path), + file_size, + ticket: CopyTicket(signal), + } + } + }) + } + + /// Plan a COW copy: update metadata (deleted set, quota reservation) + /// and handle small items (symlinks, dirs) immediately, but leave the + /// potentially large file copy to the caller. + fn plan_copy(&mut self, rel_path: &str) -> Result { self.has_changes = true; self.touch_parents(rel_path); self.touch(rel_path); let upper_file = self.upper.join(rel_path); - let lower_file = self.workdir.join(rel_path); // Already materialized in upper? Confined lstat succeeds for any // existing entry (including a dangling symlink). if crate::sys::fs::statat_in_root(&self.upper, rel_path, false).is_ok() { - return Ok(CowCopyPlan::Ready(upper_file)); + return Ok(LowerCopy::Ready(upper_file)); } if let Some(p) = parent_rel(rel_path) { @@ -1168,7 +1279,7 @@ impl SeccompCowBranch { // in the merged view (and what makes a re-created directory opaque). if self.deleted.covers(rel_path) { self.check_quota(0)?; - return Ok(CowCopyPlan::Ready(upper_file)); + return Ok(LowerCopy::Ready(upper_file)); } // Classify the lower entry confined to the workdir root, so a symlinked @@ -1177,14 +1288,14 @@ impl SeccompCowBranch { let st = match crate::sys::fs::statat_in_root(&self.workdir, rel_path, false) { Ok(st) => st, // Absent or confined-out: treat as a new file created in upper. - // EACCES gets the same disposition as execute_copy's source-open + // EACCES gets the same disposition as stage_copy's source-open // gives it: the supervisor cannot see inside the lower directory // (e.g. /root under a workdir of "/"), so the write proceeds on // an empty upper file rather than falling through to a real // permission error the virtualized child was promised not to hit. Err(libc::ENOENT) | Err(libc::EACCES) => { self.check_quota(0)?; - return Ok(CowCopyPlan::Ready(upper_file)); + return Ok(LowerCopy::Ready(upper_file)); } Err(e) => { return Err(BranchError::Io { @@ -1212,7 +1323,7 @@ impl SeccompCowBranch { source: std::io::Error::from_raw_os_error(e), })?; self.disk_used += 256; - return Ok(CowCopyPlan::Ready(upper_file)); + return Ok(LowerCopy::Ready(upper_file)); } // Directory — create immediately (no data copy) @@ -1226,7 +1337,7 @@ impl SeccompCowBranch { )?; let _ = crate::sys::fs::chmod_in_root(&self.upper, rel_path, st.st_mode & 0o7777); self.disk_used += 4096; - return Ok(CowCopyPlan::Ready(upper_file)); + return Ok(LowerCopy::Ready(upper_file)); } if Self::is_kernel_object_kind(kind) { @@ -1235,34 +1346,33 @@ impl SeccompCowBranch { // Regular file — defer the potentially expensive copy. Size comes from // the confined lstat, so the quota reservation matches the file - // execute_copy will actually read. + // stage_copy will actually read. let file_size = st.st_size as u64; self.check_quota(file_size)?; self.disk_used += file_size; - Ok(CowCopyPlan::NeedsCopy { + Ok(LowerCopy::Needed { upper: upper_file, - lower: lower_file, file_size, }) } - /// Execute a file copy synchronously. Used by `ensure_cow_copy` and the - /// async dispatch (via `spawn_blocking`). - pub fn execute_copy( + /// Stream the lower file at `rel` into a private file in `staging_dir`. + /// Nothing appears in the upper until `install_staged`, so a copy running + /// beside other handlers never shows them a half-written file. + pub fn stage_copy( workdir_root: &Path, - upper_root: &Path, + staging_dir: &Path, rel: &str, - ) -> Result, std::io::Error> { - let create_dest = || -> Result { - let fd = crate::sys::fs::openat2_in_root( - upper_root, - rel, - libc::O_WRONLY | libc::O_CREAT | libc::O_TRUNC | libc::O_NOFOLLOW | libc::O_CLOEXEC, - 0o600, - ) - .map_err(std::io::Error::from_raw_os_error)?; - Ok(unsafe { fs::File::from_raw_fd(fd) }) - }; + ) -> Result { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let n = NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let tmp = staging_dir.join(format!(".copy-{}-{}", std::process::id(), n)); + let dest = fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&tmp)?; + let mut staged = StagedCopy { tmp, digest: None }; // Read the lower source confined to the workdir root: a symlink or // `..` in `rel` cannot escape the tree (issue #112). @@ -1276,10 +1386,7 @@ impl SeccompCowBranch { // Unreadable (EACCES) or confined-out / absent (ENOENT): give the // child an empty COW file so writes proceed, never leaking the // escape target. - Err(libc::EACCES) | Err(libc::ENOENT) => { - create_dest()?; - return Ok(None); - } + Err(libc::EACCES) | Err(libc::ENOENT) => return Ok(staged), // On a kernel without openat2 (ENOSYS) the copy fails and the caller // rolls back / returns Continue; the child then hits Landlock, which // is the backstop. Do not "fix" this with an unconfined fs::copy. @@ -1293,105 +1400,183 @@ impl SeccompCowBranch { // #158). O_NONBLOCK above makes the FIFO open itself return instead // of waiting for a writer; on a regular file it is a no-op for reads. if !src.metadata()?.file_type().is_file() { - create_dest()?; - return Ok(None); + return Ok(staged); } - let mut dst = Sha256Writer::new(create_dest()?); + let mut dst = Sha256Writer::new(dest); std::io::copy(&mut src, &mut dst)?; if let Ok(meta) = src.metadata() { let _ = dst.inner.set_permissions(meta.permissions()); } - Ok(Some(dst.finish())) - } - - /// Ensure a COW copy exists in upper (synchronous). Returns the upper path. - /// For callers that don't need async two-phase behavior. - pub fn ensure_cow_copy(&mut self, rel_path: &str) -> Result { - match self.prepare_copy(rel_path)? { - CowCopyPlan::Ready(upper) => Ok(upper), - CowCopyPlan::NeedsCopy { - upper, - lower: _lower, - file_size, - } => match Self::execute_copy(&self.workdir, &self.upper, rel_path) { - Ok(digest) => { - if let Some(d) = digest { - self.record_digest(rel_path, d); + staged.digest = Some(dst.finish()); + Ok(staged) + } + + /// Move a staged copy into the upper. Never replaces: a copy that got + /// there first may already carry a writer's data. + fn install_staged( + &mut self, + rel_path: &str, + file_size: u64, + staged: Result, + ) -> Result { + let staged = match staged { + Ok(staged) => staged, + Err(e) => { + self.rollback_copy(file_size); + return Err(e); + } + }; + // Installing a path deleted while the copy ran would bring it back. + let result = if self.deleted.covers(rel_path) { + Ok(CopyInstall::Replan) + } else { + match crate::sys::fs::install_in_root(&staged.tmp, &self.upper, rel_path) { + Ok(()) => { + if let Some(digest) = staged.digest { + self.record_digest(rel_path, digest); } - Ok(upper) + return Ok(CopyInstall::Ready); } + Err(libc::EEXIST) => Ok(CopyInstall::Ready), + Err(e) => Err(std::io::Error::from_raw_os_error(e)), + } + }; + self.rollback_copy(file_size); + result + } + + /// Settle a copy `prepare_copy` handed out. The ticket drops last, so the + /// waiters it wakes find the outcome in place. + pub fn finish_copy( + &mut self, + rel_path: &str, + file_size: u64, + _ticket: CopyTicket, + staged: Result, + ) -> Result { + self.copying.remove(rel_path); + self.install_staged(rel_path, file_size, staged) + } + + /// `prepare_copy` for every entry in `rels`, all or nothing: on a refusal + /// the entries planned so far are given up and removed again, so a write + /// the branch refuses leaves no trace. Only the copies still to run come + /// back. + pub fn prepare_copies( + &mut self, + rels: &[String], + ) -> Result, BranchError> { + let mut created = Vec::new(); + let mut plans = Vec::new(); + for rel in rels { + // A whiteouted child of a staged tree stays deleted. + if self.deleted.covers(rel) && !self.upper_has(rel) && rel != &rels[0] { + continue; + } + let had = self.upper_has(rel); + match self.prepare_copy(rel) { + Ok(CowCopyPlan::Ready(_)) => { + if !had && self.upper_has(rel) { + created.push(rel.clone()); + } + } + Ok(plan) => plans.push((rel.clone(), plan)), Err(e) => { - self.rollback_copy(file_size); - Err(BranchError::Operation(format!("copy: {}", e))) + self.abandon_copies(plans); + self.remove_created(&created); + return Err(e); } - }, + } } + Ok(plans) } - /// Copy a lower tree into the upper, entry by entry, so a directory - /// rename can be staged in the branch without losing the contents - /// (issue #160). Each entry goes through the same confined, - /// quota-accounted single-entry copy-up, so symlinks are copied verbatim - /// and never followed (issue #112). - /// - /// Staging is all-or-nothing: a mid-tree failure (EACCES, EIO, quota) - /// must propagate rather than truncate the copy, because the caller - /// whiteouts the source afterwards and untraversed children would be - /// silently lost at commit. On failure every upper entry this staging - /// created is removed again and the quota re-derived. - fn copy_up_tree(&mut self, rel: &str) -> Result<(), i32> { - let mut created: Vec = Vec::new(); - let result = self.stage_tree(rel, &mut created); - if result.is_err() { - for c in created.iter().rev() { - let is_dir = crate::sys::fs::statat_in_root(&self.upper, c, false) - .map(|st| st.st_mode & libc::S_IFMT == libc::S_IFDIR) - .unwrap_or(false); - if is_dir { - let _ = crate::sys::fs::remove_dir_all_in_root(&self.upper, c); - } else { - let _ = crate::sys::fs::unlinkat_in_root(&self.upper, c, false); - } + /// Give back copies planned but never run. + pub fn abandon_copies(&mut self, plans: impl IntoIterator) { + for (rel, plan) in plans { + if let CowCopyPlan::NeedsCopy { + file_size, ticket, .. + } = plan + { + let cancelled = Err(std::io::Error::from_raw_os_error(libc::ECANCELED)); + let _ = self.finish_copy(&rel, file_size, ticket, cancelled); } - self.recalc_disk_used(); } - result } - /// The traversal half of `copy_up_tree`: an explicit worklist (child - /// directory depth must not become supervisor stack depth) that records - /// every upper entry it creates into `created` for rollback. - fn stage_tree(&mut self, root: &str, created: &mut Vec) -> Result<(), i32> { - let mut queue = std::collections::VecDeque::new(); - queue.push_back(root.to_string()); - while let Some(rel) = queue.pop_front() { - let was_in_upper = self.upper_has(&rel); - self.ensure_cow_copy(&rel).map_err(branch_errno)?; - if !was_in_upper && self.upper_has(&rel) { - created.push(rel.clone()); + fn remove_created(&mut self, created: &[String]) { + if created.is_empty() { + return; + } + for c in created.iter().rev() { + let is_dir = crate::sys::fs::statat_in_root(&self.upper, c, false) + .map(|st| st.st_mode & libc::S_IFMT == libc::S_IFDIR) + .unwrap_or(false); + if is_dir { + let _ = crate::sys::fs::remove_dir_all_in_root(&self.upper, c); + } else { + let _ = crate::sys::fs::unlinkat_in_root(&self.upper, c, false); + } + } + self.recalc_disk_used(); + } + + /// The upper copy of `rel`, which the dispatcher made before the write. + fn copied(&mut self, rel: &str) -> Result { + match self.plan_copy(rel)? { + LowerCopy::Ready(upper) => Ok(upper), + LowerCopy::Needed { file_size, .. } => { + self.rollback_copy(file_size); + Err(BranchError::Operation(format!("{rel} was not copied up"))) } - let st = match crate::sys::fs::statat_in_root(&self.workdir, &rel, false) { + } + } + + /// `root` and everything below it in the lower, parents first. It reads + /// only the lower, which the branch never writes, so it needs no lock. + /// An explicit worklist: child directory depth must not become supervisor + /// stack depth. + pub fn lower_tree(workdir: &Path, root: &str) -> Result, i32> { + let mut tree = Vec::new(); + let mut queue = std::collections::VecDeque::from([root.to_string()]); + while let Some(rel) = queue.pop_front() { + let st = crate::sys::fs::statat_in_root(workdir, &rel, false); + tree.push(rel.clone()); + let st = match st { Ok(st) => st, - // Upper-only entry: nothing below it in the lower to stage. + // Upper-only entry: nothing below it in the lower. Err(libc::ENOENT) => continue, Err(e) => return Err(e), }; if st.st_mode & libc::S_IFMT != libc::S_IFDIR { continue; } - let rd = fs::read_dir(self.workdir.join(&rel)) - .map_err(|e| e.raw_os_error().unwrap_or(libc::EIO))?; - for entry in rd { - let entry = entry.map_err(|e| e.raw_os_error().unwrap_or(libc::EIO))?; - let name = entry.file_name().to_string_lossy().into_owned(); - let child = format!("{}/{}", rel, name); - if self.deleted.covers(&child) && !self.upper_has(&child) { - continue; - } - queue.push_back(child); + let io = |e: std::io::Error| e.raw_os_error().unwrap_or(libc::EIO); + for entry in fs::read_dir(workdir.join(&rel)).map_err(io)? { + let name = entry.map_err(io)?.file_name(); + queue.push_back(format!("{}/{}", rel, name.to_string_lossy())); } } - Ok(()) + Ok(tree) + } + + /// What a rename of `rel` must copy up: a directory takes its lower tree + /// along, so the rename can be staged without losing the contents + /// (issue #160). + pub fn rename_copy_up(&self, rel: String) -> CopyUp { + match self.merged_entry_is_dir(&rel) { + Some(true) => CopyUp::Tree(rel), + _ => CopyUp::Entry(rel), + } + } + + /// What a hard link from `rel` must copy up. linkat on a directory is the + /// kernel's EPERM to give; staging it would leave a meaningless empty dir. + pub fn link_copy_up(&self, rel: String) -> CopyUp { + match self.merged_entry_is_dir(&rel) { + Some(true) => CopyUp::Nothing, + _ => CopyUp::Entry(rel), + } } /// Resolve a read path: upper if modified, else lower. @@ -1406,82 +1591,6 @@ impl SeccompCowBranch { // ---- Syscall handlers (called by cow::dispatch) ---- - /// Handle openat: resolve to upper or lower path. - /// - /// Returns `Err(QuotaExceeded)` when the write would exceed `max_disk`. - /// - /// When a quota is active and the open is a write, resync `disk_used` - /// from the real upper directory first. This catches growth from - /// `write()` syscalls on previously injected fds (which bypass the - /// seccomp supervisor) and prevents new opens once the quota is - /// exhausted. - pub fn handle_open(&mut self, path: &str, flags: u64) -> Result, BranchError> { - let rel = match self.safe_rel(path) { - Some(r) => r, - None => return Ok(None), - }; - if flags & O_DIRECTORY != 0 { - // A whiteouted directory must not fall through to the kernel: - // the lower directory still exists, so opendir would hand the - // child a live fd (and fstat its inode) where the merged answer - // is ENOENT. - if self.is_deleted(&rel) { - return Err(BranchError::Deleted); - } - return Ok(None); - } - - let is_write = flags & WRITE_FLAGS != 0; - - // Resync quota accounting before any write open so that bytes - // written through previously injected fds are counted. - if is_write && self.max_disk_bytes > 0 { - self.recalc_disk_used(); - self.check_quota(0)?; - } - - if self.is_deleted(&rel) { - if flags & O_CREAT != 0 { - return self.ensure_cow_copy(&rel).map(Some); - } - // Whiteout: the lower file still physically exists with its - // pre-delete bytes. Surface the deletion so the caller returns - // ENOENT rather than falling through to the lower file — matching - // the async prepare_open (CowOpenPlan::Deleted) and the stat/access - // handlers. - return Err(BranchError::Deleted); - } - - if self.is_kernel_object(&rel) { - return Ok(None); - } - - // O_EXCL: fail if file already exists (in upper or lower) - if flags & O_CREAT != 0 && flags & O_EXCL != 0 { - // Confined existence check: a symlinked parent component must not - // let this probe follow out of the tree, which would turn O_EXCL - // into a host-file existence oracle (issue #112). - if crate::sys::fs::statat_in_root(&self.upper, &rel, false).is_ok() - || crate::sys::fs::statat_in_root(&self.workdir, &rel, false).is_ok() - { - return Err(BranchError::Exists); - } - // File truly doesn't exist — create in upper - return self.ensure_cow_copy(&rel).map(Some); - } - - if is_write { - self.ensure_cow_copy(&rel).map(Some) - } else { - let resolved = self.resolve_read(&rel); - if resolved.exists() || resolved.is_symlink() { - Ok(Some(resolved)) - } else { - Ok(None) - } - } - } - /// Prepare an open without doing the file copy. /// /// Returns a plan that describes what I/O needs to happen after the lock @@ -1562,19 +1671,13 @@ impl SeccompCowBranch { /// Prepare a COW copy for openat — wraps `prepare_copy` into `CowOpenPlan`. fn prepare_cow_copy(&mut self, rel_path: &str) -> Result { - match self.prepare_copy(rel_path)? { - CowCopyPlan::Ready(upper) => Ok(CowOpenPlan::UpperReady { upper }), - CowCopyPlan::NeedsCopy { - upper, - lower, - file_size, - } => Ok(CowOpenPlan::NeedsCopy { - upper, - lower, - file_size, + Ok(match self.prepare_copy(rel_path)? { + CowCopyPlan::Ready(upper) => CowOpenPlan::UpperReady { upper }, + plan => CowOpenPlan::Copy { rel_path: rel_path.to_string(), - }), - } + plan, + }, + }) } /// Roll back quota reservation if the copy failed. @@ -1730,6 +1833,37 @@ impl SeccompCowBranch { .map(|st| st.st_mode & libc::S_IFMT == libc::S_IFDIR) } + /// The merged view's refusal of a rename, if any. + pub fn check_rename(&self, old_path: &str, new_path: &str) -> Result<(), i32> { + match (self.safe_rel(old_path), self.safe_rel(new_path)) { + (Some(old_rel), Some(new_rel)) => self.check_rename_rel(&old_rel, &new_rel), + _ => Ok(()), + } + } + + fn check_rename_rel(&self, old_rel: &str, new_rel: &str) -> Result<(), i32> { + let src_is_dir = match self.merged_entry_is_dir(old_rel) { + Some(d) => d, + None => return Err(libc::ENOENT), + }; + // Destination semantics come from the merged view: renaming onto an + // existing directory must replace it or refuse, never publish the + // union of the renamed tree and the pre-existing one (issue #160 + // review). The kernel cannot give these answers because it sees only + // one layer at a time. + if let Some(dest_is_dir) = self.merged_entry_is_dir(new_rel) { + match (src_is_dir, dest_is_dir) { + (false, true) => return Err(libc::EISDIR), + (true, false) => return Err(libc::ENOTDIR), + (true, true) if !self.list_merged_dir(new_rel).is_empty() => { + return Err(libc::ENOTEMPTY) + } + _ => {} + } + } + Ok(()) + } + /// Handle rename. /// /// Returns `Ok(true)` on success, `Ok(false)` when the branch does not @@ -1748,26 +1882,12 @@ impl SeccompCowBranch { }; self.touch_parents(&new_rel); self.touch(&new_rel); - let src_is_dir = match self.merged_entry_is_dir(&old_rel) { - Some(d) => d, - None => return Err(libc::ENOENT), - }; - // Destination semantics come from the merged view: renaming onto an - // existing directory must replace it or refuse, never publish the - // union of the renamed tree and the pre-existing one (issue #160 - // review). The kernel cannot give these answers because it sees only - // one layer at a time. - if let Some(dest_is_dir) = self.merged_entry_is_dir(&new_rel) { - match (src_is_dir, dest_is_dir) { - (false, true) => return Err(libc::EISDIR), - (true, false) => return Err(libc::ENOTDIR), - (true, true) if !self.list_merged_dir(&new_rel).is_empty() => { - return Err(libc::ENOTEMPTY) - } - _ => {} - } + self.check_rename_rel(&old_rel, &new_rel)?; + // The dispatcher copied the tree; a missing source would let the + // rename lose it. + if !self.upper_has(&old_rel) { + return Err(libc::EIO); } - self.copy_up_tree(&old_rel)?; if let Some(p) = parent_rel(&new_rel) { let _ = crate::sys::fs::mkdirp_in_root(&self.upper, p, 0o755); } @@ -1852,7 +1972,7 @@ impl SeccompCowBranch { if src_is_dir { return Ok(false); } - let _ = self.ensure_cow_copy(&old_rel)?; + self.copied(&old_rel)?; if let Some(p) = parent_rel(&new_rel) { let _ = crate::sys::fs::mkdirp_in_root(&self.upper, p, 0o755); } @@ -1867,7 +1987,7 @@ impl SeccompCowBranch { Some(r) => r, None => return Ok(false), }; - let _ = self.ensure_cow_copy(&rel)?; + self.copied(&rel)?; Ok(crate::sys::fs::chmod_in_root(&self.upper, &rel, mode).is_ok()) } @@ -1879,7 +1999,7 @@ impl SeccompCowBranch { Some(r) => r, None => return Ok(false), }; - let _ = self.ensure_cow_copy(&rel)?; + self.copied(&rel)?; // Best-effort: try the real chown but succeed either way — the // supervisor typically lacks CAP_CHOWN so this will fail, but // in COW/dry-run mode the ownership doesn't matter. @@ -1896,7 +2016,7 @@ impl SeccompCowBranch { None => return Ok(false), }; let new_len = length as u64; - let _ = self.ensure_cow_copy(&rel)?; + self.copied(&rel)?; let old_len = crate::sys::fs::statat_in_root(&self.upper, &rel, true) .map(|st| st.st_size as u64) .unwrap_or(0); @@ -1924,17 +2044,6 @@ impl SeccompCowBranch { Ok(ok) } - /// Handle utimensat — resolve to upper, return the upper path for the - /// caller to call utimensat on. - pub fn handle_utimensat(&mut self, path: &str) -> Result, BranchError> { - let rel = match self.safe_rel(path) { - Some(r) => r, - None => return Ok(None), - }; - let upper = self.ensure_cow_copy(&rel)?; - Ok(Some(upper)) - } - /// Handle readlink. pub fn handle_readlink(&self, path: &str) -> Option { let rel = self.safe_rel(path)?; @@ -2668,7 +2777,7 @@ impl SeccompCowBranch { /// leaves the page cache intact and the old unsynced write was already /// enough. These fsyncs change behaviour only on power loss or a kernel /// panic — and there they make the RECORD durable while the upper it names - /// is written by `ensure_cow_copy` and by the child with no sync at all, + /// is written by the copy-up and by the child with no sync at all, /// and the default storage base is `$XDG_RUNTIME_DIR` or `$TMPDIR`, i.e. /// tmpfs, where the whole tree is gone anyway. They are meaningful with an /// explicit disk-backed `fs_storage`, and inert without one. @@ -2796,9 +2905,103 @@ impl Drop for SeccompCowBranch { } } +/// Shorthands that run on the caller's thread what the dispatcher runs off +/// the loop. +#[cfg(test)] +impl SeccompCowBranch { + pub fn copy_up_now(&mut self, need: CopyUp) -> Result<(), BranchError> { + let rels = match need { + CopyUp::Nothing => return Ok(()), + CopyUp::Entry(rel) => vec![rel], + CopyUp::Tree(root) => { + Self::lower_tree(&self.workdir, &root).map_err(|e| BranchError::Io { + operation: "walk lower", + source: std::io::Error::from_raw_os_error(e), + })? + } + }; + for (rel, plan) in self.prepare_copies(&rels)? { + if let CowCopyPlan::NeedsCopy { + file_size, ticket, .. + } = plan + { + let staged = Self::stage_copy(&self.workdir, &self.storage_dir, &rel); + self.finish_copy(&rel, file_size, ticket, staged) + .map_err(|source| BranchError::Io { + operation: "copy", + source, + })?; + } + } + Ok(()) + } + + pub fn ensure_cow_copy(&mut self, rel: &str) -> Result { + self.copy_up_now(CopyUp::Entry(rel.to_string()))?; + Ok(self.upper.join(rel)) + } + + pub fn handle_open(&mut self, path: &str, flags: u64) -> Result, BranchError> { + match self.prepare_open(path, flags)? { + CowOpenPlan::Skip => Ok(None), + CowOpenPlan::Deleted => Err(BranchError::Deleted), + CowOpenPlan::Resolved(p) | CowOpenPlan::UpperReady { upper: p } => Ok(Some(p)), + CowOpenPlan::Copy { rel_path, plan } => { + self.abandon_copies([(rel_path.clone(), plan)]); + self.ensure_cow_copy(&rel_path).map(Some) + } + } + } +} + #[cfg(test)] mod tests { use super::*; + + /// Write handlers run the way the dispatcher runs them: copy-ups first. + fn chmod(b: &mut SeccompCowBranch, path: &str, mode: u32) -> Result { + copy_entry(b, path)?; + b.handle_chmod(path, mode) + } + + fn chown( + b: &mut SeccompCowBranch, + path: &str, + uid: u32, + gid: u32, + ) -> Result { + copy_entry(b, path)?; + b.handle_chown(path, uid, gid) + } + + fn truncate(b: &mut SeccompCowBranch, path: &str, length: i64) -> Result { + copy_entry(b, path)?; + b.handle_truncate(path, length) + } + + fn link(b: &mut SeccompCowBranch, old: &str, new: &str) -> Result { + if let Some(rel) = b.safe_rel(old) { + let need = b.link_copy_up(rel); + b.copy_up_now(need)?; + } + b.handle_link(old, new) + } + + fn rename(b: &mut SeccompCowBranch, old: &str, new: &str) -> Result { + b.check_rename(old, new)?; + if let Some(rel) = b.safe_rel(old) { + let need = b.rename_copy_up(rel); + b.copy_up_now(need).map_err(branch_errno)?; + } + b.handle_rename(old, new) + } + + fn copy_entry(b: &mut SeccompCowBranch, path: &str) -> Result<(), BranchError> { + match b.safe_rel(path) { + Some(rel) => b.copy_up_now(CopyUp::Entry(rel)), + None => Ok(()), + } + } use std::fs; #[test] @@ -4268,7 +4471,7 @@ mod tests { let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 4).unwrap(); let old = abs(&branch, "existing.txt"); let new = abs(&branch, "renamed.txt"); - let err = branch.handle_rename(&old, &new).unwrap_err(); + let err = rename(&mut branch, &old, &new).unwrap_err(); assert_eq!(err, libc::ENOSPC); } @@ -4278,7 +4481,7 @@ mod tests { let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 4).unwrap(); let old = abs(&branch, "existing.txt"); let new = abs(&branch, "hardlink.txt"); - let err = branch.handle_link(&old, &new).unwrap_err(); + let err = link(&mut branch, &old, &new).unwrap_err(); assert!(matches!(err, BranchError::QuotaExceeded)); } @@ -4288,7 +4491,7 @@ mod tests { // chmod triggers ensure_cow_copy. let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 4).unwrap(); let path = abs(&branch, "existing.txt"); - let err = branch.handle_chmod(&path, 0o644).unwrap_err(); + let err = chmod(&mut branch, &path, 0o644).unwrap_err(); assert!(matches!(err, BranchError::QuotaExceeded)); } @@ -4297,7 +4500,7 @@ mod tests { let (workdir, storage) = setup_workdir(); let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 4).unwrap(); let path = abs(&branch, "existing.txt"); - let err = branch.handle_chown(&path, 1000, 1000).unwrap_err(); + let err = chown(&mut branch, &path, 1000, 1000).unwrap_err(); assert!(matches!(err, BranchError::QuotaExceeded)); } @@ -4310,7 +4513,7 @@ mod tests { let path = abs(&branch, "existing.txt"); // cow copy uses 5 bytes (5 of 10 used). // Truncating to 20 bytes needs 15 more — exceeds remaining 5. - let err = branch.handle_truncate(&path, 20).unwrap_err(); + let err = truncate(&mut branch, &path, 20).unwrap_err(); assert!(matches!(err, BranchError::QuotaExceeded)); } @@ -4321,7 +4524,7 @@ mod tests { SeccompCowBranch::create(workdir.path(), Some(storage.path()), 10).unwrap(); let path = abs(&branch, "existing.txt"); // Truncate to 2 bytes — cow copy (5) + shrink is fine. - assert!(matches!(branch.handle_truncate(&path, 2), Ok(true))); + assert!(matches!(truncate(&mut branch, &path, 2), Ok(true))); // disk_used should now be 2, not 5. assert_eq!(branch.disk_used, 2); } @@ -4544,7 +4747,13 @@ mod tests { let path = abs(&branch, "existing.txt"); // O_WRONLY let plan = branch.prepare_open(&path, 0o1).unwrap(); - assert!(matches!(plan, CowOpenPlan::NeedsCopy { .. })); + assert!(matches!( + plan, + CowOpenPlan::Copy { + plan: CowCopyPlan::NeedsCopy { .. }, + .. + } + )); } #[test] @@ -4584,7 +4793,137 @@ mod tests { SeccompCowBranch::create(workdir.path(), Some(storage.path()), 100).unwrap(); let path = abs(&branch, "existing.txt"); let plan = branch.prepare_open(&path, 0o1).unwrap(); - assert!(matches!(plan, CowOpenPlan::NeedsCopy { file_size: 5, .. })); + assert!(matches!( + plan, + CowOpenPlan::Copy { + plan: CowCopyPlan::NeedsCopy { file_size: 5, .. }, + .. + } + )); + assert_eq!(branch.disk_used, 5); + } + + /// Run a copy-up of `rel` the way the off-loop dispatch does. + fn staged_copy( + branch: &mut SeccompCowBranch, + rel: &str, + ) -> (u64, CopyTicket, Result) { + let CowCopyPlan::NeedsCopy { + file_size, ticket, .. + } = branch.prepare_copy(rel).unwrap() + else { + panic!("expected a copy to be needed"); + }; + let staged = SeccompCowBranch::stage_copy(branch.workdir(), branch.staging_dir(), rel); + (file_size, ticket, staged) + } + + fn staged_files(branch: &SeccompCowBranch) -> usize { + fs::read_dir(branch.staging_dir()) + .unwrap() + .flatten() + .filter(|e| e.file_name().to_string_lossy().starts_with(".copy-")) + .count() + } + + #[test] + fn a_staged_copy_is_invisible_until_installed() { + let (workdir, storage) = setup_workdir(); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + let (size, ticket, staged) = staged_copy(&mut branch, "existing.txt"); + assert!(!branch.upper_dir().join("existing.txt").exists()); + assert_eq!( + branch + .finish_copy("existing.txt", size, ticket, staged) + .unwrap(), + CopyInstall::Ready + ); + assert_eq!( + fs::read(branch.upper_dir().join("existing.txt")).unwrap(), + b"hello" + ); + assert_eq!(staged_files(&branch), 0); + } + + #[test] + fn a_second_copier_waits_until_the_first_finishes_or_gives_up() { + let (workdir, storage) = setup_workdir(); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + let (size, ticket, staged) = staged_copy(&mut branch, "existing.txt"); + let CowCopyPlan::InFlight(done) = branch.prepare_copy("existing.txt").unwrap() else { + panic!("the second copier must wait"); + }; + assert!(!done.is_done()); + branch + .finish_copy("existing.txt", size, ticket, staged) + .unwrap(); + done.wait(); + assert!(matches!( + branch.prepare_copy("existing.txt").unwrap(), + CowCopyPlan::Ready(_) + )); + + let (_, ticket, _) = staged_copy(&mut branch, "subdir/nested.txt"); + drop(ticket); + assert!(matches!( + branch.prepare_copy("subdir/nested.txt").unwrap(), + CowCopyPlan::NeedsCopy { .. } + )); + } + + #[test] + fn a_copy_finished_after_the_path_was_deleted_is_not_installed() { + let (workdir, storage) = setup_workdir(); + let mut branch = + SeccompCowBranch::create(workdir.path(), Some(storage.path()), 100).unwrap(); + let (size, ticket, staged) = staged_copy(&mut branch, "existing.txt"); + let path = abs(&branch, "existing.txt"); + assert_eq!(branch.handle_unlink(&path, false), Ok(true)); + assert_eq!( + branch + .finish_copy("existing.txt", size, ticket, staged) + .unwrap(), + CopyInstall::Replan + ); + assert!(branch.is_deleted("existing.txt")); + assert_eq!(branch.disk_used, 0, "the reservation must be rolled back"); + assert_eq!(staged_files(&branch), 0); + } + + #[test] + fn a_copy_that_lost_the_race_keeps_the_winners_file() { + let (workdir, storage) = setup_workdir(); + let mut branch = + SeccompCowBranch::create(workdir.path(), Some(storage.path()), 100).unwrap(); + let (size, ticket, staged) = staged_copy(&mut branch, "existing.txt"); + let upper = branch.upper_dir().join("existing.txt"); + fs::write(&upper, "written").unwrap(); + assert_eq!( + branch + .finish_copy("existing.txt", size, ticket, staged) + .unwrap(), + CopyInstall::Ready + ); + assert_eq!(fs::read(&upper).unwrap(), b"written"); + assert_eq!( + branch.disk_used, 0, + "the losing copy gives its reservation back" + ); + assert_eq!(staged_files(&branch), 0); + } + + #[test] + fn a_quota_resync_keeps_the_reservation_of_a_staged_copy() { + let (workdir, storage) = setup_workdir(); + let mut branch = + SeccompCowBranch::create(workdir.path(), Some(storage.path()), 100).unwrap(); + let (size, ticket, staged) = staged_copy(&mut branch, "existing.txt"); + branch.recalc_disk_used(); + assert_eq!(branch.disk_used, size); + branch + .finish_copy("existing.txt", size, ticket, staged) + .unwrap(); + branch.recalc_disk_used(); assert_eq!(branch.disk_used, 5); } @@ -5054,12 +5393,12 @@ mod tests { fs::write(wd.join("d/sub/deep.txt"), "DEEP").unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); - assert!(branch - .handle_rename( - &format!("{}/d", wd.display()), - &format!("{}/d2", wd.display()) - ) - .unwrap()); + assert!(rename( + &mut branch, + &format!("{}/d", wd.display()), + &format!("{}/d2", wd.display()) + ) + .unwrap()); // Merged view before commit: d hidden, d2 holds the tree. assert!(branch.is_deleted("d")); @@ -5094,12 +5433,12 @@ mod tests { branch.handle_unlink(&format!("{}/d/gone.txt", wd.display()), false), Ok(true) ); - assert!(branch - .handle_rename( - &format!("{}/d", wd.display()), - &format!("{}/d2", wd.display()) - ) - .unwrap()); + assert!(rename( + &mut branch, + &format!("{}/d", wd.display()), + &format!("{}/d2", wd.display()) + ) + .unwrap()); branch.commit().unwrap(); assert!(wd.join("d2/keep.txt").exists()); assert!(!wd.join("d2/gone.txt").exists()); @@ -5112,7 +5451,8 @@ mod tests { let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); branch.mark_deleted("existing.txt"); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/existing.txt", wd.display()), &format!("{}/moved.txt", wd.display()) ), @@ -5133,7 +5473,8 @@ mod tests { fs::write(wd.join("b/b1"), "B").unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/a", wd.display()), &format!("{}/b", wd.display()) ), @@ -5157,7 +5498,8 @@ mod tests { fs::create_dir(wd.join("b")).unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/a", wd.display()), &format!("{}/b", wd.display()) ), @@ -5179,7 +5521,8 @@ mod tests { fs::write(wd.join("target.txt"), "OLD").unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/existing.txt", wd.display()), &format!("{}/target.txt", wd.display()) ), @@ -5205,7 +5548,8 @@ mod tests { let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/existing.txt", wd.display()), &format!("{}/dest", wd.display()) ), @@ -5233,14 +5577,16 @@ mod tests { let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); // File onto directory: EISDIR. Directory onto file: ENOTDIR. assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/existing.txt", wd.display()), &format!("{}/d", wd.display()) ), Err(libc::EISDIR) ); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/d", wd.display()), &format!("{}/existing.txt", wd.display()) ), @@ -5266,7 +5612,8 @@ mod tests { Ok(true) ); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/existing.txt", wd.display()), &format!("{}/b", wd.display()) ), @@ -5285,7 +5632,8 @@ mod tests { let wd = workdir.path().canonicalize().unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/nope", wd.display()), &format!("{}/dest", wd.display()) ), @@ -5309,7 +5657,8 @@ mod tests { fs::set_permissions(wd.join("d/inner"), fs::Permissions::from_mode(0o000)).unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); - let result = branch.handle_rename( + let result = rename( + &mut branch, &format!("{}/d", wd.display()), &format!("{}/moved", wd.display()), ); @@ -5343,7 +5692,8 @@ mod tests { // Quota fits the staged directory (4096) but not the file after it. let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 4100).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/d", wd.display()), &format!("{}/m", wd.display()) ), @@ -5375,7 +5725,8 @@ mod tests { let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch.handle_rename( + rename( + &mut branch, &format!("{}/d", wd.display()), &format!("{}/m", wd.display()) ), @@ -5395,12 +5746,12 @@ mod tests { let (workdir, storage) = setup_workdir(); let wd = workdir.path().canonicalize().unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); - assert!(branch - .handle_rename( - &format!("{}/existing.txt", wd.display()), - &format!("{}/renamed.txt", wd.display()) - ) - .unwrap()); + assert!(rename( + &mut branch, + &format!("{}/existing.txt", wd.display()), + &format!("{}/renamed.txt", wd.display()) + ) + .unwrap()); branch.commit().unwrap(); assert!(!wd.join("existing.txt").exists()); assert_eq!(fs::read_to_string(wd.join("renamed.txt")).unwrap(), "hello"); @@ -5415,12 +5766,12 @@ mod tests { fs::create_dir(wd.join("d")).unwrap(); let mut branch = SeccompCowBranch::create(&wd, Some(storage.path()), 0).unwrap(); assert_eq!( - branch - .handle_link( - &format!("{}/d", wd.display()), - &format!("{}/d2", wd.display()) - ) - .unwrap(), + link( + &mut branch, + &format!("{}/d", wd.display()), + &format!("{}/d2", wd.display()) + ) + .unwrap(), false ); assert!(!branch.upper_dir().join("d").exists()); @@ -5512,7 +5863,7 @@ mod tests { let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); let path = format!("{}/pipe", branch.workdir_str()); assert!(matches!( - branch.handle_chmod(&path, 0o600), + chmod(&mut branch, &path, 0o600), Err(BranchError::NotOwned) )); assert!( @@ -5536,7 +5887,7 @@ mod tests { let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); let wd = branch.workdir_str().to_string(); assert_eq!( - branch.handle_rename(&format!("{wd}/pipe"), &format!("{wd}/pipe2")), + rename(&mut branch, &format!("{wd}/pipe"), &format!("{wd}/pipe2")), Err(libc::EXDEV), ); assert!( @@ -5621,11 +5972,11 @@ mod tests { let storage = tempfile::tempdir().unwrap(); let mut branch = SeccompCowBranch::create(Path::new("/"), Some(storage.path()), 0).unwrap(); assert!(matches!( - branch.handle_chmod("/dev/null", 0o666), + chmod(&mut branch, "/dev/null", 0o666), Err(BranchError::NotOwned) )); assert!(matches!( - branch.handle_utimensat("/dev/null"), + branch.prepare_copy("dev/null"), Err(BranchError::NotOwned) )); assert_eq!( @@ -5638,19 +5989,21 @@ mod tests { } #[test] - fn execute_copy_guards_against_type_race() { + fn stage_copy_guards_against_type_race() { // Defense in depth: if the entry changes type between prepare_copy's - // lstat and execute_copy's open, the O_NONBLOCK+fstat guard must + // lstat and stage_copy's open, the O_NONBLOCK+fstat guard must // catch it and produce an empty destination instead of streaming. let (workdir, storage) = setup_workdir(); let fifo = workdir.path().join("race"); let c = std::ffi::CString::new(fifo.to_str().unwrap()).unwrap(); assert_eq!(unsafe { libc::mkfifo(c.as_ptr(), 0o644) }, 0); - let branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); - let workdir_root = branch.workdir().to_path_buf(); - let upper_root = branch.upper_dir().to_path_buf(); - SeccompCowBranch::execute_copy(&workdir_root, &upper_root, "race").unwrap(); - assert_eq!(fs::read(upper_root.join("race")).unwrap(), b""); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + let staged = SeccompCowBranch::stage_copy(branch.workdir(), branch.staging_dir(), "race"); + assert_eq!( + branch.install_staged("race", 0, staged).unwrap(), + CopyInstall::Ready + ); + assert_eq!(fs::read(branch.upper_dir().join("race")).unwrap(), b""); } /// Recursively snapshot the merged view as (rel path -> Option), @@ -5731,7 +6084,7 @@ mod tests { .unwrap(); fs::write(&up, "MODIFIED").unwrap(); assert_eq!(branch.handle_unlink(&w("d/a.txt"), false), Ok(true)); - assert!(branch.handle_rename(&w("d"), &w("moved")).unwrap()); + assert!(rename(&mut branch, &w("d"), &w("moved")).unwrap()); assert_eq!( branch.handle_unlink(&w("subdir/nested.txt"), false), Ok(true) @@ -6256,9 +6609,7 @@ mod tests { let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); let wd = workdir.path().canonicalize().unwrap(); - assert!(branch - .handle_chmod(&format!("{}/f.txt", wd.display()), 0o600) - .unwrap()); + assert!(chmod(&mut branch, &format!("{}/f.txt", wd.display()), 0o600).unwrap()); assert_eq!( branch .changes() @@ -6288,7 +6639,7 @@ mod tests { /// /// The merge used to hardcode the destination mode, so a script or binary /// the run produced arrived in the workdir un-executable — a committed - /// result that cannot be run. `execute_copy` already carries the mode on + /// result that cannot be run. `stage_copy` already carries the mode on /// the way down into the upper; this is the same property on the way back. #[test] fn the_mode_of_a_file_created_in_the_upper_survives_the_merge() { @@ -6610,9 +6961,7 @@ mod tests { let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); let wd = branch.workdir_str().to_string(); - assert!(branch - .handle_rename(&format!("{wd}/d"), &format!("{wd}/e")) - .unwrap()); + assert!(rename(&mut branch, &format!("{wd}/d"), &format!("{wd}/e")).unwrap()); let all = by_path(&branch); assert_eq!(all["d"].kind(), ChangeKind::Deleted); @@ -6650,9 +6999,8 @@ mod tests { fs::write(workdir.path().join("f.txt"), "abc").unwrap(); let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); - assert!(branch - .handle_chmod(&format!("{}/f.txt", branch.workdir_str()), 0o755) - .unwrap()); + let path = format!("{}/f.txt", branch.workdir_str()); + assert!(chmod(&mut branch, &path, 0o755).unwrap()); let c = &by_path(&branch)["f.txt"]; assert_eq!(c.kind(), ChangeKind::Modified); @@ -6682,14 +7030,12 @@ mod tests { } #[test] - fn execute_copy_returns_the_digest_of_what_it_streamed() { + fn stage_copy_returns_the_digest_of_what_it_streamed() { let workdir = tempfile::tempdir().unwrap(); - let upper = tempfile::tempdir().unwrap(); + let staging = tempfile::tempdir().unwrap(); fs::write(workdir.path().join("f"), "abc").unwrap(); - let digest = SeccompCowBranch::execute_copy(workdir.path(), upper.path(), "f") - .unwrap() - .unwrap(); - assert_eq!(hex(&digest), SHA256_ABC); + let staged = SeccompCowBranch::stage_copy(workdir.path(), staging.path(), "f").unwrap(); + assert_eq!(hex(&staged.digest.unwrap()), SHA256_ABC); } fn sorted_changes(changes: Vec) -> Vec { diff --git a/crates/sandlock-core/src/sys/fs.rs b/crates/sandlock-core/src/sys/fs.rs index f70ecfeb..5c645c90 100644 --- a/crates/sandlock-core/src/sys/fs.rs +++ b/crates/sandlock-core/src/sys/fs.rs @@ -489,6 +489,44 @@ pub(crate) fn renameat_in_root(root: &Path, old_rel: &str, new_rel: &str) -> Res } } +/// Move `src` to `rel` confined within `root`, failing with `EEXIST` rather +/// than replacing. NFS, 9p, ceph and ecryptfs reject `RENAME_NOREPLACE` but +/// can link, which is just as atomic; FAT cannot link, so neither alone does. +pub(crate) fn install_in_root(src: &Path, root: &Path, rel: &str) -> Result<(), i32> { + use std::os::unix::ffi::OsStrExt; + let csrc = CString::new(src.as_os_str().as_bytes()).map_err(|_| libc::EINVAL)?; + let (pfd, base) = parent_dir_in_root(root, rel)?; + let result = CString::new(base) + .map_err(|_| libc::EINVAL) + .and_then(|cbase| { + // Not every libc this builds against wraps renameat2. + let rc = unsafe { + libc::syscall( + libc::SYS_renameat2, + libc::AT_FDCWD, + csrc.as_ptr(), + pfd, + cbase.as_ptr(), + libc::RENAME_NOREPLACE, + ) + }; + if rc == 0 { + return Ok(()); + } + match last_errno(libc::EIO) { + libc::EINVAL | libc::ENOSYS => {} + e => return Err(e), + } + if unsafe { libc::linkat(libc::AT_FDCWD, csrc.as_ptr(), pfd, cbase.as_ptr(), 0) } < 0 { + return Err(last_errno(libc::EIO)); + } + unsafe { libc::unlink(csrc.as_ptr()) }; + Ok(()) + }); + unsafe { libc::close(pfd) }; + result +} + /// Create a hard link `new_rel -> old_rel` confined within `root`. The old path /// is not dereferenced (no `AT_SYMLINK_FOLLOW`). pub(crate) fn linkat_in_root(root: &Path, old_rel: &str, new_rel: &str) -> Result<(), i32> { @@ -652,6 +690,27 @@ mod tests { } } + #[test] + fn install_in_root_moves_the_file_and_never_replaces() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().join("root"); + std::fs::create_dir_all(root.join("d")).unwrap(); + let staged = tmp.path().join("staged"); + + std::fs::write(&staged, "first").unwrap(); + install_in_root(&staged, &root, "d/f").unwrap(); + assert_eq!(std::fs::read(root.join("d/f")).unwrap(), b"first"); + assert!(!staged.exists()); + + std::fs::write(&staged, "second").unwrap(); + assert_eq!(install_in_root(&staged, &root, "d/f"), Err(libc::EEXIST)); + assert_eq!(std::fs::read(root.join("d/f")).unwrap(), b"first"); + assert!( + staged.exists(), + "the loser is left for its owner to discard" + ); + } + #[test] fn openat2_in_root_clamps_parent_escape() { let tmp = TempDir::new().unwrap(); From d279f3dd234466e500ddfafa32c5cedf7d82107b Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 4 Nov 2026 19:36:43 -0800 Subject: [PATCH 8/8] checkpoint: hold child memory writes across the capture Handlers keep running after the sandbox is stopped: a task whose notification was already received cannot stop until it is answered, and deferred work runs on regardless. An inline handler, an FFI callback or a deferred future could therefore write child memory while a checkpoint reads it, and the image would mix memory from before and after the write. The capture now holds the sandbox's memory gate exclusively, the same gate that orders deferred writes against an exec freeze, and the notification loop installs that gate for its inline handlers too, so every write waits until the capture is done. The gate is taken only once the target is ptrace-stopped. The seize waits for the handler of whatever the target was trapped in to answer, and taking the gate earlier would block that handler's last write and deadlock. The exec freeze writes no child memory while it holds the gate, so the loop never waits on itself. Work that never writes child memory, such as a copy-up, is not held up. Signed-off-by: Cong Wang --- .../sandlock-core/src/checkpoint/capture.rs | 15 ++++++++++--- crates/sandlock-core/src/resource.rs | 1 + crates/sandlock-core/src/sandbox.rs | 13 ++++++++++- crates/sandlock-core/src/seccomp/ctx.rs | 3 +++ crates/sandlock-core/src/seccomp/dispatch.rs | 1 + crates/sandlock-core/src/seccomp/gate.rs | 14 ++++++------ crates/sandlock-core/src/seccomp/notif.rs | 22 ++++++++++++------- 7 files changed, 50 insertions(+), 19 deletions(-) diff --git a/crates/sandlock-core/src/checkpoint/capture.rs b/crates/sandlock-core/src/checkpoint/capture.rs index cd92476c..67e4e418 100644 --- a/crates/sandlock-core/src/checkpoint/capture.rs +++ b/crates/sandlock-core/src/checkpoint/capture.rs @@ -387,7 +387,11 @@ fn parse_fdinfo(pid: i32, fd: i32) -> io::Result<(i32, u64)> { /// Capture a checkpoint from a running, stopped sandbox. /// The sandbox must already be frozen (SIGSTOP'd and fork-held). -pub(crate) fn capture(pid: i32, policy: &Sandbox) -> Result { +pub(crate) fn capture( + pid: i32, + policy: &Sandbox, + mem_gate: Option<&crate::seccomp::gate::MemGate>, +) -> Result { // Credential-injection rules hold supervisor-only secrets that are // deliberately not serialized (`#[serde(skip)]`). A checkpoint image would // therefore restore with no injection rules and send every request @@ -411,6 +415,11 @@ pub(crate) fn capture(pid: i32, policy: &Sandbox) -> Result, supervisor_resource: Option>>, + #[cfg_attr(not(feature = "checkpoint"), allow(dead_code))] + supervisor_mem_gate: Option, supervisor_cow: Option>>, supervisor_network: Option>>, ctrl_fd: Option, @@ -1933,7 +1935,11 @@ impl Sandbox { return Err(SandlockError::Runtime(SandboxRuntimeError::NotRunning)); } self.freeze().await?; - let cp = crate::checkpoint::capture(target_pid, self); + let gate = self + .runtime + .as_ref() + .and_then(|rt| rt.supervisor_mem_gate.as_ref()); + let cp = crate::checkpoint::capture(target_pid, self, gate); self.thaw().await?; cp } @@ -2166,6 +2172,7 @@ impl Sandbox { #[cfg(feature = "cow")] seccomp_cow: None, supervisor_resource: None, + supervisor_mem_gate: None, supervisor_cow: None, supervisor_network: None, ctrl_fd: None, @@ -2274,6 +2281,7 @@ impl Sandbox { #[cfg(feature = "cow")] seccomp_cow: None, supervisor_resource: None, + supervisor_mem_gate: None, supervisor_cow: None, supervisor_network: None, ctrl_fd: None, @@ -2949,6 +2957,8 @@ impl Sandbox { let chroot_state = Arc::new(tokio::sync::Mutex::new(chroot_state)); let processes = Arc::new(crate::seccomp::state::ProcessIndex::new()); + let mem_gate = crate::seccomp::gate::MemGate::default(); + self.rt_mut().supervisor_mem_gate = Some(mem_gate.clone()); let ctx = Arc::new(SupervisorCtx { resource: Arc::clone(&res_state), cow: Arc::clone(&cow_state), @@ -2963,6 +2973,7 @@ impl Sandbox { policy: Arc::new(notif_policy), child_pidfd: child_pidfd_raw, notif_fd: notif_raw_fd, + mem_gate, }); // Snapshot the sandbox config for the control-socket `config` diff --git a/crates/sandlock-core/src/seccomp/ctx.rs b/crates/sandlock-core/src/seccomp/ctx.rs index b0feb728..9f770d22 100644 --- a/crates/sandlock-core/src/seccomp/ctx.rs +++ b/crates/sandlock-core/src/seccomp/ctx.rs @@ -41,4 +41,7 @@ pub struct SupervisorCtx { pub child_pidfd: Option, /// Seccomp notification fd (for on-behalf operations). pub notif_fd: RawFd, + /// Orders deferred work's writes to child memory against the freezes + /// that read it: exec and checkpoint. + pub(crate) mem_gate: super::gate::MemGate, } diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 10dea7f5..d26b2f30 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -1330,6 +1330,7 @@ mod handler_tests { }), child_pidfd: None, notif_fd: -1, + mem_gate: Default::default(), }) } diff --git a/crates/sandlock-core/src/seccomp/gate.rs b/crates/sandlock-core/src/seccomp/gate.rs index e5a1fba8..89f5ee67 100644 --- a/crates/sandlock-core/src/seccomp/gate.rs +++ b/crates/sandlock-core/src/seccomp/gate.rs @@ -1,9 +1,10 @@ -//! Orders off-loop writes to child memory against execve argv validation. +//! Orders writes to child memory against the freezes that read it. //! -//! Parked work runs with its sandbox's gate installed, and every write to -//! child memory holds the shared side for that one write. The exec freeze -//! holds the exclusive side, so no parked write can land on a peer's argv -//! between its validation and the kernel copying it. +//! Every handler, inline or parked, runs with its sandbox's gate installed, +//! and each write to child memory holds the shared side for that one write. +//! The exec freeze and a checkpoint capture hold the exclusive side, so no +//! write lands on memory they are reading. The exec freeze writes nothing +//! while it holds the gate, so the loop never waits on itself. use std::cell::RefCell; use std::future::Future; @@ -39,8 +40,7 @@ impl MemGate { } } -/// Run one write to child memory under the installed gate, if any. The -/// notification loop installs none: nothing else runs on it during a freeze. +/// Run one write to child memory under the installed gate, if any. pub(crate) fn write(work: impl FnOnce() -> T) -> T { let gate = TASK_GATE .try_with(MemGate::clone) diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index 8518efd6..55ce3262 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -152,7 +152,8 @@ impl Deferred { } /// What one sandbox's parked work shares: the gate ordering its child-memory -/// writes against exec, and the listener its native closures may outlive. +/// writes against exec and checkpoint, and the listener its native closures +/// may outlive. #[derive(Clone)] struct Offload { gate: super::gate::MemGate, @@ -2934,14 +2935,11 @@ struct ParkedSet { } impl ParkedSet { - fn new(listener: Arc) -> Self { + fn new(listener: Arc, gate: super::gate::MemGate) -> Self { Self { tasks: tokio::task::JoinSet::new(), callers: HashMap::new(), - offload: Offload { - gate: super::gate::MemGate::default(), - listener, - }, + offload: Offload { gate, listener }, } } @@ -3406,7 +3404,7 @@ pub async fn supervisor( // still per-child pidfd readiness in `spawn_pid_watcher`. let gc = tokio::spawn(process_index_gc(Arc::clone(&ctx.processes))); - let mut parked = ParkedSet::new(Arc::clone(async_fd.get_ref())); + let mut parked = ParkedSet::new(Arc::clone(async_fd.get_ref()), ctx.mem_gate.clone()); // Edge-triggered drain: each `readable().await` returns once per // epoll edge, then we drain the kernel queue via `probe_notif_fd` @@ -3453,7 +3451,15 @@ pub async fn supervisor( Err(e) if e.raw_os_error() == Some(libc::ENOENT) => continue, Err(_) => break 'outer, }; - handle_notification(notif, &ctx, &dispatch_table, fd, &mut parked).await; + let gate = parked.offload.gate.clone(); + gate.scope(handle_notification( + notif, + &ctx, + &dispatch_table, + fd, + &mut parked, + )) + .await; } NotifFdState::Empty => break, NotifFdState::Terminal => break 'outer,