From 3f4112a88152b3776855a331ac96038b88f338c9 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:06:44 +0200 Subject: [PATCH 1/3] Carry the issue this branch closes: a thread's entry is never checked The file is the audit's, filed on wt/toyos-audit-kernel (draft pull request 754) and not yet on main. It is carried here unchanged so that it lands with its fix and is deleted by it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...anonical-one-is-measured-only-under-tcg.md | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md diff --git a/issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md b/issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md new file mode 100644 index 00000000000..4a33605ac3f --- /dev/null +++ b/issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md @@ -0,0 +1,46 @@ +--- +status: open +kind: finding +opened: 2026-10-08 +--- + +# A thread's entry is never checked, and what a noncanonical one does is measured only under TCG + +`sys_thread_spawn` (`kernel/src/syscall/proc.rs`) checks `stack_base <= +stack_ptr` and nothing else. The entry reaches `thread_start` +(`kernel/src/arch/x86_64/entry.rs`) unchanged and is the `RIP` its `iretq` +returns to, so what an entry that is no canonical address does is decided by +the CPU and not by the kernel. + +**Measured under QEMU TCG** (an x86-64 guest on an AArch64 host, `tests/testcases`, +two CPUs, at `b432ed21c`): a guest binary that spawns a thread at such an entry +over a mapped stack. The spawn was accepted, the fault was taken with a Ring 3 +frame, and the kernel ended that process alone: + +``` +FAULT rip=0x0100000000000000 cr2=0x0000000000000000 err=0x0000000000000000 ... tid=1 +SIGBUS tid=1: general protection fault (error_code=0x0) + cs=0x0023 ss=0x001b rflags=0x0000000000010202 +exit: test_rs_spawn_noncanonical_ pid=10 code=-1 cpu=9ms +===TEST_END test_rs_spawn_noncanonical_entry exit=-1=== +``` + +The machine went on and the harness finished its run. So the emulator completes +the return and faults at the fetch. + +**Not measured on hardware.** Intel's SDM (Volume 2, `IRET`) raises `#GP(0)` for +a return `RIP` that is not canonical, from the instruction itself — a Ring 0 +frame, which `fatal_exception` +(`kernel/src/arch/x86_64/idt/exceptions.rs`) answers with `halt_all_cpus()`, +since only a Ring 3 fault ends a process. If the T14, an Intel machine, does +what that document says, one process halts it. Nothing has executed that; TCG +cannot, and the T14 is the orchestrator's. + +The binary and its registration are a patch on the pull request that filed +this. + +**Exit**: a metal row runs that binary on the T14. If the machine halts this is +a `defect` whose exit is that `SYS_THREAD_SPAWN` refuses an entry outside the +canonical user half, by name, on every machine; if the process ends alone +there too, the one line this leaves is `sys_thread_spawn`'s doc comment saying +the entry is the CPU's to refuse. From a45d98269a65b5ad6281307d7939760a89a40200 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:10:39 +0200 Subject: [PATCH 2/3] A return to userland takes an entry inside the user half, and an image stops a page short of its top SYS_THREAD_SPAWN bounded the stack and passed the entry on unread, so the first return to userland of the new thread carried whatever the caller wrote. For an address that is not canonical the architecture faults in the returning instruction itself, in the kernel's ring, where the fatal path halts every CPU. Under QEMU's TCG the emulator completes the return and faults at the fetch in Ring 3, which is why the audit that found this (issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md, deleted here) saw only the one process end. toyos_userbound::Entry is an instruction pointer inside the user half, by is_user_addr and by no second definition, and it has one constructor. loader::Start is now the only way to a trampoline: its Process and Thread arms carry an Entry, its Kernel arm a kernel thread's body, and the trampolines are no longer re-exported from loader. sys_thread_spawn refuses what Entry refuses with InvalidArgument; the loader's entry, already inside an image rebase_base placed, is made one the same way. The other paths by which a user-chosen instruction pointer reaches a return: - a process's entry is e_entry, which toyos-elf holds inside the image, and the image is held inside the user half by rebase_base; - no syscall writes a saved user frame: there is no signal delivery, no resumption at a registered address and no context to set, and a saved frame lives on a kernel stack; - the stack pointer is not checked by either return instruction and is the thread's own to fault on; - AArch64 takes a bad ELR_EL1 as an abort from EL0 after the ERET; - an instruction that ends at the last byte of the user half leaves the first address past it, which is not canonical, as its successor: what a syscall placed there returns to through SYSRET, and what an interrupt taken after any instruction there returns to through IRET. Nothing held this. rebase_base allowed an image to end exactly at USER_TOP, and the image is the only executable mapping that can reach it (libraries and anonymous memory are placed below ALLOC_CEILING, and anonymous memory is never executable). It now refuses an image that reaches the last 2 MiB page. This one is read off the code and the architecture's documents; nothing here executed it, and TCG would not show it. Tests: the host table in toyos-userbound holds Entry's edges and the image bound; thread_entry_noncanonical boots tests/testcases alone and runs spawn_noncanonical_entry, which asks for a thread at an address that is not canonical, at the first past the user half and at the first of the kernel half, and wants InvalidArgument for each. The binary is on RUST_SKIP: a kernel that took the entry would take a shared boot with it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...anonical-one-is-measured-only-under-tcg.md | 46 ---------------- kernel/src/loader/mod.rs | 14 +++-- kernel/src/loader/start.rs | 27 +++++++--- kernel/src/process.rs | 11 ++-- kernel/src/sched/kthread.rs | 11 ++-- kernel/src/syscall/proc.rs | 7 ++- .../src/bin/spawn_noncanonical_entry.rs | 37 +++++++++++++ tests/toyos.rs | 21 ++++++++ toyos-abi/src/syscall.rs | 3 +- toyos-userbound/src/lib.rs | 4 +- toyos-userbound/src/span.rs | 53 +++++++++++++++++-- 11 files changed, 156 insertions(+), 78 deletions(-) delete mode 100644 issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md create mode 100644 tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs diff --git a/issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md b/issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md deleted file mode 100644 index 4a33605ac3f..00000000000 --- a/issues/a-thread-entry-is-unchecked-and-a-noncanonical-one-is-measured-only-under-tcg.md +++ /dev/null @@ -1,46 +0,0 @@ ---- -status: open -kind: finding -opened: 2026-10-08 ---- - -# A thread's entry is never checked, and what a noncanonical one does is measured only under TCG - -`sys_thread_spawn` (`kernel/src/syscall/proc.rs`) checks `stack_base <= -stack_ptr` and nothing else. The entry reaches `thread_start` -(`kernel/src/arch/x86_64/entry.rs`) unchanged and is the `RIP` its `iretq` -returns to, so what an entry that is no canonical address does is decided by -the CPU and not by the kernel. - -**Measured under QEMU TCG** (an x86-64 guest on an AArch64 host, `tests/testcases`, -two CPUs, at `b432ed21c`): a guest binary that spawns a thread at such an entry -over a mapped stack. The spawn was accepted, the fault was taken with a Ring 3 -frame, and the kernel ended that process alone: - -``` -FAULT rip=0x0100000000000000 cr2=0x0000000000000000 err=0x0000000000000000 ... tid=1 -SIGBUS tid=1: general protection fault (error_code=0x0) - cs=0x0023 ss=0x001b rflags=0x0000000000010202 -exit: test_rs_spawn_noncanonical_ pid=10 code=-1 cpu=9ms -===TEST_END test_rs_spawn_noncanonical_entry exit=-1=== -``` - -The machine went on and the harness finished its run. So the emulator completes -the return and faults at the fetch. - -**Not measured on hardware.** Intel's SDM (Volume 2, `IRET`) raises `#GP(0)` for -a return `RIP` that is not canonical, from the instruction itself — a Ring 0 -frame, which `fatal_exception` -(`kernel/src/arch/x86_64/idt/exceptions.rs`) answers with `halt_all_cpus()`, -since only a Ring 3 fault ends a process. If the T14, an Intel machine, does -what that document says, one process halts it. Nothing has executed that; TCG -cannot, and the T14 is the orchestrator's. - -The binary and its registration are a patch on the pull request that filed -this. - -**Exit**: a metal row runs that binary on the T14. If the machine halts this is -a `defect` whose exit is that `SYS_THREAD_SPAWN` refuses an entry outside the -canonical user half, by name, on every machine; if the process ends alone -there too, the one line this leaves is `sys_thread_spawn`'s doc comment saying -the entry is the CPU's to refuse. diff --git a/kernel/src/loader/mod.rs b/kernel/src/loader/mod.rs index deb308799aa..cfa90ffcbd2 100644 --- a/kernel/src/loader/mod.rs +++ b/kernel/src/loader/mod.rs @@ -15,8 +15,7 @@ mod symbols; mod tls; pub use start::{build_child_handles, PendingHandles, SLOT_PAIR_LEN}; -pub(crate) use start::alloc_kernel_stack; -pub(crate) use crate::arch::entry::{kernel_start, process_start, thread_start}; +pub(crate) use start::{alloc_kernel_stack, Start}; pub use tls::{TlsBlock, DTV_INITIAL_CAPACITY, VARIANT as TLS_VARIANT}; use alloc::string::String; @@ -568,7 +567,12 @@ pub fn spawn( return Err(SyscallError::ResourceExhausted.into()); }; - let entry = (image_start + layout.entry().get()).raw(); + // Inside the image, which `rebase_base` placed inside the user half. + let Some(entry) = toyos_userbound::Entry::new((image_start + layout.entry().get()).raw()) + else { + log!("spawn: {}: the entry is outside the user half", path); + return Err(SyscallError::InvalidArgument.into()); + }; let image_end = (image_start + layout.span()).raw(); let sp = user_stack.write_argv(argv); let t_tls = crate::clock::nanos_since_boot(); @@ -585,7 +589,7 @@ pub fn spawn( bias: base, }); - let (ks_alloc, ks_sp) = match alloc_kernel_stack(process_start, entry, sp, 0) { + let (ks_alloc, ks_sp) = match alloc_kernel_stack(Start::Process { entry, sp }) { Some(ks) => ks, None => { log!("spawn: {}: failed to allocate kernel stack", path); @@ -683,7 +687,7 @@ pub fn spawn( let t3 = crate::clock::nanos_since_boot(); log!("spawn: {} pid={} tid={} dst={} base={:#x} entry={:#x} root={:#x} (layout={}ms relocs={}ms deps={}ms tls={}ms total={}ms)", - path, pid, tid, dst.0, base, entry, child_pt.lock().root().phys(), + path, pid, tid, dst.0, base, entry.addr(), child_pt.lock().root().phys(), (t1 - t0) / 1_000_000, (t2 - t1) / 1_000_000, (t_deps - t2) / 1_000_000, (t_tls - t_deps) / 1_000_000, (t3 - t0) / 1_000_000); diff --git a/kernel/src/loader/start.rs b/kernel/src/loader/start.rs index 5d42b4c7ae8..4d67e159b35 100644 --- a/kernel/src/loader/start.rs +++ b/kernel/src/loader/start.rs @@ -1,7 +1,9 @@ //! Loads a built process onto a CPU, builds the handle table it starts with //! and puts its spawner's handle to it in the spawner's table. The frame a new //! stack starts from and the trampolines it returns into are the -//! architecture's (`arch::entry`). +//! architecture's (`arch::entry`), and [`Start`] is the only way to one: a +//! trampoline that returns to userland is reached with an +//! [`Entry`](toyos_userbound::Entry) and with nothing else. use alloc::sync::Arc; use alloc::vec::Vec; @@ -21,13 +23,24 @@ use toyos_abi::syscall::{ /// One `[child_slot, parent_handle]` pair of `SpawnArgs::slot_map_ptr`, in bytes. pub const SLOT_PAIR_LEN: usize = 8; +/// Where a new context first runs. +pub(crate) enum Start { + /// A program's first instruction, on the stack its loader wrote. + Process { entry: toyos_userbound::Entry, sp: u64 }, + /// A thread's, on a stack its process chose, with its argument. + Thread { entry: toyos_userbound::Entry, sp: u64, arg: u64 }, + /// A kernel thread's body, which never returns to userland. + Kernel { body: extern "C" fn(u64) -> !, arg: u64 }, +} + /// Allocate a kernel stack and lay out the frame `context_switch` will restore. -pub(crate) fn alloc_kernel_stack( - trampoline: unsafe extern "C" fn(), - user_entry: u64, - user_sp: u64, - arg: u64, -) -> Option<(OwnedAlloc, u64)> { +pub(crate) fn alloc_kernel_stack(start: Start) -> Option<(OwnedAlloc, u64)> { + use crate::arch::entry::{kernel_start, process_start, thread_start}; + let (trampoline, user_entry, user_sp, arg): (unsafe extern "C" fn(), _, _, _) = match start { + Start::Process { entry, sp } => (process_start, entry.addr(), sp, 0), + Start::Thread { entry, sp, arg } => (thread_start, entry.addr(), sp, arg), + Start::Kernel { body, arg } => (kernel_start, body as usize as u64, 0, arg), + }; let alloc = OwnedAlloc::new(KERNEL_STACK_SIZE, 4096)?; scheduler::write_stack_canary(&alloc); let top = alloc.ptr() as u64 + KERNEL_STACK_SIZE as u64; diff --git a/kernel/src/process.rs b/kernel/src/process.rs index 78cfa1ded34..79b7a85a7a4 100644 --- a/kernel/src/process.rs +++ b/kernel/src/process.rs @@ -21,7 +21,7 @@ use crate::sched::payload::ThreadSched; use crate::time::{Deadline, Duration}; use crate::{elf, pipe, scheduler}; use crate::UserAddr; -use crate::loader::{alloc_kernel_stack, thread_start, TlsBlock}; +use crate::loader::{alloc_kernel_stack, Start, TlsBlock}; pub use toyos_abi::{Pid, Tid}; pub use crate::scheduler::TaskId; @@ -923,7 +923,12 @@ impl Drop for Admission { } /// Spawn a thread within the current process. -pub fn spawn_thread(entry: u64, stack_ptr: u64, arg: u64, stack_base: u64) -> Option { +pub fn spawn_thread( + entry: toyos_userbound::Entry, + stack_ptr: u64, + arg: u64, + stack_base: u64, +) -> Option { // Phase 1: parent's data + address space (table lock dropped after). let parent_process = current_process(); let (parent_addr_space, process_data_arc) = { @@ -962,7 +967,7 @@ pub fn spawn_thread(entry: u64, stack_ptr: u64, arg: u64, stack_base: u64) -> Op }; let tls_alloc_tcb = tls_alloc.ptr().wrapping_add(tp_offset); - let (ks_alloc, ks_sp) = match alloc_kernel_stack(thread_start, entry, stack_ptr, arg) { + let (ks_alloc, ks_sp) = match alloc_kernel_stack(Start::Thread { entry, sp: stack_ptr, arg }) { Some(ks) => ks, None => { tls_alloc.release(&parent_addr_space); diff --git a/kernel/src/sched/kthread.rs b/kernel/src/sched/kthread.rs index e63843293c5..55a7119020a 100644 --- a/kernel/src/sched/kthread.rs +++ b/kernel/src/sched/kthread.rs @@ -1,5 +1,5 @@ //! Kernel threads: ordinary tasks that name `mm::paging::kernel` as their -//! address space, enter through `loader::kernel_start`, and hold a process-table +//! address space, enter through `arch::entry::kernel_start`, and hold a process-table //! entry. One is preempted or stolen only at a preemption point its body reaches, //! and a Ring 0 loop reaches none. [`ROWS`] holds every one. @@ -74,13 +74,8 @@ pub fn is_kernel_task(id: TaskId) -> bool { /// Start a kernel thread running `body(arg)` on its own kernel stack and return its scheduler faces. pub fn spawn(name: &str, body: extern "C" fn(u64) -> !, arg: u64) -> ThreadSched { - let (stack, entry_sp) = crate::loader::alloc_kernel_stack( - crate::loader::kernel_start, - body as usize as u64, - 0, - arg, - ) - .unwrap_or_else(|| panic!("kthread: no kernel stack for {name}")); + let (stack, entry_sp) = crate::loader::alloc_kernel_stack(crate::loader::Start::Kernel { body, arg }) + .unwrap_or_else(|| panic!("kthread: no kernel stack for {name}")); // Before the table lock: a panic holding the process table hangs the machine. let claim = Claim::take(name); diff --git a/kernel/src/syscall/proc.rs b/kernel/src/syscall/proc.rs index e856d42a1c1..3251510eac1 100644 --- a/kernel/src/syscall/proc.rs +++ b/kernel/src/syscall/proc.rs @@ -128,11 +128,16 @@ pub(super) fn sys_endowments(out: &mut crate::user_ptr::UserBytesMut) -> u64 { needed as u64 } -/// Spawn a thread; refuses a `stack_base` above `stack_ptr` (no stack to clamp to). +/// Spawn a thread; refuses a `stack_base` above `stack_ptr` (no stack to clamp to) +/// and an `entry` outside the user half, which the return to it would fault on +/// in the kernel's ring. The stack pointer is the thread's own to fault on. pub(super) fn sys_thread_spawn(entry: u64, stack_ptr: u64, arg: u64, stack_base: u64) -> u64 { if stack_base > stack_ptr { return SyscallError::InvalidArgument.to_u64(); } + let Some(entry) = toyos_userbound::Entry::new(entry) else { + return SyscallError::InvalidArgument.to_u64(); + }; // A None here is a resource failure or teardown race, never a bad argument. process::spawn_thread(entry, stack_ptr, arg, stack_base) .map_or(SyscallError::ResourceExhausted.to_u64(), |t| t.raw() as u64) diff --git a/tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs b/tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs new file mode 100644 index 00000000000..58d3026fa65 --- /dev/null +++ b/tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs @@ -0,0 +1,37 @@ +//! A thread entry outside the user half is refused by name, before any thread +//! exists to return to it. + +use toyos_abi::syscall::{self, MmapFlags, MmapProt, SyscallError}; + +const PAGE_2M: usize = 2 * 1024 * 1024; + +/// Canonical under neither 48-bit nor 57-bit addressing; the first address +/// past the user half; the first of the kernel half. +const ENTRIES: [u64; 3] = [0x0100_0000_0000_0000, 0x0000_8000_0000_0000, 0xFFFF_8000_0000_0000]; + +fn main() { + // SAFETY: a fresh anonymous mapping nothing else names. + let stack = unsafe { + syscall::mmap( + core::ptr::null_mut(), + PAGE_2M, + MmapProt::READ | MmapProt::WRITE, + MmapFlags::ANONYMOUS | MmapFlags::PRIVATE, + ) + }; + assert!(!stack.is_null(), "no memory for the thread's stack"); + let base = stack as u64; + for entry in ENTRIES { + // SAFETY: the stack is the mapping above; the entry is the input under test. + let answer = unsafe { syscall::thread_spawn(entry, base + PAGE_2M as u64, 0, base) }; + if SyscallError::from_u64(answer).is_none() { + // Accepted: the thread's first return to Ring 3 is where the harm is, so wait it out before judging. + syscall::thread_join(answer); + } + assert_eq!( + SyscallError::from_u64(answer), + Some(SyscallError::InvalidArgument), + "a thread entry of {entry:#x} was not refused", + ); + } +} diff --git a/tests/toyos.rs b/tests/toyos.rs index 996ed86de37..555db546f64 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -68,6 +68,9 @@ const ACTUATOR_KERNEL: &[&str] = toyos_build::build::TEST_KERNEL; // Rust helper binaries that are spawned by tests, not tests themselves. const RUST_SKIP: &[&str] = &[ + // It asks for a thread at an entry no CPU can return to: `thread_entry_noncanonical` + // runs it on a boot of its own, since a kernel that takes the entry takes the machine. + "spawn_noncanonical_entry", // **Its exit code is a measurement, not a verdict**, and the shared block // judges every member on `exit=0` alone — so it would red on every boot // that measured anything. It also needs the real-time band, which only @@ -245,6 +248,9 @@ const SCREEN_TESTS: &[(&str, qemu::Profile)] = &[ /// The tests whose machine shape *is* the test, each on a boot of its own. /// `run_machine_test` dispatches them. const MACHINE_TESTS: &[&str] = &[ + // The return to a thread's entry is the CPU's own instruction: no host test + // executes it, and the T14's rows share their boot with what it would take down. + "thread_entry_noncanonical", // Whether QEMU's virtio functions negotiated `VIRTIO_F_ACCESS_PLATFORM` // behind its emulated VT-d unit and without one: no shipped machine has a // virtio function, so only a QEMU machine can be asked. @@ -2711,6 +2717,21 @@ fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> { "iommu_virtio_platform" => common::iommu::iommu_virtio_platform(test_config), "nested_nmi_is_loud" => faults::nested_nmi_is_loud(test_config), "machine_shutdown" => power::machine_shutdown(test_config), + "thread_entry_noncanonical" => { + let name = "spawn_noncanonical_entry"; + let tests = compile::repo_root().join("tests/toyos-rust-tests"); + let bin = (name.to_string(), qemu::build_toyos_bin(qemu::SUITE_ARCH, &tests, name)); + let mut qemu = QemuInstance::boot_with_options(test_config, &[], &[bin], BootOptions::default()); + let result = qemu.run_test(&format!("test_rs_{name}"), Duration::from_secs(60)); + match (result.exit_code, &result.error) { + (Some(0), None) => Ok(()), + (code, error) => Err(format!( + "exit {code:?}: {}\n{}", + error.as_ref().map_or(String::new(), ToString::to_string), + result.stdout + )), + } + } "acpi_power_button" => power::acpi_power_button(test_config), "machine_shutdown_short_stop" => power::machine_shutdown_short_stop(test_config), other => Err(format!("unknown machine test {other}")), diff --git a/toyos-abi/src/syscall.rs b/toyos-abi/src/syscall.rs index 3e3260d780e..259fe56cfce 100644 --- a/toyos-abi/src/syscall.rs +++ b/toyos-abi/src/syscall.rs @@ -990,7 +990,8 @@ pub fn mark_tty(handle: RawHandle) { pub const MAX_THREADS: usize = 4096; /// Spawn a new thread with the given entry point, stack pointer, argument, and stack base. -/// `stack_base` is the bottom of the user stack (for stack info queries). +/// `stack_base` is the bottom of the user stack (for stack info queries). An +/// `entry` outside the user half is `InvalidArgument`. /// /// # Safety /// `entry` must be a valid function pointer and `stack`/`stack_base` must diff --git a/toyos-userbound/src/lib.rs b/toyos-userbound/src/lib.rs index bf63df0e219..fe80d070c22 100644 --- a/toyos-userbound/src/lib.rs +++ b/toyos-userbound/src/lib.rs @@ -38,6 +38,6 @@ pub use place::{PageSpan, Window}; pub use port::{port_access, IoBitmap, PortAccess, Ports, Reserved, Undeclared, IO_PORTS}; pub use segment::{pieces, segments, Pinned, Pins, Segment}; pub use span::{ - align_2m_checked, in_user_half, is_user_addr, is_user_object, rebase_base, Access, PAGE_2M, - PAGE_4K, USER_TOP, + align_2m_checked, in_user_half, is_user_addr, is_user_object, rebase_base, Access, Entry, + PAGE_2M, PAGE_4K, USER_TOP, }; diff --git a/toyos-userbound/src/span.rs b/toyos-userbound/src/span.rs index e7ef60af548..e94514fc56b 100644 --- a/toyos-userbound/src/span.rs +++ b/toyos-userbound/src/span.rs @@ -40,6 +40,33 @@ pub fn is_user_addr(addr: u64) -> bool { addr < USER_TOP } +/// An instruction pointer the kernel may return to userland at. +/// +/// A return to an address that is not canonical faults in the returning +/// instruction itself, in the kernel's ring and not the thread's, so one that +/// crossed the trust boundary is refused before any frame carries it. No +/// other constructor: a first return to userland takes this and nothing else. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Entry(u64); + +impl Entry { + /// `None` for an address outside the user half; what is mapped at one + /// inside it is the thread's own fault to take. + pub fn new(addr: u64) -> Option { + is_user_addr(addr).then_some(Self(addr)) + } + + pub const fn addr(self) -> u64 { + self.0 + } +} + +/// One past the highest address an image may claim: the user half less its +/// last page. An instruction that ends at [`USER_TOP`] leaves that address, no +/// canonical one, as where a syscall or an interrupt taken after it returns +/// to, so nothing executable is placed where one could end there. +const IMAGE_TOP: u64 = USER_TOP - PAGE_2M; + /// Whether `[ptr, ptr + len)` is entirely in the user half. /// /// Also the bound `sys_mmap` applies to a range it will install rather than @@ -54,12 +81,13 @@ pub fn in_user_half(ptr: u64, len: u64) -> bool { /// The load base an `ET_DYN` image rebases to `vm_base` (`vm_base - vaddr_min`), /// or `None` when it cannot: a `vaddr_min` above `vm_base` underflows the -/// subtraction, and a `span` reaching from `vm_base` past the user half does not +/// subtraction, and a `span` reaching from `vm_base` past [`IMAGE_TOP`] does not /// fit. The ELF spec leaves an `ET_DYN` `p_vaddr` unconstrained, so the kernel /// that picks `vm_base` is the only place that can refuse one. pub fn rebase_base(vm_base: u64, vaddr_min: u64, span: u64) -> Option { let base = vm_base.checked_sub(vaddr_min)?; - in_user_half(vm_base, span).then_some(base) + let end = vm_base.checked_add(span)?; + (end <= IMAGE_TOP).then_some(base) } /// Whether the kernel may read or write a `size`-byte value of alignment @@ -111,6 +139,19 @@ mod tests { assert!(is_user_addr(0)); } + /// The last address of the user half is one, and the first that is not + /// canonical, the first of the kernel half and the last of all are not. + #[test] + fn an_entry_is_an_address_of_the_user_half_and_nothing_else() { + assert_eq!(Entry::new(USER_TOP - 1).map(Entry::addr), Some(USER_TOP - 1)); + assert_eq!(Entry::new(0).map(Entry::addr), Some(0)); + assert_eq!(Entry::new(USER_TOP), None); + assert_eq!(Entry::new(0x0100_0000_0000_0000), None); + assert_eq!(Entry::new(0xFFFF_7FFF_FFFF_FFFF), None); + assert_eq!(Entry::new(0xFFFF_8000_0000_0000), None); + assert_eq!(Entry::new(u64::MAX), None); + } + #[test] fn a_range_ending_past_the_bound_is_refused_and_a_wrapping_one_too() { assert!(in_user_half(USER_TOP - 8, 8)); @@ -185,9 +226,11 @@ mod tests { } #[test] - fn an_image_that_rebases_past_the_user_half_is_refused() { - assert_eq!(rebase_base(USER_VM_BASE, 0, USER_TOP - USER_VM_BASE), Some(USER_VM_BASE)); - assert_eq!(rebase_base(USER_VM_BASE, 0, USER_TOP - USER_VM_BASE + 1), None); + fn an_image_that_reaches_the_last_page_of_the_user_half_is_refused() { + assert_eq!(rebase_base(USER_VM_BASE, 0, IMAGE_TOP - USER_VM_BASE), Some(USER_VM_BASE)); + assert_eq!(rebase_base(USER_VM_BASE, 0, IMAGE_TOP - USER_VM_BASE + 1), None); + assert_eq!(rebase_base(USER_VM_BASE, 0, USER_TOP - USER_VM_BASE), None); assert_eq!(rebase_base(USER_VM_BASE, 0, u64::MAX), None); + assert_eq!(rebase_base(u64::MAX, 0, 1), None); } } From b00560c4bc1d0dc6a46af0286593411630be0c88 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 10:30:44 +0200 Subject: [PATCH 3/3] Answer review round 1: the refusals ride the thread table's row, and a window stops where an image does The three entries a thread spawn refuses move into `abuse_thread_table`, which already drives `SYS_THREAD_SPAWN` raw on stacks it mapped and asserts a refusal by name, on the T14's `process_bound_threads` row. The type already keeps an unchecked entry from a trampoline, so what the test holds is the word the caller reads, and a metal row reaches that: the tier before a QEMU guest test. The wait for an accepted thread goes with the standalone binary: a kernel that accepted one is red on the assertion or on the fault, whichever comes first. `spawn_noncanonical_entry`, its `RUST_SKIP` entry, the `MACHINE_TESTS` entry and the arm are deleted. Not `abuse_tls_alloc`: no QEMU boot runs a discovered Rust binary. The suite's QEMU half is `MACHINE_TESTS` and `SCREEN_TESTS`, and `shared_metal` is the only runner of the rest, so that binary would have held the word on the T14 as well, under the shared boot's name and not a row's. `IMAGE_TOP` becomes `PLACED_TOP` and `Window::new` asserts its ceiling against it. The claim that nothing executable reaches the last page of the user half rested on the kernel's window happening to end at `STACK_BASE`; libraries are placed through that window and are executable. It is now one declaration, and the kernel's window is a `const`, so one above it does not compile. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- .../src/bin/abuse_thread_table.rs | 14 ++++++- .../src/bin/spawn_noncanonical_entry.rs | 37 ------------------- tests/toyos.rs | 21 ----------- toyos-userbound/src/place.rs | 18 +++++---- toyos-userbound/src/span.rs | 19 +++++----- 5 files changed, 34 insertions(+), 75 deletions(-) delete mode 100644 tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs diff --git a/tests/toyos-rust-tests/src/bin/abuse_thread_table.rs b/tests/toyos-rust-tests/src/bin/abuse_thread_table.rs index 679420e8313..912ce62d2c1 100644 --- a/tests/toyos-rust-tests/src/bin/abuse_thread_table.rs +++ b/tests/toyos-rust-tests/src/bin/abuse_thread_table.rs @@ -1,6 +1,7 @@ //! A process's threads are bounded at `MAX_THREADS`, exited ones not yet //! joined among them: past it a spawn is refused by name, and a join is room -//! for one. +//! for one. An entry outside the user half is refused by name whatever room +//! there is. use std::sync::atomic::{AtomicU32, Ordering}; use std::time::{Duration, Instant}; @@ -81,6 +82,17 @@ fn main() { let ran = AtomicU32::new(0); let mut stacks = Stacks { chunk: 0, used: 0 }; + // Canonical under neither 48-bit nor 57-bit addressing; the first address + // past the user half; the first of the kernel half. + let (base, top) = stacks.next(); + for entry in [0x0100_0000_0000_0000, 0x0000_8000_0000_0000, 0xFFFF_8000_0000_0000] { + assert_eq!( + SyscallError::from_u64(unsafe { syscall::thread_spawn(entry, top, 0, base) }), + Some(SyscallError::InvalidArgument), + "a thread entry of {entry:#x} was not refused", + ); + } + let mut first = None; let mut spawned = 0usize; let mut refusal = None; diff --git a/tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs b/tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs deleted file mode 100644 index 58d3026fa65..00000000000 --- a/tests/toyos-rust-tests/src/bin/spawn_noncanonical_entry.rs +++ /dev/null @@ -1,37 +0,0 @@ -//! A thread entry outside the user half is refused by name, before any thread -//! exists to return to it. - -use toyos_abi::syscall::{self, MmapFlags, MmapProt, SyscallError}; - -const PAGE_2M: usize = 2 * 1024 * 1024; - -/// Canonical under neither 48-bit nor 57-bit addressing; the first address -/// past the user half; the first of the kernel half. -const ENTRIES: [u64; 3] = [0x0100_0000_0000_0000, 0x0000_8000_0000_0000, 0xFFFF_8000_0000_0000]; - -fn main() { - // SAFETY: a fresh anonymous mapping nothing else names. - let stack = unsafe { - syscall::mmap( - core::ptr::null_mut(), - PAGE_2M, - MmapProt::READ | MmapProt::WRITE, - MmapFlags::ANONYMOUS | MmapFlags::PRIVATE, - ) - }; - assert!(!stack.is_null(), "no memory for the thread's stack"); - let base = stack as u64; - for entry in ENTRIES { - // SAFETY: the stack is the mapping above; the entry is the input under test. - let answer = unsafe { syscall::thread_spawn(entry, base + PAGE_2M as u64, 0, base) }; - if SyscallError::from_u64(answer).is_none() { - // Accepted: the thread's first return to Ring 3 is where the harm is, so wait it out before judging. - syscall::thread_join(answer); - } - assert_eq!( - SyscallError::from_u64(answer), - Some(SyscallError::InvalidArgument), - "a thread entry of {entry:#x} was not refused", - ); - } -} diff --git a/tests/toyos.rs b/tests/toyos.rs index 555db546f64..996ed86de37 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -68,9 +68,6 @@ const ACTUATOR_KERNEL: &[&str] = toyos_build::build::TEST_KERNEL; // Rust helper binaries that are spawned by tests, not tests themselves. const RUST_SKIP: &[&str] = &[ - // It asks for a thread at an entry no CPU can return to: `thread_entry_noncanonical` - // runs it on a boot of its own, since a kernel that takes the entry takes the machine. - "spawn_noncanonical_entry", // **Its exit code is a measurement, not a verdict**, and the shared block // judges every member on `exit=0` alone — so it would red on every boot // that measured anything. It also needs the real-time band, which only @@ -248,9 +245,6 @@ const SCREEN_TESTS: &[(&str, qemu::Profile)] = &[ /// The tests whose machine shape *is* the test, each on a boot of its own. /// `run_machine_test` dispatches them. const MACHINE_TESTS: &[&str] = &[ - // The return to a thread's entry is the CPU's own instruction: no host test - // executes it, and the T14's rows share their boot with what it would take down. - "thread_entry_noncanonical", // Whether QEMU's virtio functions negotiated `VIRTIO_F_ACCESS_PLATFORM` // behind its emulated VT-d unit and without one: no shipped machine has a // virtio function, so only a QEMU machine can be asked. @@ -2717,21 +2711,6 @@ fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> { "iommu_virtio_platform" => common::iommu::iommu_virtio_platform(test_config), "nested_nmi_is_loud" => faults::nested_nmi_is_loud(test_config), "machine_shutdown" => power::machine_shutdown(test_config), - "thread_entry_noncanonical" => { - let name = "spawn_noncanonical_entry"; - let tests = compile::repo_root().join("tests/toyos-rust-tests"); - let bin = (name.to_string(), qemu::build_toyos_bin(qemu::SUITE_ARCH, &tests, name)); - let mut qemu = QemuInstance::boot_with_options(test_config, &[], &[bin], BootOptions::default()); - let result = qemu.run_test(&format!("test_rs_{name}"), Duration::from_secs(60)); - match (result.exit_code, &result.error) { - (Some(0), None) => Ok(()), - (code, error) => Err(format!( - "exit {code:?}: {}\n{}", - error.as_ref().map_or(String::new(), ToString::to_string), - result.stdout - )), - } - } "acpi_power_button" => power::acpi_power_button(test_config), "machine_shutdown_short_stop" => power::machine_shutdown_short_stop(test_config), other => Err(format!("unknown machine test {other}")), diff --git a/toyos-userbound/src/place.rs b/toyos-userbound/src/place.rs index 992d7c7fd72..274ddb78939 100644 --- a/toyos-userbound/src/place.rs +++ b/toyos-userbound/src/place.rs @@ -17,7 +17,7 @@ //! What a region already registered says about itself is the kernel's own //! ledger and is not re-checked: a wrap there is a kernel bug and traps. -use crate::span::{align_2m_checked, PAGE_2M, USER_TOP}; +use crate::span::{align_2m_checked, PAGE_2M, PLACED_TOP}; /// The part of the user half anonymous ranges are placed in, and the guard /// left above each. Built in a `const`, so a window that breaks @@ -44,15 +44,15 @@ impl PageSpan { } impl Window { - /// Every bound 2 MiB-aligned, inside the user half, with room for at least - /// the guard: the kernel's own layout, so a window that breaks any of them + /// Every bound 2 MiB-aligned, at or below [`PLACED_TOP`], with room for at + /// least the guard: the kernel's own layout, so a window that breaks any of them /// is a kernel bug. pub const fn new(floor: u64, ceiling: u64, guard: u64) -> Self { assert!( floor.is_multiple_of(PAGE_2M) && ceiling.is_multiple_of(PAGE_2M) && guard.is_multiple_of(PAGE_2M), "a placement window's bounds and guard are whole 2 MiB pages" ); - assert!(ceiling <= USER_TOP, "a placement window is inside the user half"); + assert!(ceiling <= PLACED_TOP, "a placement window stops below the last page of the user half"); assert!(floor < ceiling && ceiling - floor >= guard, "a placement window holds its guard"); Self { floor, ceiling, guard } } @@ -108,6 +108,7 @@ impl Window { #[cfg(test)] mod tests { use super::*; + use crate::span::USER_TOP; /// The kernel's own window, by value: `vma::WINDOW`. const FLOOR: u64 = 0x0002_0000_0000; @@ -132,6 +133,9 @@ mod tests { } } + /// The highest window there is compiles; the test below is one page past it. + const _: Window = Window::new(FLOOR, PLACED_TOP, PAGE_2M); + #[test] fn a_zero_length_is_refused() { assert_eq!(WINDOW.span(0), None); @@ -210,9 +214,9 @@ mod tests { } #[test] - #[should_panic(expected = "inside the user half")] - fn a_window_past_the_user_half_is_a_kernel_bug() { - let _ = Window::new(FLOOR, USER_TOP + PAGE_2M, PAGE_2M); + #[should_panic(expected = "below the last page of the user half")] + fn a_window_reaching_the_last_page_of_the_user_half_is_a_kernel_bug() { + let _ = Window::new(FLOOR, USER_TOP, PAGE_2M); } /// `find_gap` no longer pre-filters by the ceiling; `gap` is the one diff --git a/toyos-userbound/src/span.rs b/toyos-userbound/src/span.rs index e94514fc56b..e4a458950a3 100644 --- a/toyos-userbound/src/span.rs +++ b/toyos-userbound/src/span.rs @@ -61,11 +61,12 @@ impl Entry { } } -/// One past the highest address an image may claim: the user half less its -/// last page. An instruction that ends at [`USER_TOP`] leaves that address, no -/// canonical one, as where a syscall or an interrupt taken after it returns -/// to, so nothing executable is placed where one could end there. -const IMAGE_TOP: u64 = USER_TOP - PAGE_2M; +/// One past the highest address the kernel places anything at, an image or a +/// [`Window`](crate::Window)'s range: the user half less its last page. An +/// instruction that ends at [`USER_TOP`] leaves that address, no canonical +/// one, as where a syscall or an interrupt taken after it returns to, so +/// nothing executable is placed where one could end there. +pub(crate) const PLACED_TOP: u64 = USER_TOP - PAGE_2M; /// Whether `[ptr, ptr + len)` is entirely in the user half. /// @@ -81,13 +82,13 @@ pub fn in_user_half(ptr: u64, len: u64) -> bool { /// The load base an `ET_DYN` image rebases to `vm_base` (`vm_base - vaddr_min`), /// or `None` when it cannot: a `vaddr_min` above `vm_base` underflows the -/// subtraction, and a `span` reaching from `vm_base` past [`IMAGE_TOP`] does not +/// subtraction, and a `span` reaching from `vm_base` past [`PLACED_TOP`] does not /// fit. The ELF spec leaves an `ET_DYN` `p_vaddr` unconstrained, so the kernel /// that picks `vm_base` is the only place that can refuse one. pub fn rebase_base(vm_base: u64, vaddr_min: u64, span: u64) -> Option { let base = vm_base.checked_sub(vaddr_min)?; let end = vm_base.checked_add(span)?; - (end <= IMAGE_TOP).then_some(base) + (end <= PLACED_TOP).then_some(base) } /// Whether the kernel may read or write a `size`-byte value of alignment @@ -227,8 +228,8 @@ mod tests { #[test] fn an_image_that_reaches_the_last_page_of_the_user_half_is_refused() { - assert_eq!(rebase_base(USER_VM_BASE, 0, IMAGE_TOP - USER_VM_BASE), Some(USER_VM_BASE)); - assert_eq!(rebase_base(USER_VM_BASE, 0, IMAGE_TOP - USER_VM_BASE + 1), None); + assert_eq!(rebase_base(USER_VM_BASE, 0, PLACED_TOP - USER_VM_BASE), Some(USER_VM_BASE)); + assert_eq!(rebase_base(USER_VM_BASE, 0, PLACED_TOP - USER_VM_BASE + 1), None); assert_eq!(rebase_base(USER_VM_BASE, 0, USER_TOP - USER_VM_BASE), None); assert_eq!(rebase_base(USER_VM_BASE, 0, u64::MAX), None); assert_eq!(rebase_base(u64::MAX, 0, 1), None);