From 1ecfea21cf0d09df1bcc980f6d590654bd684581 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:08:39 +0200 Subject: [PATCH 1/3] Record the IPC setup rollback that panics when a sibling closed its new handle Carried from the kernel audit's branch (pull request 754), which traced it without executing it, so the record lands with the commit that closes it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...cs-when-a-sibling-closed-its-new-handle.md | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md diff --git a/issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md b/issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md new file mode 100644 index 00000000000..8db971aa6d7 --- /dev/null +++ b/issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md @@ -0,0 +1,36 @@ +--- +status: open +kind: defect +opened: 2026-10-08 +--- + +# An IPC setup rollback panics when a sibling closed its new handle first + +`sys_inbox_setup` (`kernel/src/syscall/ipc.rs:444`) installs the inbox handle +under the process-data lock, releases that lock, and then runs `ctx.copy_out` +to a user address. When the copy fails the rollback closes the handle with +`ops::close(...).expect("the inbox this call installed a moment ago")` +(`:466`). `sys_namespace_open` (`:219`) has the same shape: it installs the +client handle, releases the lock, pushes onto the port's pending queue, and on +`QueueFull`/`Closed` rolls back with +`ops::close(...).expect("the connection this call installed a moment ago")` +(`:265`). + +The install is visible to sibling threads the instant the process-data lock is +dropped, and the fallible step (`copy_out`, or the queue push) runs with no +reservation held. A sibling thread of the same process, on another CPU, can +`SYS_CLOSE` that handle — or move it with `SYS_HANDLE_SEND` — between install +and rollback. The rollback's `HandleTable::remove` +(`kernel/src/object/handle.rs`) then returns `Stale`/an error, and the `expect` +turns a correctly detected concurrent close into a kernel panic. The comments' +premise — "the handle this call installed a moment ago" is still held — is not +an invariant across the dropped lock. Untrusted ordering panics the kernel +where it must refuse. + +**Exit**: with a sibling closing (or sending) the just-installed handle while +`SYS_INBOX_SETUP` is in its failing-`copy_out` rollback, and while +`SYS_NAMESPACE_OPEN` is in its queue-full rollback, both syscalls return their +original refusal and the kernel does not panic. **Traced, not executed.** No model compiles the handle table, and staging the +window needs an actuator the kernel does not have: one that holds the +installing thread between its install and its fallible step until a sibling +releases it. From e556d595542d38395b8d1c5ee75c9942f4354f91 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:12:58 +0200 Subject: [PATCH 2/3] An IPC setup rollback no longer closes back a handle across a dropped lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sys_namespace_open`'s connect and `sys_inbox_setup` installed a handle under the process-data lock, released it, ran a step that could fail, and on failure closed the handle with `ops::close(..).expect(..)`. The install was visible to sibling threads the instant the lock dropped, so a sibling closing or moving that handle in the window turned the rollback's correctly detected stale handle into a kernel panic — untrusted ordering crashing the kernel where it must refuse. Both now commit all-or-nothing under one hold, so there is no window and no rollback: - connect reserves a handle slot (`has_room`), pushes the server's half onto the port, and installs the client's half — all inside one `with_process_data`. The room is reserved before the fallible push, so the install cannot fail; a refused push drops both halves after the hold is given up, their pipes with them. The dropped `expect` is replaced by an invariant, not a second code path. - inbox setup takes the answer's user window in the dispatcher (as the other copy-out syscalls do), before the inbox exists, so a bad address refuses before anything is installed. The handle, once installed, is never taken back. `Connector::push` now hands a refused `PendingConnection` back so its pipe ends are not dropped under the caller's handle-table lock. Closes the carried issue; deletes its file. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...cs-when-a-sibling-closed-its-new-handle.md | 36 ----------- kernel/src/object/port.rs | 8 ++- kernel/src/syscall/dispatch.rs | 8 ++- kernel/src/syscall/ipc.rs | 63 ++++++++----------- 4 files changed, 38 insertions(+), 77 deletions(-) delete mode 100644 issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md diff --git a/issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md b/issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md deleted file mode 100644 index 8db971aa6d7..00000000000 --- a/issues/an-ipc-setup-rollback-panics-when-a-sibling-closed-its-new-handle.md +++ /dev/null @@ -1,36 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-08 ---- - -# An IPC setup rollback panics when a sibling closed its new handle first - -`sys_inbox_setup` (`kernel/src/syscall/ipc.rs:444`) installs the inbox handle -under the process-data lock, releases that lock, and then runs `ctx.copy_out` -to a user address. When the copy fails the rollback closes the handle with -`ops::close(...).expect("the inbox this call installed a moment ago")` -(`:466`). `sys_namespace_open` (`:219`) has the same shape: it installs the -client handle, releases the lock, pushes onto the port's pending queue, and on -`QueueFull`/`Closed` rolls back with -`ops::close(...).expect("the connection this call installed a moment ago")` -(`:265`). - -The install is visible to sibling threads the instant the process-data lock is -dropped, and the fallible step (`copy_out`, or the queue push) runs with no -reservation held. A sibling thread of the same process, on another CPU, can -`SYS_CLOSE` that handle — or move it with `SYS_HANDLE_SEND` — between install -and rollback. The rollback's `HandleTable::remove` -(`kernel/src/object/handle.rs`) then returns `Stale`/an error, and the `expect` -turns a correctly detected concurrent close into a kernel panic. The comments' -premise — "the handle this call installed a moment ago" is still held — is not -an invariant across the dropped lock. Untrusted ordering panics the kernel -where it must refuse. - -**Exit**: with a sibling closing (or sending) the just-installed handle while -`SYS_INBOX_SETUP` is in its failing-`copy_out` rollback, and while -`SYS_NAMESPACE_OPEN` is in its queue-full rollback, both syscalls return their -original refusal and the kernel does not panic. **Traced, not executed.** No model compiles the handle table, and staging the -window needs an actuator the kernel does not have: one that holds the -installing thread between its install and its fallible step until a sibling -releases it. diff --git a/kernel/src/object/port.rs b/kernel/src/object/port.rs index a0bd6b47e30..add11bdc67c 100644 --- a/kernel/src/object/port.rs +++ b/kernel/src/object/port.rs @@ -139,13 +139,15 @@ impl Connector { } /// One lock acquisition for the check and the insert; see [`PortQueue`]. - pub fn push(&self, connection: PendingConnection) -> Result<(), PushError> { + /// A refusal hands the connection back: its caller holds a handle table's + /// lock, and the pipe ends are not dropped under one. + pub fn push(&self, connection: PendingConnection) -> Result<(), (PendingConnection, PushError)> { let mut queue = self.shared.queue.lock(); if queue.closed { - return Err(PushError::Closed); + return Err((connection, PushError::Closed)); } if queue.pending.len() >= MAX_PENDING_CONNECTIONS { - return Err(PushError::QueueFull); + return Err((connection, PushError::QueueFull)); } queue.pending.push_back(connection); Ok(()) diff --git a/kernel/src/syscall/dispatch.rs b/kernel/src/syscall/dispatch.rs index c3dacf6690c..bd932c05bac 100644 --- a/kernel/src/syscall/dispatch.rs +++ b/kernel/src/syscall/dispatch.rs @@ -533,8 +533,12 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> sys_namespace_open(RawHandle(a1 as u32), &name) } SYS_TLS_ALLOC_BLOCK => sys_tls_alloc_block(a1), - // ctx carries the copy-out: sys_inbox_setup writes its answer only once setup succeeds. - SYS_INBOX_SETUP => sys_inbox_setup(&ctx, a1 as u32, a2), + SYS_INBOX_SETUP => { + // Taken before the inbox exists: a bad address must not leave a handle the caller was never told. + let len = core::mem::size_of::() as u64; + let Some(mut out) = ctx.user_bytes_mut(UserAddr::new(a2), len) else { return bad_addr }; + sys_inbox_setup(a1 as u32, &mut out) + } SYS_INBOX_SUBMIT => { sys_inbox_submit(RawHandle(a1 as u32), a2 as u32, a3 as u32, a4) } diff --git a/kernel/src/syscall/ipc.rs b/kernel/src/syscall/ipc.rs index 6b62d9b6d42..35a92811e91 100644 --- a/kernel/src/syscall/ipc.rs +++ b/kernel/src/syscall/ipc.rs @@ -2,8 +2,10 @@ //! that name them, connections, shared memory, and inboxes. //! No peer is addressed by name or pid: ports, namespaces and connections are //! built only from connectors and handles the caller already holds. -//! Every all-or-nothing claim here is structural: names and connectors are -//! resolved and checked before anything is installed or removed. +//! Every all-or-nothing claim here is structural: whatever can refuse is +//! resolved and checked before anything is installed or removed, or under the +//! hold that installs it. No handle is taken back across a released hold: a +//! sibling thread can close or move one the moment the table's lock is given up. use alloc::vec::Vec; @@ -240,35 +242,37 @@ fn connect_through(connector: &port::Connector) -> u64 { // Cross-wired here only: the server's end is built from the same two queues when it accepts. let (to_server, to_client) = crate::object::service::ConnectionEnd::pair_queues(); - // Client's end installed first: queuing before that would let a server accept a connection whose client has no handle. let object = KObjectRef::Connection(crate::object::service::ConnectionEnd::new( sc_reader, // client reads from server→client cs_writer, // client writes to client→server to_client.clone(), // and receives what the server sent to_server.clone(), )); - let h = match process::with_process_data(|data| ops::install(&mut data.handles, object)) { - Ok(h) => h, - Err(e) => return e.to_u64(), - }; - - let queued = connector.push(port::PendingConnection { + let pending = port::PendingConnection { rx: cs_reader, // server reads from client→server tx: sc_writer, // server writes to server→client inbox: to_server, outbox: to_client, stamp: connector.stamp(), + }; + // One hold from the room to the install: the port can still refuse, and the client's end is in the table only once it has not. + let installed = process::with_process_data(|data| { + if !data.handles.has_room(1) { + return Err((SyscallError::ResourceExhausted, object, pending)); + } + match connector.push(pending) { + Ok(()) => Ok(ops::install(&mut data.handles, object).expect("room was asked for first")), + Err((pending, port::PushError::Closed)) => Err((SyscallError::Gone, object, pending)), + Err((pending, port::PushError::QueueFull)) => { + Err((SyscallError::ResourceExhausted, object, pending)) + } + } }); - if let Err(e) = queued { - process::with_process_data(|data| { - ops::close(&mut data.handles, h, &mut data.pipe_maps) - .expect("the connection this call installed a moment ago"); - }); - return match e { - port::PushError::Closed => SyscallError::Gone.to_u64(), - port::PushError::QueueFull => SyscallError::ResourceExhausted.to_u64(), - }; - } + let h = match installed { + Ok(h) => h, + // Both halves of a refused connection drop here, with the hold given up: their pipes go with them. + Err((e, _client, _server)) => return e.to_u64(), + }; // The port's one watch carries both: the server blocked in `accept` and every ring polling it. connector.port().watch().post(); h.0 as u64 @@ -440,12 +444,8 @@ pub(super) fn sys_handle_recv( } } -/// Make an inbox and tell the caller where it is; the inbox owns its page and only this mapping may name it. -pub(super) fn sys_inbox_setup(ctx: &SyscallContext, depth: u32, out: u64) -> u64 { - let out = match UserAddr::checked(out) { - Some(addr) => addr, - None => return SyscallError::InvalidArgument.to_u64(), - }; +/// Make an inbox and tell the caller where it is; the inbox owns its page and only this mapping may name it. `out` is the window the dispatch took for the answer. +pub(super) fn sys_inbox_setup(depth: u32, out: &mut crate::user_ptr::UserBytesMut) -> u64 { let (inbox, vaddr) = match crate::inbox::create(depth) { Ok(v) => v, Err(e) => return e.to_u64(), @@ -457,17 +457,8 @@ pub(super) fn sys_inbox_setup(ctx: &SyscallContext, depth: u32, out: u64) -> u64 Ok(h) => h, Err(e) => return e.to_u64(), }; - let answer = toyos_abi::syscall::InboxSetup { handle, _pad: 0, vaddr }; - match ctx.copy_out(out, &answer) { - Ok(()) => 0, - Err(e) => { - process::with_process_data(|data| { - ops::close(&mut data.handles, handle, &mut data.pipe_maps) - .expect("the inbox this call installed a moment ago"); - }); - e.to_u64() - } - } + out.write_at(0, toyos_abi::usersafe::bytes(&InboxSetup { handle, _pad: 0, vaddr })); + 0 } pub(super) fn sys_inbox_submit( From 86cdd0c094409631e6cf57ea39dd6268dc184ad5 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:46:00 +0200 Subject: [PATCH 3/3] A pipe and a port ask for both slots before installing either MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sys_pipe and sys_port_create installed one handle and took it back when the second was refused, sys_pipe through the tree's last `ops::close(..).expect("…installed a moment ago")`. Both now go through one install_pair that asks has_room(2) first, so a refused pair installs nothing, takes nothing back and spends no slot generation, and its two objects drop once the hold is given up. abuse_handle_table gains the arm that reaches the refusal: at the cap and with one slot free a pipe and a port are each ResourceExhausted, and the one slot is still free afterwards and the only one. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- kernel/src/syscall/ipc.rs | 48 +++++++++---------- .../src/bin/abuse_handle_table.rs | 32 +++++++++++++ 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/kernel/src/syscall/ipc.rs b/kernel/src/syscall/ipc.rs index 35a92811e91..b16b2760c70 100644 --- a/kernel/src/syscall/ipc.rs +++ b/kernel/src/syscall/ipc.rs @@ -28,17 +28,27 @@ pub(super) fn sys_pipe() -> u64 { let (reader, writer) = pipe::create(); let read_end = KObjectRef::PipeRead(crate::object::pipe::PipeReadEnd::new(reader)); let write_end = KObjectRef::PipeWrite(crate::object::pipe::PipeWriteEnd::new(writer)); - process::with_process_data(|data| { - let Ok(read_h) = ops::install(&mut data.handles, read_end) else { - return SyscallError::ResourceExhausted.to_u64(); - }; - let Ok(write_h) = ops::install(&mut data.handles, write_end) else { - ops::close(&mut data.handles, read_h, &mut data.pipe_maps) - .expect("the read end this call installed a moment ago"); - return SyscallError::ResourceExhausted.to_u64(); - }; - ((read_h.0 as u64) << 32) | write_h.0 as u64 - }) + install_pair(read_end, write_end) +} + +/// Install two objects or neither; the two handles come back packed into one word, which cannot be read as an error. +fn install_pair(first: KObjectRef, second: KObjectRef) -> u64 { + let installed = process::with_process_data(|data| { + // Room for both before either: a refused pair installs nothing, so nothing is taken back. + if !data.handles.has_room(2) { + return Err((first, second)); + } + let mut install = + |object| ops::install(&mut data.handles, object).expect("room was asked for first"); + let first = install(first); + let second = install(second); + Ok(((first.0 as u64) << 32) | second.0 as u64) + }); + match installed { + Ok(word) => word, + // A refused pair drops here, with the hold given up. + Err(_pair) => SyscallError::ResourceExhausted.to_u64(), + } } /// Map a pipe's ring page into the caller, tracked against the pipe so @@ -89,22 +99,10 @@ pub(super) fn sys_connection_join(rx_h: RawHandle, tx_h: RawHandle) -> u64 { process::with_process_data(|data| handle_result(ops::install(&mut data.handles, object))) } -/// Make a port and install both ends; needs no right and grants none, since a port with no clients is not authority. The two handles come back packed into one word, which cannot be read as an error. +/// Make a port and install both ends; needs no right and grants none, since a port with no clients is not authority. pub(super) fn sys_port_create() -> u64 { let (acceptor, connector) = port::create(); - process::with_process_data(|data| { - let Ok(a) = ops::install(&mut data.handles, KObjectRef::Acceptor(acceptor)) else { - return SyscallError::ResourceExhausted.to_u64(); - }; - let install_c = - ops::install(&mut data.handles, KObjectRef::Connector(connector)); - let Ok(c) = install_c else { - // The acceptor goes back so a refused pair leaves no orphaned port half. - drop(data.handles.remove(a)); - return SyscallError::ResourceExhausted.to_u64(); - }; - ((a.0 as u64) << 32) | c.0 as u64 - }) + install_pair(KObjectRef::Acceptor(acceptor), KObjectRef::Connector(connector)) } /// Build a namespace from a base's kept names plus new bindings; a refusal leaves the caller's table unchanged (every name resolved and connector checked first). diff --git a/tests/toyos-rust-tests/src/bin/abuse_handle_table.rs b/tests/toyos-rust-tests/src/bin/abuse_handle_table.rs index 997807ac353..375560b152a 100644 --- a/tests/toyos-rust-tests/src/bin/abuse_handle_table.rs +++ b/tests/toyos-rust-tests/src/bin/abuse_handle_table.rs @@ -25,6 +25,9 @@ //! the kernel's own `self` fills. So a caller's entry labelled `self` is //! refused before anything moves, and so is a vector of `MAX_ENDOWMENTS` //! entries, while one entry fewer starts. +//! +//! **A pipe and a port are two inserts answered as one**: at the cap, and one +//! slot short of fitting, each is refused with the table as it was. use toyos_abi::syscall::{ self, EndowEntry, MmapFlags, MmapProt, SpawnArgs, SyscallError, MAX_ENDOWMENTS, MAX_SLOT_MAP, SELF_LABEL, @@ -160,6 +163,8 @@ fn main() { "handle table reached {n} slots, past the {MAX_HANDLES} cap" ); + a_pair_is_refused_whole(&mut filled); + let last = filled.pop().expect("the fill installed a handle"); let entry = EndowEntry { label_off: 0, label_len: LABELS.len() as u32, handle: last, _pad: 0 }; let spawned = spawn_endowed(&[entry], LABELS); @@ -180,6 +185,33 @@ fn main() { println!("handle table capped at {MAX_HANDLES} on every insert path (refused at {n})"); } +/// A pipe and a port are two handles each: refused at the cap, and refused +/// with one slot free, which is then still free. Takes a full table and +/// leaves it full. +fn a_pair_is_refused_whole(filled: &mut Vec) { + let refused = |room: &str| { + assert_eq!( + syscall::pipe().err(), + Some(SyscallError::ResourceExhausted), + "a pipe with {room}" + ); + assert_eq!( + syscall::port_create().err(), + Some(SyscallError::ResourceExhausted), + "a port with {room}" + ); + }; + refused("no slot free"); + syscall::close(filled.pop().expect("the fill installed a handle")); + refused("one slot free"); + filled.push(syscall::dup(RawHandle(1)).expect("the one free slot survived two refused pairs")); + assert_eq!( + syscall::dup(RawHandle(1)), + Err(SyscallError::ResourceExhausted), + "a refused pair left a second slot free" + ); +} + /// `SYS_SPAWN` of this binary as a child that exits at once, carrying /// `entries` into the blob `labels`. fn spawn_endowed(entries: &[EndowEntry], labels: &[u8]) -> Result {