From 7354bfd650c2dc8e92c3bdbda4bc8a98764b0ff0 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sat, 31 Oct 2026 10:12:42 -0700 Subject: [PATCH 1/7] 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 8a855b53..d942c652 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 07915e0adad06451e3bfd4ed99338e0d4b86f5c6 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 1 Nov 2026 16:19:51 -0800 Subject: [PATCH 2/7] runtime: isolate blocking work from the caller's Tokio pool A synchronous operation can outlive the future waiting for it. Using the caller's blocking pool lets stalled work consume its threads and delay runtime shutdown, while releasing admission on timeout would allow new work to accumulate behind calls that are still running. Add process-wide Tokio blocking pools with admission owned by each native closure until it returns. Reject saturation with EAGAIN and give filesystem work separate capacity so stalled external peers cannot take its workers. Dropping a waiter stops waiting only; it does not cancel the native call. Signed-off-by: Cong Wang --- crates/sandlock-core/src/blocking.rs | 151 +++++++++++++++++++++++++++ crates/sandlock-core/src/lib.rs | 1 + 2 files changed, 152 insertions(+) create mode 100644 crates/sandlock-core/src/blocking.rs diff --git a/crates/sandlock-core/src/blocking.rs b/crates/sandlock-core/src/blocking.rs new file mode 100644 index 00000000..109f8e9c --- /dev/null +++ b/crates/sandlock-core/src/blocking.rs @@ -0,0 +1,151 @@ +//! Bounded synchronous work, isolated from the embedding runtime's blocking pool. +//! +//! Dropping a waiter does not stop running work. Its closure retains admission +//! until it returns; process-wide pools keep that bound across sandbox restarts. + +use std::io; +use std::sync::{Arc, OnceLock}; + +use tokio::runtime::{Builder, Runtime}; +use tokio::sync::Semaphore; +use tokio::task::JoinHandle; + +struct BlockingPool { + runtime: Option, + slots: Arc, +} + +impl BlockingPool { + fn new(name: &str, capacity: usize) -> io::Result { + let runtime = Builder::new_current_thread() + .max_blocking_threads(capacity) + .thread_name(name) + .build()?; + Ok(Self { + runtime: Some(runtime), + slots: Arc::new(Semaphore::new(capacity)), + }) + } + + fn spawn(&self, work: F) -> io::Result> + where + F: FnOnce() -> T + Send + 'static, + T: Send + 'static, + { + let permit = Arc::clone(&self.slots) + .try_acquire_owned() + .map_err(|_| io::Error::from_raw_os_error(libc::EAGAIN))?; + Ok(self.runtime.as_ref().unwrap().spawn_blocking(move || { + let _permit = permit; + work() + })) + } +} + +impl Drop for BlockingPool { + fn drop(&mut self) { + self.slots.close(); + if let Some(runtime) = self.runtime.take() { + runtime.shutdown_background(); + } + } +} + +fn pool( + storage: &'static OnceLock>, + name: &str, + capacity: usize, +) -> io::Result<&'static BlockingPool> { + storage + .get_or_init(|| BlockingPool::new(name, capacity)) + .as_ref() + .map_err(|e| io::Error::new(e.kind(), e.to_string())) +} + +async fn run_on(pool: io::Result<&BlockingPool>, work: F) -> io::Result +where + F: FnOnce() -> T + Send + 'static, + T: Send + 'static, +{ + pool?.spawn(work)?.await.map_err(io::Error::other) +} + +/// Run synchronous supervised work with bounded admission. +/// +/// Saturation returns EAGAIN without executing the closure. A timeout or dropped +/// future stops waiting only; running work and its owned resources may outlive +/// the caller. Capture owners rather than borrowed fd numbers or pointers. +pub async fn run(work: F) -> io::Result +where + F: FnOnce() -> T + Send + 'static, + T: Send + 'static, +{ + static POOL: OnceLock> = OnceLock::new(); + run_on(pool(&POOL, "sandlock-blocking", 64), work).await +} + +pub(crate) async fn filesystem(work: F) -> io::Result +where + F: FnOnce() -> T + Send + 'static, + T: Send + 'static, +{ + // A stalled external peer must not occupy the workers needed by filesystem work. + static POOL: OnceLock> = OnceLock::new(); + run_on(pool(&POOL, "sandlock-filesystem", 16), work).await +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn dropped_waiter_keeps_running_work_admitted() { + let pool = BlockingPool::new("blocking-test", 1).unwrap(); + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let job = pool + .spawn(move || { + let _ = started_tx.send(()); + let _ = release_rx.recv(); + }) + .unwrap(); + started_rx.await.unwrap(); + drop(job); + let error = pool.spawn(|| ()).unwrap_err(); + assert_eq!(error.raw_os_error(), Some(libc::EAGAIN)); + release_tx.send(()).unwrap(); + let permit = Arc::clone(&pool.slots).acquire_owned().await.unwrap(); + drop(permit); + assert_eq!(pool.spawn(|| 42).unwrap().await.unwrap(), 42); + } + + #[tokio::test] + async fn shutdown_does_not_wait_for_running_work() { + let pool = BlockingPool::new("blocking-shutdown-test", 1).unwrap(); + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let job = pool + .spawn(move || { + let _ = started_tx.send(()); + let _ = release_rx.recv(); + 42 + }) + .unwrap(); + started_rx.await.unwrap(); + drop(pool); + release_tx.send(()).unwrap(); + assert_eq!(job.await.unwrap(), 42); + } + + #[tokio::test] + async fn panicking_work_releases_admission() { + let pool = BlockingPool::new("blocking-panic-test", 1).unwrap(); + assert!(pool + .spawn(|| panic!("worker panic")) + .unwrap() + .await + .unwrap_err() + .is_panic()); + assert_eq!(pool.spawn(|| 42).unwrap().await.unwrap(), 42); + } +} diff --git a/crates/sandlock-core/src/lib.rs b/crates/sandlock-core/src/lib.rs index 1418a68b..355f722c 100644 --- a/crates/sandlock-core/src/lib.rs +++ b/crates/sandlock-core/src/lib.rs @@ -1,4 +1,5 @@ pub(crate) mod arch; +pub mod blocking; pub(crate) mod ca_inject; #[cfg(feature = "checkpoint")] pub(crate) mod checkpoint; From ffc40b94972c1957299196a7a05493a53e505297 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 1 Nov 2026 16:20:22 -0800 Subject: [PATCH 3/7] network: move supervised connects off the notification loop Unix and IP connects execute synchronously after policy approval. A slow peer can therefore hold up the notification loop and prevent unrelated trapped syscalls from making progress. Prepare a lazy blocking action for both connect paths and run it through the bounded pool once approved and admitted. The closure keeps the socket and pinned path alive until connect returns, and a rejected operation never starts the native call. Signed-off-by: Cong Wang --- crates/sandlock-core/src/network/connect.rs | 2 +- crates/sandlock-core/src/network/unix.rs | 2 +- crates/sandlock-core/src/seccomp/notif.rs | 49 +++++++++++++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/crates/sandlock-core/src/network/connect.rs b/crates/sandlock-core/src/network/connect.rs index f8110fb0..ccdac2f5 100644 --- a/crates/sandlock-core/src/network/connect.rs +++ b/crates/sandlock-core/src/network/connect.rs @@ -101,7 +101,7 @@ pub(super) async fn connect_on_behalf( }; let metadata_addr = addr_bytes.clone(); - NotifAction::prepare(&metadata_addr, dup_fd.as_raw_fd(), move || { + NotifAction::prepare_blocking(&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 { diff --git a/crates/sandlock-core/src/network/unix.rs b/crates/sandlock-core/src/network/unix.rs index b645c1c6..0f1bf5c1 100644 --- a/crates/sandlock-core/src/network/unix.rs +++ b/crates/sandlock-core/src/network/unix.rs @@ -132,7 +132,7 @@ pub(super) fn connect_named_unix_on_behalf( Err(e) => return NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EBADF)), }; let addr = sockaddr_un_bytes(&sun, len); - NotifAction::prepare(&addr.clone(), dup_fd.as_raw_fd(), move || { + NotifAction::prepare_blocking(&addr.clone(), dup_fd.as_raw_fd(), move || { let _pinned = pinned; let ret = unsafe { libc::connect( diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index c88250d8..bd12e27c 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -175,6 +175,14 @@ pub enum NotifAction { } impl NotifAction { + pub(crate) fn prepare_blocking( + addr: &[u8], + socket: RawFd, + execute: impl FnOnce() -> NotifAction + Send + Sync + 'static, + ) -> Self { + Self::prepare(addr, socket, move || Self::blocking(execute)) + } + pub(crate) fn prepare( addr: &[u8], socket: RawFd, @@ -212,6 +220,16 @@ impl NotifAction { NotifAction::Defer(Deferred::new(fut)) } + /// Defer owned synchronous work with admission held until the work returns. + pub fn blocking(work: impl FnOnce() -> NotifAction + Send + 'static) -> Self { + Self::defer(async move { + match crate::blocking::run(work).await { + Ok(action) => action, + Err(e) => Self::Errno(e.raw_os_error().unwrap_or(libc::EIO)), + } + }) + } + /// Inject `content` into the child as the syscall's returned fd, backed by /// a sealed (read-only, fixed-size), `O_CLOEXEC` in-memory file. /// @@ -3814,6 +3832,37 @@ mod tests { assert!(matches!(deferred.run().await, NotifAction::ReturnValue(7))); } + #[tokio::test] + async fn prepared_blocking_work_starts_only_when_admitted() { + use std::sync::atomic::{AtomicBool, Ordering}; + let ran = Arc::new(AtomicBool::new(false)); + let work_ran = Arc::clone(&ran); + let action = NotifAction::prepare_blocking(&[], -1, move || { + work_ran.store(true, Ordering::SeqCst); + NotifAction::ReturnValue(7) + }); + let NotifAction::Prepared(operation) = action else { + panic!("expected prepared operation"); + }; + let action = operation.execute(); + assert!(!ran.load(Ordering::SeqCst)); + let NotifAction::Defer(deferred) = action else { + panic!("expected deferred work"); + }; + assert!(matches!(deferred.run().await, NotifAction::ReturnValue(7))); + assert!(ran.load(Ordering::SeqCst)); + } + + #[tokio::test] + async fn rejected_blocking_work_never_starts() { + let action = NotifAction::prepare_blocking(&[], -1, || { + panic!("denied operation executed"); + }); + drop(action); + let action = NotifAction::blocking(|| panic!("unadmitted operation executed")); + drop(action); + } + #[tokio::test(start_paused = true)] async fn deferred_times_out_to_eio() { // A deferred future that exceeds its limit must fail closed (EIO) so From bc9c7d7ef3679008afc607a6348c23539f059dd7 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 1 Nov 2026 16:24:14 -0800 Subject: [PATCH 4/7] seccomp: let blocking handlers suspend and resume dispatch Awaiting an FFI callback inline still holds up notification intake even when the callback runs on a blocking thread. Terminal deferral cannot replace that wait because Continue must reach the remaining handlers. Add a suspended action that parks the notification and resumes its handler chain when the work finishes. Keep explicit deferral terminal, and use the bounded blocking pool for FFI callbacks. Retain the listener and callback owner in native work, and translate callback output there so a timed-out waiter cannot free userdata or strand an injected fd. Track native access to child memory so exec argument validation does not overlap it. Signed-off-by: Cong Wang --- crates/sandlock-core/src/seccomp/dispatch.rs | 168 +++++++++++++++++-- crates/sandlock-core/src/seccomp/notif.rs | 167 ++++++++++++++---- crates/sandlock-ffi/include/sandlock.h | 6 +- crates/sandlock-ffi/src/handler/abi.rs | 13 +- crates/sandlock-ffi/src/handler/adapter.rs | 120 ++++++------- crates/sandlock-ffi/tests/handler_smoke.rs | 120 ++++++++++--- 6 files changed, 436 insertions(+), 158 deletions(-) diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 7eec5b22..03a57512 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -17,7 +17,7 @@ // them approve a syscall based on user-memory contents. use std::collections::HashMap; -use std::os::unix::io::RawFd; +use std::os::unix::io::{AsRawFd, OwnedFd, RawFd}; use std::sync::Arc; use super::ctx::SupervisorCtx; @@ -28,7 +28,7 @@ use crate::arch; use crate::sys::structs::SeccompNotif; use thiserror::Error; -use tokio::sync::Mutex; +use tokio::sync::{Mutex, OwnedSemaphorePermit, Semaphore}; // ============================================================ // Types @@ -76,19 +76,49 @@ pub trait Handler: Send + Sync + 'static { pub struct HandlerCtx { pub notif: SeccompNotif, pub notif_fd: RawFd, + listener: Arc, + active_work: Arc, /// The open-family request decoded once for this notification. open: Option>, } impl HandlerCtx { - pub fn new(notif: SeccompNotif, notif_fd: RawFd) -> Self { + pub fn new(notif: SeccompNotif, notif_fd: OwnedFd) -> Self { + Self::with_listener( + notif, + Arc::new(notif_fd), + None, + Arc::new(Semaphore::new(super::notif::DEFER_MAX_INFLIGHT)), + ) + } + + fn with_listener( + notif: SeccompNotif, + listener: Arc, + open: Option>, + active_work: Arc, + ) -> Self { Self { notif, - notif_fd, - open: None, + notif_fd: listener.as_raw_fd(), + listener, + active_work, + open, } } + pub fn listener(&self) -> Arc { + Arc::clone(&self.listener) + } + + /// Retain this permit inside native work that can access child memory. + /// Exec argument validation requires all such work to have finished. + pub fn blocking_guard(&self) -> std::io::Result { + Arc::clone(&self.active_work) + .try_acquire_owned() + .map_err(|_| std::io::Error::from_raw_os_error(libc::EAGAIN)) + } + pub(crate) fn open(&self) -> Option> { self.open.clone() } @@ -250,18 +280,19 @@ impl DispatchTable { pub(crate) async fn dispatch( &self, notif: SeccompNotif, - notif_fd: RawFd, + listener: Arc, open: Option>, + active_work: Arc, ) -> NotifAction { let nr = notif.data.nr as i64; if let Some(chain) = self.chains.get(&nr) { - let handler_ctx = HandlerCtx { - notif, - notif_fd, - open, - }; - for handler in &chain.handlers { + let handler_ctx = HandlerCtx::with_listener(notif, listener, open, active_work); + for (index, handler) in chain.handlers.iter().enumerate() { let action = handler.handle(&handler_ctx).await; + if let NotifAction::Suspend(work) = action { + let remaining = chain.handlers[index + 1..].to_vec(); + return NotifAction::suspend(resume_chain(work, remaining, handler_ctx)); + } if !matches!(action, NotifAction::Continue) { return action; } @@ -271,6 +302,25 @@ impl DispatchTable { } } +async fn resume_chain( + work: super::notif::Deferred, + remaining: Vec>, + cx: HandlerCtx, +) -> NotifAction { + let mut action = NotifAction::Suspend(work); + let mut handlers = remaining.into_iter(); + loop { + action = match action { + NotifAction::Suspend(work) => work.run().await, + NotifAction::Continue => match handlers.next() { + Some(handler) => handler.handle(&cx).await, + None => return NotifAction::Continue, + }, + other => return other, + }; + } +} + // ============================================================ // Table builder — mechanical translation of old dispatch() // ============================================================ @@ -1369,7 +1419,12 @@ mod handler_tests { let _ctx = fake_supervisor_ctx(); let action = table - .dispatch(fake_notif(libc::SYS_openat as i32), -1, None) + .dispatch( + fake_notif(libc::SYS_openat as i32), + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) .await; assert!(matches!(action, NotifAction::Continue)); @@ -1416,7 +1471,12 @@ mod handler_tests { let _ctx = fake_supervisor_ctx(); let action = table - .dispatch(fake_notif(libc::SYS_openat as i32), -1, None) + .dispatch( + fake_notif(libc::SYS_openat as i32), + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) .await; assert!(matches!(action, NotifAction::Continue)); @@ -1461,7 +1521,12 @@ mod handler_tests { let _ctx = fake_supervisor_ctx(); let action = table - .dispatch(fake_notif(libc::SYS_openat as i32), -1, None) + .dispatch( + fake_notif(libc::SYS_openat as i32), + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) .await; match action { @@ -1475,6 +1540,61 @@ mod handler_tests { ); } + fn fake_listener() -> Arc { + Arc::new(std::fs::File::open("/dev/null").unwrap().into()) + } + + #[tokio::test] + async fn suspended_continue_resumes_remaining_handlers() { + let mut table = DispatchTable::new(); + let calls = Arc::new(AtomicUsize::new(0)); + table.register(libc::SYS_openat, |_cx: &HandlerCtx| async { + NotifAction::suspend(async { NotifAction::Continue }) + }); + let later_calls = Arc::clone(&calls); + table.register(libc::SYS_openat, move |_cx: &HandlerCtx| { + later_calls.fetch_add(1, Ordering::SeqCst); + async { NotifAction::ReturnValue(42) } + }); + let action = table + .dispatch( + fake_notif(libc::SYS_openat as i32), + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) + .await; + assert_eq!(calls.load(Ordering::SeqCst), 0); + let NotifAction::Suspend(work) = action else { + panic!("expected suspension") + }; + assert!(matches!(work.run().await, NotifAction::ReturnValue(42))); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + #[tokio::test] + async fn suspended_terminal_action_stops_remaining_handlers() { + let mut table = DispatchTable::new(); + table.register(libc::SYS_openat, |_cx: &HandlerCtx| async { + NotifAction::suspend(async { NotifAction::Errno(libc::EACCES) }) + }); + table.register(libc::SYS_openat, |_cx: &HandlerCtx| async { + panic!("handler ran after a terminal action") + }); + let action = table + .dispatch( + fake_notif(libc::SYS_openat as i32), + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) + .await; + let NotifAction::Suspend(work) = action else { + panic!("expected suspension") + }; + assert!(matches!(work.run().await, NotifAction::Errno(libc::EACCES))); + } + /// 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. @@ -1498,7 +1618,12 @@ mod handler_tests { let _ctx = fake_supervisor_ctx(); let action = table - .dispatch(fake_notif(libc::SYS_openat as i32), -1, None) + .dispatch( + fake_notif(libc::SYS_openat as i32), + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) .await; assert!( @@ -1562,7 +1687,7 @@ mod handler_tests { let _sup = fake_supervisor_ctx(); let notif = fake_notif(libc::SYS_openat as i32); - let cx = HandlerCtx::new(notif, -1); + let cx = HandlerCtx::new(notif, std::fs::File::open("/dev/null").unwrap().into()); let action = h.handle(&cx).await; assert!(matches!(action, NotifAction::Continue)); @@ -1614,7 +1739,14 @@ mod handler_tests { // Walker MUST hit the struct's handle() each time, accumulating // state on &self.calls. for _ in 0..3 { - let action = table.dispatch(notif, -1, None).await; + let action = table + .dispatch( + notif, + fake_listener(), + None, + Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + ) + .await; assert!(matches!(action, NotifAction::Continue)); } diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index bd12e27c..861f428e 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -170,6 +170,8 @@ pub enum NotifAction { /// so it short-circuits the handler chain — a deferring handler makes a /// terminal decision. See [`Deferred`]. Defer(Deferred), + /// Suspend dispatch, then resume the handler chain and policy processing. + Suspend(Deferred), /// Execute only after the policy callback permits the operation. Prepared(PreparedOperation), } @@ -220,6 +222,10 @@ impl NotifAction { NotifAction::Defer(Deferred::new(fut)) } + pub fn suspend + Send + 'static>(fut: F) -> Self { + NotifAction::Suspend(Deferred::new(fut)) + } + /// Defer owned synchronous work with admission held until the work returns. pub fn blocking(work: impl FnOnce() -> NotifAction + Send + 'static) -> Self { Self::defer(async move { @@ -316,7 +322,9 @@ pub(crate) fn named_content_memfd(content: &[u8], seal: bool, name: &str) -> io: /// 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), + NotifAction::Defer(_) | NotifAction::Suspend(_) | NotifAction::Prepared(_) => { + NotifAction::Errno(libc::EIO) + } other => other, } } @@ -2130,7 +2138,7 @@ fn send_response(fd: RawFd, id: u64, action: NotifAction) -> io::Result<()> { }, NotifAction::ReturnValue(val) => respond_value(fd, id, val), NotifAction::Hold => Ok(()), // Don't send a response. - NotifAction::Defer(_) | NotifAction::Prepared(_) => { + NotifAction::Defer(_) | NotifAction::Suspend(_) | NotifAction::Prepared(_) => { // Unexecuted work must never become a successful syscall response. debug_assert!(false, "unexecuted operation reached send_response"); respond_errno(fd, id, libc::EIO) @@ -2877,7 +2885,7 @@ async fn emit_policy_event( /// 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; +pub(crate) 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 @@ -2909,6 +2917,12 @@ async fn run_deferred_within(deferred: Deferred, limit: std::time::Duration) -> enum Parked { Approval(Box, crate::policy_fn::Verdict), Deferred(u64, NotifAction), + Dispatch { + notif: SeccompNotif, + action: NotifAction, + open: Option>, + has_denied: bool, + }, } /// Parked notifications and the tids waiting on them, which the exec freeze @@ -2916,6 +2930,7 @@ enum Parked { struct ParkedSet { tasks: tokio::task::JoinSet, callers: HashMap, + active_work: Arc, } impl ParkedSet { @@ -2923,6 +2938,7 @@ impl ParkedSet { Self { tasks: tokio::task::JoinSet::new(), callers: HashMap::new(), + active_work: Arc::new(tokio::sync::Semaphore::new(DEFER_MAX_INFLIGHT)), } } @@ -2940,7 +2956,25 @@ impl ParkedSet { return Some(parked); } Err(e) => { - self.callers.remove(&e.id()); + if let Some((_, id)) = self.callers.remove(&e.id()) { + return Some(Parked::Deferred(id, NotifAction::Errno(libc::EIO))); + } + } + } + } + } + + fn try_join_next(&mut self) -> Option { + loop { + match self.tasks.try_join_next_with_id()? { + Ok((id, parked)) => { + self.callers.remove(&id); + return Some(parked); + } + Err(e) => { + if let Some((_, id)) = self.callers.remove(&e.id()) { + return Some(Parked::Deferred(id, NotifAction::Errno(libc::EIO))); + } } } } @@ -2986,10 +3020,11 @@ async fn handle_notification( notif: SeccompNotif, ctx: &Arc, dispatch_table: &super::dispatch::DispatchTable, - fd: RawFd, + listener: &Arc, parked: &mut ParkedSet, defer_sem: &Arc, ) { + let fd = listener.as_raw_fd(); let policy = &ctx.policy; // Ensure every pid that produces a notification has per-process @@ -3017,7 +3052,7 @@ async fn handle_notification( .then(|| OpenRequest::decode(¬if, fd, policy, &ctx.processes)) .flatten() .map(Arc::new); - let mut action = { + let 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); @@ -3026,22 +3061,60 @@ async fn handle_notification( } 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 + dispatch_table + .dispatch( + notif, + Arc::clone(listener), + open.clone(), + Arc::clone(&parked.active_work), + ) + .await + } + }; + + if let NotifAction::Suspend(work) = action { + match Arc::clone(defer_sem).try_acquire_owned() { + Ok(permit) => parked.spawn(¬if, async move { + let _permit = permit; + let action = tokio::time::timeout(DEFER_TIMEOUT, work.run()) + .await + .unwrap_or(NotifAction::Errno(libc::EIO)); + Parked::Dispatch { + notif, + action, + open, + has_denied, } - _ => action, + }), + Err(_) => { + let _ = send_response(fd, notif.id, NotifAction::Errno(libc::EAGAIN)); } } - }; + return; + } + finish_dispatch(notif, action, open, has_denied, ctx, fd, parked, defer_sem).await; +} + +async fn finish_dispatch( + notif: SeccompNotif, + mut action: NotifAction, + open: Option>, + has_denied: bool, + ctx: &Arc, + fd: RawFd, + parked: &mut ParkedSet, + defer_sem: &Arc, +) { + let policy = &ctx.policy; + let nr = notif.data.nr as i64; + // Resuming a handler must still pass through pinned-open enforcement. + 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; @@ -3210,10 +3283,17 @@ async fn finish_notification( // 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; + let mut memory_quiescence = 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); + match Arc::clone(&parked.active_work).try_acquire_many_owned(DEFER_MAX_INFLIGHT as u32) { + Ok(guard) => { + memory_quiescence = Some(guard); + let (frozen_action, freeze) = freeze_exec(¬if, judged, ctx, fd, parked); + action = frozen_action; + exec_freeze = Some(freeze); + } + Err(_) => action = NotifAction::Errno(libc::EAGAIN), + } } // Ignore error — child may have exited between recv and response. @@ -3231,6 +3311,7 @@ async fn finish_notification( // reap the queued interrupts and detach. crate::freeze::reap_pending(&freeze.pending_tids); } + drop(memory_quiescence); if let Some(trace) = creation_trace { if send_result.is_ok() { @@ -3263,6 +3344,16 @@ async fn settle_parked( defer_sem: &Arc, ) { match parked { + Parked::Dispatch { + notif, + action, + open, + has_denied, + } => { + if id_valid(fd, notif.id).is_ok() { + finish_dispatch(notif, action, open, has_denied, ctx, fd, set, defer_sem).await; + } + } Parked::Approval(prepared, verdict) => { finish_notification(*prepared, Some(verdict), ctx, fd, set, defer_sem).await } @@ -3291,14 +3382,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. @@ -3355,6 +3448,9 @@ pub async fn supervisor( drop(ready); loop { + if let Some(done) = parked.try_join_next() { + settle_parked(done, &ctx, fd, &mut parked, &defer_sem).await; + } match probe_notif_fd(fd) { NotifFdState::Pending => { let notif = match recv_notif(fd) { @@ -3371,8 +3467,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, &defer_sem) - .await; + handle_notification( + notif, + &ctx, + &dispatch_table, + async_fd.get_ref(), + &mut parked, + &defer_sem, + ) + .await; } NotifFdState::Empty => break, NotifFdState::Terminal => break 'outer, diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index a172cadb..5a39fc0f 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -1853,9 +1853,9 @@ sandlock_handler_t *sandlock_handler_new(int32_t (*handler_fn)(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. + * Select terminal deferral. All callbacks run off the notification loop; + * the default resumes the handler chain after the callback returns Continue. + * 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 diff --git a/crates/sandlock-ffi/src/handler/abi.rs b/crates/sandlock-ffi/src/handler/abi.rs index 53016875..11c71b7c 100644 --- a/crates/sandlock-ffi/src/handler/abi.rs +++ b/crates/sandlock-ffi/src/handler/abi.rs @@ -458,11 +458,8 @@ 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. + /// Terminal deferral skips later handlers; ordinary callbacks suspend + /// dispatch and resume the chain on Continue. Both run off the loop. pub(super) deferred: bool, } @@ -555,9 +552,9 @@ pub unsafe extern "C" fn sandlock_handler_new( 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. +/// Select terminal deferral. All callbacks run off the notification loop; +/// the default resumes the handler chain after the callback returns Continue. +/// 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 diff --git a/crates/sandlock-ffi/src/handler/adapter.rs b/crates/sandlock-ffi/src/handler/adapter.rs index 5131e5cd..7b240d90 100644 --- a/crates/sandlock-ffi/src/handler/adapter.rs +++ b/crates/sandlock-ffi/src/handler/adapter.rs @@ -5,8 +5,9 @@ //! dispatch loop can invoke C callbacks transparently. use std::future::Future; -use std::os::unix::io::FromRawFd; +use std::os::unix::io::{AsRawFd, FromRawFd}; use std::pin::Pin; +use std::sync::Arc; use sandlock_core::seccomp::dispatch::{Handler, HandlerCtx}; use sandlock_core::seccomp::notif::NotifAction; @@ -44,7 +45,7 @@ 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, + inner: Arc, } impl FfiHandler { @@ -58,7 +59,7 @@ impl FfiHandler { pub unsafe fn from_raw(raw: *mut sandlock_handler_t) -> Self { assert!(!raw.is_null(), "FfiHandler::from_raw on null pointer"); Self { - inner: Box::from_raw(raw), + inner: Arc::from(Box::from_raw(raw)), } } @@ -88,25 +89,14 @@ 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. + // Native work retains both the callback state and the listener after timeout. let notif_snap = crate::notif_repr::sandlock_notif_data_t::from(&cx.notif); - let notif_fd = cx.notif_fd; + let listener = cx.listener(); let notif_id = cx.notif.id; let pid = cx.notif.pid; // Resolve the trapped child's process group id for use as a fallback @@ -156,73 +146,61 @@ impl Handler for FfiHandler { } } }; - let handler_fn = self.inner.handler_fn; - let ud = UdPtr(self.inner.ud); + let handler = Arc::clone(&self.inner); let on_exception_fallback = self.exception_action(child_pgid); let deferred = self.inner.deferred; + let work_guard = match cx.blocking_guard() { + Ok(guard) => guard, + Err(e) => { + return Box::pin(async move { + NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO)) + }) + } + }; - // 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); + sandlock_core::blocking::run(move || { + let _work_guard = work_guard; + let mut mem = sandlock_mem_handle_t::new(listener.as_raw_fd(), notif_id, pid); let mut out = sandlock_action_out_t::zeroed(); - let rc = match handler_fn { + let rc_or_panic = match handler.handler_fn { Some(f) => std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - f(ud_raw, ¬if_snap, &mut mem, &mut out) + f(handler.ud, ¬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, - }; - - match rc_or_panic { - 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. - // 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. + match rc_or_panic { + 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. + // 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. + 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. + // SAFETY: see the `Ok(0) -> None` branch above. 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. - // SAFETY: see the `Ok(0) -> None` branch above. - unsafe { drain_pending_inject_fd(&out) }; - on_exception_fallback } - } + }) + .await + .unwrap_or_else(|e| NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO))) }; if deferred { @@ -230,7 +208,7 @@ impl Handler for FfiHandler { // 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 { NotifAction::suspend(run) }) } } } diff --git a/crates/sandlock-ffi/tests/handler_smoke.rs b/crates/sandlock-ffi/tests/handler_smoke.rs index ee6498c4..4dcc5867 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; +async fn resolve_handler(handler: &FfiHandler, cx: &HandlerCtx) -> NotifAction { + let mut action = handler.handle(cx).await; + while let NotifAction::Suspend(work) = action { + action = work.run().await; + } + action +} + fn fake_ctx() -> HandlerCtx { HandlerCtx::new( SeccompNotif { @@ -293,7 +301,7 @@ fn fake_ctx() -> HandlerCtx { args: [0; 6], }, }, - -1, + std::fs::File::open("/dev/null").unwrap().into(), ) } @@ -358,7 +366,7 @@ fn fake_ctx_with_isolated_child() -> (HandlerCtx, std::process::Child) { args: [0; 6], }, }, - -1, + std::fs::File::open("/dev/null").unwrap().into(), ); (ctx, child) } @@ -394,7 +402,7 @@ 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_handler(&h, &cx).await; assert!(matches!(action, NotifAction::ReturnValue(42))); } @@ -414,7 +422,7 @@ async fn deferred_ffi_handler_returns_defer_that_resolves_to_callback() { 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 action = resolve_handler(&h, &cx).await; let deferred = match action { NotifAction::Defer(d) => d, other => panic!("deferred handler must return Defer, got {other:?}"), @@ -423,8 +431,7 @@ async fn deferred_ffi_handler_returns_defer_that_resolves_to_callback() { } #[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). +async fn non_deferred_ffi_handler_suspends_dispatch() { let raw = unsafe { sandlock_ffi::handler::sandlock_handler_new( Some(return_value_42), @@ -435,7 +442,68 @@ async fn non_deferred_ffi_handler_stays_inline() { }; let h = unsafe { FfiHandler::from_raw(raw) }; let cx = fake_ctx(); - assert!(matches!(h.handle(&cx).await, NotifAction::ReturnValue(42))); + let NotifAction::Suspend(work) = h.handle(&cx).await else { + panic!("callback must suspend dispatch"); + }; + assert!(matches!(work.run().await, NotifAction::ReturnValue(42))); +} + +#[tokio::test] +async fn abandoned_callback_keeps_user_data_until_worker_exits() { + struct State { + started: Option>, + release: std::sync::mpsc::Receiver<()>, + dropped: Option>, + } + extern "C-unwind" fn callback( + ud: *mut std::ffi::c_void, + _: *const sandlock_ffi::notif_repr::sandlock_notif_data_t, + _: *mut sandlock_ffi::handler::sandlock_mem_handle_t, + out: *mut sandlock_ffi::handler::sandlock_action_out_t, + ) -> i32 { + let state = unsafe { &mut *ud.cast::() }; + let _ = state.started.take().unwrap().send(()); + let _ = state.release.recv(); + unsafe { sandlock_ffi::handler::sandlock_action_set_return_value(out, 42) }; + 0 + } + extern "C-unwind" fn destroy(ud: *mut std::ffi::c_void) { + let mut state = unsafe { Box::from_raw(ud.cast::()) }; + let _ = state.dropped.take().unwrap().send(()); + } + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let (dropped_tx, mut dropped_rx) = tokio::sync::oneshot::channel(); + let state = Box::new(State { + started: Some(started_tx), + release: release_rx, + dropped: Some(dropped_tx), + }); + let raw = unsafe { + sandlock_ffi::handler::sandlock_handler_new( + Some(callback), + Box::into_raw(state).cast(), + Some(destroy), + sandlock_exception_policy_t::DenyEio as u32, + ) + }; + let handler = unsafe { FfiHandler::from_raw(raw) }; + let cx = fake_ctx(); + let NotifAction::Suspend(work) = handler.handle(&cx).await else { + panic!("expected suspension"); + }; + let waiter = tokio::spawn(work.run()); + started_rx.await.unwrap(); + drop(handler); + drop(cx); + waiter.abort(); + let _ = waiter.await; + assert!(matches!( + dropped_rx.try_recv(), + Err(tokio::sync::oneshot::error::TryRecvError::Empty) + )); + release_tx.send(()).unwrap(); + dropped_rx.await.unwrap(); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -450,7 +518,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_handler(&h, &cx).await; assert!(matches!(action, NotifAction::Errno(e) if e == libc::EPERM)); } @@ -709,7 +777,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_handler(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Continue)); } @@ -725,7 +793,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_handler(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Errno(e) if e == 13)); } @@ -741,7 +809,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_handler(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Hold)); } @@ -757,7 +825,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_handler(&h, &fake_ctx()).await; assert!(matches!( action, NotifAction::KillGroup { sig, pgid } if sig == libc::SIGTERM && pgid == 1234 @@ -837,9 +905,9 @@ async fn ffi_handler_translates_kill_zero_pgid_substitutes_child_pgid() { args: [0; 6], }, }, - -1, + std::fs::File::open("/dev/null").unwrap().into(), ); - let action = h.handle(&cx).await; + let action = resolve_handler(&h, &cx).await; // Reap the child regardless of assertion outcome. let _ = child.kill(); @@ -874,7 +942,7 @@ fn fake_ctx_with_pid(pid: u32) -> HandlerCtx { args: [0; 6], }, }, - -1, + std::fs::File::open("/dev/null").unwrap().into(), ) } @@ -911,7 +979,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(&handler, &cx).await; assert!( matches!(action, NotifAction::Continue), "expected exception-policy fallback (Continue) when no safe pgid available, got {action:?}", @@ -965,7 +1033,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(&handler, &cx).await; // The action must be Continue (exception-policy fallback), NOT a // Kill that send_response would forward to killpg(0). @@ -1019,7 +1087,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(&handler, &cx).await; // Reap the child regardless of assertion outcome. let _ = child.kill(); @@ -1054,7 +1122,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(&handler, &cx).await; assert!( matches!(action, NotifAction::Continue), "expected exception-policy fallback (Continue) on ESRCH, got {action:?}", @@ -1081,7 +1149,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_handler(&h, &cx).await; let _ = child.kill(); let _ = child.wait(); assert!(matches!(action, NotifAction::KillGroup { sig, .. } if sig == libc::SIGKILL)); @@ -1099,7 +1167,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_handler(&h, &fake_ctx()).await; assert!(matches!(action, NotifAction::Continue)); } @@ -1136,7 +1204,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_handler(&h, &cx).await; let _ = child.kill(); let _ = child.wait(); // The `catch_unwind` inside `spawn_blocking` swallows the panic and @@ -1167,7 +1235,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_handler(&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 +2096,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_handler(&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 +2171,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_handler(&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 +2386,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_handler(&h, &cx).await; assert!( matches!(action, NotifAction::Errno(e) if e == libc::EIO), "expected Errno(EIO), got {:?}", From 2cdb3796d61002f7b3526d604d1323d6466c74c2 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 1 Nov 2026 16:25:56 -0800 Subject: [PATCH 5/7] filesystem: move chroot and COW handlers onto blocking workers Chroot path lookup and COW filesystem operations perform synchronous I/O while dispatching a notification. Offloading only the copy leaves those lookups on the notification loop and can release COW state before a copy whose waiter was dropped has finished. Suspend these handlers on the isolated filesystem pool and keep the owned COW state lock through the whole operation, including copying and quota bookkeeping. Run copies directly on that worker and fail a write with EIO when copying fails. Route image helper work through the same pool. Signed-off-by: Cong Wang --- crates/sandlock-core/src/cow/dispatch.rs | 143 +++++++------------ crates/sandlock-core/src/cow/seccomp.rs | 2 +- crates/sandlock-core/src/image/mod.rs | 2 +- crates/sandlock-core/src/seccomp/dispatch.rs | 74 +++++++++- 4 files changed, 124 insertions(+), 97 deletions(-) diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index b65a789e..477fb3dc 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -28,7 +28,6 @@ use std::os::unix::io::{FromRawFd, OwnedFd, RawFd}; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; -use tokio::sync::Mutex; use tokio::sync::Mutex as AsyncMutex; use crate::arch; @@ -200,7 +199,7 @@ pub(crate) fn open_confined( /// Handle open/openat/openat2 under workdir: redirect to COW upper/lower. pub(crate) async fn handle_cow_open( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -222,9 +221,9 @@ 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) + // Keep preparation, copy and bookkeeping under the same owned state guard. let (plan, upper_root, workdir_root) = { - let mut st = cow_state.lock().await; + let st = &mut *cow_state; let cow = match st.branch.as_mut() { Some(c) => c, None => return NotifAction::Continue, @@ -253,9 +252,8 @@ pub(crate) async fn handle_cow_open( }; (plan, upper_root, workdir_root) }; - // Lock is released here - // Phase 2: execute I/O plan without holding the lock + // Execute the copy before exposing the upper file to another handler. let real_path = match plan { CowOpenPlan::Skip => return NotifAction::Continue, // Deleted in this branch (whiteout): the lower file still exists, so @@ -268,41 +266,22 @@ pub(crate) async fn handle_cow_open( 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); - } + } => match SeccompCowBranch::execute_copy(&workdir_root, &upper_root, &rel_path) { + Ok(digest) => { + if let Some(d) = digest { + if let Some(cow) = cow_state.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); + upper + } + Err(e) => { + if let Some(cow) = cow_state.branch.as_mut() { + cow.rollback_copy(file_size); } + return NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO)); } - } + }, }; // Phase 3: open the resolved path and inject fd @@ -617,34 +596,27 @@ fn cow_copy_rel<'a>( cow.safe_rel(copy_path).map(|rel| (match_path, rel)) } -/// Execute a deferred `CowCopyPlan::NeedsCopy` on a blocking thread. +/// Execute a copy plan on the filesystem worker. /// Returns the upper path on success, or rolls back quota on failure. async fn execute_deferred_copy( - cow_state: &Arc>, + cow_state: &mut CowState, 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)) => { + match SeccompCowBranch::execute_copy(&workdir_root, &upper_root, &rel) { + 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); + if let Some(cow) = cow_state.branch.as_mut() { + cow.record_digest(&rel, d); } } Some(upper) } - _ => { - let mut st = cow_state.lock().await; - if let Some(cow) = st.branch.as_mut() { + Err(_) => { + if let Some(cow) = cow_state.branch.as_mut() { cow.rollback_copy(file_size); } None @@ -655,14 +627,11 @@ async fn execute_deferred_copy( /// 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). +/// The worker owns COW state through preparation, copying and mutation so +/// another handler or branch finalization cannot observe a partial copy. pub(crate) async fn handle_cow_write( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -672,10 +641,9 @@ 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. + // Pre-copy existing data before applying the mutation. let (copy_plan, copy_workdir, copy_upper, copy_rel) = { - let mut st = cow_state.lock().await; + let st = &mut *cow_state; let cow = match st.branch.as_mut() { Some(c) => c, None => return NotifAction::Continue, @@ -712,9 +680,8 @@ pub(crate) async fn handle_cow_write( ), } }; - // Lock is released here - // Phase 2: execute the file copy outside the lock (if needed) + // Execute the copy before mutating the staged file. if let Some(crate::cow::seccomp::CowCopyPlan::NeedsCopy { upper, lower: _lower, @@ -732,13 +699,12 @@ pub(crate) async fn handle_cow_write( .await .is_none() { - return NotifAction::Continue; + return NotifAction::Errno(libc::EIO); } } - // 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; + // ensure_cow_copy is now a no-op for the staged file. + let st = &mut *cow_state; let cow = match st.branch.as_mut() { Some(c) => c, None => return NotifAction::Continue, @@ -842,7 +808,7 @@ pub(crate) async fn handle_cow_write( /// don't fail before the COW layer can redirect their writes. pub(crate) async fn handle_cow_access( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -876,7 +842,7 @@ pub(crate) async fn handle_cow_access( return NotifAction::Continue; } - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -905,7 +871,7 @@ pub(crate) async fn handle_cow_access( /// utimensat(dirfd, pathname, times, flags) pub(crate) async fn handle_cow_utimensat( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -925,7 +891,7 @@ pub(crate) async fn handle_cow_utimensat( }; let (upper_path, upper_root, workdir_root) = { - let mut st = cow_state.lock().await; + let st = &mut *cow_state; let cow = match st.branch.as_mut() { Some(c) => c, None => return NotifAction::Continue, @@ -987,7 +953,7 @@ pub(crate) async fn handle_cow_utimensat( /// Actually, simpler: for stat, we do the stat ourselves and write the result. pub(crate) async fn handle_cow_stat( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1019,7 +985,7 @@ pub(crate) async fn handle_cow_stat( }; let (real_path, upper_root, workdir_root) = { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1098,7 +1064,7 @@ pub(crate) async fn handle_cow_stat( /// statx the resolved path in the supervisor and write the buffer back. pub(crate) async fn handle_cow_statx( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1123,7 +1089,7 @@ pub(crate) async fn handle_cow_statx( }; let (real_path, upper_root, workdir_root) = { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1172,7 +1138,7 @@ pub(crate) async fn handle_cow_statx( /// them alone. pub(crate) async fn handle_cow_exec( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1208,7 +1174,7 @@ pub(crate) async fn handle_cow_exec( let resolved = resolve_at_path_with_virtual(notif, dirfd, &rel_path, virtual_cwd.as_deref()); let (upper_path, upper_root, workdir_root) = { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1282,7 +1248,7 @@ pub(crate) async fn handle_cow_exec( /// Handle readlinkat — read symlink from upper/lower, write to child buffer. pub(crate) async fn handle_cow_readlink( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1296,7 +1262,7 @@ pub(crate) async fn handle_cow_readlink( let buf_addr = notif.data.args[2]; let bufsiz = (notif.data.args[3] & 0xFFFFFFFF) as usize; - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1311,8 +1277,6 @@ pub(crate) async fn handle_cow_readlink( Some(t) => t, None => return NotifAction::Errno(libc::ENOENT), }; - drop(st); - let target_bytes = target.as_bytes(); let write_len = target_bytes.len().min(bufsiz); @@ -1334,7 +1298,7 @@ pub(crate) async fn handle_cow_readlink( /// Handle getdents64 for COW directories — merge upper + lower entries. pub(crate) async fn handle_cow_getdents( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1356,10 +1320,9 @@ pub(crate) async fn handle_cow_getdents( return NotifAction::Continue; } - // Compute rel_path under the global COW lock, but do not hold it - // across the per-process lock acquired below. + // COW handlers acquire branch state before per-process state. let rel_path = { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1403,7 +1366,7 @@ pub(crate) async fn handle_cow_getdents( // Build cache on first call. if !perproc.cow_dir_cache.contains_key(&child_fd) { let entries = { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1481,7 +1444,7 @@ pub(crate) async fn handle_cow_getdents( /// the path arg to /proc/self/fd/N so the kernel chdir succeeds. pub(crate) async fn handle_cow_chdir( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1497,7 +1460,7 @@ pub(crate) async fn handle_cow_chdir( resolve_at_path_with_virtual(notif, libc::AT_FDCWD as i64, &path, virtual_cwd.as_deref()); let (abs_path, upper_path, upper_root, workdir_root) = { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, @@ -1592,7 +1555,7 @@ pub(crate) async fn handle_cow_chdir( /// Handle getcwd after chdir into a COW-only directory. pub(crate) async fn handle_cow_getcwd( notif: &SeccompNotif, - cow_state: &Arc>, + cow_state: &mut CowState, processes: &Arc, notif_fd: RawFd, ) -> NotifAction { @@ -1603,7 +1566,7 @@ pub(crate) async fn handle_cow_getcwd( let virtual_cwd = if let Some(cwd) = cached_virtual_cwd { cwd } else { - let st = cow_state.lock().await; + let st = &*cow_state; let cow = match st.branch.as_ref() { Some(c) => c, None => return NotifAction::Continue, diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index 81101d4d..57c249b3 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -1247,7 +1247,7 @@ impl SeccompCowBranch { } /// Execute a file copy synchronously. Used by `ensure_cow_copy` and the - /// async dispatch (via `spawn_blocking`). + /// filesystem dispatch worker. pub fn execute_copy( workdir_root: &Path, upper_root: &Path, diff --git a/crates/sandlock-core/src/image/mod.rs b/crates/sandlock-core/src/image/mod.rs index 6cac1b51..bb1bb605 100644 --- a/crates/sandlock-core/src/image/mod.rs +++ b/crates/sandlock-core/src/image/mod.rs @@ -206,7 +206,7 @@ fn resolve_working_dir(rootfs: &Path, dir: &str) -> Result( f: impl FnOnce() -> Result + Send + 'static, ) -> Result { - tokio::task::spawn_blocking(f) + crate::blocking::filesystem(f) .await .map_err(|e| SandboxRuntimeError::Child(format!("image task failed: {e}")))? } diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 03a57512..040d7ac0 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -119,6 +119,28 @@ impl HandlerCtx { .map_err(|_| std::io::Error::from_raw_os_error(libc::EAGAIN)) } + fn filesystem( + &self, + work: impl std::future::Future + Send + 'static, + ) -> NotifAction { + let guard = match self.blocking_guard() { + Ok(guard) => guard, + Err(e) => return NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO)), + }; + let listener = self.listener(); + let runtime = tokio::runtime::Handle::current(); + NotifAction::suspend(async move { + crate::blocking::filesystem(move || { + let _guard = guard; + let _listener = listener; + // Filesystem handlers await state locks, but perform synchronous I/O. + runtime.block_on(work) + }) + .await + .unwrap_or_else(|e| NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO))) + }) + } + pub(crate) fn open(&self) -> Option> { self.open.clone() } @@ -864,10 +886,11 @@ fn register_chroot_handlers( let processes = Arc::clone(&processes); let notif_fd = cx.notif_fd; let policy = Arc::clone(&policy); - async move { + let action = cx.filesystem(async move { let chroot_ctx = ChrootCtx::new(&policy, &processes); $handler(¬if, &chroot_state, &cow_state, notif_fd, &chroot_ctx).await - } + }); + async move { action } } }}; } @@ -887,10 +910,11 @@ fn register_chroot_handlers( let processes = Arc::clone(&processes); let notif_fd = cx.notif_fd; let policy = Arc::clone(&policy); - async move { + let action = cx.filesystem(async move { let chroot_ctx = ChrootCtx::new(&policy, &processes); $handler(¬if, &chroot_state, &cow_state, notif_fd, &chroot_ctx).await - } + }); + async move { action } } }}; } @@ -1159,7 +1183,11 @@ fn register_cow_handlers(table: &mut DispatchTable, ctx: &Arc) { let cow_state = Arc::clone(&cow_state); let processes_state = Arc::clone(&processes_state); let notif_fd = cx.notif_fd; - async move { $handler(¬if, &cow_state, &processes_state, notif_fd).await } + let action = cx.filesystem(async move { + let mut state = cow_state.lock_owned().await; + $handler(¬if, &mut state, &processes_state, notif_fd).await + }); + async move { action } } }}; } @@ -1544,6 +1572,42 @@ mod handler_tests { Arc::new(std::fs::File::open("/dev/null").unwrap().into()) } + #[tokio::test] + async fn filesystem_work_retains_state_and_exec_guard_after_waiter_abort() { + let cx = HandlerCtx::new( + fake_notif(libc::SYS_openat as i32), + std::fs::File::open("/dev/null").unwrap().into(), + ); + let state = Arc::new(Mutex::new(0)); + let work_state = Arc::clone(&state); + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let action = cx.filesystem(async move { + let mut state = work_state.lock_owned().await; + let _ = started_tx.send(()); + let _ = release_rx.recv(); + *state = 42; + NotifAction::Continue + }); + let NotifAction::Suspend(work) = action else { + panic!("expected suspension") + }; + let waiter = tokio::spawn(work.run()); + started_rx.await.unwrap(); + waiter.abort(); + let _ = waiter.await; + assert!(state.try_lock().is_err()); + assert!(Arc::clone(&cx.active_work) + .try_acquire_many_owned(super::super::notif::DEFER_MAX_INFLIGHT as u32) + .is_err()); + release_tx.send(()).unwrap(); + assert_eq!(*state.lock().await, 42); + let _quiescent = Arc::clone(&cx.active_work) + .acquire_many_owned(super::super::notif::DEFER_MAX_INFLIGHT as u32) + .await + .unwrap(); + } + #[tokio::test] async fn suspended_continue_resumes_remaining_handlers() { let mut table = DispatchTable::new(); From 31366f299fdd00225bdce6f7fd8b15381d6e6fb0 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 1 Nov 2026 16:32:55 -0800 Subject: [PATCH 6/7] seccomp: keep resumed dispatch ordered with exec and fork Resuming the handler chain in a parked task lets it start native memory access while the notification loop validates exec arguments. Shared admission also lets stalled callbacks exclude filesystem work, and a fork slot reserved before suspension can leak when the request never resumes. Return suspended dispatch state to the notification loop before advancing the remaining handlers, keeping one deadline across suspensions. Track native memory access separately from admission and give filesystem waits their own capacity. Drain completed work during notification intake so a busy child cannot indefinitely postpone it. Reserve fork slots only after handlers and approval allow Continue, roll back failed responses, and refuse terminal deferral for fork-like calls because it bypasses accounting. Signed-off-by: Cong Wang --- crates/sandlock-core/src/seccomp/dispatch.rs | 239 ++++++++++++------- crates/sandlock-core/src/seccomp/notif.rs | 199 ++++++++++----- crates/sandlock-ffi/include/sandlock.h | 2 +- crates/sandlock-ffi/src/handler/abi.rs | 2 +- crates/sandlock-ffi/src/handler/adapter.rs | 9 +- 5 files changed, 291 insertions(+), 160 deletions(-) diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 040d7ac0..9801eee4 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -18,6 +18,7 @@ use std::collections::HashMap; use std::os::unix::io::{AsRawFd, OwnedFd, RawFd}; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use super::ctx::SupervisorCtx; @@ -28,7 +29,7 @@ use crate::arch; use crate::sys::structs::SeccompNotif; use thiserror::Error; -use tokio::sync::{Mutex, OwnedSemaphorePermit, Semaphore}; +use tokio::sync::Mutex; // ============================================================ // Types @@ -77,7 +78,7 @@ pub struct HandlerCtx { pub notif: SeccompNotif, pub notif_fd: RawFd, listener: Arc, - active_work: Arc, + active_work: Arc, /// The open-family request decoded once for this notification. open: Option>, } @@ -88,7 +89,7 @@ impl HandlerCtx { notif, Arc::new(notif_fd), None, - Arc::new(Semaphore::new(super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) } @@ -96,7 +97,7 @@ impl HandlerCtx { notif: SeccompNotif, listener: Arc, open: Option>, - active_work: Arc, + active_work: Arc, ) -> Self { Self { notif, @@ -111,25 +112,21 @@ impl HandlerCtx { Arc::clone(&self.listener) } - /// Retain this permit inside native work that can access child memory. + /// Retain this guard inside native work that can access child memory. /// Exec argument validation requires all such work to have finished. - pub fn blocking_guard(&self) -> std::io::Result { - Arc::clone(&self.active_work) - .try_acquire_owned() - .map_err(|_| std::io::Error::from_raw_os_error(libc::EAGAIN)) + pub fn blocking_guard(&self) -> BlockingGuard { + self.active_work.fetch_add(1, Ordering::Relaxed); + BlockingGuard(Arc::clone(&self.active_work)) } fn filesystem( &self, work: impl std::future::Future + Send + 'static, ) -> NotifAction { - let guard = match self.blocking_guard() { - Ok(guard) => guard, - Err(e) => return NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO)), - }; + let guard = self.blocking_guard(); let listener = self.listener(); let runtime = tokio::runtime::Handle::current(); - NotifAction::suspend(async move { + NotifAction::suspend_filesystem(async move { crate::blocking::filesystem(move || { let _guard = guard; let _listener = listener; @@ -146,6 +143,16 @@ impl HandlerCtx { } } +/// Tracks native child-memory access until its worker finishes. +#[must_use = "retain the guard until native child-memory access finishes"] +pub struct BlockingGuard(Arc); + +impl Drop for BlockingGuard { + fn drop(&mut self) { + self.0.fetch_sub(1, Ordering::Release); + } +} + // Blanket impl: any Fn(&HandlerCtx) -> Future is a Handler. // // Lets lightweight closure-style handlers work without ceremony at the @@ -298,48 +305,74 @@ impl DispatchTable { .push(handler); } - /// Dispatch a notification through the handler chain for its syscall number. + /// Dispatch until a handler produces an action or suspends. pub(crate) async fn dispatch( &self, notif: SeccompNotif, listener: Arc, open: Option>, - active_work: Arc, - ) -> NotifAction { - let nr = notif.data.nr as i64; - if let Some(chain) = self.chains.get(&nr) { - let handler_ctx = HandlerCtx::with_listener(notif, listener, open, active_work); - for (index, handler) in chain.handlers.iter().enumerate() { - let action = handler.handle(&handler_ctx).await; - if let NotifAction::Suspend(work) = action { - let remaining = chain.handlers[index + 1..].to_vec(); - return NotifAction::suspend(resume_chain(work, remaining, handler_ctx)); - } - if !matches!(action, NotifAction::Continue) { - return action; - } - } + active_work: Arc, + ) -> DispatchState { + let handlers = self + .chains + .get(&(notif.data.nr as i64)) + .map(|chain| chain.handlers.clone()) + .unwrap_or_default(); + DispatchState { + action: NotifAction::Continue, + handlers: handlers.into_iter(), + cx: HandlerCtx::with_listener(notif, listener, open, active_work), + deadline: tokio::time::Instant::now() + super::notif::DEFER_TIMEOUT, } - NotifAction::Continue + .advance() + .await } } -async fn resume_chain( - work: super::notif::Deferred, - remaining: Vec>, +pub(crate) struct DispatchState { + action: NotifAction, + handlers: std::vec::IntoIter>, cx: HandlerCtx, -) -> NotifAction { - let mut action = NotifAction::Suspend(work); - let mut handlers = remaining.into_iter(); - loop { - action = match action { - NotifAction::Suspend(work) => work.run().await, - NotifAction::Continue => match handlers.next() { - Some(handler) => handler.handle(&cx).await, - None => return NotifAction::Continue, - }, - other => return other, - }; + deadline: tokio::time::Instant, +} + +impl DispatchState { + pub(crate) fn pending(&self) -> Option { + match &self.action { + NotifAction::Suspend(work) => Some(work.filesystem), + _ => None, + } + } + + pub(crate) async fn wait(mut self) -> Self { + if let NotifAction::Suspend(work) = + std::mem::replace(&mut self.action, NotifAction::Errno(libc::EIO)) + { + self.action = tokio::time::timeout_at(self.deadline, work.run()) + .await + .unwrap_or(NotifAction::Errno(libc::EIO)); + } + self + } + + // Only the notification loop advances handlers; they may write child memory. + pub(crate) async fn advance(mut self) -> Self { + while matches!(self.action, NotifAction::Continue) { + let Some(handler) = self.handlers.next() else { + break; + }; + self.action = handler.handle(&self.cx).await; + } + self + } + + pub(crate) fn into_action(self) -> NotifAction { + match self.action { + NotifAction::Defer(work) => { + NotifAction::Defer(work.retain_listener(self.cx.listener())) + } + other => other, + } } } @@ -363,20 +396,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) // ------------------------------------------------------------------ @@ -1451,9 +1470,10 @@ mod handler_tests { fake_notif(libc::SYS_openat as i32), fake_listener(), None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) - .await; + .await + .into_action(); assert!(matches!(action, NotifAction::Continue)); let recorded = order.lock().unwrap(); @@ -1503,9 +1523,10 @@ mod handler_tests { fake_notif(libc::SYS_openat as i32), fake_listener(), None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) - .await; + .await + .into_action(); assert!(matches!(action, NotifAction::Continue)); let recorded = order.lock().unwrap(); @@ -1553,9 +1574,10 @@ mod handler_tests { fake_notif(libc::SYS_openat as i32), fake_listener(), None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) - .await; + .await + .into_action(); match action { NotifAction::Errno(e) => assert_eq!(e, libc::EACCES), @@ -1572,6 +1594,45 @@ mod handler_tests { Arc::new(std::fs::File::open("/dev/null").unwrap().into()) } + #[tokio::test] + async fn native_memory_tracking_does_not_limit_filesystem_admission() { + let cx = HandlerCtx::new( + fake_notif(libc::SYS_openat as i32), + std::fs::File::open("/dev/null").unwrap().into(), + ); + let _stalled: Vec<_> = (0..super::super::notif::DEFER_MAX_INFLIGHT) + .map(|_| cx.blocking_guard()) + .collect(); + let NotifAction::Suspend(work) = cx.filesystem(async { NotifAction::ReturnValue(42) }) + else { + panic!("filesystem admission depends on unrelated native work"); + }; + assert!(matches!(work.run().await, NotifAction::ReturnValue(42))); + } + + #[tokio::test] + async fn suspended_fork_does_not_reserve_a_process_slot() { + let ctx = fake_supervisor_ctx(); + ctx.resource.lock().await.max_processes = 2; + let mut table = build_dispatch_table(&ctx.policy, &ctx.resource, &ctx, Vec::new()); + table.register(libc::SYS_clone, |_cx: &HandlerCtx| async { + NotifAction::suspend(async { NotifAction::Errno(libc::EIO) }) + }); + let state = table + .dispatch( + fake_notif(libc::SYS_clone as i32), + fake_listener(), + None, + Arc::new(AtomicUsize::new(0)), + ) + .await; + assert!(state.pending().is_some()); + assert_eq!(ctx.resource.lock().await.proc_count, 0); + let action = state.wait().await.advance().await.into_action(); + assert!(matches!(action, NotifAction::Errno(libc::EIO))); + assert_eq!(ctx.resource.lock().await.proc_count, 0); + } + #[tokio::test] async fn filesystem_work_retains_state_and_exec_guard_after_waiter_abort() { let cx = HandlerCtx::new( @@ -1597,15 +1658,12 @@ mod handler_tests { waiter.abort(); let _ = waiter.await; assert!(state.try_lock().is_err()); - assert!(Arc::clone(&cx.active_work) - .try_acquire_many_owned(super::super::notif::DEFER_MAX_INFLIGHT as u32) - .is_err()); + assert_eq!(cx.active_work.load(Ordering::Acquire), 1); release_tx.send(()).unwrap(); assert_eq!(*state.lock().await, 42); - let _quiescent = Arc::clone(&cx.active_work) - .acquire_many_owned(super::super::notif::DEFER_MAX_INFLIGHT as u32) - .await - .unwrap(); + while cx.active_work.load(Ordering::Acquire) != 0 { + tokio::task::yield_now().await; + } } #[tokio::test] @@ -1625,14 +1683,15 @@ mod handler_tests { fake_notif(libc::SYS_openat as i32), fake_listener(), None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) .await; assert_eq!(calls.load(Ordering::SeqCst), 0); - let NotifAction::Suspend(work) = action else { - panic!("expected suspension") - }; - assert!(matches!(work.run().await, NotifAction::ReturnValue(42))); + assert!(action.pending().is_some()); + let state = action.wait().await; + assert_eq!(calls.load(Ordering::SeqCst), 0); + let action = state.advance().await.into_action(); + assert!(matches!(action, NotifAction::ReturnValue(42))); assert_eq!(calls.load(Ordering::SeqCst), 1); } @@ -1650,13 +1709,12 @@ mod handler_tests { fake_notif(libc::SYS_openat as i32), fake_listener(), None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) .await; - let NotifAction::Suspend(work) = action else { - panic!("expected suspension") - }; - assert!(matches!(work.run().await, NotifAction::Errno(libc::EACCES))); + assert!(action.pending().is_some()); + let action = action.wait().await.advance().await.into_action(); + assert!(matches!(action, NotifAction::Errno(libc::EACCES))); } /// A handler returning `Defer` is non-`Continue`, so it must short-circuit @@ -1686,9 +1744,10 @@ mod handler_tests { fake_notif(libc::SYS_openat as i32), fake_listener(), None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), + Arc::new(AtomicUsize::new(0)), ) - .await; + .await + .into_action(); assert!( matches!(action, NotifAction::Defer(_)), @@ -1804,13 +1863,9 @@ mod handler_tests { // state on &self.calls. for _ in 0..3 { let action = table - .dispatch( - notif, - fake_listener(), - None, - Arc::new(Semaphore::new(super::super::notif::DEFER_MAX_INFLIGHT)), - ) - .await; + .dispatch(notif, fake_listener(), None, Arc::new(AtomicUsize::new(0))) + .await + .into_action(); assert!(matches!(action, NotifAction::Continue)); } diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index 861f428e..f2fefc88 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -8,6 +8,7 @@ use std::io; use std::net::IpAddr; use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, IntoRawFd, OwnedFd, RawFd}; use std::pin::Pin; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use crate::arch; @@ -51,9 +52,8 @@ impl OnInjectSuccess { /// 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`. +/// resolves. The future outlives the borrowed `HandlerCtx`; capture owned +/// state and `HandlerCtx::listener()` when using the notification listener. /// /// The deferred future need only be `Send` (not `Sync`): the supervisor /// moves it onto a worker task and never shares it by reference. Requiring @@ -62,6 +62,7 @@ impl OnInjectSuccess { pub struct Deferred { future: Pin + Send + 'static>>, patient: bool, + pub(crate) filesystem: bool, } // Safety: `NotifAction` must stay `Sync` so it can live in `Sync` contexts @@ -86,6 +87,7 @@ impl Deferred { Self { future: Box::pin(f), patient: false, + filesystem: false, } } @@ -95,6 +97,7 @@ impl Deferred { Self { future: Box::pin(f), patient: true, + filesystem: false, } } @@ -103,6 +106,22 @@ impl Deferred { pub async fn run(self) -> NotifAction { self.future.await } + + pub(crate) fn retain_listener(self, listener: Arc) -> Self { + let Self { + future, + patient, + filesystem, + } = self; + Self { + future: Box::pin(async move { + let _listener = listener; + future.await + }), + patient, + filesystem, + } + } } /// An owned operation whose side effects require callback approval. @@ -226,7 +245,16 @@ impl NotifAction { NotifAction::Suspend(Deferred::new(fut)) } + pub(crate) fn suspend_filesystem + Send + 'static>( + fut: F, + ) -> Self { + let mut work = Deferred::new(fut); + work.filesystem = true; + Self::Suspend(work) + } + /// Defer owned synchronous work with admission held until the work returns. + /// Work accessing child memory must also retain `HandlerCtx::blocking_guard()`. pub fn blocking(work: impl FnOnce() -> NotifAction + Send + 'static) -> Self { Self::defer(async move { match crate::blocking::run(work).await { @@ -2882,16 +2910,15 @@ async fn emit_policy_event( /// 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. +/// Maximum parked futures per admission class. Native work has separate +/// process-wide bounds that remain occupied after a waiter times out. pub(crate) 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); +pub(crate) const DEFER_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); /// Drive a deferred future to its terminal action, bounded by `limit`. /// @@ -2919,7 +2946,7 @@ enum Parked { Deferred(u64, NotifAction), Dispatch { notif: SeccompNotif, - action: NotifAction, + state: super::dispatch::DispatchState, open: Option>, has_denied: bool, }, @@ -2930,7 +2957,8 @@ enum Parked { struct ParkedSet { tasks: tokio::task::JoinSet, callers: HashMap, - active_work: Arc, + active_work: Arc, + filesystem_sem: Arc, } impl ParkedSet { @@ -2938,10 +2966,24 @@ impl ParkedSet { Self { tasks: tokio::task::JoinSet::new(), callers: HashMap::new(), - active_work: Arc::new(tokio::sync::Semaphore::new(DEFER_MAX_INFLIGHT)), + active_work: Arc::new(AtomicUsize::new(0)), + filesystem_sem: Arc::new(tokio::sync::Semaphore::new(DEFER_MAX_INFLIGHT)), } } + fn admit( + &self, + filesystem: bool, + general: &Arc, + ) -> Result { + let admission = if filesystem { + &self.filesystem_sem + } else { + general + }; + Arc::clone(admission).try_acquire_owned() + } + fn spawn(&mut self, notif: &SeccompNotif, task: impl Future + Send + 'static) { let handle = self.tasks.spawn(task); self.callers @@ -2995,7 +3037,6 @@ impl ParkedSet { struct Prepared { notif: SeccompNotif, action: NotifAction, - fork_counted: bool, exec_image: Option, } @@ -3052,36 +3093,52 @@ async fn handle_notification( .then(|| OpenRequest::decode(¬if, fd, policy, &ctx.processes)) .flatten() .map(Arc::new); - let 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. - dispatch_table - .dispatch( - notif, - Arc::clone(listener), - open.clone(), - Arc::clone(&parked.active_work), - ) - .await - } - }; + 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 { + finish_dispatch( + notif, + NotifAction::Errno(libc::EACCES), + open, + has_denied, + ctx, + fd, + parked, + defer_sem, + ) + .await; + return; + } + let state = dispatch_table + .dispatch( + notif, + Arc::clone(listener), + open.clone(), + Arc::clone(&parked.active_work), + ) + .await; + finish_dispatch_state(notif, state, open, has_denied, ctx, fd, parked, defer_sem).await; +} - if let NotifAction::Suspend(work) = action { - match Arc::clone(defer_sem).try_acquire_owned() { +async fn finish_dispatch_state( + notif: SeccompNotif, + state: super::dispatch::DispatchState, + open: Option>, + has_denied: bool, + ctx: &Arc, + fd: RawFd, + parked: &mut ParkedSet, + defer_sem: &Arc, +) { + if let Some(filesystem) = state.pending() { + match parked.admit(filesystem, defer_sem) { Ok(permit) => parked.spawn(¬if, async move { let _permit = permit; - let action = tokio::time::timeout(DEFER_TIMEOUT, work.run()) - .await - .unwrap_or(NotifAction::Errno(libc::EIO)); + let state = state.wait().await; Parked::Dispatch { notif, - action, + state, open, has_denied, } @@ -3092,7 +3149,17 @@ async fn handle_notification( } return; } - finish_dispatch(notif, action, open, has_denied, ctx, fd, parked, defer_sem).await; + finish_dispatch( + notif, + state.into_action(), + open, + has_denied, + ctx, + fd, + parked, + defer_sem, + ) + .await; } async fn finish_dispatch( @@ -3120,9 +3187,6 @@ async fn finish_dispatch( 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); - // Read without freezing so the sandbox keeps running while policy_fn // decides; `freeze_exec` re-reads it under the freeze before Continue. let exec_image = (matches!(action, NotifAction::Continue) @@ -3134,7 +3198,6 @@ async fn finish_dispatch( let prepared = Prepared { notif, action, - fork_counted, exec_image, }; match wait { @@ -3199,11 +3262,13 @@ async fn finish_notification( parked: &mut ParkedSet, defer_sem: &Arc, ) { + if id_valid(fd, prepared.notif.id).is_err() { + return; + } let policy = &ctx.policy; let Prepared { notif, mut action, - fork_counted, exec_image, } = prepared; let nr = notif.data.nr as i64; @@ -3224,8 +3289,11 @@ async fn finish_notification( action = operation.execute(); } - if fork_counted && !matches!(action, NotifAction::Continue) { - crate::resource::rollback_fork_count(&ctx.resource).await; + 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); } // With policy_fn active, fork-like syscalls are traced for exactly @@ -3249,6 +3317,7 @@ async fn finish_notification( ); if fork_counted { crate::resource::rollback_fork_count(&ctx.resource).await; + fork_counted = false; } action = NotifAction::Errno(libc::EPERM); } @@ -3260,11 +3329,11 @@ async fn finish_notification( // 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 + // execve argv-safety freeze or fork accounting: answering later // would skip that TOCTOU-closing work. if let NotifAction::Defer(deferred) = action { if crate::freeze::requires_freeze_on_continue(nr) - || crate::resource::requires_process_creation_tracking(¬if, policy) + || arch::fork_like_syscalls().contains(&nr) { let _ = send_response(fd, notif.id, NotifAction::Errno(libc::EPERM)); return; @@ -3283,16 +3352,15 @@ async fn finish_notification( // 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; - let mut memory_quiescence = None; if let (NotifAction::Continue, Some(judged)) = (&action, &exec_image) { - match Arc::clone(&parked.active_work).try_acquire_many_owned(DEFER_MAX_INFLIGHT as u32) { - Ok(guard) => { - memory_quiescence = Some(guard); - let (frozen_action, freeze) = freeze_exec(¬if, judged, ctx, fd, parked); - action = frozen_action; - exec_freeze = Some(freeze); - } - Err(_) => action = NotifAction::Errno(libc::EAGAIN), + // Guards are acquired only while advancing handlers on this loop. + // Native workers can release them, but cannot start new guarded work here. + if parked.active_work.load(Ordering::Acquire) == 0 { + let (frozen_action, freeze) = freeze_exec(¬if, judged, ctx, fd, parked); + action = frozen_action; + exec_freeze = Some(freeze); + } else { + action = NotifAction::Errno(libc::EAGAIN); } } @@ -3311,8 +3379,10 @@ async fn finish_notification( // reap the queued interrupts and detach. crate::freeze::reap_pending(&freeze.pending_tids); } - drop(memory_quiescence); + if fork_counted && send_result.is_err() && creation_trace.is_none() { + 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 { @@ -3346,12 +3416,14 @@ async fn settle_parked( match parked { Parked::Dispatch { notif, - action, + state, open, has_denied, } => { if id_valid(fd, notif.id).is_ok() { - finish_dispatch(notif, action, open, has_denied, ctx, fd, set, defer_sem).await; + let state = state.advance().await; + finish_dispatch_state(notif, state, open, has_denied, ctx, fd, set, defer_sem) + .await; } } Parked::Approval(prepared, verdict) => { @@ -3956,6 +4028,17 @@ mod tests { assert!(ran.load(Ordering::SeqCst)); } + #[test] + fn supervised_saturation_preserves_filesystem_admission() { + let parked = ParkedSet::new(); + let general = Arc::new(tokio::sync::Semaphore::new(DEFER_MAX_INFLIGHT)); + let _occupied = Arc::clone(&general) + .try_acquire_many_owned(DEFER_MAX_INFLIGHT as u32) + .unwrap(); + assert!(parked.admit(false, &general).is_err()); + assert!(parked.admit(true, &general).is_ok()); + } + #[tokio::test] async fn rejected_blocking_work_never_starts() { let action = NotifAction::prepare_blocking(&[], -1, || { diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 5a39fc0f..7761228e 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -1862,7 +1862,7 @@ sandlock_handler_t *sandlock_handler_new(int32_t (*handler_fn)(void *ud, * 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. + * argv-safety freeze (`execve`/`execveat`) or fork accounting. * * # Safety * `h` must be a non-null pointer returned by `sandlock_handler_new` that has diff --git a/crates/sandlock-ffi/src/handler/abi.rs b/crates/sandlock-ffi/src/handler/abi.rs index 11c71b7c..4d98bc65 100644 --- a/crates/sandlock-ffi/src/handler/abi.rs +++ b/crates/sandlock-ffi/src/handler/abi.rs @@ -561,7 +561,7 @@ pub unsafe extern "C" fn sandlock_handler_new( /// 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. +/// argv-safety freeze (`execve`/`execveat`) or fork accounting. /// /// # Safety /// `h` must be a non-null pointer returned by `sandlock_handler_new` that has diff --git a/crates/sandlock-ffi/src/handler/adapter.rs b/crates/sandlock-ffi/src/handler/adapter.rs index 7b240d90..6967bf09 100644 --- a/crates/sandlock-ffi/src/handler/adapter.rs +++ b/crates/sandlock-ffi/src/handler/adapter.rs @@ -149,14 +149,7 @@ impl Handler for FfiHandler { let handler = Arc::clone(&self.inner); let on_exception_fallback = self.exception_action(child_pgid); let deferred = self.inner.deferred; - let work_guard = match cx.blocking_guard() { - Ok(guard) => guard, - Err(e) => { - return Box::pin(async move { - NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EIO)) - }) - } - }; + let work_guard = cx.blocking_guard(); let run = async move { sandlock_core::blocking::run(move || { From c996b74c1370ccb81433c4b75811aff8b2df1205 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 1 Nov 2026 20:30:23 -0800 Subject: [PATCH 7/7] ffi: drain callbacks before returning from a run A timed-out callback can keep running after its sandbox exits. Retaining the handler container does not protect caller-owned userdata, whose public lifetime ends when the FFI run returns. Keep a callback guard for each registered handler through the run. Wait for active callbacks before releasing their userdata and remove the callback so queued work cannot access it afterward. Preserve concurrent callback execution while keeping teardown outside the notification loop. Cover abandoned callbacks with borrowed userdata and queued callbacks that have not started. Document that a callback which never returns also prevents the FFI run from completing. Signed-off-by: Cong Wang --- crates/sandlock-ffi/include/sandlock.h | 2 + crates/sandlock-ffi/src/handler/abi.rs | 2 + crates/sandlock-ffi/src/handler/adapter.rs | 176 ++++++++++++++++++++- crates/sandlock-ffi/src/handler/run.rs | 1 + 4 files changed, 176 insertions(+), 5 deletions(-) diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 7761228e..258c8aa7 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -1841,6 +1841,8 @@ void sandlock_action_set_kill(sandlock_action_out_t *out, int32_t sig, int32_t p * remains valid until either (a) `sandlock_handler_free` is called or * (b) the supervisor takes ownership via `sandlock_run_with_handlers` * and the run completes. + * The run waits for active callbacks before returning, even after a syscall + * timeout or child exit. A callback that never returns prevents run completion. * If `on_exception` does not match a defined `sandlock_exception_policy_t` * discriminant (0, 1, 2, or 3), the call returns null and no allocation occurs. */ diff --git a/crates/sandlock-ffi/src/handler/abi.rs b/crates/sandlock-ffi/src/handler/abi.rs index 4d98bc65..c1183b35 100644 --- a/crates/sandlock-ffi/src/handler/abi.rs +++ b/crates/sandlock-ffi/src/handler/abi.rs @@ -506,6 +506,8 @@ impl Drop for sandlock_handler_t { /// remains valid until either (a) `sandlock_handler_free` is called or /// (b) the supervisor takes ownership via `sandlock_run_with_handlers` /// and the run completes. +/// The run waits for active callbacks before returning, even after a syscall +/// timeout or child exit. A callback that never returns prevents run completion. /// If `on_exception` does not match a defined `sandlock_exception_policy_t` /// discriminant (0, 1, 2, or 3), the call returns null and no allocation occurs. // The two callback parameters are spelled inline rather than via the diff --git a/crates/sandlock-ffi/src/handler/adapter.rs b/crates/sandlock-ffi/src/handler/adapter.rs index 6967bf09..fbbb80fc 100644 --- a/crates/sandlock-ffi/src/handler/adapter.rs +++ b/crates/sandlock-ffi/src/handler/adapter.rs @@ -7,7 +7,7 @@ use std::future::Future; use std::os::unix::io::{AsRawFd, FromRawFd}; use std::pin::Pin; -use std::sync::Arc; +use std::sync::{Arc, RwLock}; use sandlock_core::seccomp::dispatch::{Handler, HandlerCtx}; use sandlock_core::seccomp::notif::NotifAction; @@ -45,7 +45,29 @@ 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: Arc, + inner: Arc, + on_exception: sandlock_exception_policy_t, + deferred: bool, +} + +struct CallbackState { + handler: RwLock>>, +} + +pub(super) struct CallbackRunGuard(Arc); + +impl Drop for CallbackRunGuard { + fn drop(&mut self) { + // Userdata may be borrowed until the FFI run returns. Drain readers + // and remove the callback so queued work cannot borrow it afterward. + let handler = self + .0 + .handler + .write() + .unwrap_or_else(|e| e.into_inner()) + .take(); + drop(handler); + } } impl FfiHandler { @@ -58,13 +80,22 @@ 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: Arc::from(Box::from_raw(raw)), + on_exception: handler.on_exception, + deferred: handler.deferred, + inner: Arc::new(CallbackState { + handler: RwLock::new(Some(handler)), + }), } } + pub(super) fn run_guard(&self) -> CallbackRunGuard { + CallbackRunGuard(Arc::clone(&self.inner)) + } + 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, @@ -148,12 +179,16 @@ impl Handler for FfiHandler { }; let handler = Arc::clone(&self.inner); let on_exception_fallback = self.exception_action(child_pgid); - let deferred = self.inner.deferred; + let deferred = self.deferred; let work_guard = cx.blocking_guard(); let run = async move { sandlock_core::blocking::run(move || { let _work_guard = work_guard; + let callback = handler.handler.read().unwrap_or_else(|e| e.into_inner()); + let Some(handler) = callback.as_ref() else { + return NotifAction::Errno(libc::EIO); + }; let mut mem = sandlock_mem_handle_t::new(listener.as_raw_fd(), notif_id, pid); let mut out = sandlock_action_out_t::zeroed(); let rc_or_panic = match handler.handler_fn { @@ -329,3 +364,134 @@ fn translate_action(out: &sandlock_action_out_t, child_pgid: i32) -> Option HandlerCtx { + HandlerCtx::new( + SeccompNotif { + id: 1, + pid: 0, + flags: 0, + data: SeccompData { + nr: libc::SYS_getpid as i32, + arch: 0, + instruction_pointer: 0, + args: [0; 6], + }, + }, + std::fs::File::open("/dev/null").unwrap().into(), + ) + } + + #[tokio::test] + async fn run_guard_waits_for_abandoned_callback_using_borrowed_data() { + struct State { + started: Mutex>>, + release: Mutex>, + finished: Mutex>>, + } + extern "C-unwind" fn 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 state = unsafe { &*ud.cast::() }; + let _ = state.started.lock().unwrap().take().unwrap().send(()); + let _ = state.release.lock().unwrap().recv(); + let finished = state.finished.lock().unwrap().take().unwrap(); + unsafe { sandlock_action_set_return_value(out, 42) }; + let _ = finished.send(()); + 0 + } + + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let (finished_tx, finished_rx) = tokio::sync::oneshot::channel(); + let state = Box::new(State { + started: Mutex::new(Some(started_tx)), + release: Mutex::new(release_rx), + finished: Mutex::new(Some(finished_tx)), + }); + let raw = unsafe { + sandlock_handler_new( + Some(callback), + (&*state as *const State).cast_mut().cast(), + None, + sandlock_exception_policy_t::DenyEio as u32, + ) + }; + let handler = unsafe { FfiHandler::from_raw(raw) }; + let run_guard = handler.run_guard(); + let cx = context(); + let NotifAction::Suspend(work) = handler.handle(&cx).await else { + panic!("expected suspension"); + }; + let waiter = tokio::spawn(work.run()); + started_rx.await.unwrap(); + waiter.abort(); + let _ = waiter.await; + drop(handler); + + let (draining_tx, draining_rx) = tokio::sync::oneshot::channel(); + let (done_tx, mut done_rx) = tokio::sync::oneshot::channel(); + let drain = std::thread::spawn(move || { + let _ = draining_tx.send(()); + drop(run_guard); + let _ = done_tx.send(()); + }); + draining_rx.await.unwrap(); + let waited = tokio::time::timeout(std::time::Duration::from_millis(50), &mut done_rx) + .await + .is_err(); + release_tx.send(()).unwrap(); + finished_rx.await.unwrap(); + drain.join().unwrap(); + assert!( + waited, + "run returned while the callback could still access userdata" + ); + } + + #[tokio::test] + async fn run_guard_releases_userdata_and_rejects_unstarted_callbacks() { + extern "C-unwind" fn callback( + _: *mut std::ffi::c_void, + _: *const crate::notif_repr::sandlock_notif_data_t, + _: *mut sandlock_mem_handle_t, + _: *mut sandlock_action_out_t, + ) -> i32 { + panic!("callback started after its run ended"); + } + extern "C-unwind" fn destroy(ud: *mut std::ffi::c_void) { + unsafe { &*ud.cast::() }.store(true, Ordering::Release); + } + + let dropped = Box::new(AtomicBool::new(false)); + let raw = unsafe { + sandlock_handler_new( + Some(callback), + (&*dropped as *const AtomicBool).cast_mut().cast(), + Some(destroy), + sandlock_exception_policy_t::Continue as u32, + ) + }; + let handler = unsafe { FfiHandler::from_raw(raw) }; + let run_guard = handler.run_guard(); + let cx = context(); + let NotifAction::Suspend(work) = handler.handle(&cx).await else { + panic!("expected suspension"); + }; + drop(run_guard); + assert!(dropped.load(Ordering::Acquire)); + assert!(matches!(work.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..4241ded0 100644 --- a/crates/sandlock-ffi/src/handler/run.rs +++ b/crates/sandlock-ffi/src/handler/run.rs @@ -104,6 +104,7 @@ fn block_on_run( handlers: Vec<(i64, FfiHandler)>, interactive: bool, ) -> Option> { + let _callback_guards: Vec<_> = handlers.iter().map(|(_, h)| h.run_guard()).collect(); let cmd_refs: Vec<&str> = cmd.iter().map(String::as_str).collect(); // Apply `name` via the builder method on a clone — mirrors the // pattern used by `sandlock_run` in lib.rs. A `None` here means