Repository navigation
Four decisions the root-file unsafe sweep left: no reference over shared memory, and one bound that was an argument - #222
Merged
Merged
Conversation
…el's
`user_ptr.rs`'s `copy_in` reads user memory with `read_volatile` on a stated
argument: one read, not one the compiler may split, fold or repeat, because
another thread of the same process can change the bytes between any two
instructions. Two reads outside that module were a plain `*ptr` and are not any
more.
The futex word is the one that needed it. `futex_wait`'s `read` closure is a
predicate `completion::wait_until` may evaluate more than once, so a plain load
is one LLVM may hoist out of the loop that calls it or fold with a neighbour --
and what would be hoisted is the whole point of the park protocol. The crash
dump's `read_user` has the excuse the finding gave it (the value is printed and
decides nothing), and follows the rule anyway, because `dump_region` reads the
same address twice -- once to decide the region is mapped, once inside the loop
that prints it -- and a report whose two reads were folded is printing a value
it did not fetch.
The durable half is one paragraph in `user_ptr.rs`'s module header: every read
of a single user word in this kernel is a `read_volatile`, including the two
that are not in that module, with the bulk windows named as the deliberate
exception `UserBytes` already argues for. Nothing cites a site or an issue.
## The two checks
Oracle -- the emitted assembly, both sites, `--emit=asm` on the same tree with
and without the change (`cargo +toyos build --profile toyos`, x86_64-unknown-none):
scheduler::futex_wait::{closure#0}
before: .loc 22 760 29 movl (%rsi), %eax
after: .loc 82 2153 9 movl (%rax,%rdx), %eax
process::dump_crash_diagnostics::{closure#0}
before: .loc 18 1925 23 movq (%rsi), %rdx
after: .loc 82 2153 9 movq (%rdx,%rax), %rdx
File 22 is `kernel/src/scheduler.rs`, file 18 is `kernel/src/process.rs`, file
82 is `rust/library/core/src/ptr/mod.rs` -- so the load is still exactly one
`mov` in both trees, and after the change it is the one `core::ptr::read_volatile`
emits, which the optimizer may not elide, duplicate or move. The two closures'
bodies are otherwise identical modulo the alignment-precondition panic each
spelling brings (`panic_misaligned_pointer_dereference` before,
`read_volatile`'s own `panic_nounwind_fmt` after).
Negative control -- there is none, and that is the finding's own point: the
change constrains the compiler and not the machine, so on x86 a build with
either spelling behaves identically and no test in a guest can tell them apart.
The assembly above is the evidence in both directions; it was produced by
actually reverting the two lines, rebuilding into a separate target dir and
diffing the two closures.
Closes `issues/kernel/user-pages-still-read-through-a-plain-deref.md`; its
citation in `issues/build/clippy-has-never-run-here.md` goes with it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dpa781ehanBhUgrycmXcPW
…olds it
`InitrdBacking` was handed the initrd's base address and the *file's* size. The
file's size bounds how many bytes are copied out of a block and says nothing
about where the block is, so `file_offset_to_ptr` computed
`initrd_base.add(block * 4096 + off)` with nothing anywhere to compare it
against. The extents come from the bcachefs btree **inside that same image**, so
they are input that crossed a trust boundary: a corrupt or hostile initrd names
blocks past its own end and the kernel reads whatever the bootloader placed
after it, straight into a demand-paged user page.
The fix is not a length added beside the pointer. `bcachefs::SliceBlockIO` --
the image the initrd is *mounted* through -- already knows the length and
already refuses a block outside it; the backing was simply not using it. So the
backing holds the image instead of its base:
pub struct InitrdBacking { image: SliceBlockIO, extents: Vec<Extent>, size: u64 }
`SliceBlockIO` gains one accessor, `block(BlockNum) -> Option<&[u8]>`, which is
the bound stated once and `read_block` reimplemented on top of it. Zero-copy on
purpose: routing the backing through `read_block` and its `BlockBuf` would have
cost a whole extra 4 KiB `memcpy` per demand-paged page, on the path every
program's text takes.
Three things fell out of it:
- `file_backing.rs` and `bcachefs_adapter.rs` now contain no `unsafe` at all.
The `unsafe impl Send`/`Sync` pair on `InitrdBacking` and the one on
`ReadOnlyBcacheFsAdapter` went with the raw pointers they existed for;
`SliceBlockIO` is `Send + Sync` on its own argument in its own crate.
- `mount_initrd(ptr, len)` -- which the sweep noted "is not an `unsafe fn` and
by its signature ought to be" -- takes the `SliceBlockIO` now, so the claim
moves to `SliceBlockIO::new`, which is already an `unsafe fn` and already
states it. There is one claim about the initrd region in the whole kernel and
`main.rs` makes it where the region is named.
- `block.raw() as usize * BLOCK_SIZE` was an unchecked multiply in all four
image-backed readers. The kernel builds `bcachefs` with `overflow-checks =
true`, so a block number above `usize::MAX / 4096` out of a corrupt btree was
a *kernel panic from untrusted input*. It is `byte_range`'s `checked_mul` now,
one function, four call sites.
`read_page` answers `Err` for a block the image does not reach and `Ok` with
zeros for a hole past the extent list, because those are different facts: a hole
is the file's own bytes, and an extent naming a block the image does not hold is
a filesystem this kernel cannot read. `handle_page_fault` already leaves an
`Err` fault unhandled rather than handing a process zeros where its code
belongs.
## The two checks
Negative control -- `bcachefs/src/block_io.rs`'s `slice_bounds` module, five
host tests over the pure arithmetic. Reverted by mutating `SliceBlockIO::block`
back to `Some(&self.as_slice()[off..off + BLOCK_SIZE])` with an unchecked
multiply, `cargo test -p bcachefs --lib`:
test result: FAILED. 21 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out
a_block_past_the_end_is_refused_rather_than_read
range end index 16384 out of range for slice of length 12288
a_block_that_starts_inside_and_ends_outside_is_refused
range end index 12288 out of range for slice of length 8193
a_block_number_whose_byte_offset_wraps_is_refused_and_does_not_panic
attempt to multiply with overflow
an_empty_image_holds_no_block_at_all
range end index 4096 out of range for slice of length 0
Restored, 25 passed. In the kernel those four are not panics -- there is no
slice to index, only a pointer -- which is the whole finding.
Oracle -- every boot in the suite. `/bin/init` and every program it spawns is
demand-paged out of the initrd through exactly this path, so a bound that
refuses a block it should serve stops the machine before `===READY===`; the
initrd is the root filesystem and there is no second route to it. The green
suite is therefore a differential check of the new bound against the extent
lists a real `mkfs` produces, and `bcachefs`'s own 51 integration tests exercise
`byte_range`'s other three call sites over `VecBlockIO`.
Closes `issues/kernel/initrd-extents-are-not-bounded-by-the-image.md`; its
citation in `issues/build/clippy-has-never-run-here.md` goes with it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dpa781ehanBhUgrycmXcPW
…nel had
`PageAlloc` handed out `ptr() -> *mut u8` and `size()` and nothing else, so
`handle_page_fault` filled a freshly allocated 2 MiB frame through raw pointer
arithmetic at two sites and both were in bounds by argument: the file-backed
copy derived `page_offset + valid <= page_2m` from the loop's conditions, and
the relocation pass derived a whole 4 KiB page from `offset < page_2m`, which
says nothing at all about the 4096 bytes past `offset`.
**The decision the finding asked for was one abstraction or four, and the answer
is one — and it is not a new one.** `mm::KernelSlice` is already a bounds-checked
base-and-size that hands out no `&mut [u8]`: `copy_from` and `subslice` are an
assert and a raw copy, which is `UserBytesMut::write_at`'s shape on the other
side of the same boundary. What was missing was a *safe* way to build one over a
`PageAlloc`, and that is what `KernelSlice::from_raw` being unsafe is about:
it cannot check a length against an allocation it knows nothing about. So:
PageAlloc::window(&self) -> KernelSlice
sized from the pages the type owns and frees -- `OwnedAlloc::slice` one type
over, and exactly the fix shape
`issues/design-debt/kernelslice-from-raw-cannot-check-itself.md` already named
for `PageAlloc`. That issue keeps its other two call sites and stays open, with
a dated section saying which half is done.
`UserStack` holds one too. It had a private bound of its own -- a correct one,
added by the sweep -- over `phys + size`, and `size` was `USER_STACK_SIZE`
written a second time beside the `PageAlloc::new` that had just been given it.
Now `UserStack::new(vaddr, stack_pages.window())` and the argv writes are
bounded against what was actually allocated. That is three of the finding's four
(`PageAlloc`, `UserStack`, `KernelSlice`) on one type; `MappedPages` gets no
accessor because nothing outside the two trees another agent is holding would
call it, and a method with no caller is not an abstraction.
A `&mut [u8]` is still refused, and these pages are why: the frame becomes a
user mapping four statements later, at `map_window`.
## The two checks
Negative control -- one mutation, two trees, `cargo test --test toyos-build --
mmap_prot`. The mutation is the same defect either way: the file-backed fill
writes at `page_offset + page_2m`, one whole frame past the allocation.
with the fix (`page.copy_from(page_offset + page_2m as usize, ..)`):
[kernel 0.242 cpu0] DOUBLE PANIC: the cpu was already in PageFault;
first: panic at src/mm/region.rs:65:9
`src/mm/region.rs:65` is `KernelSlice::check`. The kernel dies at the bound,
before one byte leaves the frame.
without it (`copy_nonoverlapping(.., page_alloc.ptr().add(page_offset +
page_2m as usize), valid)` -- the shape this commit replaces):
[kernel 0.239 cpu0] SEGFAULT tid=0: read unmapped address at 0x0
[kernel 0.239 cpu0] 0x1000002619c _start+0x0
No bound fires. The kernel writes 4 KiB at a time into whatever physical
page the PMM handed out next, notices nothing, and the failure surfaces
later and elsewhere as *userland* faulting on a page of zeros at its own
entry point. That is the finding, staged.
restored: `PASS mmap_prot (160ms)`.
A second, more realistic mutation is worth recording because it is the one a
bounds check cannot catch: shifting the relocation pass by a single 4 KiB page
(`subslice(offset + 4096, 4096)`) stays *inside* the frame, so the assert never
fires and `/bin/init` dies with a GP fault in `malloc`. A bound answers "is this
write inside the allocation", never "is it at the right offset" -- and the
in-bounds case is where the boot-time signal already is.
Oracle -- the guest suite's demand-paging names, which are also every boot:
`allocator_stress` (787 ms of faulting), `mmap_stress`, `mmap_prot`, `std_mmap`,
`std_tls_dlopen`, and `fault_gates`. Every PIE this machine runs is filled and
relocated through these two sites, so a bound that refuses a write it should
allow stops the machine before `===READY===` and one that is too loose lands in
a neighbour's page -- which the second arm above shows is what used to happen
silently.
Closes `issues/kernel/pagealloc-has-no-checked-window.md`; its citation in
`issues/build/clippy-has-never-run-here.md` goes with it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dpa781ehanBhUgrycmXcPW
…fter `kernel/src/inbox.rs` turned the inbox's 2 MiB shared page into Rust references at six of its seven unsafe blocks. Every one named memory the owning process maps writable, which is the shape `user_ptr.rs`'s `UserBytes` header argues is the bug rather than a style preference. Two separate things were wrong. **The order.** `create` called `shm.map_into(pid, &addr_space)` and *then* built the `RingLayout` and both `RingHeader`s through `&mut`. From the map onward a sibling thread of that process can write those bytes, and a `&mut` says nothing else may touch them at all. Nothing was ever observed going wrong -- the caller has not returned from `SYS_INBOX_SETUP`, so nothing in userland knows the address -- but "no thread knows the address" is not "no thread may write it", and a wild store reaches an address nobody named. The page is now laid out before it is mapped, by `write_ring_page`, and the ordering is *enforced*: `SharedMemObject::phys_before_mapping` asserts the region is mapped nowhere, so reversing the two statements is a named panic rather than a comment nobody re-derives. One uncontended lock and a length test, once per region ever created. **The shape.** `&RingHeader` was a data race whatever the kernel then read through it: three of its four fields are atomics, but `ring_size` is a plain `u32` the process can store to at any moment. `&Submission` was worse in the other direction -- `Submission` is all integers and therefore `Freeze`, so that borrow really did carry `noalias` and `readonly`, and the single `*` copy the caller took was one LLVM was free to split back apart. So: - the header words are reached one at a time as `&AtomicU32` (`AtomicU32::from_ptr`), which is the only way to do an atomic operation in Rust and is *sound* over shared memory: an atomic's `UnsafeCell` is precisely what tells LLVM the bytes may change under it. No reference covers `ring_size`, and the kernel still never reads it -- it holds `submission_size`/`completion_size` itself. - `submission_at` returns a `Submission` by value, read with `read_volatile`: one read of the whole entry, not one the compiler may split, fold or repeat. - `completion_at` was already a `*mut Completion` and stays one; its `+ 16` is `size_of::<RingHeader>()` now, and the header field offsets come from `offset_of!`, so the ABI layout is claimed where the struct is and not re-spelled here. Not one Rust reference to that page survives. Every ordering is unchanged: the same `Acquire` on both heads and the submission tail, the same `Release` on the submission head and the completion tail, in the same places. ## The two checks Negative control -- `phys_before_mapping`'s assert, which exists to be this. Reverted by moving `write_ring_page` back below `map_into`, `cargo test --test toyos-build -- abuse_inbox`: [kernel 0.313 cpu0] PANIC: panicked at src/object/shm.rs:166:9: shm koid 15: the region is mapped into a process already, so a kernel write through the direct map is not exclusive [kernel 0.313 cpu0] Backtrace: [kernel 0.314 cpu0] <kernel::object::shm::SharedMemObject>::phys_before_mapping+0xcd [kernel 0.314 cpu0] kernel::inbox::create+0x163 [kernel 0.314 cpu0] kernel::arch::syscall::sys_inbox_setup+0x2e [kernel 0.315 cpu0] Process: logd pid=4 state=Live [kernel 0.315 cpu0] <toyos::poller::Poller>::new+0x42 `/bin/logd` builds a `Poller` before the machine is ready, so the reversed order does not reach one test -- it stops the boot. Restored: `PASS abuse_inbox (42ms)`. Oracle -- the guest tests that drive these rings end to end: `abuse_inbox`, `inbox_cancel_wakes`, `poller_capacity`, `port_poll_churn`, `log_poll_outlives_a_close` and `metal_sim_ipc_hostile_peer`, plus every daemon in every boot, all of which park in `submit` on a completion the kernel posts through the accessors this rewrites. A header word reached at the wrong offset or an entry copied out wrong is a poll that never completes, which is what those names are for. `kernel-loom` is unchanged and re-run: no model names this ring, and the release/acquire edges here are byte-identical to what they were. The ABI side of the same question stays open (`issues/build/ring-rs-shared-slice-over-a-userland-writable-page.md`), and the SDK's mirror image of this file is filed as `issues/design-debt/poller-borrows-the-rings-the-kernel-writes.md` -- five references in `toyos/src/poller.rs` over the same page, with the kernel as the second writer. Closes `issues/kernel/inbox-rings-are-borrowed-not-copied.md`; its citation in `issues/build/clippy-has-never-run-here.md` goes with it, and that file's "filed rather than fixed" heading now says all four were decided. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dpa781ehanBhUgrycmXcPW
The required checks are strict, so GitHub refuses the merge button until this branch contains main — which also makes the checks that run on this head checks on the merged result.
Japabu
marked this pull request as ready for review
August 22, 2026 11:42
Japabu
enabled auto-merge
August 22, 2026 11:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The kernel root-file unsafe sweep (#215) filed four findings it judged to be
decisions past a documentation sweep. All four are decided and fixed here, one
commit each, and all four issue files are deleted with their citations.
The through-line is one rule, which the four sites now state where they are
rather than where they were filed: the kernel forms no Rust reference over
memory another writer can change, and every single-word read of such memory is
one read. Three of the four are that rule; the fourth is the bound that was an
argument.
Per commit
ed816e49— the futex word and the crash dump read once. Both were a plain*ptrwhereuser_ptr.rs'scopy_inusesread_volatileon a statedargument. The futex one is a predicate
completion::wait_untilmay evaluatemore than once, so a plain load is one LLVM may hoist out of the loop that calls
it. The rule is now a paragraph in
user_ptr.rs's module header, naming thebulk windows as the deliberate exception.
Negative control: none can exist, and that is the finding's own point — the
change constrains the compiler, not the machine, so on x86 both spellings behave
identically and no guest test can tell them apart.
Oracle: the emitted assembly at both sites,
--emit=asmon the same tree withand without the change.
scheduler::futex_wait::{closure#0}:.loc 22 760 29 movl (%rsi), %eaxbefore,.loc 82 2153 9 movl (%rax,%rdx), %eaxafter.process::dump_crash_diagnostics::{closure#0}:.loc 18 1925 23 movq (%rsi), %rdxbefore,.loc 82 2153 9 movq (%rdx,%rax), %rdxafter. File 82 isrust/library/core/src/ptr/mod.rs— the load is still exactly onemov, andafter the change it is the one
core::ptr::read_volatileemits.c159aa7b— the initrd's extents are bounded by the initrd.InitrdBackingheld the image's base and the file's size, sofile_offset_to_ptrcomputed an address for any block the btree named withnothing to compare it against — and the btree is inside that same image, so a
corrupt initrd read whatever the bootloader placed after it. It holds the image
now (
bcachefs::SliceBlockIO, madeCopy) and reads through a newSliceBlockIO::block, which is the one thing that knows both numbers.file_backing.rsandbcachefs_adapter.rscontain nounsafeat all any more;mount_initrdtakes the image rather than(ptr, len), so the one claim aboutthe region is
SliceBlockIO::new's.block.raw() as usize * BLOCK_SIZEwasalso an unchecked multiply in all four image-backed readers, which under the
kernel's
overflow-checks = truewas a kernel panic from a number an imagechose.
Negative control:
bcachefs/src/block_io.rs'sslice_bounds, five host unittests over the pure arithmetic. Mutated
SliceBlockIO::blockback to anunchecked multiply and a bare slice index:
test result: FAILED. 21 passed; 4 failed—a_block_past_the_end...(range end index 16384 out of range for slice of length 12288),a_block_that_starts_inside_and_ends_outside...(range end index 12288 out of range for slice of length 8193),a_block_number_whose_byte_offset_wraps...(
attempt to multiply with overflow),an_empty_image_holds_no_block_at_all(
range end index 4096 out of range for slice of length 0). Restored: 25passed.
Oracle: every boot.
/bin/initand every program it spawns is demand-pagedout of the initrd through this exact path, and there is no second route to the
root filesystem — so the green suite is a differential check of the new bound
against the extent lists a real
mkfsproduces.bcachefs's own 51 integrationtests cover
byte_range's other three call sites.77134593— the demand-paging fill is bounds-checked.PageAllochandedout a bare
*mut u8, sohandle_page_fault's file-backed copy and itsrelocation pass were in bounds by argument (the relocation one derived a whole
4 KiB page from
offset < page_2m, which says nothing about the 4096 bytes pastoffset). The decision the finding asked for was one abstraction or four,and the answer is one and it is not a new one:
mm::KernelSlicealready is abounded window that hands out no
&mut [u8], and what was missing was a safeway to build one.
PageAlloc::window()is that, sized from the allocation thetype owns — which is exactly the fix shape
issues/design-debt/kernelslice-from-raw-cannot-check-itself.mdalready namedfor
PageAlloc; that issue keeps its other two call sites and gains a datedsection.
UserStackholds one too, soUSER_STACK_SIZEis named once.Negative control: one mutation, two trees,
cargo test --test toyos-build -- mmap_prot. The fill writes atpage_offset + page_2m, one frame past theallocation. With the window:
DOUBLE PANIC: the cpu was already in PageFault; first: panic at src/mm/region.rs:65:9—KernelSlice::check, before a byteleaves the frame. With the bare pointer this commit replaces: no bound fires at
all, and the failure surfaces later and elsewhere as
SEGFAULT tid=0: read unmapped address at 0x0at_start+0x0— userland faulting on a page of zeros,after the kernel has written into whatever the PMM handed out next. Restored:
PASS mmap_prot (160ms).Oracle:
allocator_stress,mmap_stress,mmap_prot,std_mmap,std_tls_dlopen,fault_gates— and every boot, since every PIE this machineruns is filled and relocated through these two sites.
0e63b5ee— the inbox page is built before it is shared. Two things werewrong. The order:
createcalledmap_intoand then built theRingLayoutand both
RingHeaders through&mut, so from the map onward a sibling threadcould write the same bytes. The page is laid out before it is mapped now, and
the ordering is enforced by
SharedMemObject::phys_before_mapping, whichasserts the region is mapped nowhere. The shape:
&RingHeaderwas a data raceon
ring_size(a plainu32the process can store to) whatever the kernel readthrough it, and
&Submission— all integers, thereforeFreeze— really didcarry
noalias, so the one-*copy was one LLVM could split back apart. Headerwords are now reached one
&AtomicU32at a time (AtomicU32::from_ptr, soundover shared memory because an atomic's
UnsafeCellis what withdrawsnoalias), and a submission is aread_volatileof the whole entry. No Rustreference to that page survives; every memory ordering is unchanged.
Negative control:
phys_before_mapping's assert. Movedwrite_ring_pagebackbelow
map_into,cargo test --test toyos-build -- abuse_inbox:PANIC: panicked at src/object/shm.rs:166:9: shm koid 15: the region is mapped into a process already, so a kernel write through the direct map is not exclusive, backtracephys_before_mapping<-inbox::create<-sys_inbox_setup, inlogd'sPoller::new./bin/logdbuilds a poller beforethe machine is ready, so the reversed order does not red one test — it stops the
boot. Restored:
PASS abuse_inbox (42ms).Oracle:
abuse_inbox,inbox_cancel_wakes,poller_capacity,port_poll_churn,log_poll_outlives_a_close,metal_sim_ipc_hostile_peer,and every daemon in every boot — all of which park in
submiton a completionposted through the accessors this rewrites.
kernel-loomre-run and unchanged:no model names this ring, and the release/acquire edges are byte-identical.
Gates
Run after merging
origin/mainat5ceb8641(the drivers sweep, #210):cargo run -- --build-only— exit 0.cargo test—test result: ok. 272 passed, 272 total (50.4s), 53 held back for the nightly tier. NoFAIL,STALL,INVLorALONEline.cargo test --workspace --exclude toyos-build— exit 0, 100test result: oklines, no failures. That run includes
kernel-loom's eight models(
inbox,log_publish,log_record,log_wake,reap_gate,sleep_lock,ticket_lock,tlb_shootdown).No red was adjudicated; there were none.
Filed, not fixed
issues/design-debt/poller-borrows-the-rings-the-kernel-writes.md— writing thekernel half of the inbox fix surfaced the mirror image in the SDK:
toyos/src/poller.rsbuilds&RingHeader,&mut Submissionand&Completionover the same page, with the kernel as the second writer. The two entry
borrows are
Freezetypes and therefore carrynoalias. Same two moves apply;the SDK was out of this task's scope.
issues/design-debt/kernelslice-from-raw-cannot-check-itself.mdgains a datedsection saying
PageAllochas its safe constructor now and the loader andDmaPoolstill do not, so it stays open.