Mte/tagged module heap - #290
Merged
Merged
Conversation
Module *images* carry a per-domain tag, so a pointer into one derived from
another domain faults. Their *allocations* did not: `narf_kmalloc` handed
back ordinary kernel heap, which is plain Normal and unchecked, so every
buffer a module allocated stayed reachable from every domain. This closes
that for the one allocation path modules have.
Two constraints shaped it, and both rule out the obvious approach.
The kernel heap cannot simply be marked ATTR_TAGGED. Tag checks are per
page and SCTLR_EL1.TCF is per CPU, and the kernel touches heap memory
constantly inside domain scopes -- every one of those untagged accesses
would fault. The tagged set has to stay bounded to memory whose accesses
the kernel controls, which is what a dedicated region gives.
`slab` cannot back it either. It threads its free list through the objects
themselves, writing `FreeBlock { next }` into freed memory from allocator
code holding an untagged pointer, so over a tagged region every free would
fault. An allocator for tagged memory has to keep its metadata out of
band, which is why this is a bitmap and not a free list.
So: one L0 slot (278), carved into a fixed 1 MiB window per domain,
first-fit over 64-byte blocks with the bitmap in ordinary kernel memory.
Windows are mapped and tagged lazily on a domain's first allocation, and
tagging runs after mapping -- the same ordering `bpf_arena` and
`module_text` need, because a store through a non-tagged alias may leave a
granule's tag UNKNOWN.
`narf_kfree` routes by ADDRESS, not by current domain. A buffer may be
freed outside the scope that allocated it, or by a different domain, and
sending it to the wrong allocator corrupts one of them. Fixed per-domain
windows make the owner pure arithmetic.
The tag is `module_text::domain_tag`, not a fresh one. They have to be the
identical value: module code reaches its buffers through pointers derived
from its own tagged image, so a differently-tagged heap would make a
module fault on memory it had just allocated.
Falling through to the ordinary heap when the tagged path declines -- no
MTE, FRAME, oversized, window full -- is a capacity answer, not an error.
x86 is untouched; its module isolation is the PKS protection key.
Four smokes, including one asserting freed blocks are reused. A bump
allocator would have passed every other assertion here while leaking until
a long-running module exhausted its window, and nothing would have failed
until then.
One limit, deliberate: tags are written once per window and not rotated on
free. That buys domain isolation -- a pointer derived outside the domain
carries a different tag -- not use-after-free detection, which needs a
fresh tag per allocation and a matching pointer handed back.
aarch64 6087 pass / 0 fail / 39 skip; x86_64 8105 / 0 / 77. Both baselines
moved +5 from upstream work merged into main since the last run; the name
diff confirms none of those are from this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
x86 had the same hole aarch64 did. Module images carry `pk(D)`, but module *allocations* came from the ordinary kernel heap -- unkeyed, and therefore reachable from every domain. Same gap, on the reference platform. One module now serves both mechanisms. Region layout, bitmap allocator and `owns`/`owner` arithmetic are shared; only how a page is made unreachable differs -- ATTR_TAGGED plus granule tags on aarch64, `PtFlags::pk(D)` on x86. Keeping it in one file is deliberate: that the owner is derived from the address, that windows are fixed-stride, and that allocator metadata is out of band are properties of the design rather than of either mechanism, and they had been getting restated per architecture elsewhere in this tree. Two differences are encoded rather than smoothed over. x86 pointers stay ordinary, because protection lives in the PTE; there is no tagged alias and `canonical()` is the identity there. The key spaces are not the same size. MTE loses tag 15 to untagged kernel pointers, so FRAME goes unprotected; PKS has all sixteen keys, so every domain including FRAME gets one. FRAME's key is 0, which `enter_domain` always permits, so a FRAME allocation stays reachable everywhere -- by design, not by accident. The two fault tests are deliberately NOT mirror images, and the comments say why. MTE denies a mismatched *pointer*, so the aarch64 test enters SCRATCH's own scope and uses an untagged pointer. PKS denies a *key in the wrong scope*, so the x86 test enters a different domain's scope and touches SCRATCH's memory. Written symmetrically, one of them would have been testing the wrong thing. Noted in the docs: x86 could have tolerated `slab`'s inline metadata, since a free running inside the domain's own scope has pk(D) permitted -- but only there, and only for in-scope frees, which `free` deliberately does not require. The bitmap is what makes both architectures behave identically. Unlike the PCID confinement earlier in this work, both paths execute on runners we have. aarch64 6087 pass / 0 fail / 39 skip; x86_64 8109 / 0 / 77. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scoping pass for per-domain module stacks. The useful finding is not about stacks: domain state leaks across preemption, today. `bpf::domain::enter` takes a `PreemptGuard`; `modules::domain::enter` does not, nothing disables preemption around `invoke_init`, and no scheduler code saves IA32_PKRS or SCTLR_EL1.TCF across a context switch. Those are per-CPU registers, so a module `init()` preempted mid scope leaves the next task on that CPU running with the narrowed domain state. That was nearly harmless until this branch. On aarch64 `Mte::enter_domain` was a no-op so there was no TCF to leak, and on x86 the only keyed pages were module images, which few tasks touch. Both changed: images and the per-domain heap now carry pk(D), and TCF genuinely flips. A leaked scope now means an unrelated task running with fifteen keys denied, or with tag checking on against pages whose tags it does not carry -- and on aarch64 that is a fatal fault in code with no extable entry, attributed to whatever ran next rather than to the module that leaked it. The doc records both candidate fixes and why they are not equivalent. A PreemptGuard mirroring bpf::domain is one line and wrong in general -- module init is arbitrary code that may legitimately sleep, so holding preemption across it trades latency for deadlock. Per-task domain state saved by the context switch is the real fix, and is a prerequisite for stacks, because a per-domain stack makes a leaked scope worse. On the stacks themselves: SP must carry the domain tag on aarch64 or every push faults; ordinary CPL0 exceptions on x86 land on the interrupted stack (only NMI/#DF/#MC have IST), so kernel exception frames would sit in domain-owned memory; overflow needs a guard page plus somewhere to land, which aarch64 lacks; and backtrace attribution breaks silently against a tagged SP. And the part worth saying plainly: it may not be worth doing. Live frames are already unreachable in practice, so the gain is against stale frames left by a previous domain -- a real leak, but the same class the kernel has anywhere it reuses stack memory. Scrubbing the used extent on scope exit buys the same thing with no SP switch, no guard page and no overflow stack. Recommended order is to fix the leak, then measure scrub-on-exit before committing to stacks. Documentation only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CURRENT_DOMAIN`, the per-CPU byte behind `current_domain()`, was not in either architecture's `KernelContext`, so a context switch dropped it. A task preempted inside a domain scope left `current_domain()` answering with that domain for whatever ran next. Silent when wrong, which is the worst property for this to have. Nothing faults: `domain_heap::alloc` picks the wrong window for the next task's allocation, and `block::encrypted`'s constructor assertion reads a domain that is not its own and passes. Introduced by the commit that made `current_domain()` real, not longstanding. Fixed by carrying the byte with the task, the way the hardware state already rides in `ctx` -- restored before the switch, re-captured after. Both a voluntary yield and an involuntary preemption resume at the same point in `poll_to_yield`, so one capture covers both. The test asserts both halves, because they fail differently. A task must RESUME holding the domain it yielded with, which is what makes a scope survive a yield at all; and it must not leave that domain behind for the caller, which is the leak. A fix that merely reset to FRAME on switch-out would satisfy the second and break the first. It lives in `scheduler` and drives `narf_arch` directly rather than going through `modules::domain::enter`: this is a property of the switch, and `modules` does not depend on `narf-scheduler` -- reaching for `block_on` from there would have pointed a crate at one that depends on it. Also revises `domain-stacks.md`, whose first revision led with a prerequisite that does not exist. It claimed no scheduler code saves IA32_PKRS or SCTLR_EL1.TCF across a switch. Both architectures have carried that state all along -- x86 in `domain_state` at offset 72 with a `domain_kind` discriminant, aarch64 in `domain_active`/`domain_sctlr`/ `domain_gcr` with mrs/msr around the swap -- and `smoke_stackful_switch_preserves_domain_state` already asserted it. The revision keeps the wrong claim visible with the reason it was wrong, because the reason generalises: the check was a grep against `sched/src/` when the crate is `scheduler/`, with `2>/dev/null` swallowing the "no such file" error, so an empty result read as "nothing saves it". A search that cannot distinguish "found nothing" from "looked nowhere" is not evidence. Per-domain stacks are correspondingly unblocked, not gated on a preemption fix. x86_64 8110 pass / 0 fail / 77 skip; aarch64 6088 / 0 / 39 (+1 each). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`domain-stacks.md` proposed scrubbing the stack on domain-scope exit as a cheaper alternative to per-domain stacks, and asked for a measurement before committing. `smoke_measure_stack_scrub_cost` provides one: median `write_bytes` cost over 512 B to 32 KiB, 65 runs after a warm pass. aarch64 reads 6164 ticks for a full 32 KiB stack at CNTFRQ_EL0 = 1 GHz, so about 6 microseconds; x86 reads 12418 raw TSC cycles with no frequency anchor. Both are QEMU TCG and are recorded as such -- a memset under an emulator has no particular relationship to one on silicon. The only property the decision needs is that cost is linear in bytes with no cliff, which survives the caveat. The useful finding is that the question has two answers, which the original framing missed by treating "domain scope exit" as one site. Module scopes run twice per module *lifetime* -- `loader.rs` enters one around init() and one around exit(). Scrubbing a whole 32 KiB stack at both ends adds ~12 us to a module load that already parses ELF, relocates, maps and seals. It needs no depth tracking either: everything below the current SP is dead by definition and is exactly where stale frames live. BPF scopes wrap every program run, four sites in `prog.rs`. A 4 KiB scrub is ~850 ns on these figures, the same order as an entire program invocation. That is not a tax on the hot path, it is the hot path. So: adopt scrub-on-exit for module scopes, do not scrub on BPF scope exit. Per-domain stacks stay unbuilt and, for the module case, unnecessary. Whether stale BPF frames are worth anything is a threat-model question, not a measurement one, and it is left open rather than answered here. The measurement prints and passes rather than asserting. A threshold baked into a test would be a guess hardened into a regression, and these numbers are emulator numbers -- worth keeping so they can be re-run when there is hardware, not worth gating on. aarch64 6089 pass / 0 fail / 39 skip; x86_64 8111 / 0 / 77. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…verted
Two things, both corrections to work committed earlier on this branch.
**The domain-byte test was vacuous.** `bfc6ae56` added
`smoke_domain_byte_follows_the_task_across_a_yield` as proof that the
reported domain now follows a task across a context switch, and drove it
with `block_on`. `block_on` polls a future inline on the caller's stack --
no `KernelTask`, no `kernel_switch`. The "yield" crossed nothing, the
per-CPU byte was preserved trivially, and the test passed whether or not
the fix it named was present. It now uses `KernelTask` + `poll_to_yield`
against an explicit `KernelContext`, the way the neighbouring
`smoke_stackful_switch_preserves_domain_state` does, and additionally
asserts the executor does not inherit the task's domain at the yield --
the half `block_on` could never have reached.
**Scrub-on-exit was implemented and is reverted.** It works on aarch64 --
a pattern planted in dead stack is gone after a scope closes -- and faults
on x86 with `#PF`, `rip = 0x0`: a live return address was zeroed. A scrub
that corrupts the stack is worse than the leak it closes, so it is out
rather than shipped on one architecture.
The x86 failure is NOT understood, and the doc says so instead of
guessing. Two candidates are recorded for the next attempt: the red zone
(data below RSP is live unless the target builds -mno-red-zone), and the
scrub running at a shallower RSP than the frames it means to erase.
Also recorded, because it cost three wrong diagnoses: `&0u8 as *const u8`
is not a stack address. Rust const-promotes the literal to a `'static`, so
it yields `.rodata` in the kernel image. Comparing that against real stack
bounds produced first "the aarch64 scheduler reports the wrong stack" and
then "async locals live on the heap", both stated confidently and both
wrong. The giveaway was the address being byte-identical across two
rewrites that should have moved it. Read RSP/SP with `asm!` and nothing
else.
The containment check that survived is load-bearing rather than
defensive: requiring the live SP to lie inside the reported task stack is
what turned that `.rodata` extent into a decline instead of an 8 KiB write
into the kernel image.
Kept from the attempt, both sound on their own:
* `current_stack_range()` -- the current task's stack bounds, for anything
writing into the dead part of a stack.
* `run_on_stackful_task()` -- drives a future to completion on a real
`KernelTask`. A test that needs a task stack and reaches for `block_on`
silently tests nothing, which is exactly what happened above.
The measurement conclusion is unaffected: it never depended on the
implementation. Module scopes run twice per module lifetime and can afford
a scrub; BPF scopes wrap every program run and cannot.
aarch64 6089 pass / 0 fail / 39 skip; x86_64 8111 / 0 / 77.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes a module's frames outliving its scope: data left on the shared kernel stack by one domain and readable by whatever runs there next. That is the concrete gain per-domain stacks were considered for, and this buys it with no SP switch, no guard page, no overflow stack and no change to exception entry. Module scopes only. `loader.rs` enters two, around init() and exit(), so this runs twice per module lifetime. `bpf::domain::enter` wraps every program run, where an 8 KiB erase (~1.75 us measured) is the same order as the invocation it follows -- that is not a tax on the hot path, it is the hot path. The measurement behind the split is in `arch/specification/domain-stacks.md`. The first attempt at this was reverted for faulting on x86 with `#PF`, `rip = 0x0`. The cause: erasing via `write_bytes` CALLS memset, which pushes a return address at `SP - 8` -- inside the region being erased. It zeroed its own return slot and `ret` jumped to null. aarch64 survived the identical code only because its return address lives in `LR`, so the calling convention alone decided whether the routine destroyed itself. Fixed by erasing in inline `asm!` with `options(nostack)`: `rep stosb` on x86, an 8-byte store loop on aarch64. Nothing is pushed, so the erased region provably cannot hold anything this function returns through. The alternative -- a guard band below SP sized for the scrub's own frames -- was rejected as a guess about callee frame sizes, and memset may be a builtin whose prologue varies. Three other things are load-bearing rather than defensive, each having already gone wrong once: SP is read with `asm!`. `&0u8 as *const u8` is not a stack address -- Rust const-promotes the literal to a `'static`, giving `.rodata` in the kernel image. An extent computed from that produced two confident and wrong diagnoses before the address being byte-identical across rewrites gave it away. The live SP must lie inside the stack `current_stack_range()` reports. Clamping to a bottom belonging to a different stack is not a clamp; it is what turned the `.rodata` extent into a decline instead of an 8 KiB write into the kernel image. Interrupts are masked for the erase. A handler taken at CPL0 pushes below SP, into exactly the region being cleared, and would have its live frame zeroed underneath it. The test drives a real `KernelTask` via `run_on_stackful_task`, not `block_on`: `block_on` polls inline on the caller's stack, where `current_stack_range()` is `None` and the scrub declines, so a test reaching for it proves nothing. It plants a pattern below the live SP and requires every byte gone after a scope closes. aarch64 6097 pass / 0 fail / 39 skip; x86_64 8119 / 0 / 78. x86's pass count is flat because `smoke_audio_format_unsupported_rate_rejects` independently drifted pass->skip; it has done so twice this session, both times unrelated to the change in flight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent skip `smoke_audio_format_unsupported_rate_rejects` drifted between pass and skip across runs, reporting "no audio backend probed" on a machine where the HDA controller had demonstrably probed -- both a passing and a skipping run logged `probe: hda-intel-ich6 [8086:2668] -> ok`. It was not flaky. `smoke_audio_picker_no_backend_when_unprobed` manufactures an unprobed world with `snd_pci::__reset_for_test()`, `hda::__reset_for_test()` and `driver_match::__reset_for_test()`, asserts the picker returns None, and returns without restoring any of it. Those three resets are global, so after that test the audio backend stays unprobed for the remainder of the boot and every later test needing one skips. `hda::is_probed()` is `CONTROLLER.lock().is_some()`, and the reset clears exactly that. Whether the format test skipped came down to whether it ran before or after the picker test, and kernel-test ordering varies between runs -- the two logs differ in what precedes it. Adding unrelated tests elsewhere perturbs that order, which is why this surfaced twice while working on something else, each time looking like an unrelated flake. The fix belongs in the test that breaks the world rather than the one that notices. The picker test now captures its assertions, calls `restore_audio_probe_state()` on every path, and only then returns. The restore is best-effort on purpose: a machine with no audio device has nothing to restore, and failing there would turn "no sound card" into a test failure. What it must not do -- and what it did -- is leave the buses unregistered behind it. Verified by three consecutive x86 runs at 8120 pass / 0 fail / 77 skip with both tests passing every time, where the count previously alternated between 8119/78 and 8120/77. Three runs is not proof for an order-dependent bug, but it is consistent with the cause being removed rather than the symptom having moved. aarch64 6097 / 0 / 39, both tests passing. Worth a follow-up beyond this commit: `hda_tests.rs` has four more `bus_reset()` call sites. Any that do not restore are the same latent bug waiting on a different test order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up sweep after `smoke_audio_picker_no_backend_when_unprobed` was
found leaving the audio backend unprobed for the rest of the boot. Every
`driver_match::__reset_for_test` site in `hda_tests.rs` had the same shape,
and one of them the same bug.
`smoke_hda_writer_submit_round_trip` calls `bus_reset()` AFTER its
`Skip("no intel-hda (ICH9)")` check, so on any machine without ICH9 --
including the one these suites run on, which has ICH6 (8086:2668) -- it
emptied the driver-match table and returned without putting it back. Live
instance of the bug just fixed, waiting on a test order that noticed.
The three match tests degrade state more subtly. Each empties the whole
table with `bus_reset()` and then re-registers only its own subject, so
after `smoke_acp6_pci_match_registered` the table holds acp6 and nothing
else. That cannot produce the specific skip we chased -- it does not touch
`CONTROLLER` -- but anything later relying on a different driver being
registered would misbehave, order-dependently. `restore_audio_probe_state`
therefore registers hda, acp6 and snd_pci rather than any single one.
Restoration is a `Drop` guard, not a call at the end of each test. These
tests have several early `return TestResult::Fail(..)` paths, and a manual
restore only covers the happy one -- which is exactly the shape of the bug
being fixed: state cleared, early return taken, world left broken. The
guard is declared immediately after the reset so every path out is
covered.
x86_64 8120 pass / 0 fail / 77 skip on two consecutive runs; aarch64 6097
/ 0 / 39.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
No description provided.