From 6699efb9b9d34d584e5d53818a962c6d817a453d Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 21 Oct 2026 01:24:41 +0200 Subject: [PATCH] Remove toyos-untrusted; each decoder bounds the words it is handed The owner decided (2026-10-21) to remove `toyos-untrusted` and keep every boundary check. `Untrusted` made a bound a review prompt, not a guarantee: `map` let a closure narrow or copy the value out before the widening compare, and a trivial bound (`at_most(u32::MAX)`, blockring's `opaque`) was an unconditional exit. Every check it carried is one comparison, so each is now written at the decode that makes the validated type, in plain integers, compared before any narrowing. - Kernel syscalls: the six register bounds (inventory, counters, partition blocks, badge, TRACE_FLOOD, thread name) are direct comparisons of the `u64` argument. - Kernel virtio: `cap_subwindow` refuses with its own `CapWindow::NoBar` and `PastBar`; `parse_used` takes the device's two words and stays the only way to a `DescSlot`. - Kernel xHCI: slot 0 is `checked_sub(1)`, not a wrap the bound happened to catch. - `toyos-virtio`: `UsedRefusal::Head { id, size }` and `Written { head, len, writable }` carry the numbers, not a borrowed `Refused`. - `toyos-vdagent`: the event, port, chunk and data bounds are direct. - `toyos-transport`: `Consumer::pop` returns the peer's `[u32; E]`; `Run::decode` takes two words and leaves the bound to `Geometry::run`, which already refuses every run past the arena, overflow included; `Inflight::answer` takes the tag. The `rustc-dep-of-std` hop to the crate goes. - `toyos-blockring`: `Request::decode` and `Completion::decode` match on constants and refuse reserved words directly; `opaque` and the client's re-wrap of an already-decoded tag go. The crate, its workspace membership, its `PUBLISHED` entry and std's `[patch]`, sources and manifests for it are deleted, and the Rust fork's `library/Cargo.lock` drops it (fork e1c5e93d342). The record of the crates crates.io lacks now owes only `toyos-transport`. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 13 - Cargo.toml | 1 - build/sdkversion.rs | 1 - build/sysroot.rs | 19 +- build/toolchain.rs | 2 +- ...ublish-until-someone-puts-it-up-by-hand.md | 7 +- kernel/Cargo.toml | 1 - kernel/dma/Cargo.toml | 5 +- kernel/pci/src/bar.rs | 7 +- kernel/src/drivers/virtio.rs | 62 +-- kernel/src/drivers/xhci/mod.rs | 8 +- kernel/src/mm/dma.rs | 2 +- kernel/src/syscall/dispatch.rs | 37 +- lib/blockring/src/client.rs | 4 +- lib/blockring/src/entry.rs | 55 +-- lib/blockring/src/model.rs | 6 +- lib/blockring/src/server.rs | 13 +- lib/untrusted/Cargo.toml | 24 -- lib/untrusted/src/lib.rs | 377 ------------------ lib/vdagent/Cargo.toml | 4 - lib/vdagent/src/control.rs | 13 +- lib/vdagent/src/stream.rs | 11 +- lib/virtio/Cargo.toml | 3 - lib/virtio/src/queue.rs | 33 +- lib/virtio/src/tests.rs | 24 +- rust | 2 +- sdk/libc/Cargo.lock | 7 - sdk/transport/Cargo.toml | 3 +- sdk/transport/src/arena.rs | 14 +- sdk/transport/src/inflight.rs | 30 +- sdk/transport/src/lib.rs | 7 +- sdk/transport/src/queue.rs | 27 +- sdk/transport/src/stream.rs | 14 +- sdk/transport/tests/loom.rs | 8 +- tests/guest/rust-tests/Cargo.lock | 8 - .../rust-tests/src/bin/abuse_thread_name.rs | 14 +- 36 files changed, 184 insertions(+), 682 deletions(-) delete mode 100644 lib/untrusted/Cargo.toml delete mode 100644 lib/untrusted/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 73f4c9e18da..bdc4d7057b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1981,7 +1981,6 @@ dependencies = [ "toyos-symbols", "toyos-tco", "toyos-tsc", - "toyos-untrusted", "toyos-usbhid", "toyos-userbound", "toyos-wallclock", @@ -4857,20 +4856,12 @@ version = "0.1.0" dependencies = [ "loom", "rustc-std-workspace-core", - "toyos-untrusted", ] [[package]] name = "toyos-tsc" version = "0.1.0" -[[package]] -name = "toyos-untrusted" -version = "0.1.0" -dependencies = [ - "rustc-std-workspace-core", -] - [[package]] name = "toyos-update" version = "0.1.0" @@ -4901,9 +4892,6 @@ dependencies = [ [[package]] name = "toyos-vdagent" version = "0.1.0" -dependencies = [ - "toyos-untrusted", -] [[package]] name = "toyos-vdagent-bridge" @@ -4917,7 +4905,6 @@ name = "toyos-virtio" version = "0.1.0" dependencies = [ "toyos-device-memory", - "toyos-untrusted", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index f1405cf2d56..a05fee81c99 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,7 +71,6 @@ members = [ "lib/tmpdir", "lib/trace", "lib/tsc", - "lib/untrusted", "lib/update", "lib/userbound", "lib/vdagent", diff --git a/build/sdkversion.rs b/build/sdkversion.rs index 22cb6d2ba0d..3c9c0a6868b 100644 --- a/build/sdkversion.rs +++ b/build/sdkversion.rs @@ -22,7 +22,6 @@ pub struct Crate { /// The published crates, in the order a publisher must take them. pub const PUBLISHED: &[Crate] = &[ Crate { name: "toyos-abi", dir: "sdk/abi" }, - Crate { name: "toyos-untrusted", dir: "lib/untrusted" }, Crate { name: "toyos-transport", dir: "sdk/transport" }, Crate { name: "toyos-keymap", dir: "sdk/keymap" }, Crate { name: "toyos-font", dir: "sdk/font" }, diff --git a/build/sysroot.rs b/build/sysroot.rs index 4932efe775f..6ec7b64ce54 100644 --- a/build/sysroot.rs +++ b/build/sysroot.rs @@ -65,14 +65,13 @@ use crate::toolchain::{self, host_triple, GuestTarget, Owner, Role, GUEST_TARGET use whole_toolchain::{whole, Whole}; /// The per-worktree sources that end up inside a sysroot: std links the -/// `toyos-abi`, `toyos`, `toyos-transport` and `toyos-untrusted` [`std_patch`] -/// hands it and compiles its ToyOS backend from `sdk/std`, and -/// `libtoyos_c.a` is `sdk/libc` with `toyos-elf` and `toyos-osrelease`. -pub const SYSROOT_SOURCES: [&str; 9] = [ +/// `toyos-abi`, `toyos` and `toyos-transport` [`std_patch`] hands it and +/// compiles its ToyOS backend from `sdk/std`, and `libtoyos_c.a` is `sdk/libc` +/// with `toyos-elf` and `toyos-osrelease`. +pub const SYSROOT_SOURCES: [&str; 8] = [ "sdk/abi/src", "sdk/toyos/src", "sdk/transport/src", - "lib/untrusted/src", "sdk/std", "lib/elf/src", "sdk/osrelease/src", @@ -82,11 +81,10 @@ pub const SYSROOT_SOURCES: [&str; 9] = [ /// Their manifests, and the lockfile and cargo configuration libc is built /// under: the features, versions and flags of the same build. -pub(crate) const SYSROOT_MANIFESTS: [&str; 9] = [ +pub(crate) const SYSROOT_MANIFESTS: [&str; 8] = [ "sdk/abi/Cargo.toml", "sdk/toyos/Cargo.toml", "sdk/transport/Cargo.toml", - "lib/untrusted/Cargo.toml", "lib/elf/Cargo.toml", "sdk/osrelease/Cargo.toml", "sdk/libc/Cargo.toml", @@ -97,8 +95,7 @@ pub(crate) const SYSROOT_MANIFESTS: [&str; 9] = [ /// Of [`SYSROOT_MANIFESTS`], the crates std names by version, which std's /// lockfile resolves with the fork's own through [`std_patch`]: what of a /// worktree can move a freestanding target's dependency versions. -const STD_MANIFESTS: [&str; 4] = - ["sdk/abi/Cargo.toml", "sdk/toyos/Cargo.toml", "sdk/transport/Cargo.toml", "lib/untrusted/Cargo.toml"]; +const STD_MANIFESTS: [&str; 3] = ["sdk/abi/Cargo.toml", "sdk/toyos/Cargo.toml", "sdk/transport/Cargo.toml"]; /// One of the crates std names by version, in a worktree. pub(crate) struct StdCrate { @@ -1279,7 +1276,7 @@ pub(crate) mod tests { /// The manifests [`std_crates`] reads, in a worktree at `root` that has /// no other source: what a bootstrap run is handed std's patch from. pub(crate) fn std_manifests(root: &Path) { - for (manifest, name) in STD_MANIFESTS.into_iter().zip(["toyos-abi", "toyos", "toyos-transport", "toyos-untrusted"]) { + for (manifest, name) in STD_MANIFESTS.into_iter().zip(["toyos-abi", "toyos", "toyos-transport"]) { write(&root.join(manifest), &format!("[package]\nname = \"{name}\"\n")); } } @@ -1322,8 +1319,6 @@ pub(crate) mod tests { format!("patch.crates-io.toyos.path=\"{at}/sdk/toyos\""), "--config".to_string(), format!("patch.crates-io.toyos-transport.path=\"{at}/sdk/transport\""), - "--config".to_string(), - format!("patch.crates-io.toyos-untrusted.path=\"{at}/lib/untrusted\""), ], ); for name in ["work tree", "work\"tree", "work\\tree", "work\ttree"] { diff --git a/build/toolchain.rs b/build/toolchain.rs index 39aec9473cc..25907e1aa84 100644 --- a/build/toolchain.rs +++ b/build/toolchain.rs @@ -825,7 +825,7 @@ mod tests { let registry = |name: &str, version: &str| { format!("/home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/{name}-{version}/src/lib.rs") }; - let rest = format!("{} {}", ours("sdk/transport"), ours("lib/untrusted")); + let rest = ours("sdk/transport"); let built_from = |abi: &str, toyos: &str| { fs::write(built.join("dist/libstd.d"), format!("{}/dist/libstd.rlib: {abi} {toyos} {rest}\n", built.display())) .unwrap(); diff --git a/issues/a-crate-new-to-the-published-set-reds-every-publish-until-someone-puts-it-up-by-hand.md b/issues/a-crate-new-to-the-published-set-reds-every-publish-until-someone-puts-it-up-by-hand.md index 4dfffc8cda9..9f35678eda2 100644 --- a/issues/a-crate-new-to-the-published-set-reds-every-publish-until-someone-puts-it-up-by-hand.md +++ b/issues/a-crate-new-to-the-published-set-reds-every-publish-until-someone-puts-it-up-by-hand.md @@ -40,7 +40,10 @@ adds to `PUBLISHED` is not on crates.io. Owner: the owner's crates.io account for the first versions; `build/sdkversion.rs` for the rule. -**Exit**: `toyos-untrusted` and `toyos-transport` are on crates.io, each naming -`publish.yml` its trusted publisher; a `publish` run at main's tip and the next +`toyos-untrusted` has since been removed from the tree and from `PUBLISHED`, +so `toyos-transport` is the one crate still owed. + +**Exit**: `toyos-transport` is on crates.io, naming `publish.yml` its trusted +publisher; a `publish` run at main's tip and the next nightly's `release` are green; and `PUBLISHED`'s doc comment says a crate joins it only once crates.io holds it under that publisher. diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index 4d2c908f539..10def215757 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -499,7 +499,6 @@ toyos-rootimage = { path = "../lib/rootimage" } toyos-quiesce = { path = "../lib/quiesce" } toyos-random = { path = "../lib/random" } toyos-symbols = { path = "../lib/symbols" } -toyos-untrusted = { path = "../lib/untrusted" } toyos-usbhid = { path = "usbhid" } toyos-userbound = { path = "../lib/userbound" } toyos-wallclock = { path = "../lib/wallclock" } diff --git a/kernel/dma/Cargo.toml b/kernel/dma/Cargo.toml index 7a4e29b17f4..5111c3b5cf2 100644 --- a/kernel/dma/Cargo.toml +++ b/kernel/dma/Cargo.toml @@ -1,6 +1,5 @@ -# A member of the host workspace (root `Cargo.toml`), like toyos-userbound, -# toyos-untrusted and toyos-pci: it is pure, it has no dependencies, and its -# tests run on the host. +# A member of the host workspace (root `Cargo.toml`), like toyos-userbound and +# toyos-pci: it is pure, it has no dependencies, and its tests run on the host. # # What lives here is where a typed access into a DMA region lands and whether it # may land there. The kernel's `mm::Dma` is the only caller; it is a crate diff --git a/kernel/pci/src/bar.rs b/kernel/pci/src/bar.rs index 7ab7c6770ad..527ca28e95a 100644 --- a/kernel/pci/src/bar.rs +++ b/kernel/pci/src/bar.rs @@ -19,12 +19,7 @@ //! these bits are the device's, and a field that says "this is not memory" is //! not a field to decode past. //! -//! **Why this is a decode and not a `toyos_untrusted::Untrusted`.** That type -//! is for a number used as an *index* or a *length*, where the fix is a -//! comparison with a bound. Nothing here is indexed and nothing is sized: the -//! defect was reading the register as the wrong *kind* of thing, and the fix is -//! a type that has no address in it until the encoding said there was one. -//! [`Width`] is that type — a caller cannot reach an address without having +//! [`Width`] is the decode — a caller cannot reach an address without having //! been handed a [`Memory`], and it cannot be handed one for an I/O BAR. //! //! **What this does not bound is the address.** A BAR that says memory names a diff --git a/kernel/src/drivers/virtio.rs b/kernel/src/drivers/virtio.rs index f8dee90b0e0..b4f3b82e9f8 100644 --- a/kernel/src/drivers/virtio.rs +++ b/kernel/src/drivers/virtio.rs @@ -1,7 +1,5 @@ use crate::arch::barrier; -use toyos_untrusted::{Refused, Untrusted}; - use crate::mm::Mmio; use super::pci::PciDevice; use crate::mm::policy::MmioPolicy; @@ -92,8 +90,21 @@ struct VirtioPciConfig { /// Why a virtio PCI capability names no config window, or the window it names. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum CapWindow { - Refused(Refused), Unmapped, + NoBar { index: u8 }, + PastBar { offset: u32, length: u32, window: u64 }, +} + +impl core::fmt::Display for CapWindow { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + Self::Unmapped => write!(f, "names a BAR no window was mapped for"), + Self::NoBar { index } => write!(f, "names BAR {index}, past the six a header has"), + Self::PastBar { offset, length, window } => { + write!(f, "names {length:#x} bytes at {offset:#x}, past its BAR's {window:#x}") + } + } + } } /// The required capability a device's chain never produced a window for; its @@ -168,15 +179,16 @@ fn cap_subwindow( offset: u32, length: u32, ) -> Result { - let bar_idx = Untrusted::new(bar_byte).index(mapped_bars.len()).map_err(CapWindow::Refused)?; - let bar = mapped_bars[bar_idx].as_ref().ok_or(CapWindow::Unmapped)?; + let slot = mapped_bars.get(usize::from(bar_byte)).ok_or(CapWindow::NoBar { index: bar_byte })?; + let bar = slot.as_ref().ok_or(CapWindow::Unmapped)?; // The mapped window is the BAR's advertised size or the ceiling, never under 16 bytes. let window = bar.size(); - let size = (length as u64).max(4); - // length ≤ window keeps `window - size` from underflowing and offset + size inside the window. - Untrusted::new(length).at_most(window).map_err(CapWindow::Refused)?; - let offset = Untrusted::new(offset).at_most(window - size).map_err(CapWindow::Refused)?; - Ok(bar.subregion(offset, size)) + let size = u64::from(length).max(4); + // size ≤ window keeps `window - size` from underflowing and offset + size inside the window. + if size > window || u64::from(offset) > window - size { + return Err(CapWindow::PastBar { offset, length, window }); + } + Ok(bar.subregion(u64::from(offset), size)) } impl VirtioPciConfig { @@ -191,7 +203,10 @@ impl VirtioPciConfig { for cap in pci_dev.capabilities() { if cap.id() != PCI_CAP_ID_VENDOR { continue; } // The BAR index is the device's; one past the six the array holds skips the capability rather than indexing it. - let Ok(bar_idx) = Untrusted::new(cap.read_u8(4)).index(mapped_bars.len()) else { continue }; + let bar_idx = usize::from(cap.read_u8(4)); + if bar_idx >= mapped_bars.len() { + continue; + } if mapped_bars[bar_idx].is_none() { // A capability naming a non-memory or unsizable BAR maps to no window. match pci_dev.memory_bar(bar_idx as u8) { @@ -226,7 +241,7 @@ impl VirtioPciConfig { let mmio = match cap_subwindow(&mapped_bars, cap.read_u8(4), cap.read_u32(8), cap.read_u32(12)) { Ok(mmio) => mmio, Err(CapWindow::Unmapped) => continue, - Err(CapWindow::Refused(why)) => { + Err(why) => { log!("VirtIO: PCI {:02x}:{:02x}.{} capability {why} — skipped", pci_dev.bus, pci_dev.dev, pci_dev.func); continue; @@ -354,13 +369,12 @@ impl<'pool> Virtqueue<'pool> { USED_RING_OFF + i as usize * USED_ELEM_SIZE } - /// The two fields of a used element, returned as [`Untrusted`] so no caller can use them without naming a bound. - /// The only reads of a used element's fields in this queue; nothing else here touches them unwrapped. - fn used_ring_id(&self, i: u16) -> Untrusted { - Untrusted::new(self.used.read(self.used_elem_at(i))) + /// The two fields of a used element, as the device wrote them: [`Self::parse_used`] is their only reader. + fn used_ring_id(&self, i: u16) -> u32 { + self.used.read(self.used_elem_at(i)) } - fn used_ring_len(&self, i: u16) -> Untrusted { - Untrusted::new(self.used.read(self.used_elem_at(i) + 4)) + fn used_ring_len(&self, i: u16) -> u32 { + self.used.read(self.used_elem_at(i) + 4) } /// The initial pool of descriptor slots; call once after construction. @@ -462,17 +476,15 @@ impl<'pool> Virtqueue<'pool> { /// What a used-ring element must satisfy: a head inside this queue's table, at a published /// chain, written no further than that chain. Refused rather than clamped: there is nothing /// here to recover from a forged completion. - fn parse_used(&self, id: Untrusted, len: Untrusted) -> Option<(DescSlot, u32)> { + fn parse_used(&self, id: u32, len: u32) -> Option<(DescSlot, u32)> { // `chain_bytes` is exactly `size` long: the descriptor table's own bound, not a constant beside it. - let head = id.index(self.chain_bytes.len()).ok()?; + let head = usize::try_from(id).ok().filter(|&head| head < self.chain_bytes.len())?; let chain = self.chain_bytes[head]; - if chain == 0 { + if chain == 0 || len > chain { return None; } - let written = len.at_most(chain as u64).ok()?; - // Exact: `index` proved `head < size`, a `u16`, and `at_most` that `written` is no more - // than `chain`, a `u32`. - Some((DescSlot(head as u16), written as u32)) + // Exact: `head < size`, a `u16`. + Some((DescSlot(head as u16), len)) } /// Submit a descriptor chain and block until the device completes it, returning the recovered `DescSlot`. diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index c906403e550..fba836a0a50 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -21,7 +21,6 @@ use crate::mm::Dma; use crate::log; use super::pci::PciDevice; use crate::sync::{OwedGuard, OwedLock}; -use toyos_untrusted::Untrusted; use toyos_xhci::job::{Await, Outcome, Outstanding, Stages, CC_SHORT_PACKET, CC_SUCCESS}; use toyos_xhci::port::{self as portmachine, GaveUp, Gone, PortState, Reset, Step}; use toyos_xhci::call::AfterBreak; @@ -530,10 +529,11 @@ impl Layout { } /// The block for a 1-based slot id, or `None` when the pool has no room; `slot_id` is untrusted, from the controller's own answer. - /// - /// `wrapping_sub` is sound only because `index` bounds the result: slot 0 wraps to `u8::MAX`, never `< dev_blocks`. fn device(&self, slot_id: u8) -> Option { - let index = Untrusted::new(slot_id).map(|v| v.wrapping_sub(1)).index(self.dev_blocks).ok()?; + let index = usize::from(slot_id.checked_sub(1)?); + if index >= self.dev_blocks { + return None; + } Some(self.dev_base + index * DEV_STRIDE) } diff --git a/kernel/src/mm/dma.rs b/kernel/src/mm/dma.rs index e4ae299869f..0081a1370ff 100644 --- a/kernel/src/mm/dma.rs +++ b/kernel/src/mm/dma.rs @@ -129,7 +129,7 @@ impl<'pool, D: Discipline> Dma<'pool, D> { unsafe { copy_nonoverlapping(self.base.add(offset), dst.as_mut_ptr(), dst.len()) } } - // Panics, not `Result`: every offset/length here is the driver's own arithmetic, never a device-chosen number (those arrive as `Untrusted` and are bounded before becoming an offset). + // Panics, not `Result`: every offset/length here is the driver's own arithmetic, never a device-chosen number (those are bounded at their decode before becoming an offset). #[inline] fn check(self, offset: usize, len: usize) { if let Err(why) = toyos_dma::within(offset, len, self.size) { diff --git a/kernel/src/syscall/dispatch.rs b/kernel/src/syscall/dispatch.rs index abb8c5d08aa..78b68c1c410 100644 --- a/kernel/src/syscall/dispatch.rs +++ b/kernel/src/syscall/dispatch.rs @@ -17,7 +17,6 @@ use toyos_abi::handle::{RawHandle, Rights}; #[cfg(feature = "test-actuators")] use toyos_abi::syscall::debug_action as DA; use toyos_abi::syscall::*; -use toyos_untrusted::Untrusted; use super::HANDLE_LEN; #[cfg(feature = "test-actuators")] @@ -268,9 +267,10 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> // A census of the hardware and of who drives it, so it costs // Rights::INVENTORY. The count is bounded before it becomes a window. SYS_DEVICE_INVENTORY => { - let Ok(count) = Untrusted::new(a3).at_most(MAX_INVENTORY_RECORDS) else { + let count = a3; + if count > MAX_INVENTORY_RECORDS { return SyscallError::InvalidArgument.to_u64(); - }; + } let len = count * toyos_abi::inventory::RECORD_BYTES as u64; let Some(mut buf) = ctx.user_bytes_mut(UserAddr::new(a2), len) else { return bad_addr }; sys_device_inventory(RawHandle(a1 as u32), &mut buf) @@ -278,9 +278,10 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> // What every CPU counts, so it costs Rights::COUNTERS; a CPU per record, // so the count is bounded by the most CPUs there can be. SYS_COUNTERS => { - let Ok(count) = Untrusted::new(a3).at_most(crate::scheduler::MAX_CPUS as u64) else { + let count = a3; + if count > crate::scheduler::MAX_CPUS as u64 { return SyscallError::InvalidArgument.to_u64(); - }; + } let len = count * toyos_abi::counters::RECORD_BYTES as u64; let Some(mut buf) = ctx.user_bytes_mut(UserAddr::new(a2), len) else { return bad_addr }; sys_counters(RawHandle(a1 as u32), &mut buf) @@ -405,11 +406,8 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> // The count is bounded before it becomes a window, and a transfer of // nothing is refused rather than sent to a device as a zero-length command. SYS_PARTITION_READ | SYS_PARTITION_WRITE => { - let Ok(count) = Untrusted::new(a4).at_most(toyos_abi::part::MAX_BLOCKS_PER_CALL as u64) - else { - return SyscallError::InvalidArgument.to_u64(); - }; - if count == 0 { + let count = a4; + if count == 0 || count > toyos_abi::part::MAX_BLOCKS_PER_CALL as u64 { return SyscallError::InvalidArgument.to_u64(); } let len = count * toyos_abi::part::BLOCK_BYTES as u64; @@ -477,10 +475,8 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> SYS_PORT_CREATE => sys_port_create(), SYS_PORT_MINT => { // Copied in before any handle is looked up: the kernel holds the badge, never the caller's page. - let Ok(len) = Untrusted::new(a3).at_most(MAX_BADGE as u64) else { - return SyscallError::InvalidArgument.to_u64(); - }; - if len == 0 { + let len = a3; + if len == 0 || len > MAX_BADGE as u64 { return SyscallError::InvalidArgument.to_u64(); } let Some(bytes) = ctx.user_bytes(UserAddr::new(a2), len) else { return bad_addr }; @@ -602,10 +598,7 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> // bound and its stale answer are the shipped paths. DA::COUNTERS_DEAF => crate::counters::deaf::stage(a2), DA::COUNTERS_HEAR => crate::counters::deaf::end(), - DA::TRACE_FLOOD => match Untrusted::new(a2).at_most(DA::TRACE_FLOOD_MOST) { - Ok(count) => crate::trace::flood(count), - Err(_) => SyscallError::InvalidArgument.to_u64(), - }, + DA::TRACE_FLOOD if a2 <= DA::TRACE_FLOOD_MOST => crate::trace::flood(a2), // An interrupt inside a syscall's body, which nothing a guest does puts there on demand. DA::RING0_TIMER_IN_SYSCALL => ring0_timer_in_syscall(), DA::ACPI_FIRMWARE_LOCK => crate::arch::acpi_mode::debug_firmware_lock(a2), @@ -627,11 +620,11 @@ pub(crate) fn syscall_dispatch(num: u64, a1: u64, a2: u64, a3: u64, a4: u64) -> SYS_SET_THREAD_NAME => { // Refused by name, not clamped: a length past the bound is the caller's // bad argument, not a shorter name to silently write. - let Ok(len) = Untrusted::new(a2).at_most(process::THREAD_NAME_LEN as u64) else { + if a2 > process::THREAD_NAME_LEN as u64 { return SyscallError::InvalidArgument.to_u64(); - }; - let len = len as usize; - let Some(bytes) = ctx.user_bytes(UserAddr::new(a1), len as u64) else { + } + let len = a2 as usize; + let Some(bytes) = ctx.user_bytes(UserAddr::new(a1), a2) else { return bad_addr; }; let mut name = [0u8; process::THREAD_NAME_LEN]; diff --git a/lib/blockring/src/client.rs b/lib/blockring/src/client.rs index 7f69ca411d1..73c0485f2a1 100644 --- a/lib/blockring/src/client.rs +++ b/lib/blockring/src/client.rs @@ -36,7 +36,7 @@ use alloc::collections::{BTreeMap, VecDeque}; use alloc::vec::Vec; -use toyos_transport::{Inflight, Run, Untrusted, Violation}; +use toyos_transport::{Inflight, Run, Violation}; use crate::entry::{Completion, Op, Request, Status}; use crate::layout::DEPTH; @@ -196,7 +196,7 @@ impl Client { /// What the server answered. pub fn complete(&mut self, completion: Completion) -> Result<(), Violation> { - let sent = self.wire.answer(Untrusted::new(completion.tag))?; + let sent = self.wire.answer(completion.tag)?; let q = sent.queued; match (q.kind, completion.status) { (Kind::User(ticket), Status::Ok) => { diff --git a/lib/blockring/src/entry.rs b/lib/blockring/src/entry.rs index a6fcf280543..af9ee697dc1 100644 --- a/lib/blockring/src/entry.rs +++ b/lib/blockring/src/entry.rs @@ -4,7 +4,7 @@ //! request is bounded here against the arena and the partition, and a word //! this protocol does not define is a refusal, never a default. -use toyos_transport::{Run, Untrusted}; +use toyos_transport::Run; use crate::layout::{ARENA, CQE_WORDS, MAX_REQUEST_BLOCKS, SQE_WORDS}; @@ -57,22 +57,18 @@ impl Request { /// `partition_blocks`: every block it names is inside both the arena and /// the partition, a transfer moves at least one block and at most /// [`MAX_REQUEST_BLOCKS`], and a flush names nothing. - pub fn decode(words: [Untrusted; SQE_WORDS], partition_blocks: u64) -> Result { + pub fn decode(words: [u32; SQE_WORDS], partition_blocks: u64) -> Result { let [op, tag, lba_low, lba_high, blocks, arena, reserved @ ..] = words; - let tag = opaque(tag); let refused = Refused::Malformed { tag }; - if !reserved.iter().all(|word| word.is(0)) { + if reserved.iter().any(|&word| word != 0) { return Err(refused); } - let lba = u64::from(opaque(lba_low)) | (u64::from(opaque(lba_high)) << 32); - let transfer: fn(Run, u64) -> Op = if op.is(READ) { - |run, lba| Op::Read { run, lba } - } else if op.is(WRITE) { - |run, lba| Op::Write { run, lba } - } else if op.is(FLUSH) && lba == 0 && blocks.is(0) && arena.is(0) { - return Ok(Self { op: Op::Flush, tag }); - } else { - return Err(refused); + let lba = u64::from(lba_low) | (u64::from(lba_high) << 32); + let transfer: fn(Run, u64) -> Op = match op { + READ => |run, lba| Op::Read { run, lba }, + WRITE => |run, lba| Op::Write { run, lba }, + FLUSH if lba == 0 && blocks == 0 && arena == 0 => return Ok(Self { op: Op::Flush, tag }), + _ => return Err(refused), }; let run = Run::decode(arena, blocks, &ARENA).map_err(|_| refused)?; let blocks = run.count(); @@ -83,15 +79,6 @@ impl Request { } } -/// A word every value of which means something: a tag its answer echoes, or -/// half of an `lba` the partition bounds whole. -fn opaque(word: Untrusted) -> u32 { - word.at_most(u32::MAX.into()) - .ok() - .and_then(|word| u32::try_from(word).ok()) - .expect("every u32 is at most u32::MAX") -} - /// What a completion says of its request. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum Status { @@ -139,13 +126,13 @@ impl Completion { /// `None` for words no server of this protocol writes: the client treats a /// server that wrote them as one that broke the session. - pub fn decode(words: [Untrusted; CQE_WORDS]) -> Option { + pub fn decode(words: [u32; CQE_WORDS]) -> Option { let [tag, status, reserved @ ..] = words; - if !reserved.iter().all(|word| word.is(0)) { + if reserved.iter().any(|&word| word != 0) { return None; } - let status = Status::ALL.into_iter().find(|s| status.is(s.word()))?; - Some(Self { tag: opaque(tag), status }) + let status = Status::ALL.into_iter().find(|s| s.word() == status)?; + Some(Self { tag, status }) } } @@ -159,10 +146,6 @@ mod tests { ARENA.run(first, count).unwrap() } - fn peer(words: [u32; N]) -> [Untrusted; N] { - words.map(Untrusted::new) - } - #[test] fn a_request_survives_its_words() { let last = ARENA.slots() - MAX_REQUEST_BLOCKS; @@ -171,10 +154,10 @@ mod tests { Request { op: Op::Write { run: run(last, MAX_REQUEST_BLOCKS), lba: 0 }, tag: u32::MAX }, Request { op: Op::Flush, tag: 0 }, ] { - assert_eq!(Request::decode(peer(request.encode()), PARTITION), Ok(request)); + assert_eq!(Request::decode(request.encode(), PARTITION), Ok(request)); } let wide = Request { op: Op::Read { run: run(3, 2), lba: 1 << 40 }, tag: 1 }; - assert_eq!(Request::decode(peer(wide.encode()), u64::MAX), Ok(wide)); + assert_eq!(Request::decode(wide.encode(), u64::MAX), Ok(wide)); } /// Every field a hostile client can set out of range is refused, and the @@ -202,7 +185,7 @@ mod tests { ]; for (what, words) in cases { assert_eq!( - Request::decode(peer(words), PARTITION), + Request::decode(words, PARTITION), Err(Refused::Malformed { tag: 42 }), "{what} was not refused" ); @@ -213,9 +196,9 @@ mod tests { fn a_completion_survives_its_words_and_refuses_what_it_does_not_define() { for status in Status::ALL { let c = Completion { tag: 9, status }; - assert_eq!(Completion::decode(peer(c.encode())), Some(c)); + assert_eq!(Completion::decode(c.encode()), Some(c)); } - assert_eq!(Completion::decode(peer([1, 5, 0, 0])), None); - assert_eq!(Completion::decode(peer([1, 0, 1, 0])), None); + assert_eq!(Completion::decode([1, 5, 0, 0]), None); + assert_eq!(Completion::decode([1, 0, 1, 0]), None); } } diff --git a/lib/blockring/src/model.rs b/lib/blockring/src/model.rs index 433755e272f..3f431ee1daa 100644 --- a/lib/blockring/src/model.rs +++ b/lib/blockring/src/model.rs @@ -35,7 +35,7 @@ use core::sync::atomic::Ordering; use std::collections::HashSet; use toyos_blockhold::Holds; -use toyos_transport::{Consumer, Place, Producer, Untrusted, Violation, Word}; +use toyos_transport::{Consumer, Place, Producer, Violation, Word}; use crate::client::{Client, Outcome, Ticket, MAX_ATTEMPTS}; use crate::entry::{Completion, Op, Request}; @@ -156,7 +156,7 @@ impl Queues { } /// The oldest request, as the server's ring gives it. - fn take(&mut self) -> Option<[Untrusted; SQE_WORDS]> { + fn take(&mut self) -> Option<[u32; SQE_WORDS]> { let want = self.sq.pop_front()?; let words = self.server.0.pop(&self.page).expect("an honest client's tail"); let words = words.expect("the ring holds what the queue does"); @@ -174,7 +174,7 @@ impl Queues { /// The oldest completion, as the client's ring gives it. fn read(&mut self) -> Option { let want = self.cq.pop_front()?; - let words: [Untrusted; CQE_WORDS] = + let words: [u32; CQE_WORDS] = self.client.1.pop(&self.page).expect("an honest server's tail").expect("the ring holds what the queue does"); self.client.1.release(&self.page); assert_eq!(Completion::decode(words), Some(want), "the ring gave the client what the queue holds"); diff --git a/lib/blockring/src/server.rs b/lib/blockring/src/server.rs index ef94f3e3737..c2745355485 100644 --- a/lib/blockring/src/server.rs +++ b/lib/blockring/src/server.rs @@ -18,7 +18,6 @@ use alloc::vec::Vec; use toyos_blockhold::{Holds, Writer}; -use toyos_transport::Untrusted; use crate::entry::{Completion, Op, Refused, Request, Status}; use crate::layout::SQE_WORDS; @@ -81,7 +80,7 @@ impl ServerSession { /// not let it make, are answered at once and never reach the device: the /// second would make one tag two requests, and the client could not tell /// which answer was whose. - pub fn take(&mut self, words: [Untrusted; SQE_WORDS]) -> Taken { + pub fn take(&mut self, words: [u32; SQE_WORDS]) -> Taken { let request = match Request::decode(words, self.blocks) { Ok(request) => request, Err(Refused::Malformed { tag }) => { @@ -141,11 +140,11 @@ mod tests { use super::*; use crate::layout::ARENA; - fn write(tag: u32) -> [Untrusted; SQE_WORDS] { - Request { op: Op::Write { run: ARENA.run(0, 1).unwrap(), lba: 0 }, tag }.encode().map(Untrusted::new) + fn write(tag: u32) -> [u32; SQE_WORDS] { + Request { op: Op::Write { run: ARENA.run(0, 1).unwrap(), lba: 0 }, tag }.encode() } - fn flush(tag: u32) -> [Untrusted; SQE_WORDS] { - Request { op: Op::Flush, tag }.encode().map(Untrusted::new) + fn flush(tag: u32) -> [u32; SQE_WORDS] { + Request { op: Op::Flush, tag }.encode() } #[test] @@ -201,7 +200,7 @@ mod tests { assert_eq!(session.take(write(1)), Taken::Answer(Completion { tag: 1, status: Status::ReadOnly })); assert_eq!(session.inflight().len(), 0); let read = Request { op: Op::Read { run: ARENA.run(0, 1).unwrap(), lba: 0 }, tag: 2 }; - assert!(matches!(session.take(read.encode().map(Untrusted::new)), Taken::Issue(_))); + assert!(matches!(session.take(read.encode()), Taken::Issue(_))); assert!(matches!(session.take(flush(3)), Taken::Issue(_))); } } diff --git a/lib/untrusted/Cargo.toml b/lib/untrusted/Cargo.toml deleted file mode 100644 index 89f5e1cd396..00000000000 --- a/lib/untrusted/Cargo.toml +++ /dev/null @@ -1,24 +0,0 @@ -# A member of the host workspace (root `Cargo.toml`), like toyos-pci and -# toyos-gpt: it is pure, it has no dependencies, and its tests run on the host. -# -# What lives here is one type. Every other pure crate in this tree decodes one -# *format* a device or a peer published — a capability structure, a GPT header, -# an ELF file — and each of them reaches, at the bottom, for the same sentence: -# this number crossed a trust boundary, so it is not an index until something -# compared it with the table. That sentence had no type, so it was written by -# hand where somebody remembered to and not written where nobody did. - -[package] -name = "toyos-untrusted" -description = "A number that crossed a trust boundary, and the four ways it gets out." -version = "0.1.0" # never bumped; crates.io's is assigned on publish (build/sdkversion.rs) -edition = "2021" -license = "MIT OR Apache-2.0" -repository = "https://github.com/ToyOSOrg/ToyOS" - -[features] -# std links it through `toyos-transport`. -rustc-dep-of-std = ["core"] - -[dependencies] -core = { version = "1.0.0", optional = true, package = "rustc-std-workspace-core" } diff --git a/lib/untrusted/src/lib.rs b/lib/untrusted/src/lib.rs deleted file mode 100644 index f3aa2d5858e..00000000000 --- a/lib/untrusted/src/lib.rs +++ /dev/null @@ -1,377 +0,0 @@ -//! A number that crossed a trust boundary, and the four ways it gets out. -//! -//! CLAUDE.md's law is *"input that crossed a trust boundary is never trusted -//! and never panics the kernel — it is refused"*, and the recorded failures of -//! it are all one shape: a `u32` a device wrote, or a `u16` a peer wrote, was -//! carried in a plain integer and then used as an index, a length or an -//! address. A plain integer records nothing about where it came from, so the -//! comparison that should stand between it and the array is a thing an author -//! has to remember. Six filed issues are six authors who did not. -//! -//! [`Untrusted`] is that provenance, in the type. It has **no** arithmetic, -//! no `Deref`, no `From`, no cast and no accessor: the only functions on it -//! that return a bare integer take the bound to check against and answer -//! [`Result`]. So a value that came off DMA cannot reach an index without the -//! comparison, and a reviewer does not have to notice that it should — -//! the compiler refuses the code. -//! -//! **This is not a second vocabulary.** `UserAddr` (`kernel/src/mm/mod.rs`) is -//! the same idea for the *address* half of the same boundary, and says so: -//! *"the type's name is a claim, and this is the only constructor that makes it -//! true of a number userland chose"*. What was missing was the value half. -//! `toyos-pci`'s module doc already states the principle for one format — -//! *"a function that names a reserved BAR indicator is not a function to -//! truncate into range"* — and this is the noun that sentence was reaching for. -//! -//! # What it costs -//! -//! Nothing at runtime. [`Untrusted`] is `#[repr(transparent)]` over `T` and -//! every method is `#[inline]`, so it compiles to the integer it wraps; the -//! checks are the compare-and-branch a correct call site had to perform anyway, -//! and the incorrect ones were only cheaper because they were wrong. -//! -//! # What it does not do -//! -//! It does not decide *what* the bound is — that is the driver's knowledge and -//! it stays at the driver. It carries no source: a value a device wrote and a -//! value a peer wrote are the same kind of not-known-yet, and splitting them -//! would put the interesting question ("which of these is reachable from -//! userland?") into a type where it cannot be answered instead of into the -//! module header where it can. -//! -//! `no_std`, no allocation, no dependencies, `forbid(unsafe_code)`. - -#![no_std] -#![forbid(unsafe_code)] - -use core::fmt; - -/// The integer widths a boundary hands over. Sealed: the point of the type is -/// that the set of things it can be is closed. -mod sealed { - pub trait Sealed {} -} - -/// An integer that can be widened losslessly for comparison. -pub trait Raw: sealed::Sealed + Copy + fmt::Debug { - /// This value as a `u64`, which every bound below is expressed in. Lossless - /// for every implementor, which is what makes a comparison against a bound - /// mean what it says. - fn widen(self) -> u64; -} - -macro_rules! raw { - ($($t:ty),*) => { $( - impl sealed::Sealed for $t {} - impl Raw for $t { - #[inline] - fn widen(self) -> u64 { self as u64 } - } - )* }; -} - -raw!(u8, u16, u32, u64); - -/// Why a value that crossed a boundary is not the thing it was asked to be. -/// -/// Carries both numbers, so the caller's message can name the noun and this can -/// name the disagreement — `msix::Unusable`'s division of labour, which is what -/// keeps a refusal readable without every bound owning its own error enum. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Refused { - /// It was asked to be an index into a table of `len` entries and is not. - PastTable { value: u64, len: u64 }, - /// It was asked to be at most `bound` and is more. - PastBound { value: u64, bound: u64 }, - /// It was asked to be exactly `wanted`. - NotExactly { value: u64, wanted: u64 }, -} - -impl fmt::Display for Refused { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::PastTable { value, len } => write!(f, "{value} indexes no entry of {len}"), - Self::PastBound { value, bound } => write!(f, "{value} is past the bound of {bound}"), - Self::NotExactly { value, wanted } => write!(f, "{value} where {wanted} was required"), - } - } -} - -/// A number this side of the machine did not choose. -/// -/// Wrap one at the read that produces it — the volatile load out of a used -/// ring, the config-space dword, the header field — and it stays wrapped until -/// something compares it with a bound. There is deliberately no way to unwrap -/// it without one: -/// -/// ```compile_fail -/// # use toyos_untrusted::Untrusted; -/// let len = Untrusted::new(4096u32); -/// let n: u32 = len.into(); // no `From`, no `Into` -/// ``` -/// -/// ```compile_fail -/// # use toyos_untrusted::Untrusted; -/// let id = Untrusted::new(3u32); -/// let next = id + 1; // no arithmetic -/// ``` -/// -/// ```compile_fail -/// # use toyos_untrusted::Untrusted; -/// let id = Untrusted::new(3u32); -/// let table = [0u8; 16]; -/// let entry = table[id as usize]; // no cast -/// ``` -/// -/// ```compile_fail -/// # use toyos_untrusted::Untrusted; -/// let id = Untrusted::new(3u32); -/// if id < 16 { } // no ordering against a bare integer -/// ``` -/// -/// What it is for, in one line each: -/// -/// ``` -/// # use toyos_untrusted::{Untrusted, Refused}; -/// let table = [0u8; 16]; -/// // The device named descriptor 3 of 16 — an index, once compared. -/// assert_eq!(Untrusted::new(3u32).index(table.len()), Ok(3)); -/// // And descriptor 0x1_0003, which narrows to 3 and is not. -/// assert_eq!( -/// Untrusted::new(0x1_0003u32).index(table.len()), -/// Err(Refused::PastTable { value: 0x1_0003, len: 16 }), -/// ); -/// ``` -#[derive(Clone, Copy, PartialEq, Eq)] -#[repr(transparent)] -pub struct Untrusted(T); - -impl Untrusted { - /// Wrap a value that came from outside. - /// - /// Unrestricted on purpose: *making* one of these is always sound, and a - /// constructor nobody could call would only move the laundering to whoever - /// could. What is restricted is getting a number back out. - #[inline] - pub const fn new(value: T) -> Self { - Self(value) - } - - /// As an index into a table of `len` entries. - /// - /// The one exit that produces a `usize`, and the reason: a `usize` is what - /// indexes, so every path to one goes past this comparison. `len` is the - /// table's own length — `slice.len()`, or the queue size the descriptor - /// table was allocated from — never a constant retyped beside it. - #[inline] - pub fn index(self, len: usize) -> Result { - let value = self.0.widen(); - if value >= len as u64 { - return Err(Refused::PastTable { value, len: len as u64 }); - } - Ok(value as usize) - } - - /// As a count, size or length of at most `bound`. - /// - /// Inclusive, because the values this guards are byte counts and a buffer - /// entirely filled is the ordinary case. - #[inline] - pub fn at_most(self, bound: u64) -> Result { - let value = self.0.widen(); - if value > bound { - return Err(Refused::PastBound { value, bound }); - } - Ok(value) - } - - /// As exactly `wanted` — for a field a protocol fixes, where anything else - /// is not a value to clamp but a peer that is not speaking the protocol. - #[inline] - pub fn exactly(self, wanted: T) -> Result { - let (value, wanted) = (self.0.widen(), wanted.widen()); - if value != wanted { - return Err(Refused::NotExactly { value, wanted }); - } - Ok(value) - } - - /// Whether it is this value. Answering a `bool` decides nothing about - /// memory, so this is not an exit — the caller learns only what it already - /// wrote down. - #[inline] - pub fn is(self, value: T) -> bool { - self.0.widen() == value.widen() - } - - /// Decode it — mask a field out, shift, narrow — and keep the provenance. - /// - /// **The escape hatch that is not one.** Arithmetic on a value from outside - /// is ordinary and necessary; what must not happen is the *result* becoming - /// trusted because a computation happened to it. So this exists, and it - /// returns another [`Untrusted`]. - #[inline] - pub fn map(self, f: impl FnOnce(T) -> U) -> Untrusted { - Untrusted(f(self.0)) - } -} - -/// Prints the number, so a refusal can name what arrived. -/// -/// Formatting is not an exit: it produces text, and no caller indexes with -/// text. Without it every refusal would have to launder the value to say what -/// it refused, which is the one thing that would make laundering necessary. -impl fmt::Debug for Untrusted { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Untrusted({:?})", self.0) - } -} - -impl fmt::Display for Untrusted { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt(&self.0, f) - } -} - -impl fmt::LowerHex for Untrusted { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::LowerHex::fmt(&self.0, f) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn an_index_is_below_the_table_it_indexes() { - assert_eq!(Untrusted::new(0u32).index(16), Ok(0)); - assert_eq!(Untrusted::new(15u32).index(16), Ok(15)); - assert_eq!( - Untrusted::new(16u32).index(16), - Err(Refused::PastTable { value: 16, len: 16 }) - ); - } - - /// The defect this type is named for. `desc_to_rx` is `[u8; 16]` and the - /// used ring's id is a `u32`; the code that read it wrote `id as u16`, so - /// `0x1_0003` became descriptor 3 — a real entry — and every check written - /// after the cast agreed. Widening rather than narrowing is the whole - /// difference, and it is not a thing a call site can get wrong here. - #[test] - fn a_value_whose_low_bits_are_in_range_is_still_out_of_range() { - assert_eq!( - Untrusted::new(0x1_0003u32).index(16), - Err(Refused::PastTable { value: 0x1_0003, len: 16 }) - ); - assert_eq!( - Untrusted::new(0xFFFF_0000u32).index(16), - Err(Refused::PastTable { value: 0xFFFF_0000, len: 16 }) - ); - } - - #[test] - fn an_empty_table_indexes_nothing() { - assert_eq!( - Untrusted::new(0u32).index(0), - Err(Refused::PastTable { value: 0, len: 0 }) - ); - } - - /// A length is inclusive of its bound and an index is not, and the two - /// exits are separate so that a call site cannot pick the wrong one by - /// writing `len - 1` and getting it wrong at the boundary. - #[test] - fn a_length_may_equal_its_bound_where_an_index_may_not() { - assert_eq!(Untrusted::new(256u32).at_most(256), Ok(256)); - assert_eq!( - Untrusted::new(257u32).at_most(256), - Err(Refused::PastBound { value: 257, bound: 256 }) - ); - assert_eq!( - Untrusted::new(256u32).index(256), - Err(Refused::PastTable { value: 256, len: 256 }) - ); - } - - #[test] - fn every_width_widens_rather_than_wrapping() { - assert_eq!(Untrusted::new(u8::MAX).at_most(255), Ok(255)); - assert_eq!(Untrusted::new(u16::MAX).at_most(65_535), Ok(65_535)); - assert_eq!(Untrusted::new(u32::MAX).at_most(u32::MAX as u64), Ok(u32::MAX as u64)); - assert_eq!(Untrusted::new(u64::MAX).at_most(u64::MAX), Ok(u64::MAX)); - assert_eq!( - Untrusted::new(u64::MAX).index(16), - Err(Refused::PastTable { value: u64::MAX, len: 16 }) - ); - } - - #[test] - fn exactly_refuses_by_naming_both_numbers() { - assert_eq!(Untrusted::new(12u32).exactly(12), Ok(12)); - assert_eq!( - Untrusted::new(13u32).exactly(12), - Err(Refused::NotExactly { value: 13, wanted: 12 }) - ); - } - - /// Deciding a `bool` reaches no memory, so equality against a constant the - /// caller wrote is not a way out — and it must keep working, because a - /// driver comparing a status word against a known code is the ordinary - /// thing to do with a value it does not trust. - #[test] - fn asking_whether_it_is_a_known_value_is_not_an_exit() { - assert!(Untrusted::new(0u32).is(0)); - assert!(!Untrusted::new(1u32).is(0)); - } - - /// The property the compile-fail cases above state negatively: a decode - /// produces another untrusted value, so the bound is still owed after it. - #[test] - fn decoding_keeps_the_provenance() { - let bar = Untrusted::new(0xFEBD_4004u32); - let low_bits = bar.map(|v| (v & 0xF) as u8); - assert!(low_bits.is(4)); - assert_eq!( - bar.map(|v| v >> 4).index(16), - Err(Refused::PastTable { value: 0x0FEB_D400, len: 16 }) - ); - } - - /// Two `Untrusted`s compare, because neither side of that comparison is a - /// claim about memory. This is what lets a driver notice that the device - /// said the same thing twice. - #[test] - fn two_untrusted_values_compare_with_each_other() { - assert_eq!(Untrusted::new(7u32), Untrusted::new(7u32)); - assert_ne!(Untrusted::new(7u32), Untrusted::new(8u32)); - } - - #[test] - fn a_refusal_says_which_number_and_which_bound() { - extern crate std; - use std::string::ToString; - assert_eq!( - Refused::PastTable { value: 0x1_0003, len: 16 }.to_string(), - "65539 indexes no entry of 16" - ); - assert_eq!( - Refused::PastBound { value: 4097, bound: 4096 }.to_string(), - "4097 is past the bound of 4096" - ); - assert_eq!( - Refused::NotExactly { value: 13, wanted: 12 }.to_string(), - "13 where 12 was required" - ); - } - - /// `#[repr(transparent)]` and nothing else in the struct: the type is the - /// integer, so adopting it at a site in a hot path costs that site nothing. - #[test] - fn it_is_the_integer_it_wraps() { - use core::mem::{align_of, size_of}; - assert_eq!(size_of::>(), size_of::()); - assert_eq!(align_of::>(), align_of::()); - assert_eq!(size_of::>(), size_of::()); - } -} diff --git a/lib/vdagent/Cargo.toml b/lib/vdagent/Cargo.toml index 0e3dac86b81..17eb27823bc 100644 --- a/lib/vdagent/Cargo.toml +++ b/lib/vdagent/Cargo.toml @@ -11,9 +11,5 @@ edition = "2021" license = "MIT OR Apache-2.0" publish = false -[dependencies] -# The type a host word stays in until it has met its bound. -toyos-untrusted = { path = "../untrusted" } - [lib] doctest = false diff --git a/lib/vdagent/src/control.rs b/lib/vdagent/src/control.rs index fd411e93f19..8deca61deef 100644 --- a/lib/vdagent/src/control.rs +++ b/lib/vdagent/src/control.rs @@ -6,8 +6,6 @@ //! "without a NUL terminator", and QEMU 11.1.1 ends it with one; the port is //! the declared [`crate::PORT_ID`] instead. -use toyos_untrusted::Untrusted; - /// One control message's bytes. pub const BYTES: usize = 8; @@ -83,13 +81,10 @@ pub fn decode(bytes: &[u8], max_nr_ports: u32) -> Result { let id = u32::from_le_bytes([message[0], message[1], message[2], message[3]]); let event = u16::from_le_bytes([message[4], message[5]]); let value = u16::from_le_bytes([message[6], message[7]]); - let event = Untrusted::new(event) - .index(Event::ALL.len()) - .map(|at| Event::ALL[at]) - .map_err(|_| Refused::Event { event })?; - Untrusted::new(id) - .index(max_nr_ports as usize) - .map_err(|_| Refused::Port { id, max_nr_ports })?; + let event = *Event::ALL.get(usize::from(event)).ok_or(Refused::Event { event })?; + if id >= max_nr_ports { + return Err(Refused::Port { id, max_nr_ports }); + } Ok(Control { id, event, value }) } diff --git a/lib/vdagent/src/stream.rs b/lib/vdagent/src/stream.rs index ed1e22b3d08..0f68b369312 100644 --- a/lib/vdagent/src/stream.rs +++ b/lib/vdagent/src/stream.rs @@ -18,8 +18,6 @@ use alloc::vec::Vec; -use toyos_untrusted::Untrusted; - use crate::wire; /// `VD_AGENT_MAX_DATA_SIZE`: the most one chunk carries. @@ -151,9 +149,7 @@ impl Decoder { self.chunk_filled = 0; let port = word(&self.chunk, 0); let size = word(&self.chunk, 4); - self.chunk_left = Untrusted::new(size) - .at_most(MAX_CHUNK as u64) - .map_err(|_| Unframed { size })? as usize; + self.chunk_left = usize::try_from(size).ok().filter(|&n| n <= MAX_CHUNK).ok_or(Unframed { size })?; self.ours = port == wire::CLIENT_PORT; if !self.ours { each(Err(Refusal::OtherPort { port })); @@ -242,10 +238,9 @@ fn admit(protocol: u32, kind: u32, size: u32) -> Result { if protocol != wire::PROTOCOL { return Err(Refusal::Protocol { protocol }); } - if Untrusted::new(size).at_most(MAX_DATA as u64).is_err() { + let Some(len) = usize::try_from(size).ok().filter(|&n| n <= MAX_DATA) else { return Err(Refusal::TooLarge { kind, size }); - } - let len = size as usize; + }; let (admitted, fits) = match kind { wire::ANNOUNCE_CAPABILITIES => (Kind::Capabilities, len >= 8), wire::GRAB => (Kind::Grab, len >= 4 && len.is_multiple_of(4) && (len - 4) / 4 <= MAX_GRAB_TYPES), diff --git a/lib/virtio/Cargo.toml b/lib/virtio/Cargo.toml index 805797f1afd..3a114c31650 100644 --- a/lib/virtio/Cargo.toml +++ b/lib/virtio/Cargo.toml @@ -20,9 +20,6 @@ license = "MIT OR Apache-2.0" publish = false [dependencies] -# The type a used-ring element's two fields stay in until each has met its -# bound. -toyos-untrusted = { path = "../untrusted" } # `Registers` and `DmaBuffers`: the boundary every userland driver is written # against, and the order of its two barriers. toyos-device-memory = { path = "../device-memory" } diff --git a/lib/virtio/src/queue.rs b/lib/virtio/src/queue.rs index 87aae8a855d..4504996fc73 100644 --- a/lib/virtio/src/queue.rs +++ b/lib/virtio/src/queue.rs @@ -21,8 +21,6 @@ use alloc::vec; use alloc::vec::Vec; -use toyos_untrusted::{Refused, Untrusted}; - use toyos_device_memory::DmaBuffers; /// §2.7: "The maximum Queue Size value is 32768." @@ -118,14 +116,14 @@ pub struct Used { pub enum UsedRefusal { /// An element's `id` is past the descriptor table. The element is taken /// and names no chain. - Head(Refused), + Head { id: u32, size: u16 }, /// An element's `id` is a head no chain is in flight at: one the device /// already gave back, or one never published. The element is taken. NoChain { head: u16 }, /// An element's `len` is more than its chain's device-writable elements /// hold (§2.7.8). The element is taken and its chain stays in flight: its /// buffers are the device's still, and are not handed up. - Written { head: u16, len: Refused }, + Written { head: u16, len: u32, writable: u32 }, /// The used index stands further on than the chains made available: the /// device says it used buffers it was never given, so no element from the /// last one taken on is one this driver can tell from a stale one. @@ -137,14 +135,16 @@ pub enum UsedRefusal { impl core::fmt::Display for UsedRefusal { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Self::Head(why) => write!(f, "a used element's head {why}"), + Self::Head { id, size } => { + write!(f, "a used element's head {id} is past the queue's {size} descriptors") + } Self::NoChain { head } => { write!(f, "a used element names head {head}, where no chain is in flight") } - Self::Written { head, len } => write!( + Self::Written { head, len, writable } => write!( f, "the used element for head {head} claims more bytes than its chain may be \ - written: {len}" + written: {len} where it may take {writable}" ), Self::Jumped { used, taken, available } => write!( f, @@ -345,24 +345,25 @@ impl Virtqueue { let at = self.parts.used + RING_ENTRIES + (self.last_used % self.size) as usize * USED_ELEM_BYTES; - let id = Untrusted::new(self.mem.read32(at)); - let len = Untrusted::new(self.mem.read32(at + USED_ELEM_LEN)); + let id = self.mem.read32(at); + let len = self.mem.read32(at + USED_ELEM_LEN); self.last_used = self.last_used.wrapping_add(1); - let first = id.index(self.slots.len()).map_err(UsedRefusal::Head)?; - // Exact: `index` proved it below the table's length, a `u16`. + let Some(first) = usize::try_from(id).ok().filter(|&first| first < self.slots.len()) else { + return Err(UsedRefusal::Head { id, size: self.size }); + }; + // Exact: below the table's length, a `u16`. let head = first as u16; let chain = self.slots[first]; if chain.descs == 0 { return Err(UsedRefusal::NoChain { head }); } - let written = len - .at_most(chain.writable as u64) - .map_err(|len| UsedRefusal::Written { head, len })?; + if len > chain.writable { + return Err(UsedRefusal::Written { head, len, writable: chain.writable }); + } for slot in &mut self.slots[first..first + chain.descs as usize] { *slot = Slot::default(); } - // Exact: `at_most` proved it no more than `chain.writable`, a `u32`. - Ok(Some(Used { head, written: written as u32 })) + Ok(Some(Used { head, written: len })) } } diff --git a/lib/virtio/src/tests.rs b/lib/virtio/src/tests.rs index b437035d020..aa4fa463116 100644 --- a/lib/virtio/src/tests.rs +++ b/lib/virtio/src/tests.rs @@ -4,7 +4,6 @@ use std::collections::BTreeMap; use std::vec::Vec; -use toyos_untrusted::Refused; use crate::pci::{ status, Layout, Live, Offer, Refusal, Setup, Source, VendorCap, WalkRefusal, @@ -731,7 +730,7 @@ fn a_head_past_the_table_is_refused() { ring.use_chain(id, 0); assert_eq!( queue.poll_used(), - Err(UsedRefusal::Head(Refused::PastTable { value: id as u64, len: SIZE as u64 })) + Err(UsedRefusal::Head { id, size: SIZE }) ); } assert_eq!(queue.poll_used(), Ok(None)); @@ -783,10 +782,7 @@ fn more_bytes_than_the_chain_may_be_written_is_refused() { ring.use_chain(0, len); assert_eq!( queue.poll_used(), - Err(UsedRefusal::Written { - head: 0, - len: Refused::PastBound { value: len as u64, bound: 64 } - }) + Err(UsedRefusal::Written { head: 0, len, writable: 64 }) ); ring.use_chain(0, 64); assert_eq!(queue.poll_used(), Ok(Some(Used { head: 0, written: 64 }))); @@ -795,7 +791,7 @@ fn more_bytes_than_the_chain_may_be_written_is_refused() { ring.use_chain(2, 1); assert_eq!( queue.poll_used(), - Err(UsedRefusal::Written { head: 2, len: Refused::PastBound { value: 1, bound: 0 } }) + Err(UsedRefusal::Written { head: 2, len: 1, writable: 0 }) ); ring.use_chain(2, 0); assert_eq!(queue.poll_used(), Ok(Some(Used { head: 2, written: 0 }))); @@ -851,7 +847,7 @@ fn a_refused_element_does_not_hide_the_ones_behind_it() { ring.use_chain(0x55, 0); ring.use_chain(1, 64); ring.use_chain(2, 3); - assert!(matches!(queue.poll_used(), Err(UsedRefusal::Head(_)))); + assert!(matches!(queue.poll_used(), Err(UsedRefusal::Head { .. }))); assert_eq!(queue.poll_used(), Ok(Some(Used { head: 1, written: 64 }))); assert_eq!(queue.poll_used(), Ok(Some(Used { head: 2, written: 3 }))); assert_eq!(queue.poll_used(), Ok(None)); @@ -1005,15 +1001,9 @@ fn a_hostile_device_is_believed_only_where_the_model_agrees() { taken = taken.wrapping_add(1); let chain = u16::try_from(id).ok().and_then(|head| flying.get(&head).map(|c| (head, *c))); let want = match chain { - _ if id >= SIZE as u32 => Err(UsedRefusal::Head(Refused::PastTable { - value: id as u64, - len: SIZE as u64, - })), + _ if id >= SIZE as u32 => Err(UsedRefusal::Head { id, size: SIZE }), None => Err(UsedRefusal::NoChain { head: id as u16 }), - Some((head, (_, writable))) if len > writable => Err(UsedRefusal::Written { - head, - len: Refused::PastBound { value: len as u64, bound: writable as u64 }, - }), + Some((head, (_, writable))) if len > writable => Err(UsedRefusal::Written { head, len, writable }), Some((head, _)) => { flying.remove(&head); believed += 1; @@ -1021,7 +1011,7 @@ fn a_hostile_device_is_believed_only_where_the_model_agrees() { } }; match want { - Err(UsedRefusal::Head(_)) => refused[0] += 1, + Err(UsedRefusal::Head { .. }) => refused[0] += 1, Err(UsedRefusal::NoChain { .. }) => refused[1] += 1, Err(UsedRefusal::Written { .. }) => refused[2] += 1, Err(UsedRefusal::Jumped { .. }) | Ok(_) => {} diff --git a/rust b/rust index 73f63e2441b..e1c5e93d342 160000 --- a/rust +++ b/rust @@ -1 +1 @@ -Subproject commit 73f63e2441b93b979229144b58033954ffb1d40d +Subproject commit e1c5e93d34236ee3e57d510920f94c0256c647eb diff --git a/sdk/libc/Cargo.lock b/sdk/libc/Cargo.lock index 2d766086466..f3b061534dd 100644 --- a/sdk/libc/Cargo.lock +++ b/sdk/libc/Cargo.lock @@ -59,13 +59,6 @@ version = "0.1.0" [[package]] name = "toyos-transport" version = "0.1.0" -dependencies = [ - "toyos-untrusted", -] - -[[package]] -name = "toyos-untrusted" -version = "0.1.0" [[package]] name = "windows-link" diff --git a/sdk/transport/Cargo.toml b/sdk/transport/Cargo.toml index 3665df21744..8bfca088cb6 100644 --- a/sdk/transport/Cargo.toml +++ b/sdk/transport/Cargo.toml @@ -17,7 +17,7 @@ repository = "https://github.com/ToyOSOrg/ToyOS" [features] # std links it through `toyos`, whose stream end is a session's client. -rustc-dep-of-std = ["core", "toyos-untrusted/rustc-dep-of-std"] +rustc-dep-of-std = ["core"] # Declared, never enabled by any build that ships: each reverts one decision so # the model that refuses it is shown able to red at all. `build/ci.rs`'s # `CONTROLS` runs each and demands the named test's own FAILED line. @@ -61,7 +61,6 @@ head-only = [] [dependencies] core = { version = "1.0.0", optional = true, package = "rustc-std-workspace-core" } -toyos-untrusted = { path = "../../lib/untrusted" } [dev-dependencies] loom = "0.7" diff --git a/sdk/transport/src/arena.rs b/sdk/transport/src/arena.rs index d85fbe9fe9c..63686e3247a 100644 --- a/sdk/transport/src/arena.rs +++ b/sdk/transport/src/arena.rs @@ -6,7 +6,7 @@ use core::fmt; -use crate::{Untrusted, Violation}; +use crate::Violation; /// Bytes `offset..offset + len` of the region. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] @@ -69,12 +69,8 @@ impl fmt::Debug for Run { impl Run { /// The run a peer's two words name. - pub fn decode(first: Untrusted, count: Untrusted, geometry: &Geometry) -> Result { - let bounded = |word: Untrusted| word.at_most(u64::from(geometry.slots)).ok()?.try_into().ok(); - bounded(first) - .zip(bounded(count)) - .and_then(|(first, count)| geometry.run(first, count)) - .ok_or(Violation::Run) + pub fn decode(first: u32, count: u32, geometry: &Geometry) -> Result { + geometry.run(first, count).ok_or(Violation::Run) } pub fn first(&self) -> u32 { @@ -108,11 +104,11 @@ mod tests { /// last slot is a run and one past it is not, however the sum wraps. #[test] fn a_run_a_peer_names_is_inside_the_arena() { - let run = Run::decode(Untrusted::new(510), Untrusted::new(1), &PAGES).unwrap(); + let run = Run::decode(510, 1, &PAGES).unwrap(); assert_eq!(run.span(), Span { offset: 0x1000 + 510 * 4096, len: 4096 }); for (first, count) in [(510, 2), (0, 0), (511, 1), (1, u32::MAX), (u32::MAX, 1), (0, 512)] { assert_eq!( - Run::decode(Untrusted::new(first), Untrusted::new(count), &PAGES), + Run::decode(first, count, &PAGES), Err(Violation::Run), "{first}+{count}" ); diff --git a/sdk/transport/src/inflight.rs b/sdk/transport/src/inflight.rs index 9a45176bdb9..b692dce6097 100644 --- a/sdk/transport/src/inflight.rs +++ b/sdk/transport/src/inflight.rs @@ -12,7 +12,7 @@ use core::fmt; -use crate::{Untrusted, Violation}; +use crate::Violation; #[derive(Clone, PartialEq, Eq, Hash)] struct Slot { @@ -53,10 +53,10 @@ impl Inflight { /// What the peer's `tag` answers; a tag nothing is in flight under is a /// violation. - pub fn answer(&mut self, tag: Untrusted) -> Result { - let index = tag.map(|t| t & Self::INDEX_MASK).index(D).map_err(|_| Violation::Tag)?; + pub fn answer(&mut self, tag: u32) -> Result { + let index = usize::try_from(tag & Self::INDEX_MASK).map_err(|_| Violation::Tag)?; let slot = self.slots.get_mut(index).ok_or(Violation::Tag)?; - if !tag.map(|t| t.wrapping_shr(Self::INDEX_BITS)).is(slot.seq) { + if tag.wrapping_shr(Self::INDEX_BITS) != slot.seq { return Err(Violation::Tag); } slot.value.take().ok_or(Violation::Tag) @@ -110,18 +110,18 @@ mod tests { let a = inflight.insert("a").unwrap(); let b = inflight.insert("b").unwrap(); assert_eq!(inflight.insert("c"), Err("c")); - assert_eq!(inflight.answer(Untrusted::new(a)), Ok("a")); - assert_eq!(inflight.answer(Untrusted::new(a)), Err(Violation::Tag), "answered twice"); + assert_eq!(inflight.answer(a), Ok("a")); + assert_eq!(inflight.answer(a), Err(Violation::Tag), "answered twice"); let c = inflight.insert("c").unwrap(); assert_ne!(c, a, "the slot's next tag is not its last"); let mut held: Vec<&str> = inflight.values().copied().collect(); held.sort_unstable(); assert_eq!(held, ["b", "c"], "what is in flight, and only that"); assert_eq!(inflight.tags().collect::>(), [c, b], "the tags in flight, by slot"); - assert_eq!(inflight.answer(Untrusted::new(a)), Err(Violation::Tag), "a replay answers nothing"); - assert_eq!(inflight.answer(Untrusted::new(2)), Err(Violation::Tag), "an index past the table"); - assert_eq!(inflight.answer(Untrusted::new(b)), Ok("b")); - assert_eq!(inflight.answer(Untrusted::new(c)), Ok("c")); + assert_eq!(inflight.answer(a), Err(Violation::Tag), "a replay answers nothing"); + assert_eq!(inflight.answer(2), Err(Violation::Tag), "an index past the table"); + assert_eq!(inflight.answer(b), Ok("b")); + assert_eq!(inflight.answer(c), Ok("c")); assert!(inflight.insert("d").is_ok() && inflight.insert("e").is_ok(), "every slot is free again"); } @@ -131,25 +131,25 @@ mod tests { fn a_slot_refilled_past_sixteen_bits_refuses_its_first_tag() { let mut inflight = Inflight::<(), 64>::new(); let first = inflight.insert(()).unwrap(); - inflight.answer(Untrusted::new(first)).unwrap(); + inflight.answer(first).unwrap(); for _ in 0..u16::MAX { let tag = inflight.insert(()).unwrap(); - inflight.answer(Untrusted::new(tag)).unwrap(); + inflight.answer(tag).unwrap(); } inflight.insert(()).unwrap(); - assert_eq!(inflight.answer(Untrusted::new(first)), Err(Violation::Tag)); + assert_eq!(inflight.answer(first), Err(Violation::Tag)); } #[test] fn an_end_answers_every_tag_once_and_a_late_completion_nothing() { let mut inflight = Inflight::::new(); let tags: Vec = (0..3).map(|n| inflight.insert(n).unwrap()).collect(); - assert_eq!(inflight.answer(Untrusted::new(tags[1])), Ok(1)); + assert_eq!(inflight.answer(tags[1]), Ok(1)); let mut ended = Vec::new(); inflight.end(|tag, value| ended.push((tag, value))); assert_eq!(ended, [(tags[0], 0), (tags[2], 2)]); for tag in tags { - assert_eq!(inflight.answer(Untrusted::new(tag)), Err(Violation::Tag)); + assert_eq!(inflight.answer(tag), Err(Violation::Tag)); } let mut again = 0; inflight.end(|_, _| again += 1); diff --git a/sdk/transport/src/lib.rs b/sdk/transport/src/lib.rs index eb3f6779c43..7ba61e6677e 100644 --- a/sdk/transport/src/lib.rs +++ b/sdk/transport/src/lib.rs @@ -12,7 +12,7 @@ //! [`Span`], once: no reference to the peer's bytes is formed. //! //! **What the peer writes is untrusted until decoded.** An entry comes out as -//! [`Untrusted`] words; a peer's cursor is bounded against the ring before it +//! the peer's words, for the protocol's decoder to bound; a peer's cursor is bounded against the ring before it //! is believed, and the variant of [`Violation`] names what it claimed; a run //! is bounded against the [`Geometry`], and a tag against the [`Inflight`] //! table. An end keeps its own cursor locally and only stores the shared one, @@ -38,7 +38,6 @@ use core::sync::atomic::{AtomicU32, Ordering}; pub use arena::{Geometry, Run, Span}; pub use inflight::Inflight; pub use queue::{Consumer, Place, Producer}; -pub use toyos_untrusted::Untrusted; /// One shared 32-bit word of a region: an atomic over the mapping, or a /// model's. @@ -75,9 +74,9 @@ fn word(page: &[W; N], at: usize) -> &W { } /// A distance the peer's cursor claims, believed only up to `cap`. -fn clamp(claimed: Untrusted, cap: u32, broken: Violation) -> Result { +fn clamp(claimed: u32, cap: u32, broken: Violation) -> Result { let cap = if cfg!(feature = "no-clamp") { u32::MAX } else { cap }; - claimed.at_most(u64::from(cap)).ok().and_then(|n| u32::try_from(n).ok()).ok_or(broken) + if claimed > cap { Err(broken) } else { Ok(claimed) } } /// The peer wrote what no peer of the protocol writes. The session is over; diff --git a/sdk/transport/src/queue.rs b/sdk/transport/src/queue.rs index 865c66ac991..a9b3afed68a 100644 --- a/sdk/transport/src/queue.rs +++ b/sdk/transport/src/queue.rs @@ -8,7 +8,7 @@ use core::sync::atomic::Ordering; -use crate::{clamp, word, Untrusted, Violation, Word, PUBLISH}; +use crate::{clamp, word, Violation, Word, PUBLISH}; /// Where a queue of `E`-word entries, `D` deep, is among `N` words: the word /// its consumer stores its head in, the word its producer stores its tail in, @@ -66,13 +66,13 @@ impl Place { /// How far past `released` the producer's tail is: at most `D`. fn published(&self, page: &[W; N], released: u32) -> Result { let tail = word(page, self.tail).load(Ordering::Acquire); - clamp(Untrusted::new(tail).map(|t| t.wrapping_sub(released)), D, Violation::TailPastDepth) + clamp(tail.wrapping_sub(released), D, Violation::TailPastDepth) } /// How far behind `published` the consumer's head is: at most `D`. fn unreleased(&self, page: &[W; N], published: u32) -> Result { let head = word(page, self.head).load(Ordering::Acquire); - clamp(Untrusted::new(head).map(|h| published.wrapping_sub(h)), D, Violation::HeadPastTail) + clamp(published.wrapping_sub(head), D, Violation::HeadPastTail) } } @@ -145,8 +145,9 @@ impl Consumer { Self { place, local: 0, released: 0, ready: 0 } } - /// The next published entry, or `None` for none. - pub fn pop(&mut self, page: &[W; N]) -> Result; E]>, Violation> { + /// The next published entry, or `None` for none. Its words are the peer's, + /// for the protocol's decoder to bound before any is believed. + pub fn pop(&mut self, page: &[W; N]) -> Result, Violation> { if self.ready == 0 { let published = self.place.published(page, self.released)?; self.ready = published.saturating_sub(self.local.wrapping_sub(self.released)); @@ -154,9 +155,9 @@ impl Consumer { return Ok(None); } } - let mut words = [Untrusted::new(0); E]; + let mut words = [0; E]; for (out, shared) in words.iter_mut().zip(self.place.entry(page, self.local)) { - *out = Untrusted::new(shared.load(Ordering::Relaxed)); + *out = shared.load(Ordering::Relaxed); } self.local = self.local.wrapping_add(1); self.ready = self.ready.wrapping_sub(1); @@ -189,20 +190,16 @@ mod tests { (Producer::new(page, PLACE), Consumer::new(page, PLACE)) } - fn plain(words: Option<[Untrusted; 2]>) -> Option<[u32; 2]> { - words.map(|w| w.map(|w| w.at_most(u32::MAX.into()).unwrap() as u32)) - } - #[test] fn an_entry_is_nobodys_before_it_is_published() { let page = page(); let (mut tx, mut rx) = ends(&page); assert_eq!(tx.push(&page, [3, 4]), Ok(true)); - assert_eq!(rx.pop(&page).map(plain), Ok(None)); + assert_eq!(rx.pop(&page), Ok(None)); assert!(tx.publish(&page)); assert!(!tx.publish(&page), "nothing new, nothing published"); - assert_eq!(rx.pop(&page).map(plain), Ok(Some([3, 4]))); - assert_eq!(rx.pop(&page).map(plain), Ok(None)); + assert_eq!(rx.pop(&page), Ok(Some([3, 4]))); + assert_eq!(rx.pop(&page), Ok(None)); } /// The ring wraps many times over, and space is exactly what the consumer @@ -223,7 +220,7 @@ mod tests { } tx.publish(&page); for _ in 0..1 + round % 7 { - let Some(words) = plain(rx.pop(&page).unwrap()) else { break }; + let Some(words) = rx.pop(&page).unwrap() else { break }; assert_eq!(words, [popped, !popped], "entries come out whole, in the order they went in"); popped += 1; } diff --git a/sdk/transport/src/stream.rs b/sdk/transport/src/stream.rs index 6ea71516689..193a2b09ac0 100644 --- a/sdk/transport/src/stream.rs +++ b/sdk/transport/src/stream.rs @@ -29,7 +29,7 @@ use core::sync::atomic::Ordering; -use crate::{clamp, word, Span, Untrusted, Violation, Word, PUBLISH}; +use crate::{clamp, word, Span, Violation, Word, PUBLISH}; /// A sleeper's exchange: what the waker published before clearing the want is /// seen by the look that follows. @@ -113,8 +113,8 @@ impl Place { /// How far the peer's `claimed` position is from this end's, by `apart`: /// at most the ring. A word past the lap names no position, so it is /// farther than any ring. - fn reach(claimed: Untrusted, apart: impl FnOnce(u32) -> u32, broken: Violation) -> Result { - clamp(claimed.map(|at| if at < Self::LAP { apart(at) } else { u32::MAX }), C, broken) + fn reach(claimed: u32, apart: impl FnOnce(u32) -> u32, broken: Violation) -> Result { + clamp(if claimed < Self::LAP { apart(claimed) } else { u32::MAX }, C, broken) } /// The region's bytes `len` on from position `at`, in ring order: the @@ -148,7 +148,7 @@ impl Producer { /// How many bytes past the tail may be written, the consumer's head read /// again. pub fn room(&mut self, page: &[W; N]) -> Result { - let head = Untrusted::new(word(page, self.place.head).load(TAKE_BACK)); + let head = word(page, self.place.head).load(TAKE_BACK); let published = self.published; let unreleased = Place::::reach(head, |head| Place::::distance(head, published), Violation::HeadPastTail)?; @@ -198,7 +198,7 @@ impl Producer { /// `ended`. With neither, nothing is exchanged and the want waits for the /// next publish. pub fn ring_back(&self, page: &[W; N], ended: bool) -> Result { - let head = Untrusted::new(word(page, self.place.head).load(TAKE_BACK)); + let head = word(page, self.place.head).load(TAKE_BACK); let published = self.published; let held = Place::::reach(head, |head| Place::::distance(head, published), Violation::HeadPastTail)?; @@ -227,7 +227,7 @@ impl Consumer { /// How many bytes past the head are published, the producer's tail read /// again. pub fn ready(&mut self, page: &[W; N]) -> Result { - let tail = Untrusted::new(word(page, self.place.tail).load(Ordering::Acquire)); + let tail = word(page, self.place.tail).load(Ordering::Acquire); let released = self.released; let published = Place::::reach(tail, |tail| Place::::distance(released, tail), Violation::TailPastDepth)?; @@ -276,7 +276,7 @@ impl Consumer { /// or `fatal`. With neither, nothing is exchanged and the want waits for /// the next release. pub fn ring_back(&self, page: &[W; N], fatal: bool) -> Result { - let tail = Untrusted::new(word(page, self.place.tail).load(Ordering::Acquire)); + let tail = word(page, self.place.tail).load(Ordering::Acquire); let released = self.released; let unreleased = Place::::reach(tail, |tail| Place::::distance(released, tail), Violation::TailPastDepth)?; diff --git a/sdk/transport/tests/loom.rs b/sdk/transport/tests/loom.rs index 4fca91b59e7..b898f11f0ba 100644 --- a/sdk/transport/tests/loom.rs +++ b/sdk/transport/tests/loom.rs @@ -19,7 +19,7 @@ use std::rc::Rc; use loom::cell::Cell; use loom::sync::atomic::AtomicU32; use loom::sync::{Arc, Notify}; -use toyos_transport::{stream, Consumer, Place, Producer, Untrusted, Violation, Word}; +use toyos_transport::{stream, Consumer, Place, Producer, Violation, Word}; /// A loom atomic as a region word: the trait is this crate's and the type is /// loom's, so the two meet through a wrapper. @@ -54,10 +54,6 @@ fn entry(n: u32) -> [u32; E] { [100 + n, 7 * n + 3] } -fn plain(words: [Untrusted; E]) -> [u32; E] { - words.map(|w| w.at_most(u32::MAX.into()).unwrap() as u32) -} - /// Two entries published one at a time, read by the other end as they /// arrive: each is whole, in order, and exactly what was written. #[test] @@ -70,7 +66,7 @@ fn a_published_entry_is_read_whole() { let mut read = Vec::new(); while read.len() < 2 { match rx.pop(&consumer_page).expect("the producer keeps the protocol") { - Some(words) => read.push(plain(words)), + Some(words) => read.push(words), None => loom::thread::yield_now(), } } diff --git a/tests/guest/rust-tests/Cargo.lock b/tests/guest/rust-tests/Cargo.lock index 3602c06f71e..53fd760422a 100644 --- a/tests/guest/rust-tests/Cargo.lock +++ b/tests/guest/rust-tests/Cargo.lock @@ -997,20 +997,12 @@ dependencies = [ [[package]] name = "toyos-transport" version = "0.1.0" -dependencies = [ - "toyos-untrusted", -] - -[[package]] -name = "toyos-untrusted" -version = "0.1.0" [[package]] name = "toyos-virtio" version = "0.1.0" dependencies = [ "toyos-device-memory", - "toyos-untrusted", ] [[package]] diff --git a/tests/guest/rust-tests/src/bin/abuse_thread_name.rs b/tests/guest/rust-tests/src/bin/abuse_thread_name.rs index 3059d3ec74f..c8e98f4e358 100644 --- a/tests/guest/rust-tests/src/bin/abuse_thread_name.rs +++ b/tests/guest/rust-tests/src/bin/abuse_thread_name.rs @@ -1,8 +1,5 @@ -//! `SYS_SET_THREAD_NAME` used to clamp an oversized name with `(a2 as -//! usize).min(THREAD_NAME_LEN)` and set the truncated prefix — a silent -//! clamp. `Untrusted::at_most` replaced it with a refusal, which is a -//! behaviour change worth its own gate: this proves the refusal actually -//! fires, rather than the clamp it replaced. +//! `SYS_SET_THREAD_NAME` refuses a name longer than `THREAD_NAME_LEN` rather +//! than setting its truncated prefix: this proves the refusal fires. //! //! `toyos_abi::syscall::set_thread_name` throws its own return value away — //! it always has, and that is not this test's to fix — so the return code @@ -80,11 +77,8 @@ fn main() { ); println!(" at the bound: accepted, and reads back whole"); - // One byte over: before this conversion this call silently set the first - // THREAD_NAME_LEN bytes of `over` — indistinguishable, from calling code - // that does not check the (discarded) return value, from asking for - // `at_bound` again. `Untrusted::at_most` refuses it instead, and this is - // the only way to see that from userland: the name is still `at_bound`. + // One byte over is refused, and the only way to see that from userland is + // that the name is still `at_bound`, not a truncated `over`. let mut over = [b'b'; THREAD_NAME_LEN + 1]; over[THREAD_NAME_LEN] = b'!'; syscall::set_thread_name(&over);