diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ce7ce37..2cd70e3 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -118,7 +118,7 @@ flowchart BT | 2 | `page_io.rs` | Raw `pread`/`pwrite` of fixed-**stride** pages, exclusive `flock`, `fsync`, in-memory `Vec` backing. Tracks cumulative successful `fsync_calls` via `Cell`. Stride = `PAGE_SIZE` (plaintext) or `ENC_PAGE_SIZE` (encrypted), set once via `set_stride`. | The **only** module that touches the filesystem; crypto-agnostic (moves `stride`-byte blobs at `page_id * stride`). | | 2 | `lru.rs` | O(1) intrusive doubly-linked LRU index over `u64` page ids (`FxHashMap`-backed, I77). | Replaces the O(n) `VecDeque::retain` LRU; consumed only by `page_cache`. | | 3 | `page_cache.rs` | LRU cache over `PageIo`, dirty tracking, checksum validation on load, `PageCipher` seal/open at the I/O boundary, spillway overflow, `CacheFull`/`SpillwayFull` errors. Owns three `Cell` engine-activity counters (cache hits/misses, pages allocated) and aggregates them with `PageIo::fsync_count` into `counters()`. | Soft eviction at `max_pages`; dirty overflow spills to a sidecar (cap = `spillway_max_bytes`); `CacheFull` at strict `max_pages` when spillway disabled (`spillway_max_bytes=0`); checksums verified on disk LOAD only. | -| 3 | `spillway.rs` | Sidecar `.spillway` file for dirty pages the LRU is forced to spill. Per-slot XXH3 over `page_id ‖ payload`; crypto-agnostic (plaintext 8192-byte page or sealed 8232-byte blob). | Never `fsync`ed; truncated at open/commit/rollback — its content is always discardable uncommitted state. | +| 3 | `spillway.rs` | Sidecar `.spillway` file for dirty pages the LRU is forced to spill. Per-slot XXH3 over `page_id ‖ payload`; crypto-agnostic (plaintext 8192-byte page or sealed 8232-byte blob). | Never `fsync`ed; truncated at commit/rollback and re-created at open — its content is always discardable uncommitted state. Created `O_EXCL | O_NOFOLLOW` mode 0600; a pre-existing entry is unlinked only if it is a plain file this uid owns (see "File permissions" below). | | 4 | `freemap.rs` | Single-page bitmap primitive: `allocate_first` / `mark_free` on one `[u8; PAGE_SIZE]` buffer. | Pure buffer manipulation; no cache or I/O. Composed into the multi-page tree by `freemap_tree.rs`. | | 4 | `freemap_tree.rs` | COW radix tree of FreeMap leaves; the full multi-page freemap. | All structural COW pages sourced out-of-band (never from the bitmap); session-COW dedup (one COW per node per commit). | | 4 | `data_page.rs` | Slotted page layout (R1): slot directory grows forward, packed value data grows backward. The directory is append-only — nothing is ever reclaimed within a page. | Slot indices are immutable for the page's lifetime; the handle table stores `(page_id, slot_index)` and relies on it. There is no intra-page compaction: reclamation is whole-page. | @@ -589,6 +589,14 @@ Spillway slots carry their own per-slot XXH3 checksum over `page_id ‖ page_byt The no-spill commit cost is **3 fsyncs**: pre-drain flush (I28) + main-pages flush + superblock. The pre-drain handles a subtle interaction in the commit protocol (see [Commit protocol](#commit-protocol) step 1). +### File permissions and the sidecar path + +Both files Chisel creates are created mode **0600**. This is a contract, not an implementation detail: a caller may rely on a database it creates not being readable by other users on the host. The mode applies only at creation, so reopening an existing database never alters permissions its owner deliberately set — with one exception: a *zero-length* file adopted by the create path is tightened to 0600, because an empty file is not yet a database and a permissive empty file at the database path is more likely a squatter than a deliberate choice. + +The sidecar needs more than a mode, because its path is fully derived from the database path and is therefore predictable, and because it is created lazily *mid-transaction* under cache pressure rather than at open. A local user who can create entries in the database's directory could otherwise plant `.spillway` and either (a) point it at a file the database owner can write, which the old `truncate(true)` open would have zeroed, or (b) leave a world-readable regular file that Chisel would adopt — writing uncommitted user values somewhere the planter can read, since `mode` does not apply to a file the open did not create. + +So the sidecar is created with `O_EXCL | O_NOFOLLOW`. A pre-existing entry is unlinked and retried **only** when it is a plain file, owned by this uid, with exactly one link — the crash-debris case the lifecycle was written for. A symlink, a foreign-owned file, or an extra hard link is treated as tampering and surfaces as a fatal `IoError`, which poisons the handle. Encryption does not substitute for either guard: the hazards are the truncate and the file's ownership, not the confidentiality of the bytes. + ### Slot packing and overflow Values up to `MAX_INLINE_VALUE` (~`PAGE_BODY_SIZE`) are stored inline in a data-page slot. Larger values get an overflow chain, and that path allocates **no data page and no slot at all**: the `HandleEntry` itself carries `HandleFlags::Overflow`, with `page_id` pointing directly at the first chain page and `slot_index = 0` (an unused placeholder, not a real slot). `HandleFlags` is a field of the handle-table entry, not of a data-page slot-directory entry — the slot directory has no flag but `SLOT_FLAG_LIVE`. diff --git a/README.md b/README.md index 0002f92..a1b0546 100644 --- a/README.md +++ b/README.md @@ -372,7 +372,7 @@ let options = Options::default() **Fatal errors** — storage integrity is in question. Drop the handle and reopen. -`IoError`, `ChecksumMismatch`, `CorruptSuperblock`, `FileSizeMismatch`, `LockFailed`, `UnsupportedFormatVersion`, `UnsupportedPageSize`, `CorruptPage`, `InvalidPageId`, `DecryptionFailed`. +`IoError`, `ChecksumMismatch`, `CorruptSuperblock`, `FileSizeMismatch`, `LockFailed`, `UnsupportedFormatVersion`, `UnsupportedPageSize`, `InvalidFreemapDepth`, `CorruptPage`, `InvalidPageId`, `DecryptionFailed`. `DecryptionFailed { page_id }` is fatal: an AEAD authentication failure while decrypting an already-read page means the ciphertext or session key can no longer be trusted, so it poisons the handle exactly like `ChecksumMismatch` (see the poison model below). It is distinct from the operational `InvalidEncryptionKey`, which fires at open time when the supplied key unwraps no key slot — before any data page is served. diff --git a/bench/src/chisel_engine.rs b/bench/src/chisel_engine.rs index 96f6f1e..2d379c9 100644 --- a/bench/src/chisel_engine.rs +++ b/bench/src/chisel_engine.rs @@ -89,15 +89,21 @@ impl Engine for ChiselEngine { } fn delete_many(&mut self, ids: &[Identifier]) -> EngineResult<()> { - // SAFETY: Identifier and chisel::Handle are both - // #[repr(transparent)] over u64, so a slice of Identifier and a - // slice of Handle have identical layout. The borrow ends with - // this call; no aliasing concern; no 'static lifetime escapes. - // Saves the per-call Vec allocation that the safe-collect form - // would require (audit F5). - let handles: &[chisel::Handle] = - unsafe { std::slice::from_raw_parts(ids.as_ptr() as *const chisel::Handle, ids.len()) }; - Ok(self.db.delete_many(handles)?) + // This used to reinterpret `&[Identifier]` as `&[chisel::Handle]` + // through `slice::from_raw_parts`, on the strength of both types being + // `#[repr(transparent)]` over u64. The layout guarantee is real, but + // nothing enforced it across the crate boundary: chisel's const + // assertions check only size and align, which a `#[repr(Rust)] + // struct Handle(u64)` also satisfies. Deleting `#[repr(transparent)]` + // would have left both crates building green while this line became UB. + // + // The transmute bought one Vec allocation per `delete_many` call, + // against an operation that performs three fsyncs. That is far below the + // floor this harness can measure, so the safe form costs nothing real + // and removes an unsafe block that no compiler check was guarding. + let handles: Vec = + ids.iter().map(|id| chisel::Handle::from(id.0)).collect(); + Ok(self.db.delete_many(&handles)?) } fn file_size_bytes(&self) -> EngineResult { diff --git a/src/error.rs b/src/error.rs index 2151417..bde271b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -159,6 +159,23 @@ pub enum ChiselError { stored: u32, compiled: u32, }, + // Raised at open time when the superblock's `freemap_depth` exceeds the + // depth the freemap radix can represent. Depth 5 already spans every u64 + // page id, so a larger value cannot have been written by any correct + // binary — it is a corrupt or forged field. + // + // Deliberately NOT `CorruptSuperblock`: that variant means "no readable + // superblock at all" and is documented as recoverable by reopening, + // because slot selection may pick a different slot next time. Here the + // superblock parsed and validated fine apart from this one field, and + // every sibling slot carries the same rejected depth — so a reopen + // returns the same error forever. It carries the offending value for the + // same reason `UnsupportedPageSize` does: a bare "corrupt" tells an + // operator nothing about which field to look at. + InvalidFreemapDepth { + stored: u32, + max: u32, + }, // Operational — the caller supplied the wrong key material or none, or // asked an unencrypted-only build to open an encrypted DB. The on-disk @@ -223,6 +240,7 @@ impl ChiselError { | ChiselError::CorruptPage { .. } | ChiselError::InvalidPageId { .. } | ChiselError::UnsupportedPageSize { .. } + | ChiselError::InvalidFreemapDepth { .. } | ChiselError::DecryptionFailed { .. } ) } @@ -313,6 +331,10 @@ impl fmt::Display for ChiselError { f, "page size mismatch: file was written with {stored}-byte pages, this build uses {compiled}-byte pages" ), + ChiselError::InvalidFreemapDepth { stored, max } => write!( + f, + "superblock declares freemap depth {stored}, which exceeds the maximum {max} (corrupt or forged superblock)" + ), ChiselError::NoEncryptionKey => write!( f, "database is encrypted but no encryption_key was supplied" @@ -571,6 +593,7 @@ mod tests { | ChiselError::CorruptPage { .. } | ChiselError::InvalidPageId { .. } | ChiselError::UnsupportedPageSize { .. } + | ChiselError::InvalidFreemapDepth { .. } | ChiselError::DecryptionFailed { .. } => true, } } @@ -616,6 +639,7 @@ mod tests { stored: 0, compiled: 0, }, + ChiselError::InvalidFreemapDepth { stored: 0, max: 0 }, ChiselError::NoEncryptionKey, ChiselError::InvalidEncryptionKey, ChiselError::EncryptionNotSupported, @@ -630,10 +654,13 @@ mod tests { "is_fatal() disagrees with the documented Fatal/Operational block for {e:?}" ); } - // Tripwire: exactly 10 variants are fatal today. If this count moves, the + // Tripwire: exactly 11 variants are fatal today. If this count moves, the // Fatal/Operational split changed — confirm that was intentional (it is a // breaking change for callers doing error-class matching, per the header). - assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 10); + // Last moved 10 -> 11 by InvalidFreemapDepth, which is fatal because a + // superblock field outside its representable range cannot be resolved by + // retrying: every sibling slot carries the same value. + assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 11); } // Phase 4: the three operational encryption errors are recoverable (the diff --git a/src/freemap_tree.rs b/src/freemap_tree.rs index 9d0ea02..673bf22 100644 --- a/src/freemap_tree.rs +++ b/src/freemap_tree.rs @@ -58,9 +58,14 @@ const PTRS_PER_INTERIOR: usize = (CHECKSUM_OFFSET - DATA_PAGE_HEADER_SIZE) / PTR // The leaf fans out to 2^16 and each level multiplies by ~2^10, so depth 5 // already covers 2^16 * 2^50 = 2^66 > u64::MAX page ids — the bound is 5 // (distinct from the membership tree's 6, which has no 2^16 leaf factor). A -// spine or stored depth claiming deeper is corrupt; capacity() saturates so a -// bad on-disk depth fails closed (rejects the descent) rather than overflowing. -const MAX_DEPTH: u32 = 5; +// spine or stored depth claiming deeper is corrupt. capacity() saturates, but +// saturation only prevents arithmetic overflow — on its own it rejects nothing +// and does NOT make a bad on-disk depth fail closed. The actual rejection is +// the `freemap_depth > MAX_DEPTH` gate in `open_existing`, which is what keeps +// `scan_node`'s per-level recursion and `cow_descend`'s O(depth^2) loop from +// being driven by a hostile superblock. `pub(crate)` so that gate can name the +// constant instead of repeating the literal. +pub(crate) const MAX_DEPTH: u32 = 5; fn read_child(buf: &[u8; PAGE_SIZE], index: usize) -> u64 { let off = DATA_PAGE_HEADER_SIZE + index * PTR_SIZE; @@ -189,11 +194,37 @@ impl FreeMapTree { span } + // Reject an out-of-range `depth` before any traversal begins. `open_existing` + // already gates `freemap_depth > MAX_DEPTH` at the trust boundary, so this is + // defense in depth — it matches what HandleTable::recover_depth and + // RadixU64/MembershipIndex already do at every entry point, and it means a + // future path that builds a tree from roots WITHOUT passing through the open + // gate (a savepoint rewind, a rollback to a restored root) cannot resurrect + // the hazard. + // + // It must run BEFORE the first `cache.get`, not merely before the recursion: + // the traversals happen to fail type validation first for some root shapes, + // which is luck, not a guarantee. A FreeMapInterior root with a + // self-referential child pointer passes every type check and is precisely the + // shape that drives scan_node into a stack overflow and cow_descend into its + // O(depth^2) page-materializing loop. + // + // Note the saturating arithmetic above does NOT stand in for this: saturation + // keeps `capacity()`/`child_span()` from overflowing, but a saturated span + // simply yields child index 0 — it rejects nothing. + fn check_depth(&self) -> Result<()> { + if self.depth > MAX_DEPTH { + return Err(ChiselError::CorruptPage { page_id: self.root }); + } + Ok(()) + } + /// Descend (read-only) to the leaf covering `id`. Returns the leaf page id, /// or `None` if `id` is past the tree's reach OR any subtree on the path is /// absent (a zero child pointer = "that whole range is all in use"). Every /// page read is type-validated. fn find_leaf(&self, cache: &mut PageCache, id: u64) -> Result> { + self.check_depth()?; // Reject ids past the tree's reach. When capacity saturated to u64::MAX // (depth where the real capacity exceeds u64), every id is in reach, so // skip the guard — otherwise id == u64::MAX would be wrongly rejected. @@ -285,6 +316,7 @@ impl FreeMapTree { extend: &mut dyn FnMut(&mut PageCache) -> Result, leaf_op: &mut dyn FnMut(&mut [u8; PAGE_SIZE], u64), ) -> Result<()> { + self.check_depth()?; // COW the root first; the new root replaces self.root and the old one is // superseded. (grow has already ensured depth covers id, if needed.) // The root's position type follows depth exactly as the read paths see it: @@ -448,6 +480,13 @@ impl FreeMapTree { id: u64, extend: &mut dyn FnMut(&mut PageCache) -> Result, ) -> Result<()> { + // Depth-check FIRST. The `while` condition below evaluates `capacity()` + // before it evaluates `self.depth < MAX_DEPTH`, and `capacity()` loops + // `depth` times — so a forged depth of u32::MAX spins ~4.3e9 saturating + // multiplies (tens of seconds) before the guard inside `mark_free` ever + // runs. It does eventually fail closed, but "eventually" is not what the + // other entry points promise, and this is the manager-facing one. + self.check_depth()?; // Grow until capacity covers id. MAX_DEPTH covers all of u64, so the loop // terminates; grow() no-ops at MAX_DEPTH and capacity() saturates to // u64::MAX, breaking the loop. @@ -484,6 +523,7 @@ impl FreeMapTree { /// subtrees. Returns the global id (not leaf-local). Every page is type- /// validated on the way down. fn scan_from(&self, cache: &mut PageCache, lo: u64) -> Result> { + self.check_depth()?; self.scan_node(cache, self.root, self.depth, 0, lo) } @@ -547,6 +587,7 @@ impl FreeMapTree { &self, cache: &mut PageCache, ) -> Result> { + self.check_depth()?; let mut set = rustc_hash::FxHashSet::default(); if self.root != crate::page::PAGE_ID_NONE { self.collect_reachable(cache, self.root, self.depth, &mut set)?; @@ -882,4 +923,70 @@ mod tests { } } } + + // Defense in depth for the SECURITY-SWEEP-3 hazard. `open_existing` rejects + // an over-deep `freemap_depth` at the trust boundary; this pins the tree's + // own entry-point guards, so a path that builds a tree from roots without + // passing through that gate cannot reach the recursion. + // + // The root here is a page id that was never allocated, which is what makes + // the test meaningful: the ONLY way to get a typed error rather than a + // read failure is for check_depth to run before the first cache.get. That + // also keeps the test terminating — an unguarded traversal at this depth is + // a stack overflow, not a failure the harness could report. + #[test] + fn an_over_deep_depth_is_rejected_before_any_page_is_read() { + let mut c = make_cache(256); + let phantom_root = 4_242; + let bad_depth = MAX_DEPTH + 1; + + let assert_rejected = |label: &str, r: Result<()>| { + match r { + Err(ChiselError::CorruptPage { page_id }) => assert_eq!( + page_id, phantom_root, + "{label}: the guard must blame the tree root it refused to descend" + ), + other => panic!("{label}: expected CorruptPage, got {other:?}"), + }; + }; + + // Read paths. + let t = FreeMapTree::from_roots(phantom_root, bad_depth); + assert_rejected("is_free", t.is_free(&mut c, 7).map(|_| ())); + assert_rejected("reachable_pages", t.reachable_pages(&mut c).map(|_| ())); + + // Allocation path (scan_from), and the commit-path writer (cow_descend). + let mut t = FreeMapTree::from_roots(phantom_root, bad_depth); + let mut hint = 0u64; + assert_rejected( + "allocate_first", + t.allocate_first(&mut c, &mut hint, &mut extend).map(|_| ()), + ); + assert_rejected("mark_free", t.mark_free(&mut c, 7, &mut extend)); + // The manager-facing entry point, and the one the first version of this + // guard missed. Its `while` condition evaluates `capacity()` — which + // loops `depth` times — BEFORE it evaluates `depth < MAX_DEPTH`, so at + // u32::MAX it spun ~4.3e9 saturating multiplies before the guard inside + // mark_free fired. It reached the same verdict, but only after tens of + // seconds; a bounded-but-absurd delay is not what the other entry points + // promise. `u32::MAX` rather than `MAX_DEPTH + 1` is deliberate: at the + // small over-bound the missing check is invisible. + let mut t = FreeMapTree::from_roots(phantom_root, u32::MAX); + assert_rejected( + "mark_free_growing", + t.mark_free_growing(&mut c, 7, &mut extend), + ); + + // A depth AT the bound is legal and must still be accepted — the guard + // is `>`, not `>=`. It fails on the phantom page instead, which proves + // it got past check_depth to the read. + let t = FreeMapTree::from_roots(phantom_root, MAX_DEPTH); + assert!( + !matches!( + t.is_free(&mut c, 7), + Err(ChiselError::CorruptPage { page_id }) if page_id == phantom_root + ), + "MAX_DEPTH itself must not be rejected by the depth guard" + ); + } } diff --git a/src/handle.rs b/src/handle.rs index 513672b..a83300f 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -19,26 +19,35 @@ use std::num::NonZeroU32; /// A stable, opaque chunk handle. Newtype over the engine's `u64` id. /// -/// `#[repr(transparent)]` is load-bearing, not decoration: the bench adapter -/// reinterprets a `&[u64]` slice as `&[Handle]` without copying, which is sound -/// only because `Handle` has identical layout to `u64`. +/// `#[repr(transparent)]` is kept deliberately so the type stays layout- +/// identical to `u64`, which is what lets a future FFI or zero-copy adapter +/// depend on it. Nothing does today — see the tripwire below for exactly what +/// the const assertions can and cannot enforce. #[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct Handle(u64); -// Pin the layout the bench adapter's `&[u64]` -> `&[Handle]` transmute depends -// on (bench/src/chisel_engine.rs). With this, dropping `#[repr(transparent)]` or -// adding a field fails THIS crate's build with a clear message, instead of -// turning the cross-crate transmute into silent UB the bench can't detect -// (review 2026-06-22). const assertions are evaluated at compile time. +// Layout tripwire for `#[repr(transparent)]` above. +// +// Be precise about what these catch, because the previous comment was not: +// they fire if a field is ADDED (size changes) or alignment shifts. They do +// NOT catch removal of `#[repr(transparent)]` itself — a `#[repr(Rust)] +// struct Handle(u64)` has the same size and align, so both assertions still +// pass. Rust offers no stable way to assert `repr(transparent)`, so treat the +// attribute as the contract and these as a partial backstop. +// +// The bench adapter's `&[Identifier]` -> `&[Handle]` transmute, which these +// were originally written to guard, has been replaced with a safe collect +// (bench/src/chisel_engine.rs) precisely because that gap was unclosable — so +// no unsafe code currently rests on them. const _: () = { assert!( core::mem::size_of::() == core::mem::size_of::(), - "Handle must stay layout-identical to u64 (bench transmute relies on repr(transparent))" + "Handle must stay layout-identical to u64 (repr(transparent) is the contract)" ); assert!( core::mem::align_of::() == core::mem::align_of::(), - "Handle must stay layout-identical to u64 (bench transmute relies on repr(transparent))" + "Handle must stay layout-identical to u64 (repr(transparent) is the contract)" ); }; diff --git a/src/overflow.rs b/src/overflow.rs index 3c372ca..b2a50c9 100644 --- a/src/overflow.rs +++ b/src/overflow.rs @@ -174,12 +174,42 @@ impl Overflow { page_id: first_page, }); } + // `total_length` is disk-controlled and, at this point, unauthenticated: + // the page checksum is XXH3, which is non-cryptographic and publicly + // recomputable, so an attacker with byte-level control over the file + // chooses this value freely. The type and zero-length guards above do + // NOT constrain it — they only reject a page that isn't Overflow-typed + // at all, which is exactly the case a crafted file avoids. + // + // Bound it against what the file could physically hold before it is + // used for anything. This single check covers both hazards: + // + // * the `Vec::with_capacity` below, which on a forged u64::MAX either + // panics with "capacity overflow" or reaches `handle_alloc_error` + // and ABORTS the process — bypassing the poison model entirely, from + // a plain `Chisel::read` on an untrusted file; and + // * `max_pages`, derived from the same value, which is the loop's only + // termination bound. A self-referential next_page link under a + // forged length would otherwise spin ~2^51 cache-hit iterations. + // + // The ceiling is the allocator's high-water mark, NOT the file length. + // A chain cannot span more pages than have ever been allocated, and + // `next_page_id` counts pages allocated in the current transaction + // too — pages that live in the cache and have not extended the file + // yet. Using `file_page_count` here instead would reject a perfectly + // valid read-your-own-writes of a large value inside the transaction + // that wrote it. + // + // Generous (it ignores the superblock, handle-table and data pages + // that must also be in there) but bounded by something an attacker + // cannot inflate without actually supplying the bytes. + let ceiling = (cache.next_page_id() as usize).saturating_mul(OVERFLOW_PAYLOAD); + if total_length > ceiling { + return Err(ChiselError::CorruptPage { + page_id: first_page, + }); + } let max_pages = total_length.div_ceil(OVERFLOW_PAYLOAD); - // Ordering matters: the wrong-type and zero-length guards above run - // BEFORE this allocation, so an untrusted `total_length` (e.g. a - // stale handle pointing at a non-overflow page whose bytes 16..24 - // read as u64::MAX) can never drive a speculative giant allocation - // here. The per-page loop guard alone would be too late. let mut result = Vec::with_capacity(total_length); let mut current_page = first_page; @@ -263,6 +293,23 @@ impl Overflow { page_id: first_page, }); } + // The SAME disk-controlled `total_length` that `read` bounds, reached + // from `Chisel::delete` and `Chisel::update` instead of `Chisel::read`. + // It needs the identical ceiling and for the identical reason: it is the + // sole source of `max_pages`, which is this loop's only termination + // bound. A forged u64::MAX yields max_pages ~2.26e15, so a chain whose + // `next_page` points at itself pushes into `freed` until the allocator + // aborts — the process-killing outcome the poison model cannot intercept. + // + // See `read` for why the ceiling is `next_page_id` (the allocator + // high-water mark) rather than the file length: it must not reject a + // read-your-own-writes of a large value inside the writing transaction. + let ceiling = (cache.next_page_id() as usize).saturating_mul(OVERFLOW_PAYLOAD); + if total_length > ceiling { + return Err(ChiselError::CorruptPage { + page_id: first_page, + }); + } let max_pages = total_length.div_ceil(OVERFLOW_PAYLOAD); let mut freed = Vec::new(); diff --git a/src/page_io.rs b/src/page_io.rs index 618f6c9..c207c11 100644 --- a/src/page_io.rs +++ b/src/page_io.rs @@ -125,6 +125,14 @@ impl PageIo { /// read-write path, so read-only opens of a missing file correctly fail /// rather than materializing an empty file. /// + /// On unix, a newly created database file is created with mode `0600` + /// (owner read/write only) rather than the process umask's default. This is + /// part of the contract, not an implementation detail: a caller may rely on + /// a database it creates not being readable by other users on the host. + /// The mode applies ONLY at creation — reopening an existing file never + /// alters permissions its owner has deliberately set, so widening access is + /// the owner's call to make with `chmod`. + /// /// The exclusive flock is taken even for `read_only` opens. This is /// intentional: even a reader needs to block concurrent writers, because /// shadow paging means a writer could be mid-commit (old superblock @@ -135,12 +143,44 @@ impl PageIo { let mut file = if read_only { OpenOptions::new().read(true).open(path)? } else { - OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(path)? + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true).truncate(false); + // Create at 0600 rather than 0666 & ~umask (typically 0644). A + // storage engine's file holds whatever the caller stored in it; on + // a shared host the default made every value in a plaintext + // database world-readable, which is a surprising default for a + // crate that also ships at-rest encryption. + // + // `mode` applies ONLY when the file is created, so reopening an + // existing database never changes permissions the owner has + // deliberately set — this tightens new files without migrating old + // ones. + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); + let f = opts.open(path)?; + // `mode` above applies only when the open CREATES the file, which + // leaves one gap in the contract this function documents. Since a + // zero-length file is a legitimate create target, another local user + // can pre-plant an empty world-readable file at the database path; + // Chisel would then adopt it and write every value into a file the + // planter can read, with the promised 0600 never applied. + // + // Close it by tightening any zero-length file we adopt. A file with + // no bytes is not yet a database, so there are no permissions its + // owner has deliberately set on a database to preserve. If the file + // belongs to someone else the fchmod fails EPERM and the open fails + // — the correct outcome for "something is squatting on my path". + // + // Non-empty files are untouched, exactly as documented: reopening an + // existing database never alters its permissions. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if f.metadata()?.len() == 0 { + f.set_permissions(std::fs::Permissions::from_mode(0o600))?; + } + } + f }; Self::try_lock(&file)?; // I51: seed the page-count cache from the current file length. diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index b0a6716..e554656 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -1716,3 +1716,347 @@ fn tampered_key_slot_cost_params_do_not_reach_the_allocator() { "expected InvalidEncryptionKey (the slot is skipped as non-matching), got {err:?}" ); } + +// ── SECURITY-SWEEP hardening (2026-07-29 review) ──────────────────────── +// +// These three model the review's stated threat: the attacker controls the +// on-disk bytes and can recompute the XXH3 page checksum, which is +// non-cryptographic and publicly computable. Each previously reached a +// process-level failure — an allocator abort, a stack overflow — that the +// poison model cannot intercept, because neither is a Rust error. The +// assertions are therefore "returns a typed error" rather than any +// particular value: the property under test is that the process survives. + +#[test] +fn forged_overflow_total_length_is_corrupt_page_not_an_allocation_abort() { + // SECURITY-SWEEP-2. `Overflow::read` sized a Vec directly from the u64 at + // bytes 16..24 of an Overflow-typed page. A forged u64::MAX either panics + // with "capacity overflow" or reaches handle_alloc_error and ABORTS — + // from a plain Chisel::read on an untrusted file. + // + // The old comment claimed the preceding guards made this unreachable, but + // they only reject a page that is not Overflow-typed. A crafted file keeps + // the type tag valid, which is exactly the case that was unguarded. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + + let handle; + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + handle = db.allocate(&vec![0xAB; 20_000]).unwrap(); + db.commit().unwrap(); + } + + let overflow_page = find_page_of_type(&path, PageType::Overflow); + { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.read_exact(&mut buf).unwrap(); + // Keep the type tag intact; forge only the length, then re-stamp the + // checksum so the page passes validation exactly as an attacker's would. + buf[16..24].copy_from_slice(&u64::MAX.to_le_bytes()); + page::stamp_checksum(&mut buf); + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.write_all(&buf).unwrap(); + f.sync_all().unwrap(); + } + + let db = Chisel::open(&path, Default::default()).expect("open reads no overflow page"); + match db.read(handle) { + Err(ChiselError::CorruptPage { .. }) => {} + other => panic!("expected CorruptPage for a forged total_length, got {other:?}"), + } +} + +#[test] +fn forged_freemap_depth_is_rejected_at_open() { + // SECURITY-SWEEP-3. `freemap_depth` was copied from the superblock into + // FreeMapTree::from_roots with no bound, while the handle table and the + // membership index both cap theirs at MAX_DEPTH. A forged depth drives + // scan_node's per-level recursion into a stack overflow (SIGSEGV) and + // cow_descend into an O(depth^2) loop that extends the file per absent + // child — the latter on the ordinary commit path. + // + // Rewriting the superblock through Superblock::serialize keeps magic, + // checksum and every other field valid, so `freemap_depth` is the single + // variable under test. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + db.allocate(b"payload").unwrap(); + db.commit().unwrap(); + } + + // Rewrite every superblock slot so slot selection cannot dodge the forgery. + let sb_count = { + let mut f = fs::File::open(&path).unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.read_exact(&mut buf).unwrap(); + Superblock::deserialize(&buf) + .map(|sb| sb.superblock_count) + .unwrap_or(2) + }; + { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + for slot in 0..sb_count as u64 { + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + if f.read_exact(&mut buf).is_err() { + continue; + } + if let Some(mut sb) = Superblock::deserialize(&buf) { + sb.freemap_depth = 500_000; + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + f.write_all(&sb.serialize()).unwrap(); + } + } + f.sync_all().unwrap(); + } + + match Chisel::open(&path, Default::default()) { + // Not CorruptSuperblock: that variant means "no readable superblock at + // all" and is documented as reopen-recoverable via slot selection. This + // superblock parsed fine apart from one field, and every sibling slot + // carries the same rejected depth, so the error must name the field. + Err(ChiselError::InvalidFreemapDepth { stored, max }) => { + assert_eq!(stored, 500_000); + assert_eq!(max, crate::freemap_tree::MAX_DEPTH); + } + Ok(_) => panic!("a freemap_depth of 500_000 must not open"), + Err(other) => panic!("expected InvalidFreemapDepth, got {other:?}"), + } +} + +#[test] +#[cfg(unix)] +fn a_new_database_file_is_created_private_to_its_owner() { + // SECURITY-SWEEP-4. The file was created at 0666 & ~umask — typically + // 0644 — so on a shared host every value in a plaintext database was + // world-readable. `mode` applies only on creation, so an existing + // database's permissions are left exactly as its owner set them. + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let path = dir.path().join("perm.db"); + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + db.allocate(b"private").unwrap(); + db.commit().unwrap(); + } + + // Assert the security property, not the exact bits. The kernel applies + // `mode & ~umask`, so a developer running with umask 0277 gets 0400 here — + // still private, but not equal to 0600. Testing `mode == 0o600` would fail + // for them on a file that is if anything MORE restrictive than required. + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode & 0o077, + 0, + "a freshly created database must not be group- or world-accessible (got {mode:o})" + ); +} + +#[test] +#[cfg(unix)] +fn a_symlinked_spillway_path_is_refused_rather_than_followed() { + // SECURITY-SWEEP-4, the sharp half. The sidecar path is fully derived from + // the database path, so it is predictable; it is created lazily mid + // transaction under cache pressure; and it is opened with truncate(true) on + // the assumption that anything already there is garbage from a crashed + // prior run. That assumption fails for a symlink: a local user who can + // create entries in the database's directory could plant .spillway + // pointing at a file the owner can write, and the first spill would + // truncate it to zero. + // + // O_NOFOLLOW turns that into a failed open. The engine surfaces it as an + // error and poisons, which is the right answer to "someone is tampering + // with my sidecar path" — the point of the test is that the victim file is + // still intact afterwards. + use std::os::unix::fs::symlink; + + let dir = TempDir::new().unwrap(); + let db_path = dir.path().join("spill.db"); + let victim = dir.path().join("victim.txt"); + fs::write(&victim, b"important contents that must survive").unwrap(); + + // Plant the sidecar as a symlink to the victim before it is ever opened. + symlink(&victim, dir.path().join("spill.db.spillway")).unwrap(); + + // Tiny cache so a modest transaction is forced to spill. + let opts = Options::default() + .cache_max_bytes(16 * PAGE_SIZE as u64) + .spillway_max_bytes(64 * PAGE_SIZE as u64); + let mut db = Chisel::open(&db_path, opts).unwrap(); + db.begin().unwrap(); + let mut spill_attempted = false; + for _ in 0..400 { + if db.allocate(&vec![0x5A; PAGE_SIZE + 64]).is_err() { + spill_attempted = true; + break; + } + } + let _ = db.rollback(); + drop(db); + + assert!( + spill_attempted, + "expected the spill to be refused; if this stops firing the workload no \ + longer reaches ensure_spillway and the test has stopped testing anything" + ); + assert_eq!( + fs::read(&victim).unwrap(), + b"important contents that must survive", + "the symlink target must not have been truncated" + ); +} + +#[test] +#[cfg(unix)] +fn a_planted_regular_file_at_the_spillway_path_is_not_adopted() { + // The other half of SECURITY-SWEEP-4, missed by the first fix. O_NOFOLLOW + // closes the truncate direction (a planted SYMLINK) but not the disclosure + // direction: a planted REGULAR file is not a symlink, so the open succeeds, + // and `mode(0600)` applies only when the open creates the file. The planter + // keeps ownership and their permissive mode, and the engine then writes + // spilled pages — uncommitted user values — into a file they can read. + // + // The fix creates the sidecar exclusively and only unlinks a pre-existing + // entry that is a plain file we own. A foreign-owned plant cannot be + // distinguished from ours in a portable test (both are owned by the test + // user), so this asserts the property that IS observable and that the bug + // violated: whatever the engine ends up writing to must not carry the + // planted permissive mode. + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let db_path = dir.path().join("plant.db"); + let sidecar = dir.path().join("plant.db.spillway"); + + fs::write(&sidecar, b"planted").unwrap(); + fs::set_permissions(&sidecar, fs::Permissions::from_mode(0o666)).unwrap(); + + let opts = Options::default() + .cache_max_bytes(16 * PAGE_SIZE as u64) + .spillway_max_bytes(64 * PAGE_SIZE as u64); + let mut db = Chisel::open(&db_path, opts).unwrap(); + db.begin().unwrap(); + for _ in 0..400 { + if db.allocate(&vec![0x5A; PAGE_SIZE + 64]).is_err() { + break; + } + } + let _ = db.rollback(); + drop(db); + + let mode = fs::metadata(&sidecar).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode & 0o077, + 0, + "the spillway must not inherit a planted file's group/world bits (got {mode:o})" + ); + // And the planted bytes must be gone — the sidecar in use is a new file, + // not the planted one reused. + assert_ne!( + fs::read(&sidecar).unwrap(), + b"planted", + "the planted file must have been replaced, not adopted" + ); +} + +#[test] +#[cfg(unix)] +fn a_database_created_over_a_planted_empty_file_is_still_private() { + // `mode` on OpenOptions applies only when the open CREATES the file, and a + // zero-length file is a legitimate create target (that boundary is pinned + // by the PR #127 regression test). So a local user could pre-plant an empty + // world-readable file at the database path and Chisel would adopt it, + // writing every stored value into a file the planter can read while the + // promised 0600 was never applied. + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let path = dir.path().join("planted.db"); + fs::write(&path, b"").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o666)).unwrap(); + + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + db.allocate(b"private").unwrap(); + db.commit().unwrap(); + } + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode & 0o077, + 0, + "adopting an empty file must not leave the database group/world-accessible (got {mode:o})" + ); +} + +#[test] +fn a_forged_overflow_length_cannot_run_the_delete_path_out_of_memory() { + // SECURITY-SWEEP-2 reached through `delete`/`update` instead of `read`. + // `Overflow::collect_chain_pages` reads the same disk-controlled + // total_length at bytes 16..24 and derives `max_pages` from it. The first + // fix bounded the value in `read` only, so a forged u64::MAX still yielded + // max_pages ~2.26e15 here — and with a self-referential next_page link the + // loop pushes into an unbounded Vec until the allocator aborts the process, + // which is exactly the poison-model bypass the fix set out to close. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + + let handle; + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + handle = db.allocate(&vec![0xC3; 20_000]).unwrap(); + db.commit().unwrap(); + } + + let overflow_page = find_page_of_type(&path, PageType::Overflow); + { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.read_exact(&mut buf).unwrap(); + // Forge the length AND point next_page at this same page: the + // self-reference is what turns an unbounded max_pages into an unbounded + // walk rather than a chain that simply ends. + buf[16..24].copy_from_slice(&u64::MAX.to_le_bytes()); + buf[24..32].copy_from_slice(&overflow_page.to_le_bytes()); + page::stamp_checksum(&mut buf); + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.write_all(&buf).unwrap(); + f.sync_all().unwrap(); + } + + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + match db.delete(handle) { + Err(ChiselError::CorruptPage { .. }) => {} + Ok(()) => panic!("a forged overflow length must not delete cleanly"), + Err(other) => panic!("expected CorruptPage, got {other:?}"), + } +} diff --git a/src/spillway.rs b/src/spillway.rs index bcaf261..a022305 100644 --- a/src/spillway.rs +++ b/src/spillway.rs @@ -6,9 +6,13 @@ // allocation would push it past its strict cap. // // Lifecycle (spec 2026-05-03-chisel-spillway-design.md, "Lifecycle"): -// open file is created (or reused) and truncated to zero. Any -// pre-existing content is garbage from a crashed prior -// process and unconditionally discarded. +// open file is created fresh (O_EXCL | O_NOFOLLOW, mode 0600). Any +// pre-existing content is garbage from a crashed prior process +// and is discarded — but only after the entry is confirmed to be +// a plain file this user owns. The path is derived from the +// database path and is therefore predictable, so a symlink or a +// foreign-owned file there is a plant, not debris, and is refused +// (see `reclaim_stale_sidecar`). // spill page_id allocates a slot (or overwrites its existing // one), bytes + per-slot checksum are written. // Writes are deliberately NOT fsynced: spillway content never @@ -104,11 +108,56 @@ pub struct Spillway { payload_size: usize, } +/// Remove a pre-existing sidecar entry, but ONLY when it is plausibly debris +/// from a crashed run of this same user — a plain file, owned by us, with +/// exactly one link. Anything else (a symlink, a file owned by someone else, a +/// hard link into a directory we do not control) is a plant at a predictable +/// path, and is refused rather than cleaned up. +/// +/// The distinction matters because the two cases want opposite handling. Debris +/// is expected and must not break an open; a plant is an active attempt to +/// redirect or read uncommitted user data and must surface. `IoError` is fatal, +/// so a plant poisons the handle. +#[cfg(unix)] +fn reclaim_stale_sidecar(path: &Path) -> Result<()> { + use std::os::unix::fs::MetadataExt; + // symlink_metadata, not metadata: the whole point is to inspect the entry + // itself rather than whatever it may point at. + let md = std::fs::symlink_metadata(path).map_err(ChiselError::IoError)?; + // SAFETY: geteuid() is a pure read of process credentials. It cannot fail + // and touches no memory we own. + let ours = + md.file_type().is_file() && md.nlink() == 1 && md.uid() == unsafe { libc::geteuid() }; + if !ours { + return Err(ChiselError::IoError(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "spillway sidecar path is occupied by an entry this process did not create \ + (symlink, foreign owner, or extra hard link) — refusing to use it", + ))); + } + std::fs::remove_file(path).map_err(ChiselError::IoError) +} + +/// Non-unix fallback. Without `st_uid`/`st_nlink` there is nothing to +/// discriminate on, so keep the historical behaviour: treat a pre-existing +/// entry as crash debris and discard it. +#[cfg(not(unix))] +fn reclaim_stale_sidecar(path: &Path) -> Result<()> { + std::fs::remove_file(path).map_err(ChiselError::IoError) +} + impl Spillway { - /// Open (or create + truncate) a file-backed spillway alongside the - /// main database. The path is `.spillway`. Any pre-existing - /// content is discarded — no superblock can possibly point at - /// spillway bytes, so this is always safe. + /// Open a fresh file-backed spillway alongside the main database. The path + /// is `.spillway`. + /// + /// Any pre-existing entry at that path is REMOVED, not adopted: the sidecar + /// is created exclusively (`O_EXCL | O_NOFOLLOW`, mode 0600). No superblock + /// can point at spillway bytes, so discarding old content is always safe — + /// but reusing the old file is not, because the path is predictable and + /// another local user may have planted something there. A losing race for + /// the create (someone re-planted between the unlink and the open) is a hard + /// error that poisons the handle, which is the correct outcome for "someone + /// is tampering with my sidecar path". /// /// `payload_size` is `PAGE_SIZE` for plaintext DBs and `ENC_PAGE_SIZE` /// for encrypted DBs. It determines the slot size and capacity accounting. @@ -130,13 +179,62 @@ impl Spillway { // in-flight cache state, not a "fix your path and retry" condition, so it // must poison rather than mislead the caller into continuing (review // 2026-06-22). - let file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(true) - .open(&path) - .map_err(ChiselError::IoError)?; + // The sidecar needs sharper handling than the main database file. + // + // Its path is fully derived from the database path, so it is predictable + // to anyone who can see the database. It is created lazily, mid + // transaction, whenever cache pressure forces a spill — not at open, + // where a caller might notice something wrong. And it used to be opened + // `create(true).truncate(true)`, on the documented assumption that any + // pre-existing content is garbage from a crashed prior process. + // + // That assumption fails against a local user who can create entries in + // the database's directory, in TWO directions: + // + // * a planted SYMLINK pointing at any file the database owner can + // write would be followed and the victim's file truncated to zero; + // * a planted REGULAR file, owned by the attacker and mode 0666, would + // simply be adopted — `truncate` empties it but does not change its + // owner or mode, and `mode(0600)` applies only when the open itself + // CREATES the file. Chisel would then write spilled pages, which are + // uncommitted user values, into a file the attacker can read. + // + // Closing only the first direction leaves a data-disclosure hole behind + // a fix labelled as closing the hazard. So: unlink whatever is there, + // then create EXCLUSIVELY. `create_new` sets O_EXCL, which makes the + // open fail rather than adopt anything that reappears between the two + // syscalls, so the attacker cannot win the race by re-planting — the + // worst they achieve is a denial of service, which a local user who can + // write to this directory already has by other means. + // + // Encryption does not help with either direction: the hazards are the + // truncate and the file's ownership, not the contents. + // + // Crash debris is still discarded, as the lifecycle doc promises — but + // only after `reclaim_stale_sidecar` confirms it really is debris this + // user could have left, rather than another user's plant. That keeps the + // documented behaviour for the case it was written for (a prior run of + // ours died) and fails closed for the case it was not. + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600).custom_flags(libc::O_NOFOLLOW); + } + let file = match opts.open(&path) { + Ok(f) => f, + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + reclaim_stale_sidecar(Path::new(&path))?; + // Exclusive again on the retry: if the planter re-created the + // entry between the unlink and this open, we must fail rather + // than adopt it. A local user who can win that race can already + // deny service by other means, so a hard error is the correct + // trade — it never adopts a file we did not create. + opts.open(&path).map_err(ChiselError::IoError)? + } + Err(e) => return Err(ChiselError::IoError(e)), + }; Ok(Spillway { backing: Backing::File { file }, slots: HashMap::new(), diff --git a/src/transaction/recovery.rs b/src/transaction/recovery.rs index 81c3cec..d380be0 100644 --- a/src/transaction/recovery.rs +++ b/src/transaction/recovery.rs @@ -410,6 +410,36 @@ impl TransactionManager { }); } + // Same trust boundary, same reasoning: `freemap_depth` comes off the + // superblock unvalidated (`deserialize` reads the field, `validate` + // checks only checksum/magic/superblock_count) and is then handed + // verbatim to `FreeMapTree::from_roots`. Depth 5 already covers every + // u64 page id, so anything larger is corrupt by construction. + // + // This bound is load-bearing, not tidiness. Without it a hostile file + // reaches two process-killing paths that the poison model cannot + // intercept, because neither is a Rust error: + // + // * `scan_node` recurses once per level. A forged depth of ~500k plus + // one self-referential FreeMapInterior page overflows the stack — + // SIGSEGV — from any allocation that consults the freemap, and from + // `defrag`'s `collect_reachable`. + // * `cow_descend` loops over the depth and calls `child_span`, itself + // O(level), inside — O(depth^2), ~10^19 operations at u32::MAX — + // while materializing a page per absent child, extending the file + // without bound. That one fires on the ordinary commit path. + // + // The handle table and the membership index both already cap their + // depth this way; the freemap was the odd one out. Its own comment + // claimed `capacity()` saturation "fails closed (rejects the descent)", + // but saturation only prevents arithmetic overflow — it rejects nothing. + if sb.freemap_depth > crate::freemap_tree::MAX_DEPTH { + return Err(ChiselError::InvalidFreemapDepth { + stored: sb.freemap_depth, + max: crate::freemap_tree::MAX_DEPTH, + }); + } + // I29 write-gate: a file whose MINOR exceeds this binary's may contain // version-requiring page layouts we cannot safely write — we would // stamp pages at our older minor and drop the newer fields. Reads ARE