diff --git a/.gitignore b/.gitignore index 2e56867..ace1ad1 100644 --- a/.gitignore +++ b/.gitignore @@ -53,6 +53,7 @@ __pycache__/ # Local-only technical analyses (reading notes on external codebases) docs/analysis/technical-analysis/ +docs/analysis/private/ # Local-only transient workbooks (follow-up ledgers, drafts, scratch notes) docs/analysis/temp/ diff --git a/docs/roadmap/current.md b/docs/roadmap/current.md index f6cdc9c..5def050 100644 --- a/docs/roadmap/current.md +++ b/docs/roadmap/current.md @@ -4,6 +4,8 @@ A short pointer file updated as work progresses. For the full plan see [`phases/ --- +> **2026-07-15 update β€” πŸ” Full-repository review completed; its findings + excellence opportunities are now a phased roadmap backlog (not a side report).** A systematic, multi-dimensional review of the entire repository (all code, docs, and infrastructure) was run at the Phase B/C boundary. Its **241 verified findings** (πŸ”΄ 2 critical Β· 🟠 27 high Β· 🟑 78 medium Β· βšͺ 113 low Β· ⚫ 21 info) and **147 excellence/polish opportunities** are recorded as actionable roadmap work: a new consolidation bridge phase **[Phase B.2 β€” Consolidation & Hardening](phases/phase-b2.md)** collects the near-term, feature-independent debt (CI-enforcement gaps; the Phase-B documentation cascade; and the review's correctness/robustness/quality/unsafe-audit fixes β€” organized as Tracks B.2-1…B.2-6), and each later phase gained a **"Review-derived work items"** section routing its findings to where they belong: [Phase C](phases/phase-c.md#review-derived-work-items-2026-07-15-full-repository-review) (the two SMP-critical defects β€” `StaticCell`'s blanket `unsafe impl Sync` and the non-atomic PMM bitmap read-modify-write β€” both harmless single-core but UB the instant a second core boots β€” plus the broader concurrency/SMP-readiness list), [Phase D](phases/phase-d.md#review-derived-work-items-2026-07-15-full-repository-review) (aarch64 hardware-correctness that is silent on QEMU but can be fatal on Pi 4 silicon: break-before-make, `CPTR_EL2`, FP/SIMD trap frames), [Phase E](phases/phase-e.md#review-derived-work-items-2026-07-15-full-repository-review) (capability-ownership gaps β€” `cap_map` memory-ownership, `cap_derive` correlation, W^X, DEVICE-mapping β€” that must close in the same change that first wires each syscall), [Phase G](phases/phase-g.md#review-derived-work-items-2026-07-15-full-repository-review) (property/fuzz + formal-verification "prove-it" work), and [Phase H](phases/phase-h.md#review-derived-work-items-2026-07-15-full-repository-review) (test-HAL fidelity for new BSPs). **Phase A and Phase B files are unchanged** (both phases are closed). **Verdict:** shipped single-core behaviour is sound and the security-first discipline is real (zero critical defects in current runtime behaviour; ~200 non-test `unsafe` sites all audited, zero drift); the backlog is the checklist from "correct" to "first-class". Phase C remains the active phase; **Phase B.2 is the recommended bridge to run first**, so Phase C starts on a base whose CI gates are real and whose docs can be trusted. This banner records the review and its backlog; it supersedes nothing below. +> > **2026-06-01 update β€” 🏁 PHASE B CLOSED: B6 Done; the microkernel core is complete (EL1 boot β†’ first EL0 userspace task).** B6's closure quartet (step 7) is filed, closing the phase. **Guide:** [`first-userspace.md`](../guides/first-userspace.md). **Performance:** the [B6 perf baseline](../analysis/reviews/performance-optimization-reviews/2026-06-01-B6-closure.md) β€” boot-to-end band p10/p50/p90 = 13.143 / 15.507 / 18.469 ms, with a **same-host control proving running the first EL0 task adds no measurable boot-to-end cost** (the EL0 round-trip is sub-floor; the raw B6 > **2026-06-01 update β€” πŸŽ‰ FIRST USERSPACE TASK RUNS: T-028 (EL0 wire-up) implemented, In Review; B6's functional milestone hit.** **[T-028](../analysis/tasks/phase-b/T-028-el0-userspace-wireup.md)** runs Tyrne's first real EL0 userspace task end-to-end. The boot wires `load_image` β†’ `task_create_from_image` (T-024 bridge; resolve the Task cap β†’ `TaskHandle` + the AS cap β†’ `AddressSpaceHandle`) β†’ seed `USER_TASK_TABLE` with a `DebugConsole`/`CONSOLE_WRITE` cap at handle 0 (`HELLO_CONSOLE_CAP`, asserted) β†’ `add_user_task` (with a `USER_TASK_STACK` `SP_EL1`); the dormant `+0x200` EL1-stub smoke is retired. **QEMU smoke proves the full round-trip:** `ERET`β†’EL0 `@0x800000` β†’ `console_write` SVC takes the lower-EL **`+0x400`** vector β†’ gate #1 translates the buffer through the task's own `TTBR0` β†’ **`hello from userspace`** β†’ `ERET`β†’EL0 β†’ `task_exit` SVC β†’ `tyrne: userspace task exited` β†’ `tyrne: all tasks complete` β€” only the 2 expected EL0 SVC exceptions, **zero new fault class**. Gates #1 (per-page translate), #2 (EL0 entry + register scrub), #3 (the task's own cap table), the high-half kernel reachable via `TTBR1`, and EPD0-cleared-on-activation are all **proven at runtime**. **Beyond the planned plumbing:** the cooperative scheduler had no termination path (`task_exit`'s `Terminate` was a T-026 dormant no-op β†’ the task ran past it β†’ BRK β†’ panic), so T-028 adds `Scheduler::task_exit_current` β€” extracting `start`'s dispatch tail into a shared `run_dispatched` (DRY, no duplicated context-switch) β€” and wires `syscall_entry`'s `Terminate` β†’ `task_exit_current` + `Reschedule` β†’ `yield_now`. **EL0-boundary security review = Approve** ([2026-06-01](../analysis/reviews/security-reviews/2026-06-01-T-028-el0-userspace-wireup.md); the carry-forward DoD gate β€” a 5-lens / 9-agent adversarial pass across the eight axes, **0 confirmed exploitable defects**; 4 lenses clean, 2 findings verified non-exploitable; **SEC-T028-01** object-lifecycle-on-exit forward-flagged). Audit: UNSAFE-2026-0008 + 0029 Amendments. Gates: fmt, host + kernel clippy `-D warnings`, **host tests 259 kernel** (+`start_prelude` test) / 46 hal / 58 test-hal / 3 doc, kernel build, `tools/smoke.sh --int` PASS, **Miri `--workspace --exclude BSP` 0 UB**. **All B6 functional steps (1–6) are done; only step 7 (closure) remains:** the [B6 closure trio](phases/phase-b.md#milestone-b6--first-userspace-hello) β€” the `docs/guides/first-userspace.md` guide, the first hypothesis-driven performance cycle (EL0 round-trip / IPC / context-switch vs the A6 baseline), and the Phase B business retrospective β€” which closes **Phase B**. This banner supersedes the step-5 banner below. @@ -71,12 +73,12 @@ A short pointer file updated as work progresses. For the full plan see [`phases/ --- -- **Active phase:** **C** β€” opened 2026-06-01 (Phase B Closed 2026-06-01 via the B6 closure quartet, [PR #43](https://github.com/HodeTech/Tyrne/pull/43)); no Phase C milestone/task scoped yet. **Phase B (B0–B6) complete** β€” opened 2026-04-21: **B0 closed 2026-04-27**; **B1 closed 2026-05-07**; **B2 closed 2026-05-09**; **B3 closed 2026-05-14** via PR #29's closure trio (merge `b425dc1`); **B4 closed 2026-05-28** via its closure trio (T-019, PR #31 `7f876af`); **B5 closed 2026-05-29** via its closure trio (T-020 + T-021, PR #34 `f98e1af`); **B6 closed 2026-06-01** via its closure quartet ([PR #43](https://github.com/HodeTech/Tyrne/pull/43)). All seven closures lifted `Done` after a verbatim QEMU smoke trace + clean `-d guest_errors` count per the [business master-plan Β§Acceptance criteria](../analysis/reviews/business-reviews/master-plan.md#acceptance-criteria) rule. **The 2026-04-28 implementation-complete claim for B1 was rolled back on 2026-05-06 by the smoke regression and re-issued 2026-05-07 as a smoke-verified Done** β€” that remains the only re-open arc to date; B2 and B3 both closed cleanly on first attempt. -- **Active milestone:** none active. **B6 β€” First userspace "hello".** B5 (Syscall boundary) was formally **Closed 2026-05-29** via its closure trio (see the top banner + the [B5 business retrospective](../analysis/reviews/business-reviews/2026-05-29-B5-closure.md)). B6 per [phase-b.md Β§B6](phases/phase-b.md#milestone-b6--first-userspace-hello): a real EL0 task β€” loaded by the deferred [`task_create_from_image`](phases/phase-b.md#milestone-b4--task-loader) bridge (B4 Β§3, the `LoadedImage` β†’ runnable `CapHandle{CapObject::Task(...)}` wrapper) β€” runs in its own AS, makes a `console_write` syscall through the lower-EL `VBAR_EL1+0x400` vector (the real EL0↔EL1 round-trip B5's `+0x200` proxy could not prove), and exits via `task_exit`. B6 **closed** the three [T-021 carry-forward gates](phases/phase-b.md#milestone-b6--first-userspace-hello) (per-task `console_write` window + per-page user-VA translation; `SP_EL1` init for `+0x400`; `SYSCALL_STUB_TABLE` β†’ current-task table) before the first real EL0 task ran, paired with the **ADR-0033 high-half** migration. B6 was the **Phase-B-closing** milestone (its review doubled as the [Phase B retrospective](../analysis/reviews/business-reviews/2026-06-01-phase-b-closure.md)). +- **Active phase:** **C** β€” opened 2026-06-01 (Phase B Closed 2026-06-01 via the B6 closure quartet, [PR #43](https://github.com/HodeTech/Tyrne/pull/43)); no Phase C milestone/task scoped yet; **[Phase B.2 β€” Consolidation & Hardening](phases/phase-b2.md) is the recommended next bridge** before C's multi-core work begins (see the 2026-07-15 banner above). **Phase B (B0–B6) complete** β€” opened 2026-04-21: **B0 closed 2026-04-27**; **B1 closed 2026-05-07**; **B2 closed 2026-05-09**; **B3 closed 2026-05-14** via PR #29's closure trio (merge `b425dc1`); **B4 closed 2026-05-28** via its closure trio (T-019, PR #31 `7f876af`); **B5 closed 2026-05-29** via its closure trio (T-020 + T-021, PR #34 `f98e1af`); **B6 closed 2026-06-01** via its closure quartet ([PR #43](https://github.com/HodeTech/Tyrne/pull/43)). All seven closures lifted `Done` after a verbatim QEMU smoke trace + clean `-d guest_errors` count per the [business master-plan Β§Acceptance criteria](../analysis/reviews/business-reviews/master-plan.md#acceptance-criteria) rule. **The 2026-04-28 implementation-complete claim for B1 was rolled back on 2026-05-06 by the smoke regression and re-issued 2026-05-07 as a smoke-verified Done** β€” that remains the only re-open arc to date; B2 and B3 both closed cleanly on first attempt. +- **Active milestone:** none active β€” Phase C is not yet scoped; **[Phase B.2](phases/phase-b2.md) (Consolidation & Hardening) is the recommended next bridge**. *Historical (B6 β€” First userspace "hello" β€” was the Phase-B-closing milestone, Closed 2026-06-01):* B5 (Syscall boundary) was formally **Closed 2026-05-29** via its closure trio (see the top banner + the [B5 business retrospective](../analysis/reviews/business-reviews/2026-05-29-B5-closure.md)). B6 per [phase-b.md Β§B6](phases/phase-b.md#milestone-b6--first-userspace-hello): a real EL0 task β€” loaded by the deferred [`task_create_from_image`](phases/phase-b.md#milestone-b4--task-loader) bridge (B4 Β§3, the `LoadedImage` β†’ runnable `CapHandle{CapObject::Task(...)}` wrapper) β€” runs in its own AS, makes a `console_write` syscall through the lower-EL `VBAR_EL1+0x400` vector (the real EL0↔EL1 round-trip B5's `+0x200` proxy could not prove), and exits via `task_exit`. B6 **closed** the three [T-021 carry-forward gates](phases/phase-b.md#milestone-b6--first-userspace-hello) (per-task `console_write` window + per-page user-VA translation; `SP_EL1` init for `+0x400`; `SYSCALL_STUB_TABLE` β†’ current-task table) before the first real EL0 task ran, paired with the **ADR-0033 high-half** migration. B6 was the **Phase-B-closing** milestone (its review doubled as the [Phase B retrospective](../analysis/reviews/business-reviews/2026-06-01-phase-b-closure.md)). - **Active task:** none β€” Phase C is not yet scoped into tasks (B6's tasks T-022…T-028 are all **Done**; see *Last completed task* below). *Historical (B6 step 3):* [T-024 β€” `task_create_from_image`](../analysis/tasks/phase-b/T-024-task-create-from-image.md) was **implemented (dormant) on `t-024-task-create-from-image`** (the `LoadedImage` β†’ runnable `CapHandle{CapObject::Task}` bridge, B6 step 3; **no new ADR** β€” composes ADR-0029/0028/0016/0037, Task cap kind-only; **no new `unsafe`**). Resolves the loaded AS cap β†’ mints a `Task` + a root `CapKind::Task` cap, rolling the task back on a full cap-table; **347 host tests** (+4) + Miri 0 UB + byte-stable fault-clean smoke (dormant β€” no BSP caller). **Next:** the security-critical gate #1 (per-task user-VAβ†’kernel-VA translation) + gate #3, then `userland/hello` + `tyrne-user` + the wire-up smoke. β€” **T-023 (EL0 entry context) Done 2026-05-31** (merged [PR #37](https://github.com/HodeTech/Tyrne/pull/37), merge `eb3125f`); it closed T-021 carry-forward gate #2 ([ADR-0037 Accepted](../decisions/0037-el0-entry-context.md) 2026-05-31). Adds `ContextSwitch::init_user_context` + the BSP `enter_el0` `ERET` trampoline (which **scrubs the EL0-visible register file before `ERET`** β€” `x0`–`x30` + `v0`–`v31` + `FPCR`/`FPSR`/`TPIDR_EL0`/`TPIDRRO_EL0`; the 2026-05-31 review-round register-leak fix, so no kernel state reaches EL0) + `Scheduler::add_user_task`; dormant until the B6 wire-up. New UNSAFE-2026-0032 (second-reviewer-flagged); 342 host tests + Miri 0 UB + byte-stable fault-clean smoke. β€” **T-022 (B6's gating prerequisite) is now Done 2026-05-31** (merged [PR #36](https://github.com/HodeTech/Tyrne/pull/36) `b6549d7`; security-reviewed [Approve](../analysis/reviews/security-reviews/2026-05-31-T-022-high-half-migration.md)): [ADR-0033 Accepted](../decisions/0033-kernel-high-half-migration.md) 2026-05-30 (separate commit `db892c1`); T-022 implements the boot-time migration β€” the kernel now runs in `TTBR1_EL1` (`tyrne: high-half active` boot marker) and `TTBR0_EL1` is freed for per-task userspace. Whole-image-high-linked + forced-low ELF entry + a single linear high-half offset (`0xFFFF_FFFF_0000_0000`); the ADR's Option-2 fallback was not needed. All gates green (340 host tests, host+kernel clippy, fmt, release build, Miri Stacked-Borrows, QEMU smoke fault-clean β€” exactly 2 SVC exceptions, zero new Translation/Permission faults); hardened against a third multi-lens adversarial pass (4 findings, all addressed). **Security-reviewed 2026-05-31 β€” [Approve](../analysis/reviews/security-reviews/2026-05-31-T-022-high-half-migration.md)** (eight axes; no live finding β€” the migration is a structural kernel/user isolation *strengthening*, kernel absent from `TTBR0_EL1`; UNSAFE-2026-0031 policy-conformant + second-reviewer-signed). **Prior tasks Done: T-020 + T-021 β€” 2026-05-29** (PR #34, merge `f98e1af`): T-020 split `IpcError::InvalidCapability` β†’ `StaleHandle`/`WrongObjectKind`/`MissingRight` + redacted `Capability`/`CapObject` `Debug` (zero new `unsafe`); T-021 landed the architecture-agnostic kernel `syscall` module (`SyscallError` / ABI / `UserAccessWindow` / dispatcher) + the BSP `tyrne_sync_trampoline` (current-EL `+0x200` + lower-EL `+0x400`) + `CapObject::DebugConsole` + `CapRights::CONSOLE_WRITE` + `CapHandle::from_raw`, exercised at B5 by an EL1-kernel-stub `SVC` (current-EL path; the real EL0 `+0x400` round-trip deferred to B6) β€” UNSAFE-2026-0029 / 0030. -- **In review:** the **Phase B closure quartet** β€” [PR #43](https://github.com/HodeTech/Tyrne/pull/43) (guide + perf baseline + the two security reviews + the retrospective). +- **In review:** none. *(Historical: the Phase B closure quartet β€” [PR #43](https://github.com/HodeTech/Tyrne/pull/43), guide + perf baseline + the two security reviews + the retrospective β€” merged 2026-06-01, closing Phase B.)* - **In progress:** none. -- **Working branch:** `b6-closure` (off `main`; the Phase B closure quartet, [PR #43](https://github.com/HodeTech/Tyrne/pull/43)). *(B6 task branches `t-022`…`t-028` retired after their merges, PRs #36–#42.)* Earlier: `t-024-task-create-from-image` (off `main` at `eb3125f`, the merged T-023 [PR #37](https://github.com/HodeTech/Tyrne/pull/37)). The prior B6-opening arc on `b6-el0-entry-context` (the [T-022 security review](../analysis/reviews/security-reviews/2026-05-31-T-022-high-half-migration.md) `9a51b73` + [ADR-0037](../decisions/0037-el0-entry-context.md) Propose `00d342e` / Accept `546febc` + T-023) **merged to `main` 2026-05-31 via [PR #37](https://github.com/HodeTech/Tyrne/pull/37)** (`eb3125f`); that branch + `t-022-high-half-kernel-mapping` ([PR #36](https://github.com/HodeTech/Tyrne/pull/36) `b6549d7`) retired. *(The earlier `sec-review-b5-syscall-boundary` branch carried the B5 security review + closure trio and is retired.)* **[PR #34](https://github.com/HodeTech/Tyrne/pull/34)** (T-020 + T-021, 9 commits) **merged to `main` 2026-05-29** (`f98e1af`); branches `t-020-syscall-error-taxonomy` / `t-021-syscall-dispatch` retired. +- **Working branch:** none active (Phase C is not yet scoped into tasks). *Historical:* `b6-closure` (off `main`; the Phase B closure quartet, [PR #43](https://github.com/HodeTech/Tyrne/pull/43)) is retired. *(B6 task branches `t-022`…`t-028` retired after their merges, PRs #36–#42.)* Earlier: `t-024-task-create-from-image` (off `main` at `eb3125f`, the merged T-023 [PR #37](https://github.com/HodeTech/Tyrne/pull/37)). The prior B6-opening arc on `b6-el0-entry-context` (the [T-022 security review](../analysis/reviews/security-reviews/2026-05-31-T-022-high-half-migration.md) `9a51b73` + [ADR-0037](../decisions/0037-el0-entry-context.md) Propose `00d342e` / Accept `546febc` + T-023) **merged to `main` 2026-05-31 via [PR #37](https://github.com/HodeTech/Tyrne/pull/37)** (`eb3125f`); that branch + `t-022-high-half-kernel-mapping` ([PR #36](https://github.com/HodeTech/Tyrne/pull/36) `b6549d7`) retired. *(The earlier `sec-review-b5-syscall-boundary` branch carried the B5 security review + closure trio and is retired.)* **[PR #34](https://github.com/HodeTech/Tyrne/pull/34)** (T-020 + T-021, 9 commits) **merged to `main` 2026-05-29** (`f98e1af`); branches `t-020-syscall-error-taxonomy` / `t-021-syscall-dispatch` retired. - **Last completed milestone:** **B6 β€” First userspace "hello", Closed 2026-06-01** via its closure quartet β€” the guide, the [perf baseline](../analysis/reviews/performance-optimization-reviews/2026-06-01-B6-closure.md), the [T-028 EL0-boundary](../analysis/reviews/security-reviews/2026-06-01-T-028-el0-userspace-wireup.md) + [consolidated Phase-B](../analysis/reviews/security-reviews/2026-06-01-phase-b-closure.md) security reviews, and the [retrospective](../analysis/reviews/business-reviews/2026-06-01-phase-b-closure.md); functional spine T-022…T-028 (PRs #36–#42). **Earlier: B5 β€” Syscall boundary, Closed 2026-05-29** via the closure trio ([security](../analysis/reviews/security-reviews/2026-05-29-B5-syscall-boundary.md) Approve + [business](../analysis/reviews/business-reviews/2026-05-29-B5-closure.md) + [performance](../analysis/reviews/performance-optimization-reviews/2026-05-29-B5-closure.md)). Tasks Done: T-020 + T-021 (PR #34 `f98e1af`). The trio is the **canonical source for B5's closing metrics**; headline: **339** host tests (incl. local Miri 0 UB), QEMU smoke clean (712 release / 776 debug guest-errors, all pre-existing PL011, + 2 expected `SVC` exceptions, zero new fault class), release perf band 17.645 / 20.300 / 24.706 ms (same-host control proves a real ~+2.9 ms one-time boot `SVC`-smoke cost, not host jitter), audit log 30 entries (29 Active). **Previous closures:** **B4** 2026-05-28 (closure trio; T-019 PR #31 `7f876af`); **B3** 2026-05-14 (PR #29 `b425dc1`); **B2** 2026-05-09; **B1** 2026-05-07 (PR #15 `e9fa019` + PR #16 `95b15aa`); **B0** 2026-04-27 (PR #9 `9a66e8b`). - **Last completed task:** **T-028 β€” EL0 userspace wire-up, Done** (merged [PR #42](https://github.com/HodeTech/Tyrne/pull/42) `0040f23`) β€” Tyrne's first real EL0 task end-to-end + `task_exit_current` termination. **Earlier in B6:** T-027 build pipeline ([PR #41](https://github.com/HodeTech/Tyrne/pull/41)), T-026 gate #3 ([PR #40](https://github.com/HodeTech/Tyrne/pull/40)), T-025 gate #1 ([PR #39](https://github.com/HodeTech/Tyrne/pull/39)), T-024 `task_create_from_image` ([PR #38](https://github.com/HodeTech/Tyrne/pull/38)), T-023 EL0-entry ([PR #37](https://github.com/HodeTech/Tyrne/pull/37)), T-022 high-half ([PR #36](https://github.com/HodeTech/Tyrne/pull/36)). **Earlier (B5): T-020 + T-021 β€” Done 2026-05-29 via PR #34 (`f98e1af`)** β€” the B5 syscall boundary: T-020 (the `IpcError::InvalidCapability` β†’ `StaleHandle`/`WrongObjectKind`/`MissingRight` split + `Capability`/`CapObject` `Debug` redaction, zero new `unsafe`) + T-021 (the architecture-agnostic kernel `syscall` module β€” `SyscallError` / ABI / `UserAccessWindow` / dispatcher β€” + the BSP `tyrne_sync_trampoline` at `+0x200`/`+0x400` + `CapObject::DebugConsole` + `CapRights::CONSOLE_WRITE` + `CapHandle::from_raw`; two audit entries UNSAFE-2026-0029 / 0030; the real EL0 `+0x400` round-trip deferred to B6). **Earlier:** **T-019 β€” Done 2026-05-16, merged to `main` via PR #31** (branch `t-019-task-loader`, merge commit `7f876af`) β€” Task loader: `load_image` produces a `LoadedImage` descriptor of a freshly populated userspace AS (10-variant `LoadError`, leak-path-closure preflight chain, UNSAFE-2026-0027 byte-copy entry); does **not** mint a runnable `TaskCap` (B5/B6 prerequisite). **Earlier:** **T-018 β€” Done 2026-05-11, live on `main` 2026-05-14 via PR #28** (branch `t-018-address-space-kernel-object`, merge commit `47b0a86`). T-018 implementation: [`AddressSpace`](../../kernel/src/mm/address_space.rs) kernel-object struct + per-type [`AddressSpaceArena`](../../kernel/src/mm/address_space.rs) (ADR-0016 pattern); `CapKind::AddressSpace` + `CapObject::AddressSpace(AddressSpaceHandle)` variants in [`kernel/src/cap/mod.rs`](../../kernel/src/cap/mod.rs); capability-gated wrappers `cap_create_address_space` / `cap_map` / `cap_unmap` with step-by-step preflights (DERIVE rights β†’ no-widening β†’ depth preflight β†’ arena/cap-table capacity β†’ PMM alloc β†’ arena commit β†’ `cap_derive` cap-table insert); `Task` struct extension with `address_space_handle`; activation-on-context-switch hook threaded through `yield_now` / `start` / `ipc_recv_and_yield` / `ipc_send_and_yield` (closure-as-parameter, fires only when outgoing and incoming task ASes differ β€” short-circuits in v1's bootstrap-shared topology); BSP wiring in [`bsp-qemu-virt/src/main.rs`](../../bsp-qemu-virt/src/main.rs) wraps the already-live bootstrap root via the new `QemuVirtAddressSpace::from_existing_root` `pub unsafe fn` companion. Cross-cutting additions during the review-round arc: `MmuError::BlockMapped` variant (commit `8b9f52e`) so unmap into a bootstrap block descriptor surfaces a distinct typed error from `AlreadyMapped`; `CapabilityTable::depth_of` `pub(crate)` preflight helper closing the PMM-leak path; UNSAFE-2026-0014 fifth Amendment scope-extends the umbrella to the activation hook + BSP-side activation closure (zero new audit entries β€” additive scope on the existing `&mut Scheduler` momentary-borrow umbrella). Smoke trace gains one new line `tyrne: address-space-arena ready (1 / 8 slots used; bootstrap AS root = 0x4008d000)` immediately after `tyrne: pmm initialized (...)` and before `tyrne: timer ready (...)`. Full demo runs to `tyrne: all tasks complete`; `-d int,unimp,guest_errors` reports only the pre-existing PL011-disabled-UART noise (unchanged baseline). **Earlier:** T-017 β€” Done 2026-05-10 (PR #27, branch `t-017-physical-memory-manager`) β€” Physical Memory Manager (`Pmm` bitmap allocator + `FrameProvider` trait + UNSAFE-2026-0026 zero-fill audit). **Earlier:** T-016 β€” Done 2026-05-08 (branch `t-016-mmu-activation`) β€” MMU activation, VMSAv8 descriptor encoders, `MapperFlush` flush-token, UNSAFE-2026-0022 / 0023 / 0024 / 0025 introduced. **Earlier:** T-015 β€” Done 2026-05-07 (PR #17, branch `t-015-endpoint-rollback-cancel-recv`) β€” `ipc_cancel_recv` recovery primitive + symmetric scheduler+endpoint rollback in `ipc_recv_and_yield`'s Phase 2 Deadlock branch (ADR-0032). **Earlier:** T-014 (2026-05-07 via PR #15), T-012 (2026-04-28 via PR #10), T-013 (2026-04-27 via PR #9). - **Last reviews:** diff --git a/docs/roadmap/phases/README.md b/docs/roadmap/phases/README.md index 02f33b5..c72e8d6 100644 --- a/docs/roadmap/phases/README.md +++ b/docs/roadmap/phases/README.md @@ -1,6 +1,6 @@ # Phase plan -Ten phases, rough execution order. Each phase is a separate file so detail can grow without any single document becoming unnavigable. +Ten phases plus a short consolidation bridge ([Phase B.2](phase-b2.md)), in rough execution order. Each phase is a separate file so detail can grow without any single document becoming unnavigable. The plan is a **sequence**, not a schedule: there are no dates. Later phases are sketched at lower resolution than earlier ones; detail appears as its turn approaches. The *why* behind the whole system lives in [ADR-0013](../../decisions/0013-roadmap-and-planning.md). @@ -10,6 +10,7 @@ The plan is a **sequence**, not a schedule: there are no dates. Later phases are |-------|-------|--------------|----------| | [A](phase-a.md) | Kernel core on QEMU `virt` | Detailed | Two kernel tasks exchange IPC messages under capability control on QEMU. | | [B](phase-b.md) | Real userspace | Detailed | A userspace task runs in its own address space and makes syscalls. | +| [B.2](phase-b2.md) | Consolidation & hardening (bridge) | Detailed | CI enforces every claimed gate; docs reflect Phase B's closure; the review-surfaced correctness/robustness/quality defects are fixed. Recommended before Phase C. | | [C](phase-c.md) | Multi-core | Detailed | Preemptive scheduler across two or more cores with working cross-core IPC. | | [D](phase-d.md) | Raspberry Pi 4 (first real hardware) | Detailed | `bsp-pi4` boots on a real Pi 4 at feature parity with QEMU virt. | | [E](phase-e.md) | Driver model and essential services | Medium | Userspace drivers + log + storage + filesystem + network services composed. | @@ -24,8 +25,9 @@ The plan is a **sequence**, not a schedule: there are no dates. Later phases are ```mermaid flowchart LR A[Phase A
kernel core] --> B[Phase B
real userspace] - B --> C[Phase C
multi-core] - B --> D[Phase D
Pi 4] + B --> B2[Phase B.2
consolidation & hardening] + B2 --> C[Phase C
multi-core] + B2 --> D[Phase D
Pi 4] C --> D D --> E[Phase E
drivers + services] E --> F[Phase F
smart home] diff --git a/docs/roadmap/phases/phase-b2.md b/docs/roadmap/phases/phase-b2.md new file mode 100644 index 0000000..5f8194d --- /dev/null +++ b/docs/roadmap/phases/phase-b2.md @@ -0,0 +1,677 @@ +# Phase B.2 β€” Consolidation & Hardening + +Phase B (B0–B6) closed 2026-06-01, and Phase C (multi-core) is the next planned body of feature work. Before Phase C's scheduler and address-space code grows a second, concurrent dimension, this short bridge phase pays down the debt surfaced by the 2026-07-15 full-repository review: CI gaps that let real defects (an inline-asm register-clobber UB bug, a release-profile security gate that has never once been exercised, a host/target confusion that breaks builds on aarch64 dev machines) go undetected; a documentation tree that in dozens of places still describes Phase B as unfinished, describes the current syscall boundary as "dormant," or has simply drifted out of sync with the task/review corpus it is supposed to index; and a long tail of correctness, robustness, code-quality, and unsafe-audit findings that are cheap to fix now and materially riskier to leave lying around once Phase C's concurrency work starts touching the same files. None of this is multi-core work. The point of Phase B.2 is narrow and mechanical: make CI actually enforce what the project's own standards claim it enforces, make the documentation tell the truth about the project's current state, and fix the concrete defects the review found β€” so that Phase C starts on a base whose gates are real and whose docs can be trusted. + +**Exit bar:** CI enforces every gate the project's own standards docs claim it enforces; the documentation tree accurately reflects Phase B's 2026-06-01 closure and Phase C's start; and the feature-independent correctness, robustness, and quality defects surfaced by the 2026-07-15 full-repository review are fixed. + +**Scope:** Near-term, feature-independent hardening β€” CI/merge-gate wiring, documentation-cascade correction, and the correctness/robustness/quality/unsafe-audit/performance findings from the review that do not require new kernel features or new syscall surface. + +**Out of scope:** +- **Multi-core work of any kind** β€” secondary-core start, per-core state, preemptive scheduling, cross-core IPC, TLB shootdown. That is Phase C, and nothing in this phase implements it (some findings below merely note where a fix *gates* a Phase C milestone's acceptance criteria). +- **Real-hardware / aarch64 register-and-barrier hygiene** for a non-QEMU target. That is Phase D (Raspberry Pi 4). +- **Capability enforcement that requires new syscall wiring** (e.g. a new `MemoryRegionCap`-driven device-mapping syscall, driver-facing capabilities). That is Phase E (first real driver). +- **Test-HAL / HAL abstraction parity for new BSPs.** That is Phase H. + +This phase's findings and remediation actions are drawn from the 2026-07-15 full-repository review β€” an internal review pass, not itself a document published anywhere in this tree. Every item below stands on its own file:line location in the shipped tree, so remediation does not depend on that source being available. + +To avoid colliding with Phase B's B0–B6 milestone IDs, the work below is organized as **Track B.2-1** through **Track B.2-6** rather than numbered milestones. + +--- + +## Track B.2-1 β€” CI enforcement & merge gates + +The project's standards docs (`infrastructure.md`, `code-review.md`, `error-handling.md`) describe a CI posture that is, in several concrete and security-relevant ways, more thorough than what `.github/workflows/ci.yml` actually runs. This track closes that gap: every claimed gate either becomes a real, merge-blocking CI check, or the claim is corrected to match reality (the documentation half of that correction is tracked under Track B.2-2 where it doesn't require a CI change). + +### Sub-breakdown + +- **[🟠 HIGH]** The security-relevant release-profile contract (`console_write` debug-gate) is never built or tested by CI β€” `.cargo/config.toml:29-37` (kernel-build alias); `.github/workflows/ci.yml:96-101,140-143` + **Action:** Add a CI step (or job) that runs `cargo build --release --target aarch64-unknown-none -p tyrne-bsp-qemu-virt` and `cargo test --release -p tyrne-kernel` (or workspace-wide `--release`) so the `not(debug_assertions)` code and its pinning tests are exercised on every PR. Mirror `kernel-build`/`kernel-run` with `kernel-build-release`/`kernel-run-release` aliases in `.cargo/config.toml`. + +- **[🟠 HIGH]** Userland crates carrying the raw unsafe syscall-trap asm are never clippy-checked in CI, on any target β€” `Cargo.toml:23-27` (default-members); `.cargo/config.toml:32-34,44-45` (kernel-clippy/host-clippy aliases); `bsp-qemu-virt/Cargo.toml:15-17`; `tools/build-userland.sh:30-34`; `userland/tyrne-user/src/lib.rs:92-141` + **Action:** Add `cargo clippy -p tyrne-user -p tyrne-userland-hello --target aarch64-unknown-none -- -D warnings` alongside `kernel-clippy` in the kernel-build job (after `build-userland.sh`, reusing the aarch64 toolchain already installed there); update `docs/guides/ci.md`'s job table. + +- **[🟠 HIGH]** `tools/smoke.sh` β€” the only automated check of actual kernel boot/IPC/syscall-boundary behavior β€” is never invoked by CI β€” `.github/workflows/ci.yml` (entire jobs block, lines 65-277 β€” no job references `tools/smoke.sh`); `docs/guides/ci.md:55`; `docs/standards/infrastructure.md:82` + **Action:** Add a `qemu-smoke` job: install `qemu-system-aarch64` (`apt-get install qemu-system-arm` on `ubuntu-latest`), run `tools/build-userland.sh && cargo kernel-build && tools/smoke.sh`, gate merge on its exit code. File a fresh, explicitly Phase-B.2-scoped task rather than relying on the stale T-009 cross-reference. This gates Milestone C1's own "QEMU run with `-smp 4` brings all four cores to a known checkpoint" acceptance criterion β€” the smoke harness needs to already be a real, exercised CI gate before Phase C tasks extend it to multi-core. + +- **[🟠 HIGH]** The release debug-gate's regression test never executes in CI; a broken gate would go undetected β€” `kernel/src/syscall/abi.rs:377-383` (`console_write_is_absent_in_release_builds`); `kernel/src/syscall/abi.rs:87` (the `cfg!(debug_assertions)` guard it verifies); `.github/workflows/ci.yml:100-101,188-189,224` + **Action:** Add a dedicated CI job/step running `cargo test --release --lib -p tyrne-kernel -- console_write_is_absent_in_release_builds` (narrow filter, keeps it fast). Add a comment next to `[profile.release]` in Cargo.toml (mirroring the existing overflow-checks comment) stating explicitly that `debug-assertions` must stay at its Cargo default (false) in release because `SyscallNumber::decode` gates `console_write` on it. + +- **[🟠 HIGH]** QEMU smoke tests (the only test layer exercising BSP hardware-facing code) are not wired into CI, right as Phase C (concurrency) begins β€” `.github/workflows/ci.yml` (no smoke job); `tools/smoke.sh` (exists, unused by CI); `docs/roadmap/phases/phase-b.md:291` (flag K3-7) + **Action:** Same remedy as the `smoke.sh`-in-CI finding above β€” this is the cross-cut instance of the identical gap, independently surfaced. Land the `qemu-smoke` job before any SMP-touching PR lands so the first concurrency regression is caught by CI, not by a human running `smoke.sh` manually. + +- **[🟑 MEDIUM]** CI never builds, lints, or smoke-tests the `--release` profile β€” only `--debug` is ever exercised β€” `.github/workflows/ci.yml:135-143` + **Action:** Add a release-profile build (ideally + kernel-clippy + smoke) step to the kernel-build job, or a matrix axis `profile: [debug, release]`, so fat-LTO-specific codegen issues surface in CI. At minimum, record the gap explicitly in `infrastructure.md`'s "Planned gates" section until it lands. + +- **[🟑 MEDIUM]** `clippy::unwrap_used`/`expect_used`/`panic` are denied only in `tyrne-kernel`, not in `tyrne-hal` or `tyrne-bsp-qemu-virt` β€” the standard's own "Tooling" claim is false for two of the three in-scope crates β€” `hal/src/lib.rs:1-51` (no `#![deny(...)]`); `bsp-qemu-virt/src/main.rs:22-40` (no `#![deny(...)]`); contrast `kernel/src/lib.rs:62-66`; `docs/standards/error-handling.md:60` + **Action:** Add `#![deny(clippy::panic)]`, `#![deny(clippy::unwrap_used)]`, `#![deny(clippy::expect_used)]` to `hal/src/lib.rs` and `bsp-qemu-virt/src/main.rs` (with scoped `#[allow(...)]` on legitimate one-shot init-path call sites, matching the pattern kernel's `#[cfg(test)]` blocks already use). Preferable to narrowing the standard's wording, given `bsp-qemu-virt/src/syscall.rs` and `exceptions.rs` are now live hostile-input surfaces post-Phase-B6. + +- **[βšͺ LOW]** The `perf-bench` feature (542 lines, alters `main.rs` control flow) is never compiled by CI β€” `bsp-qemu-virt/Cargo.toml:19-24` + **Action:** Add `cargo build --target aarch64-unknown-none -p tyrne-bsp-qemu-virt --features perf-bench` (and a matching clippy pass) as a CI step in the `kernel-build` job, or at minimum add an entry to `infrastructure.md`'s "Planned gates" section. + +- **[βšͺ LOW]** `tools/perf-harness.sh` is likewise never invoked by CI β€” no automated perf-regression signal β€” `.github/workflows/ci.yml` (no job references it); `docs/standards/infrastructure.md:114` + **Action:** Lower priority than the smoke-test gate; consider a scheduled (nightly/weekly) `workflow_dispatch`/cron job running `tools/perf-harness.sh --iterations=20 --report=ci-nightly` and archiving the report as a build artifact, once the smoke job lands. + +- **[βšͺ LOW]** No shellcheck (or any shell linter) runs in CI, despite the scripts already anticipating it β€” `.github/workflows/ci.yml` (no shellcheck step); `tools/smoke.sh:53,55` + **Action:** Add a cheap `shellcheck tools/*.sh` step to the fast lane (or a dedicated job); `ubuntu-latest` ships shellcheck preinstalled. + +- **[βšͺ LOW]** QEMU wrapper tooling hardcodes `-smp 1` with no override, ahead of Phase C multi-core work β€” `tools/run-qemu.sh:92`; `tools/smoke.sh:73` + **Action:** Add a `--smp N` (default 1) passthrough to `run-qemu.sh` and `smoke.sh` now, before Phase C tasks start depending on these scripts β€” this directly gates Milestone C1's `-smp 4` acceptance criterion; cheaper to do as an isolated tooling change now than to retrofit under Phase C time pressure. + +- **[⚫ INFO]** "What gates merge" is documented only in comments, not verifiable as code β€” `.github/workflows/ci.yml:7-9`; `docs/guides/ci.md:85-91` + **Action:** Low priority given repo size, but worth tracking: GitHub repository rulesets (`gh api repos/{owner}/{repo}/rulesets`) or Terraform (`github_branch_protection`/`github_repository_ruleset`) would make merge-gate configuration reviewable and diffable like any other infra change instead of trusted-by-comment. + +### Acceptance criteria + +- `cargo build --release` + `cargo test --release` for the kernel and BSP crates run on every PR, including the `console_write_is_absent_in_release_builds` regression. +- `tyrne-user` and `tyrne-userland-hello` are clippy-checked in CI on the `aarch64-unknown-none` target. +- A `qemu-smoke` job runs `tools/smoke.sh` on every PR and gates merge on its exit code. +- `hal` and `bsp-qemu-virt` carry the same `clippy::panic`/`unwrap_used`/`expect_used` denies `kernel` already has. +- `tools/run-qemu.sh` and `tools/smoke.sh` accept an `--smp N` override, unblocking Milestone C1's own smoke acceptance criterion. +- The remaining low/info items (perf-bench build, perf-harness.sh scheduling, shellcheck, "what gates merge" as code) are tracked even if not all land within B.2. + +--- + +## Track B.2-2 β€” Documentation cascade (Phase-B closure) + +Phase B closed 2026-06-01, but the closure was never cascaded through the full documentation tree: the entry-point architecture doc, several subsystem chapters, the task/review indices, nine task files' own frontmatter, CONTRIBUTING.md/SECURITY.md, several ADRs, and a long tail of standards docs and cross-references all still describe an earlier project state β€” in a few places (SECURITY.md's "not yet a userspace-bearing OS") the drift has become an outright false statement about the shipped security boundary. This track is the closure quartet's missed follow-through, run once as a batch. + +### Sub-breakdown + +**Entry-point & core architecture docs** + +- **[🟠 HIGH]** overview.md β€” the designated entry-point document β€” describes the system as stuck at end-of-Phase-A, but Phase B is fully closed and Phase C is active β€” `docs/architecture/overview.md:5` + **Action:** Rewrite the status banner to reflect Phase B closure and Phase C's start; thread ADR-0027..0039 into the relevant sections (Address spaces, Syscall surface, Boot flow). Add this doc to whatever checklist/skill governs phase closure so it is updated as part of every phase's closure quartet, not left behind. + +- **[🟠 HIGH]** task-loader.md's central claims are now false: the loader is described as producing a non-runnable descriptor loading an unexecuted placeholder blob, but the code runs a real EL0 task end-to-end β€” `docs/architecture/task-loader.md:13-21,92-103` + **Action:** Rewrite the "Scope boundary" and "Embedded image content" sections to describe the landed state: `load_image` β†’ `task_create_from_image` (T-024) β†’ `add_user_task` (T-023) β†’ scheduled EL0 execution (T-028), with the real `hello` image and ADR-0039's build pipeline. Retire or clearly historically-mark the 8-byte placeholder code sample. + +- **[🟑 MEDIUM]** hal.md, scheduler.md and exceptions.md describe the EL0 entry-context mechanism and the `+0x400` syscall path as "dormant"/only "runtime-verified in B6", but B6 has landed and the path is live, tested, and security-reviewed β€” `docs/architecture/hal.md:94`; `docs/architecture/scheduler.md:73,90`; `docs/architecture/exceptions.md:58,105,133` + **Action:** Update all three documents to say the mechanism is live (not dormant), cite T-024/T-025/T-026/T-028 and ADR-0039, and change exceptions.md's "exercised at runtime in B6" to past tense with the T-028 QEMU-smoke evidence. + +- **[🟑 MEDIUM]** security-model.md's capability-type table omits DebugConsole β€” the one capability kind that is actually implemented and gates the only currently-live, attacker-reachable privileged operation β€” while listing several kinds that do not exist in the code β€” `docs/architecture/security-model.md:132-143` + **Action:** Split the table into "Implemented (v1)" (Task, Endpoint, Notification, AddressSpace, DebugConsole β€” with DebugConsole's CONSOLE_WRITE right) and "Planned" (MemoryRegionCap, IrqCap, TimerCap, etc.); cross-link ADR-0031. + +- **[🟑 MEDIUM]** overview.md's "Syscall surface" enumeration does not match the Accepted, implemented syscall ABI and never cites ADR-0030 / ADR-0031 β€” `docs/architecture/overview.md:194-206` + **Action:** Either update the list to the real v1 ABI and cite ADR-0030/ADR-0031, or explicitly relabel the section "Syscall surface (design-time speculation, superseded by ADR-0030/ADR-0031 β€” see exceptions.md)". + +- **[🟑 MEDIUM]** docs/architecture/README.md's per-document Status column is not kept in sync with the linked documents' own content β€” `docs/architecture/README.md:11-23` + **Action:** Update each Status cell to name the latest task/ADR that touched the document (e.g. task-loader.md β†’ "T-019, T-024, T-028"); fold into the same phase-closure checklist that fixes the three findings above. + +- **[🟑 MEDIUM]** memory-management.md's "Frame allocation discipline" section frames the high-half PA-to-VA helper as a future need, but it already landed and is in active use β€” `docs/architecture/memory-management.md:206` + **Action:** State (past tense) that the high-half migration landed via T-022/ADR-0033 and that `phys_to_kernel_va`/`phys_frame_kernel_ptr` is the realised helper, cross-linking the UNSAFE-2026-0026/0027 Amendments the document already cites elsewhere. + +- **[🟑 MEDIUM]** docs/architecture/boot.md is stale relative to the current main.rs boot flow (missing banner still shown as present; EL0 execution now real but documented as not-yet-executed) β€” `docs/architecture/boot.md:19,76-77` (vs. `bsp-qemu-virt/src/main.rs` β€” no `tyrne: image loaded` banner exists anywhere in the file; EL0 task setup at main.rs:1438-1628) + **Action:** Update boot.md's stage-3 description and Mermaid sequence diagram: drop the removed banner line, correct the "does NOT execute" claim, and add the EL0-task bring-up steps (AS/Task cap resolution, USER_TASK_TABLE console-cap seeding, `add_user_task`, FAILCLOSED_TABLE publish) as their own documented stage. + +- **[βšͺ LOW]** boot.md's boot-time memory map is an untagged, ASCII-art diagram β€” violates two explicit documentation-style rules simultaneously β€” `docs/architecture/boot.md:89-101` + **Action:** Convert to a Mermaid diagram (e.g. `block-beta` or a vertically-stacked `flowchart TB`), or at minimum tag the fence ```text``` and drop the box-drawing connectors in favour of a plain table. + +- **[βšͺ LOW]** ipc.md links an in-repo file with an absolute GitHub blob/main URL instead of a relative path β€” `docs/architecture/ipc.md:65` + **Action:** Change to a relative path: `[cancel-recv]: ../../kernel/src/ipc/mod.rs`. + +- **[⚫ INFO]** ipc.md's file-size claim ("~1425-line file") has drifted ~20% out of date β€” `docs/architecture/ipc.md:14` + **Action:** Either drop the specific line count or update it; the underlying architectural claim is unaffected. + +- **[βšͺ LOW]** docs/architecture/README.md's "Status" prose directly contradicts its own Index table two paragraphs below β€” `docs/architecture/README.md:7` + **Action:** Replace with reality-matching prose, e.g. "Most subsystem chapters are written and Accepted; `drivers.md` and `userspace.md` remain planned pending the corresponding Phase E/driver-model ADRs." + +**Roadmap & task/review index synchronization** + +- **[🟠 HIGH]** current.md's live-state bullets are stale relative to the repository's actual HEAD β€” the most recently completed work (T-029) is entirely unreflected β€” `docs/roadmap/current.md:76-81` + **Action:** Mark PR #43 and the b6-closure branch merged/retired, set "Last completed task" to T-029 (PR #44/#45), add a fresh dated banner recording T-029's Phase 1 + Phase 2 completion. Recurrence of Blocker D4-003 from the 2026-05-22 master review. + +- **[🟠 HIGH]** docs/analysis/tasks/phase-b/README.md task index is missing 8 of the phase's 23 tasks (T-022 through T-029) and shows wrong statuses for the two it does list β€” `docs/analysis/tasks/phase-b/README.md:22-23` + **Action:** Add index rows for T-022–T-029 with correct milestones/statuses; correct the T-020/T-021 rows to "Done". Recurrence of Blocker D4-005 from the 2026-05-22 master review. + +- **[🟑 MEDIUM]** The same task-index gap, restated with the milestone/status detail β€” all 8 missing tasks delivered the phase's closing milestone (B6) β€” `docs/analysis/tasks/phase-b/README.md:9-23` + **Action:** Same fix as the finding above (near-duplicate finding from a separate review pass, both listed for completeness); additionally note each missing task's own frontmatter still reads "In Review" (see the frontmatter-status finding below) as a related-but-separate defect. Run an `ls`-vs-table reconciliation as a periodic check, or fold into a future `sync-adr-index`-style skill for task indices. + +- **[🟠 HIGH]** Nine task files' frontmatter Status field is stuck at "In Review" (or an internally self-contradictory Status line) despite the roadmap declaring them Done/merged with commit hashes and dates β€” `docs/analysis/tasks/phase-b/T-020-syscall-error-taxonomy.md:5` (and identically T-021 through T-029) + **Action:** Flip the Status field to Done (with a `date_done` entry, following the T-019 precedent) for every task current.md/phase-b.md already treats as merged and closed; add a merge-confirmation row to each task's Review history. Recurrence of Major D4-004 from the 2026-05-22 master review. + +- **[🟑 MEDIUM]** Two of the six forward-flagged items from the 2026-06-01 Phase-B-closure security seam review β€” the ones most relevant to Phase C's own scope β€” are absent from every carry-forward tracking artifact β€” `docs/roadmap/phases/phase-c.md:126-132` (Β§Carry-forwards from Phase B); business/security 2026-06-01 phase-b-closure review artifacts (Β§Adjustments / source of the forward-flags) + **Action:** Add explicit bullets for gate-#3 context-resolution atomicity and gate-#1 loader-trust to phase-c.md's Β§Carry-forwards, cross-referenced to Milestones C2/C3 (the milestones that make them live); add an SP_EL1 high-water-mark bullet distinct from the already-tracked IRQ/TTBR1 item. + +- **[🟑 MEDIUM]** The business-, security-, and performance-optimization-review index READMEs are missing their most recent (2026-05-31/2026-06-01, Phase-B-closing) entries, including the T-028 EL0-boundary and consolidated Phase-B security reviews β€” `docs/analysis/reviews/business-reviews/README.md:38`; `docs/analysis/reviews/security-reviews/README.md:41`; `docs/analysis/reviews/performance-optimization-reviews/README.md:31` + **Action:** Add the missing rows to all three index tables, following each table's existing format (Date | Scope | Verdict | File). Consider a lightweight CI check or a `conduct-review` skill acceptance criterion that greps the directory listing against the README table. + +- **[🟑 MEDIUM]** Three of four review-family README indices were never updated for the Phase-B-closure batch β€” the project's most important reviews to date are undiscoverable via the documented index β€” `docs/analysis/reviews/security-reviews/README.md:41`; `docs/analysis/reviews/business-reviews/README.md:38`; `docs/analysis/reviews/performance-optimization-reviews/README.md:31` + **Action:** Same fix as the finding above (near-duplicate finding, both listed for completeness); should become a checked acceptance-criterion of the closure-trio workflow. + +- **[🟑 MEDIUM]** docs/guides/README.md is stale on both its status prose and its own guide inventory: it calls the project "the architecture phase", uses an orphaned pre-lettered-phase numbering scheme, and omits 2 of the 4 guide files that actually exist β€” including the guide for the project's flagship EL0-userspace milestone β€” `docs/guides/README.md:7,12-19` + **Action:** Rewrite the Status paragraph to match README.md's framing. Add rows for `two-task-demo.md` and `first-userspace.md` with status "Accepted". Replace "Planned β€” Phase 2/3/4" with the lettered-phase scheme or drop the tag. Add a `first-userspace.md` link to README.md's "Documentation map" Β§"Reader who wants to do something concrete" list. + +- **[βšͺ LOW]** Phase-plan README's living-document note still claims Phase A is the active phase, contradicted by the project's own current-focus banner β€” `docs/roadmap/phases/README.md:51` + **Action:** Update the parenthetical to name the actually-active phase, or better, drop the specific phase name from the illustrative aside entirely ("the currently active phase carries the most detail; later phases are sketches") so this sentence doesn't need editing every phase transition. + +- **[βšͺ LOW]** The most recent performance review (B6 closure) deviates from the master-plan's mandated six-section template and its own acceptance-criteria checklist silently drops the "Index in README.md updated" item β€” which was in fact not done β€” `docs/analysis/reviews/performance-optimization-reviews/2026-06-01-B6-closure.md:88-93`; master-plan.md (Β§Acceptance criteria) + **Action:** Add a "Regression check" section (host-test pass, smoke pass, unsafe-diff already exist elsewhere and just need restating) plus the README index row, or add a one-line note explaining the abbreviated shape is an intentional exemption (mirroring how B2–B5 handled baseline-only cycles). + +- **[βšͺ LOW]** The top-level reviews index never mentions the master-review family, which produced the corpus's largest and most consequential single artifact β€” `docs/analysis/reviews/README.md:7-14` ("The four types" table) + **Action:** Add a fifth row (or a short "Master reviews" subsection linking to master-review/README.md), noting the trigger ("on-demand, whole-tree sweep") that document already documents. + +- **[βšͺ LOW]** Coverage and Miri validation reports are frozen at 2026-04-23/27 (pre-T-016) despite roughly a dozen unsafe-touching tasks (T-016 through T-029) landing since β€” `docs/analysis/reports/2026-04-27-coverage-rerun.md`; `docs/analysis/reports/2026-04-23-miri-validation.md` + **Action:** Already correctly triaged as Nit-severity in 2026-05-22 and remains low-urgency (administrative snapshot gap, not correctness β€” Miri/coverage are re-run and reported inline at every milestone closure). If the dedicated-report convention should stay alive, a single fresh report at the next natural checkpoint (Phase C open, or first Phase-C milestone close) closes it cheaply. + +- **[βšͺ LOW]** Dead cross-references to the pre-migration `.claude/skills/` path in at least 10 review-archive files β€” `docs/analysis/reviews/business-reviews/2026-05-09-B2-closure.md:26`; `docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-g-process.md` + **Action:** Either leave as an acknowledged historical-snapshot artifact with a one-line note added to `docs/analysis/reviews/README.md` warning that pre-2026-05-14 review links into `.claude/skills/` are stale, or do a mechanical `.claude/skills/` β†’ `.agents/skills/` string replace across the archive (low-risk pure path rename). + +- **[⚫ INFO]** T-029's frontmatter omits the required Milestone field β€” `docs/analysis/tasks/phase-b/T-029-perf-microbench.md:3-6` + **Action:** Add a Milestone line (e.g. "B6 follow-on / Phase B closure tail β€” not gated on a numbered B-milestone"). + +- **[🟑 MEDIUM]** README's literal Quick-Start boot trace shows a console line the kernel no longer prints β€” `README.md:95` (vs. `bsp-qemu-virt/src/main.rs:1318-1335`) + **Action:** Remove the `tyrne: image loaded (...)` line from README's Quick-Start trace, or add a real print of the loaded-image parameters at the `task_create_from_image`/`add_user_task` call site and keep the README line honest. Prefer eliding fragile trace segments with `...`, as `first-userspace.md` already does. + +- **[🟑 MEDIUM]** docs/roadmap/current.md and T-029's own task file are stale relative to the actual (already-merged) git history β€” T-029 is complete but shown as active/Draft/In Review β€” `docs/roadmap/current.md:7,76,81` (vs. `docs/analysis/tasks/phase-b/T-029-perf-microbench.md:4`) + **Action:** Flip T-029's Status line to Done (Phase 1 + Phase 2 both merged); add a closing roadmap banner recording T-029's completion, updating "Active task"/"Last completed task"/"Working branch" accordingly. + +- **[βšͺ LOW]** README's "small audited set" enumeration of kernel-crate unsafe omits the security-critical copy-from/to-user unsafe operation β€” `README.md:56` (vs. `docs/audits/unsafe-log.md:676` and `kernel/src/syscall/user_access.rs:243,311`) + **Action:** Extend the sentence to name the copy-from/to-user unsafe explicitly (arguably the one most worth naming, given it is the syscall-boundary trust gate), or reword from an implicitly-exhaustive list to an explicitly illustrative one. + +- **[βšͺ LOW]** docs/roadmap/current.md line 77 self-contradicts line 74 on PR #43's merge status β€” `docs/roadmap/current.md:77` (vs. `:74`) + **Action:** Update the "In review" bullet to "none" (or list whatever is actually in review post-T-029); fold PR #43 into the "Last completed milestone" framing already present at line 81. + +**Standards docs** + +- **[🟠 HIGH]** CONTRIBUTING.md and SECURITY.md describe a project state roughly two milestones and a whole phase behind reality; SECURITY.md's core factual claim is now false β€” `CONTRIBUTING.md:3,14`; `SECURITY.md:7` + **Action:** Update both files' opening status paragraphs to match README.md's "Status at a glance"/CLAUDE.md's project-state paragraph: Phase B (B0–B6) closed 2026-06-01; Phase C active; first EL0 userspace task running through the syscall boundary. In SECURITY.md specifically, remove or correct "not yet a userspace-bearing OS" β€” that claim is now the opposite of true, and the syscall/capability boundary is exactly the surface SECURITY.md exists to describe accurately. Also update the trailing "refined as Phase B progresses" (SECURITY.md:7) since Phase B is closed. + +- **[🟑 MEDIUM]** testing.md's mandated test-naming convention has 0% adoption in the actual codebase β€” `docs/standards/testing.md:63-77` + **Action:** Either update testing.md to document the convention actually in use (`__`, without a `test_` prefix, which real names already follow reasonably well), or do a mechanical rename sweep and add a clippy/CI check flagging new `#[test]` fns lacking the prefix. + +- **[🟑 MEDIUM]** error-handling.md claims the kernel panic handler "dumps register state"; the actual handler only prints the `PanicInfo` message and location β€” `docs/standards/error-handling.md:73` + **Action:** Either implement a register dump (x0-x30/SP/ELR_EL1/ESR_EL1/FAR_EL1 via inline asm, printed before the spin loop) and keep the standard as-is, or soften the claim to describe what exists today and track the register dump as a named follow-up. + +- **[🟑 MEDIUM]** error-handling.md misstates where and how `panic = "abort"` is configured β€” `docs/standards/error-handling.md:143` + **Action:** Correct to: "The bare-metal target sets `-C panic=abort` via `.cargo/config.toml`'s `[target.aarch64-unknown-none] rustflags` β€” not a `Cargo.toml` profile key β€” scoped to the kernel/BSP build only; host-side `cargo test` builds of kernel/hal/test-hal retain unwinding for the test harness." + +- **[🟑 MEDIUM]** code-review.md's Tooling section says CI is "(planned)"; infrastructure.md and the actual workflow show CI has been live and merge-blocking since Phase 4 β€” `docs/standards/code-review.md:124-129` + **Action:** Update to: "CI (live, Phase 4+) enforces format, clippy (host + kernel aliases), host tests, kernel build, and Miri; coverage is informational only and does not block merge β€” see infrastructure.md." Keep "Branch protection (planned)" as-is; that claim is still accurate. + +- **[🟑 MEDIUM]** error-handling.md claims CI "flags" both `todo!()` and `unimplemented!()`, but only `todo!()` has an actual deny-level lint β€” `docs/standards/error-handling.md:139` + **Action:** Either add `unimplemented = "deny"` to `[workspace.lints.clippy]` (mirroring `todo`), or narrow the sentence to "`todo!()` presence is denied by the `clippy::todo` workspace lint; `unimplemented!()` has no automated CI check today and relies on review." + +- **[🟑 MEDIUM]** logging-and-observability.md presents a fully-designed, non-existent logging facade in present-tense prescriptive language, without a status marker β€” `docs/standards/logging-and-observability.md:50-60` + **Action:** Add an explicit banner at the top: "Status: forward-looking. The `tyrne-log` crate and log-service architecture described here do not exist yet; the kernel currently emits diagnostics only via the raw Console/UART path. This document records the intended design." + +- **[🟑 MEDIUM]** error-handling.md claims uniform kernel+HAL+userspace coverage, but the enforcing deny-lints exist only in `kernel/src/lib.rs` β€” `docs/standards/error-handling.md:3`; `docs/standards/code-style.md:129` + **Action:** Either add the same `#![deny(...)]` block to `hal/src/lib.rs` (see Track B.2-1's clippy-denies finding), or narrow the standards language to "deny-enforced in the kernel crate; HAL and BSP crates rely on `clippy::pedantic` (warn) plus review." + +- **[βšͺ LOW]** commit-style.md's allowed-scope list is stale against actual, current commit history (including the 5 most recent commits) β€” `docs/standards/commit-style.md:53-56` + **Action:** Refresh the scope list from `git log --format=%s | grep -oE '\(([a-z0-9/_.+, -]+)\)'` (at minimum add `perf`, `roadmap`, `analysis`, `audits`, `architecture`, `mmu`); consider explicit sub-lists for process/doc scopes vs. code scopes. + +- **[βšͺ LOW]** code-style.md's "canonical" lint list omits three active workspace lints, most notably `clippy::todo` β€” `docs/standards/code-style.md:118-130` + **Action:** Add the three missing lints, keeping the `todo` entry's rationale (P12 tie-in) visible. + +- **[βšͺ LOW]** bsp-boot-checklist.md's QEMU exception-log path is stale β€” actual script produces a PID-suffixed filename, not the fixed path the doc quotes β€” `docs/standards/bsp-boot-checklist.md:218-224` + **Action:** Replace with the accurate description: "the logfile path is printed at startup as `${TMPDIR:-/tmp}/qemu_int..log`; grep the printed path, not a fixed name." + +- **[βšͺ LOW]** documentation-style.md's own "never use an untagged fence" rule is violated three times within docs/standards itself β€” `docs/standards/commit-style.md:15,25`; `docs/standards/testing.md:67` + **Action:** Tag the three fences `text` (they are illustrative templates/patterns, not executable code). + +- **[βšͺ LOW]** testing.md's "(to be added with the workspace)" for the test-hal crate is stale β€” the crate already exists β€” `docs/standards/testing.md:82` + **Action:** Drop the parenthetical. + +- **[⚫ INFO]** release.md still carries the "an Tyrne-specific" grammar error flagged by the prior review two months ago β€” `docs/standards/release.md:16` + **Action:** Change "an Tyrne-specific" to "a Tyrne-specific". + +**ADR corrections & cross-links** + +- **[🟑 MEDIUM]** Unsubstantiated, technically dubious "safety-relevant" rationale baked into an Accepted ADR and duplicated into source rustdoc β€” `docs/decisions/0008-cpu-trait.md:158` (Β§Revision notes, 2026-05-22 entry); duplicated at `hal/src/cpu.rs:99-104` + **Action:** Either attach concrete evidence (a rustc/LLVM issue link, a Miri trace, or a disassembly) as an append-only addition to the 0008 revision note and the rustdoc, or add a follow-up append-only note retracting the unverified aliasing claim and keeping only the defensible "avoid vtable indirection on the hot critical-section path" rationale. + +- **[🟑 MEDIUM]** ADR-0033 has no top-of-file pointer to its own load-bearing post-Accept correction (stale KBASE / two-offset model stays the first thing a reader sees) β€” `docs/decisions/0033-kernel-high-half-migration.md:28,67,105-112,128` vs. `:186-192` (Revision notes) + **Action:** Add a top-of-file blockquote in the same style already used at `docs/decisions/0022-idle-task-and-typed-scheduler-deadlock.md:7-11`, recording that the shipped `KBASE` is `0xFFFF_FFFF_4008_0000` (not `0xFFFF_FFFF_8008_0000`) and a single offset (`KERNEL_HIGH_HALF_OFFSET`) replaces the two-offset model described below. + +- **[🟑 MEDIUM]** Phase C's 2026-06-01 ADR-number renumbering was not cascaded downstream, recreating a direct collision with Phase D's ADR ledger on ADR-0042/0043/0044 β€” `docs/roadmap/phases/phase-c.md:118-122` vs. `docs/roadmap/phases/phase-d.md:162-167` + **Action:** Re-run the cascading renumbering: shift Phase D (and E–I if they also now collide) up past Phase C's new ceiling (ADR-0044); add a "downstream-renumbering note" to phase-d.md analogous to the one already present in phase-e.md. Recurrence of Blocker D4-001/D4-002 from the 2026-05-22 master review. + +- **[🟑 MEDIUM]** phase-b.md's own B6 dependency-ordered task sequence self-contradicts "DONE"/"landed" against "In Review" for the same task in the same sentence, with no historical-record framing to disambiguate β€” `docs/roadmap/phases/phase-b.md:250,254,255` + **Action:** Flip the three parenthetical task-status tags (steps 1, 5, 6) to "Done" with their merge PR numbers (T-022 β†’ PR #36, T-027 β†’ PR #41, T-028 β†’ PR #42, matching current.md's own citations), or explicitly wrap the pre-closure narrative in a "preserved as historical record" framing the way the B5 section above it already does. + +- **[βšͺ LOW]** ADR-0011 still asserts "QEMU virt is GICv3" β€” the exact claim ADR-0036 was written to correct β€” with no reader-facing redirect β€” `docs/decisions/0011-irq-controller-trait.md:11` + **Action:** Add the same one-line append-only top-of-file redirect banner already used on ADR-0004/0006/0012: "Correction: QEMU `virt` is GICv2 in v1, not GICv3; see ADR-0036. The trait surface described here is version-agnostic and unaffected." + +- **[βšͺ LOW]** ADR-0012's "EL drop" open question was never marked resolved, despite ADR-0024 settling it a month before the file was next edited β€” `docs/decisions/0012-boot-flow-qemu-virt.md:148` (Β§Open questions) + **Action:** Append a strikethrough + resolution note mirroring the existing "Boot-time MMU activation" treatment: "*Resolved 2026-04-27 by ADR-0024 (EL drop to EL1 policy).*" + +- **[βšͺ LOW]** security-model.md's cross-table-CDT open question does not cross-link ADR-0023, which already contains the option catalogue for resolving it β€” `docs/architecture/security-model.md:330` vs. `docs/decisions/0023-cross-table-capability-revocation-policy.md` + **Action:** Add "(see ADR-0023 for the deferred decision and its candidate options)", mirroring the IOMMU/SMMU bullet at line 328. + +- **[⚫ INFO]** ADR-0018 is the only ADR in 0001-0020 that omits any pros/cons comparison of its considered options β€” `docs/decisions/0018-badge-scheme-and-reply-recv-deferral.md` (Β§Considered options, lines 33-49) + **Action:** Append (purely additive) a "## Pros and cons of the options" section restating the six options' tradeoffs, for structural uniformity with the rest of the corpus. + +**Misc doc hygiene** + +- **[βšͺ LOW]** Two Phase-A task files have relative links to source files that are one directory level short and therefore resolve to a nonexistent path β€” `docs/analysis/tasks/phase-a/T-004-cooperative-scheduler.md:25` + **Action:** Add one more `../` to both links (`../../../../hal/src/context_switch.rs` and `../../../../kernel/src/cap/table.rs`). Consider adding a markdown-link-checker to CI given the density of relative cross-links and that at least 3 broken links have survived multiple prior review passes undetected. + +- **[βšͺ LOW]** T-011's coverage-baseline link is broken while an identical, correctly-formed link to the same document sits 75 lines later in the same file β€” `docs/analysis/tasks/phase-b/T-011-missing-tests-bundle.md:25` + **Action:** Fix line 25 to match line 100's correct relative path. + +- **[βšͺ LOW]** Glossary is missing entries for PMM and AddressSpace β€” two heavily-used, Tyrne-specific kernel-object/subsystem terms that sit at the same conceptual tier as already-defined siblings (MMU, Endpoint, Notification) β€” `docs/glossary.md` (insertion points: before line 9 for AddressSpace, before line 75 for PMM) + **Action:** Add a PMM (Physical Memory Manager) entry citing ADR-0035, and an AddressSpace entry citing ADR-0028, following the `update-glossary` skill's format and cross-linking conventions. + +- **[βšͺ LOW]** Glossary violates its own stated alphabetical-order invariant in two places β€” `docs/glossary.md:57-67,81-83` + **Action:** Reorder the M-section to MADR, MAIR, MapperFlush, Microkernel, Miri, MMU; swap Reply capability/Rendezvous IPC to Rendezvous IPC, Reply capability. Both are pure cut-and-paste moves with no content changes. + +- **[βšͺ LOW]** add-bsp skill's "verify the smoke test" example trace is stale β€” it shows a round-robin loop format the kernel no longer produces, which would mislead a BSP porter about what success looks like β€” `.agents/skills/add-bsp/SKILL.md:159-166` + **Action:** Replace the example trace with the current minimal-success shape plus a note ("exact trailing lines depend on kernel version β€” see README.md Quick Start for the current reference trace") so this doesn't re-drift on the next kernel demo change. + +- **[⚫ INFO]** NOTICE and LICENSE's Apache-2.0 appendix name different copyright holders for the same codebase β€” `NOTICE:2`, `LICENSE:189` + **Action:** Align the two, or make the relationship explicit (e.g. "Copyright 2026 HodeTech (Cemil Δ°lik) and Tyrne contributors" in both places). + +### Acceptance criteria + +- overview.md, task-loader.md, hal.md, scheduler.md, exceptions.md, security-model.md, memory-management.md, boot.md, and docs/architecture/README.md all reflect Phase B closed / Phase C active, with correct ADR cross-links. +- current.md's live-state bullets match HEAD; the phase-b task index lists all 23 tasks with correct statuses; all nine "In Review" task frontmatters flip to Done with `date_done` entries. +- CONTRIBUTING.md, SECURITY.md, and the affected standards docs (testing.md, error-handling.md, code-review.md, logging-and-observability.md, commit-style.md, code-style.md) match current CI/tooling reality. +- All four review-family README indices include every Phase-B-closing entry. +- The Phase C ADR ledger renumbering is cascaded to Phase D (and E–I if colliding). +- Every broken/stale relative link identified above is fixed. + +--- + +## Track B.2-3 β€” Immediate correctness & robustness fixes + +These are live defects in shipped code β€” not documentation drift, not stylistic preference. Two are memory-safety-adjacent (an inline-asm register clobber that is Rust `asm!` UB, and a host/target confusion that silently miscompiles on aarch64 dev machines); the rest are correctness gaps in the syscall boundary, the PMM, and the test-HAL fakes the kernel's own test suite trusts. + +### Sub-breakdown + +- **[🟠 HIGH]** MapperFlush's documented "single address space" assumption is already stale relative to Phase B's shipped multi-AS infrastructure β€” `hal/src/mmu/mod.rs:376-386` + **Action:** Land the AS/ASID discriminant on `MapperFlush` (e.g. a `PhantomData` AS-id or a stored ASID, with `flush` rejecting a mismatch) as a prerequisite for, or in the same change as, the first real per-task `cap_map` call β€” not deferred to "the multi-AS step" as an indefinite future milestone, since that infrastructure is already merged. Relevant to Milestone C5 (multi-core TLB shootdown), which needs a per-AS/ASID-aware flush discipline before it can safely broadcast invalidations. + +- **[🟠 HIGH]** `userland/hello/build.rs` picks the userland linker script by `target_arch == "aarch64"`, which is true on aarch64 HOST targets too, not just the bare-metal target β€” `userland/hello/build.rs:16-20` + **Action:** Discriminate on `CARGO_CFG_TARGET_OS == "none"` (the property that actually distinguishes the bare-metal target from every hosted aarch64 triple), matching the pattern already used in `bsp-qemu-virt/src/main.rs:144`. Apply the identical fix to `userland/hello/src/main.rs:23`'s `cfg_attr`. Add a one-line comment at each site cross-referencing the other, since this class of mistake has now occurred twice in the same crate pair (see the two related findings below). + +- **[🟠 HIGH]** console_write's exposure is worse than a bounded loop: each byte write can spin indefinitely inside a masked-interrupt, non-preemptible context if the UART backpressures β€” `kernel/src/syscall/dispatch.rs:295` (`ctx.console.write_bytes(&buf[..chunk]);`) + **Action:** Either bound `Pl011Uart::write_bytes`'s per-byte spin with an iteration cap or a Timer-based deadline and surface a typed failure the dispatcher can translate into a syscall error instead of hanging forever, or β€” if flow control is never enabled on the target hardware and this is judged structurally unreachable β€” add an explicit doc comment (or audit-log-style justification) recording that assumption so a future BSP port (Pi 4) doesn't silently inherit an unbounded-spin console path under masked interrupts. This directly gates Milestone C3 item 6 (the `Cpu::without_interrupts` interrupt-masked critical-section primitive): an unbounded spin under masked interrupts is exactly the deadlock shape C3's discipline exists to prevent once real IRQs can interrupt kernel code. + +- **[🟑 MEDIUM]** console_write's chunk loop has no length cap and runs entirely with interrupts masked β€” `kernel/src/syscall/dispatch.rs:252-300` + **Action:** Add an explicit maximum (e.g. `const MAX_CONSOLE_WRITE_LEN: usize = 4096;`) checked before Gate 1/2/3, returning `SyscallError::BadArgument` when `len` exceeds it, with a test pinning the boundary. Resolve together with the busy-spin finding above, since capping `len` alone does not fix the per-byte unbounded-spin risk. + +- **[🟑 MEDIUM]** No `MappingFlags::DEVICE` exclusion β€” `copy_from_user`/`copy_to_user` would silently memcpy MMIO registers once userspace drivers exist β€” `kernel/src/syscall/user_access.rs:152-160` (`probe_user_pages`), `:213-218` (copy_from_user pass 2), `:296-301` (copy_to_user pass 2) + **Action:** Add an explicit `!flags.contains(MappingFlags::DEVICE)` guard (or a documented, deliberate decision to allow it with a volatile/typed-MMIO API instead) to `probe_user_pages`, and record the decision either inline or as a forward-referenced open question in ADR-0038 or the Phase E (first real driver) task list, so the gap is a designed decision rather than an oversight discovered when the first driver syscall lands. + +- **[🟑 MEDIUM]** Bootstrap address-space handle is a compile-time assumption, never reconciled against the real allocation β€” `kernel/src/obj/arena.rs:57-73` (`SlotId::first_slot`), corroborated by `kernel/src/mm/address_space.rs:59-60` and `bsp-qemu-virt/src/main.rs:1176-1186/1424-1434` + **Action:** Add `debug_assert_eq!(bootstrap_as_handle, tyrne_kernel::mm::BOOTSTRAP_ADDRESS_SPACE_HANDLE, "bootstrap AS allocation invariant violated: first_slot() assumption broken")` immediately after the real allocation in main.rs, converting the prose discipline into a loud, testable invariant. Better still, thread the real `bootstrap_as_handle` through to every `Task::new(...)` call site instead of the assumed constant. Relevant to Milestone C2's per-core current-task-pointer work, which will multiply the number of places this assumption is relied on. + +- **[βšͺ LOW]** `PmmError::MisalignedAddress` doc-comment claims `free_frame` returns it defensively, but `free_frame` never constructs this variant β€” `kernel/src/mm/pmm.rs:46-51` (doc) vs. `:504-543` (free_frame body) + **Action:** Add the defensive check the doc already promises β€” `if !pa.0.is_multiple_of(PAGE_SIZE) { return Err(PmmError::MisalignedAddress); }` at the top of free_frame β€” matching the project's fail-fast/defense-in-depth preference and the reserved-range inversion check's own rationale a few lines above. + +- **[βšͺ LOW]** `Pmm::new`'s pairwise reserved-range overlap check produces a false-positive `OverlappingReservedRanges` rejection for a zero-length reserved range nested inside another range β€” `kernel/src/mm/pmm.rs:247-253` + **Action:** Skip the overlap check for any range with `frame_count() == 0` inside the pairwise loop, since such a range can never affect bitmap-vs-counter parity regardless of its position. + +- **[βšͺ LOW]** `Pmm::new` validates reserved-range inversion explicitly but has no equivalent explicit check for the top-level extent's own inversion β€” `kernel/src/mm/pmm.rs:199-214` (extent validation) vs. `:234-236` (reserved-range inversion check) + **Action:** Add a symmetric `if extent.end.0 < extent.start.0 { return Err(PmmError::OutOfRange); }` immediately after the alignment check, matching the treatment already given to every reserved range. + +- **[βšͺ LOW]** `probe_user_pages` does not special-case `len == 0`, so a future direct caller with an unaligned pointer gets a spurious fault instead of the documented trivial-accept β€” `kernel/src/syscall/user_access.rs:140-170` + **Action:** Add `if len == 0 { return Ok(()); }` at the top of `probe_user_pages` itself, matching `UserAccessWindow::validate`'s own handling. + +- **[βšͺ LOW]** The "ends in the top page must not spuriously fault" regression is pinned only for `copy_from_user`, not the structurally-duplicated `copy_to_user` β€” `kernel/src/syscall/user_access.rs:600-615` (existing test) vs. `:286-317` (copy_to_user pass 2, untested at this edge) + **Action:** Add a symmetric `copy_to_user_range_ending_in_top_page_does_not_spuriously_fault` test (or extract the shared per-page cursor arithmetic per Track B.2-4's dedup item, making a single test suffice for both directions). + +- **[🟠 HIGH]** `BlockMappedMmu::map` returns `MmuError::BlockMapped` instead of the contractually-required `AlreadyMapped` β€” `test-hal/src/mmu.rs:533-547` (also self-tested wrong at `test-hal/src/mmu.rs:1108-1117`) + **Action:** Change `BlockMappedMmu::map` to return `Err(MmuError::AlreadyMapped)` for a blocked `va` (leave `unmap`/`translate` returning `BlockMapped`, which are correct today). Update the in-file test `block_mapped_mmu_injects_block_mapped_on_map_and_unmap` to assert `AlreadyMapped` from `map()`. Update the two dependent kernel tests (`kernel/src/mm/address_space.rs:1384-1439`, `kernel/src/obj/task_loader.rs:2504-2539`) and the `LoadError::MapFailed` doc-comment at `task_loader.rs:394-401` to match. + +- **[🟠 HIGH]** console_write's inline asm declares `x2` as `in`-only, but the kernel unconditionally overwrites `x2` on every syscall return β€” unmarked register clobber (Rust `asm!` UB) β€” `userland/tyrne-user/src/lib.rs:102-115` (esp. line 108); cross-referenced with `bsp-qemu-virt/src/syscall.rs:224` and `bsp-qemu-virt/src/vectors.s:237` + **Action:** Change the operand to `inout("x2") buf.len() as u64 => _,` (discarding the output, matching the style already used for x3-x7's `lateout(...) _`). Update the SAFETY comment (line 99) to read "x2..x7 are marked clobbered" and correct the UNSAFE-2026-0033 audit-log entry (`docs/audits/unsafe-log.md:756,761`) to match. + +- **[🟠 HIGH]** `hello/build.rs`'s target check (`CARGO_CFG_TARGET_ARCH == "aarch64"`) cannot distinguish the real bare-metal target from a host build on an aarch64 host, breaking `cargo check --workspace` / miri / llvm-cov on Apple Silicon or ARM64 Linux dev machines β€” `userland/hello/build.rs:16-20` + **Action:** Gate on the OS component too (or the full triple), not just the arch: `if std::env::var("CARGO_CFG_TARGET_OS").as_deref() == Ok("none") { ... }`, or compare the full `TARGET` env var to the literal `"aarch64-unknown-none"`. (Same underlying bug as the finding above; independently surfaced by a second review pass and listed separately per the source review's own routing.) + +- **[🟠 HIGH]** The arch-only cfg gate is not confined to `build.rs` β€” the identical flaw recurs in `main.rs`'s crate attribute and throughout `lib.rs`'s cfg gates, giving the bug a much larger blast radius than the linker-script symptom alone β€” `userland/hello/src/main.rs:23`; `userland/tyrne-user/src/lib.rs:28,35,44,83,134` + **Action:** Fix all three files' target-selection gates uniformly: replace every `target_arch = "aarch64"` guard with a check that also pins the OS/environment β€” e.g. `#[cfg(all(target_arch = "aarch64", target_os = "none"))]` β€” or better, centralize the condition as a single `cfg` alias (`#[cfg(tyrne_bare_metal)]` set via `.cargo/config.toml`'s `[target.aarch64-unknown-none] rustflags = ["--cfg", "tyrne_bare_metal"]`) so the three call sites cannot drift independently in the future. Track B.2-1 tracks adding the arm64-host CI signal that would catch a regression here. + +- **[βšͺ LOW]** `smoke.sh`'s `--timeout` flag crashes with an unbound-variable error instead of a usage message if no value is given β€” `tools/smoke.sh:29` + **Action:** Either check `[[ $# -ge 2 ]]` before consuming `$2` and emit a clear "--timeout requires a value" error, or switch to the `--timeout=SECONDS` style already used by `perf-harness.sh` for consistency. + +### Acceptance criteria + +- `MapperFlush` carries an AS/ASID discriminant and rejects a mismatched flush. +- `userland/hello/build.rs`, `main.rs`, and `tyrne-user/src/lib.rs`'s target gates all discriminate on `target_os == "none"` (or a shared `cfg` alias). +- `console_write`'s per-byte spin is bounded and its chunk length is capped; both are regression-tested. +- `BlockMappedMmu::map` returns `AlreadyMapped`; dependent kernel tests and the `LoadError::MapFailed` doc-comment are updated to match. +- The userland `console_write` syscall shim marks `x2` as clobbered, matching the kernel's actual register usage; the UNSAFE-2026-0033 audit entry is corrected. +- `Pmm::new` rejects an inverted top-level extent and no longer false-positives on a zero-length nested reserved range; `free_frame` defensively checks alignment. +- `smoke.sh --timeout` fails with a clear usage message, not an unbound-variable crash. + +--- + +## Track B.2-4 β€” Code quality & API design polish + +None of these are live bugs; all are cheap-now, expensive-later hygiene: newtype discipline that would turn a class of unit-confusion mistakes into compile errors, duplicated security-critical logic (the capability-resolution pattern, the `Arena` free-list machinery) that risks a fix landing in one copy and not its twin, and a handful of test-coverage gaps on already-designed error paths. + +### Sub-breakdown + +**Newtype discipline** + +- **[🟑 MEDIUM]** PA↔VA direct-map conversion functions use raw `usize`, not the `PhysAddr`/`VirtAddr` newtypes they exist to keep distinct β€” `hal/src/mmu/mod.rs:104` and `hal/src/mmu/mod.rs:142` + **Action:** Change the signatures to `phys_to_kernel_va(pa: PhysAddr) -> VirtAddr` and `kernel_va_to_phys(va: VirtAddr) -> PhysAddr` (or keep a `pub(crate)` raw-usize escape hatch for asm/linker-symbol interop and add the typed wrappers as the public surface). + +- **[🟑 MEDIUM]** `PhysAddr`/`VirtAddr` are naked pub-usize newtypes with zero arithmetic/alignment methods β€” `hal/src/mmu/mod.rs:164` (`VirtAddr(pub usize)`) and `hal/src/mmu/mod.rs:168` (`PhysAddr(pub usize)`) + **Action:** Add `checked_add`/`wrapping_add(usize) -> Self`, `align_up(align: usize) -> Self`, `is_aligned_to(align: usize) -> bool` directly on both types, mirroring the pattern already used for `PhysFrame::from_aligned`. + +- **[🟑 MEDIUM]** `tyrne-user`'s syscall wrappers pass capability handles as bare `u64` with no local newtype β€” `userland/tyrne-user/src/lib.rs:57` (`HELLO_CONSOLE_CAP: u64`) and `userland/tyrne-user/src/lib.rs:89` (`console_write(cap: u64, buf: &[u8])`) + **Action:** Add a small local newtype in `tyrne-user` (e.g. `pub struct CapWord(pub u64);`), independent of the kernel crate per the module's own "restate the ABI, don't share the type" philosophy, and change `console_write`'s `cap` parameter and `HELLO_CONSOLE_CAP`'s type to it. + +- **[βšͺ LOW]** `UserAccessWindow` and `copy_from_user`/`copy_to_user` take raw `usize` pointers instead of `VirtAddr` β€” `kernel/src/syscall/user_access.rs:77` (`UserAccessWindow::new(base: usize, len: usize)`) and `kernel/src/syscall/user_access.rs:186-192` + **Action:** Change `UserAccessWindow::new`'s `base` and the `user_ptr` parameters of `copy_from_user`/`copy_to_user`/`probe_user_pages` to `VirtAddr`, keeping `len` as `usize`. + +- **[βšͺ LOW]** `ContextSwitch::init_user_context` mixes raw `usize` addresses with a raw pointer for conceptually similar arguments β€” `hal/src/context_switch.rs:122-128` + **Action:** Use `VirtAddr` consistently for all three, or add a lightweight marker type distinguishing entry-VA from stack-VA so a transposed pair fails to compile. + +- **[⚫ INFO]** `Pmm::could_yield_pa_overlapping` drops the `PhysAddr` newtype at its own API boundary β€” `kernel/src/mm/pmm.rs:620` (`could_yield_pa_overlapping(&self, pa_range: core::ops::Range) -> bool`) + **Action:** Change the parameter to `core::ops::Range`; trivial, drop-in change at the one call site. + +**Dead-code / duplication removal** + +- **[🟑 MEDIUM]** `CapabilityTable` hand-reimplements the generic `Arena`'s slot/free-list/generation machinery instead of using it β€” `kernel/src/cap/table.rs:86-156,563-654` vs. `kernel/src/obj/arena.rs:16-215` + **Action:** Rebase `CapabilityTable`'s storage on `Arena` (as `AddressSpaceArena` already does), converting Arena's `Option`-based errors to the appropriate `CapError` at the boundary. Collapses two independently-audited copies of the single most security-critical free-list/generation-counter pattern in the kernel into one. + +- **[🟑 MEDIUM]** The "resolve capability β†’ check kind β†’ check rights" pattern is hand-rolled five separate times across four modules β€” `kernel/src/ipc/mod.rs:589-602,604-616`; `kernel/src/mm/address_space.rs:430-441`; `kernel/src/syscall/dispatch.rs:321-333`; `kernel/src/sched/mod.rs:555-570` + **Action:** Factor a single crate-internal primitive on `CapabilityTable`, e.g. `fn resolve_typed(&self, handle: CapHandle, extract: impl FnOnce(CapObject) -> Option, required: CapRights) -> Result`, performing lookup β†’ extract-or-WrongKind β†’ rights-check-or-InsufficientRights once. Pins the ADR-0030 ordering in exactly one place instead of five. + +- **[🟑 MEDIUM]** `copy_from_user`/`copy_to_user` duplicate the entire per-page validated-copy loop β€” `kernel/src/syscall/user_access.rs:204-248` (copy_from_user pass 2) vs. `:287-315` (copy_to_user pass 2) + **Action:** Extract the shared per-page loop into a private generic helper, e.g. `fn walk_user_pages(mmu, task_as, user_ptr, len, require_write, per_page: impl FnMut(...)) -> Result<(), SyscallError>` that does the translate+flags+arithmetic once; `copy_from_user`/`copy_to_user` supply only a three-line direction-specific closure around `core::ptr::copy`. Keeps both `unsafe` sites' SAFETY comments distinct. + +- **[βšͺ LOW]** Endpoint/Notification/Task kernel-object modules duplicate the identical handle-wrapper + create/destroy/get boilerplate three times β€” `kernel/src/obj/endpoint.rs:37-120`; `kernel/src/obj/notification.rs:48-109`; `kernel/src/obj/task.rs:64-128` + **Action:** Introduce a `macro_rules! typed_object_arena!` (or a generic `TypedHandle` + generic free functions) in `obj/arena.rs`, and have the three modules invoke it. Pure mechanical de-duplication, reduces the audited-code surface by roughly two-thirds for this pattern. + +- **[βšͺ LOW]** `MappingFlags` and `CapRights` are structurally identical hand-rolled bitflag types duplicated across two crates β€” `hal/src/mmu/mod.rs:220-315` vs. `kernel/src/cap/rights.rs:18-129` + **Action:** Add a small internal `macro_rules! bitset_type!` (living in `hal` or a tiny near-zero `tyrne-bits` internal crate) that both `MappingFlags` and `CapRights` invoke, with a flag to omit `BitAndAssign`/`SubAssign` for the deliberately-narrowing `CapRights` type. + +- **[βšͺ LOW]** `block_descriptor`/`page_descriptor` encoders duplicate their bit-composition body β€” `hal/src/mmu/vmsav8.rs:451-463` (block_descriptor) vs. `:479-490` (page_descriptor) + **Action:** A private `fn leaf_descriptor(pa: u64, bits: DescriptorBits, is_page: bool, oa_mask: u64) -> u64` removes the duplication with no loss of clarity and guarantees the two encoders can't silently diverge on a shared field. + +- **[βšͺ LOW]** `QemuVirtGic::enable`/`disable` duplicate register-offset and bit-mask computation β€” `bsp-qemu-virt/src/gic.rs:316-340` (enable) vs. `:342-362` (disable) + **Action:** Factor a private `fn set_bit_register(&self, base: usize, irq: IrqNumber)` taking the register base as a parameter; `enable`/`disable` become one-line callers passing `GICD_ISENABLER_BASE`/`GICD_ICENABLER_BASE`. + +- **[🟑 MEDIUM]** `KERNEL_IMAGE_PHYS_BASE` (linker.ld) and `KERNEL_IMAGE_START` (main.rs) are independently hardcoded with no compile-time cross-check, unlike the sibling `KERNEL_HH_OFFSET` constant β€” `bsp-qemu-virt/linker.ld:41` and `bsp-qemu-virt/src/main.rs:102` + **Action:** Add a compile-time cross-check analogous to `KERNEL_HH_OFFSET`'s β€” `PROVIDE(__kernel_image_phys_base = KERNEL_IMAGE_PHYS_BASE);` in the linker script, exposed as an `extern "C"` symbol, asserted equal to `KERNEL_IMAGE_START` at `kernel_entry` (a runtime assert, since linker-script constants can't be const-asserted against Rust directly). Alternatively, follow ADR-0039's `--defsym` upgrade path when the BSP layout is next touched (e.g. the Pi 4 port). + +- **[βšͺ LOW]** `bsp-qemu-virt/linker.ld`'s `/DISCARD/` list is less rigorous than `hello.ld`'s β€” no `.got`/`.got.plt` handling or ASSERT, despite the file's own header claiming the design depends entirely on PC-relative addressing β€” `bsp-qemu-virt/linker.ld:115-120` + **Action:** Mirror `hello.ld`'s pattern: add a `.got : { *(.got .got.plt) }` output section plus `ASSERT(SIZEOF(.got) == 0, "kernel image must be non-PIC...")`, turning the header comment's prose invariant into a build-time-checked one. + +**API design & taxonomy** + +- **[🟑 MEDIUM]** `SyscallError` has no variant for the "no current task" control-plane failure; `dispatch.rs` is forced to misreport it as a capability error β€” `kernel/src/syscall/error.rs:59-85` (the `SyscallError` enum); consumed at `kernel/src/syscall/dispatch.rs:144-161` + **Action:** Add a `SyscallError::NoCurrentTask` variant with its own stable top-level status code (e.g. `4`, next free after `BadArgument`=2/`FaultAddress`=3), and have `dispatch.rs`'s `task_yield`/`task_exit` no-current-task branches return it instead of borrowing `CapError::InvalidHandle`. Low-risk since `SyscallError` is `#[non_exhaustive]` and the branch is unreachable from a real EL0 caller in v1 (defensive-only) β€” a good moment to fix the taxonomy before any userspace binding pattern-matches on `0x102`. + +- **[βšͺ LOW]** Status-code block encoding relies on manual discipline with no compile-time bound guard, unlike the file's own `NULL_CAP_HANDLE` precedent β€” `kernel/src/syscall/error.rs:46-48` (`CAP_STATUS_BASE`/`IPC_STATUS_BASE`) and `:122-148` + **Action:** Add a `const _: () = assert!(...)` (mirroring `abi.rs:41-44`) bounding the maximum literal used in `cap_error_code`/`ipc_error_code` below `CAP_STATUS_BASE`'s width. + +- **[🟑 MEDIUM]** `tyrne-user::SyscallError` exposes only the raw status word, with no decode/classification API β€” `userland/tyrne-user/src/lib.rs:61-68` + **Action:** Add a decode step β€” either a small mirrored enum with `From`/`TryFrom`, or at minimum classification methods (`is_cap_error`, `is_ipc_error`, `top_level_kind`) built from the same `0x100`/`0x200` block constants the kernel already documents. + +- **[🟑 MEDIUM]** The BSP-side syscall trust boundary (fail-closed gate #3 selection logic) has zero automated test coverage of any kind β€” `bsp-qemu-virt/src/syscall.rs:143-294` (`syscall_entry`, esp. the match at lines 182-193) + **Action:** Extract the match's branch-selection logic into a small, host-testable pure function β€” e.g. `fn resolve_syscall_context(current_table, current_window, resolved_as) -> (bool, ...)` β€” mirroring the `start_prelude` extraction pattern already used successfully in `kernel/src/sched/mod.rs` (T-011). Pair with a QEMU-smoke assertion (once CI-gated per Track B.2-1) that a syscall issued with no current task actually observes FAILCLOSED_TABLE behavior end-to-end. + +- **[🟑 MEDIUM]** The only real EL0 userspace program exercises just two syscalls (console_write, task_exit); no end-to-end test proves the confused-deputy defenses against a real (non-fake) MMU β€” `userland/hello/src/main.rs:44-49` (`_start`); `kernel/src/syscall/user_access.rs` tests (all against `FakeMmu`/`BlockMappedMmu`, never `bsp-qemu-virt`'s `QemuVirtMmu`) + **Action:** Add a second, deliberately-adversarial userland image (or extend `hello` behind a feature flag) that attempts an out-of-window `console_write`, a `console_write` against a page without USER, and an IPC call with a stale capability handle, asserting each is rejected via the real trap frame + real page tables. Wire into the future `qemu-smoke` CI job (Track B.2-1). + +- **[βšͺ LOW]** `IpcQueues::peek_state` is named and documented as a non-mutating read but silently mutates state on the stale-generation path β€” `kernel/src/ipc/mod.rs:254-271` (`reset_if_stale_generation`), `:278-281` (`peek_state`), `:330-333` (call-site comment) + **Action:** Add one sentence to `peek_state`'s doc-comment distinguishing "non-destructive to any live endpoint's observable state" from "side-effect-free" (it takes `&mut self`), or rename to `state_for_read` to remove the ambiguity directly. + +**Testing gaps & misc hygiene** + +- **[βšͺ LOW]** No regression test pins self-transfer (`transfer == Some(ep_cap)`) semantics in `ipc_send` β€” `kernel/src/ipc/mod.rs:303-374` (`ipc_send`); test module `:668-1720` + **Action:** Add a regression test that calls `ipc_send(..., ep_cap, ..., transfer: Some(ep_cap))` and asserts it succeeds, that the sender's table no longer resolves `ep_cap` afterward, and that the receiver obtains a valid capability to the same endpoint. + +- **[βšͺ LOW]** Arena capacity const-assert is off-by-one (overly conservative, not exploitable) β€” `kernel/src/obj/arena.rs:109-112` + **Action:** Change the condition to `N <= (Index::MAX as usize) + 1` (underflow-guarded for `N == 0`) and update the assertion message. Low priority β€” no current instantiation is anywhere near this boundary. + +- **[βšͺ LOW]** `Arena::allocate` has no must-use protection despite an unrecoverable-leak failure mode β€” `kernel/src/obj/arena.rs:145` + **Action:** Add an explicit `#[must_use]` to `Arena::allocate` (overriding the clippy `&mut self` heuristic). Optional: the same for `Arena::free`'s `Option` for symmetry. + +- **[βšͺ LOW]** Generation counter is unwidened/unguarded against wraparound-induced stale-handle revalidation (ABA) β€” `kernel/src/obj/arena.rs:169` + **Action:** At minimum, add a doc comment on `Generation`/`Arena::free` acknowledging the accepted wraparound bound so it reads as a reviewed, deliberate tradeoff. For genuine hardening, consider retiring a slot from the free list once its generation reaches `Generation::MAX` rather than wrapping (the arena has 16 slots of headroom to lose one permanently). + +- **[βšͺ LOW]** Test coverage gaps on three explicitly-designed panic/error paths: `register_idle` double-registration, `enqueue_ready`'s invariant panic, and `add_task`/`add_user_task`'s `QueueFull` error β€” `kernel/src/sched/mod.rs:787-800` (register_idle assert), `:598-609` (enqueue_ready panic), `:392` and `:497` (QueueFull production sites) + **Action:** Add three small tests mirroring the file's existing white-box style: (a) `#[should_panic(expected = "register_idle called twice")]` calling `register_idle` twice; (b) fill `sched.ready` to capacity then trigger `enqueue_ready` inside a `#[should_panic]` test; (c) fill the ready queue to `TASK_ARENA_CAPACITY` and assert the `(N+1)`th `add_task` call returns `Err(SchedError::QueueFull)`. + +- **[⚫ INFO]** Bridge entry points document a "pointer validity" precondition but perform zero defensive checks on it, inconsistent with the `debug_assert` precedent set elsewhere in the same file β€” `kernel/src/sched/mod.rs:667-668` (documented precondition), `:772-815,1093-1107,1259-1290,1369-1407` (none add a `debug_assert` on their `*mut` parameters) + **Action:** Optional polish: add `debug_assert!(!sched.is_null())` (and companions for `ep_arena`/`queues`/`caller_table` where present) at the top of each bridge entry point, matching the precedent already set by `add_user_task`. Low priority given the caller surface is BSP-only, not attacker-reachable. + +- **[⚫ INFO]** `start_prelude`'s panic message hardcodes 'start' even though `task_exit_current` shares the same helper β€” `kernel/src/sched/mod.rs:872` + **Action:** Reword to be caller-agnostic, e.g. "scheduler dispatch called with empty ready queue and no idle task", or thread a `&'static str` caller-tag into `start_prelude`. + +- **[βšͺ LOW]** The endpoint cap-leak-on-destroy `debug_assert!` regression test injects private state directly rather than driving the documented leak scenario through the real public-API sequence β€” `kernel/src/ipc/mod.rs:1668-1686` (`stale_send_pending_with_some_cap_panics_in_debug`); `kernel/src/obj/endpoint.rs:80-102` (C3-001 doc) + **Action:** Add a companion test that drives the real sequence: `ipc_send(..., Some(xfer_cap))` to reach `SendPending{cap:Some(_)}`, then `table.cap_drop(ep_cap); destroy_endpoint(...)`, then a fresh `create_endpoint` in the likely-same slot, then a subsequent IPC op β€” `#[should_panic]` in debug, confirming the actual production call path trips the guard. + +- **[⚫ INFO]** Redundant fully-qualified path for an already-imported constant β€” `test-hal/src/mmu.rs:569` + **Action:** Drop the `tyrne_hal::` qualifier for consistency with the rest of the file. + +**Misc** + +- **[βšͺ LOW]** Stale comment: `task_a`'s IPC send no longer "yields to B" once the EL0 `hello` task shares the ready FIFO β€” it yields to `hello` first β€” `bsp-qemu-virt/src/main.rs:671-673` (and the mirrored comment at `:692-696`) + **Action:** Update the comment in `task_a` (and its `ipc_recv_and_yield` counterpart) to describe the actual post-T-028 interleaving (send unblocks B β†’ generic FIFO yield actually dispatches `hello` first β†’ `hello` exits β†’ B resumes), or, if determinism of the demo trace is considered valuable, consider whether `hello` should be added to the scheduler *after* the A/B demo tasks are known to have finished their round trip β€” the current comment should not claim behavior the FIFO does not produce. + +- **[βšͺ LOW]** The debug-console security gate is keyed off the general-purpose `debug_assertions` cfg rather than a dedicated feature flag β€” `kernel/src/syscall/abi.rs:69-91` + **Action:** Introduce a dedicated, explicit Cargo feature (e.g. `debug-console`, off by default) that gates `SyscallNumber::ConsoleWrite` instead of reusing `debug_assertions`. Only `cargo kernel-build`/dev workflows enable it explicitly (e.g. via a `kernel-build-debug` alias that passes `--features debug-console`), decoupling the security posture from any future profile that happens to turn on `debug-assertions` for unrelated reasons. + +### Acceptance criteria + +- `PhysAddr`/`VirtAddr` gain arithmetic/alignment helpers; `phys_to_kernel_va`/`kernel_va_to_phys` and `UserAccessWindow` use typed addresses at their public boundaries. +- `CapabilityTable` is rebased on the shared `Arena` type; the five hand-rolled resolve-capability call sites use one shared primitive. +- `copy_from_user`/`copy_to_user` share their per-page validated-copy loop. +- `SyscallError` gains a `NoCurrentTask` variant; `tyrne-user::SyscallError` gains a decode/classification API. +- The BSP syscall trust boundary's branch-selection logic is extracted into a host-testable pure function with test coverage. +- The remaining low/info dead-code and doc-nit items are cleaned up opportunistically; none are release-blocking for this phase. + +--- + +## Track B.2-5 β€” Unsafe-audit & doc-comment hygiene + +Separate from Track B.2-2's architecture-level documentation, this track is the code-level counterpart: `docs/audits/unsafe-log.md` entries whose Location/Invariants fields no longer match their call sites, and rustdoc/doc-comments in source files that describe a state of the world Phase B has since moved past. Several of these discharge security-relevant invariants (the PMM's `could_yield_pa_overlapping`, the copy-user path's `FrameProvider::alloc_frame` contract) β€” leaving them stale risks a future maintainer reasoning from a wrong premise. + +### Sub-breakdown + +- **[βšͺ LOW]** Doc comment incorrectly claims an L0 block descriptor exists in VMSAv8-64 β€” `hal/src/mmu/vmsav8.rs:222-228` + **Action:** Change the second bullet to "L1/L2 with bit1=0 β†’ block descriptor (huge page)" and add an explicit bullet: "L0 with bit1=0 β†’ reserved (translation fault); VMSAv8-64 defines no L0 block descriptor." + +- **[βšͺ LOW]** UNSAFE-2026-0028's Location/Invariants fields contradict its own Amendment β€” `docs/audits/unsafe-log.md:636,640,651` (call site: `bsp-qemu-virt/src/main.rs:1176-1186`, inside `kernel_main_high`) + **Action:** Update the Location: and Invariants: fields to say `kernel_main_high` (or add a one-line correction note pointing to the Amendment), consistent with how sibling entries UNSAFE-2026-0022/0023/0024/0025/0026/0027 each got a T-022 Amendment. + +- **[βšͺ LOW]** A second `Pl011Uart::new` call site (`kernel_main_high`, high-half alias) also cites UNSAFE-2026-0001 despite mismatching both the entry's Location and Operation fields β€” `bsp-qemu-virt/src/main.rs:1013-1015` (vs. `docs/audits/unsafe-log.md:17-18`) + **Action:** Add an Amendment to UNSAFE-2026-0001 explicitly covering the high-half persistent-Console construction in `kernel_main_high` (same underlying device, different address expression and function), following the same append-only Amendment discipline used elsewhere. Same root cause as the two findings above: audit-log entries not updated when T-022 split `kernel_entry` into two functions. + +- **[⚫ INFO]** Doc-comment typo: duplicated word "dispatch dispatch" β€” `bsp-qemu-virt/src/exceptions.rs:34` + **Action:** Change to "because the IRQ-dispatch table is the natural home for". + +- **[βšͺ LOW]** `bsp-qemu-virt/src/main.rs`'s `PMM_EXTENT_START` comment cites a `linker.ld` `MEMORY`/`RAM`/`ORIGIN` construct that was deleted from linker.ld during the ADR-0033 high-half migration and never existed again since β€” `bsp-qemu-virt/linker.ld` (whole file, no `MEMORY` block present) cross-referenced from `bsp-qemu-virt/src/main.rs:95-96` + **Action:** Update the main.rs:95-96 doc comment to stop referencing a `MEMORY`/`RAM`/`ORIGIN` construct that no longer exists β€” e.g. "Matches linker.ld's KERNEL_IMAGE_PHYS_BASE-derived KBASE placement (`. = KBASE` at line ~50) for QEMU virt" or point at ADR-0033 instead. If Track B.2-4's linker-symbol cross-check lands, fold this comment fix into the same change. + +- **[βšͺ LOW]** Crate-level doc comment claims the kernel "defines... interrupt dispatch", contradicting both reality and the file's own P6 claim two lines later β€” `kernel/src/lib.rs:5-10` + **Action:** Reword to accurately scope the crate, e.g.: "This crate defines the capability system, scheduler, IPC primitives, memory management, and the EL0β†’EL1 syscall boundary's kernel-side half (`syscall`). Asynchronous interrupt handling is board-specific and lives in the BSP per [P6](../../standards/architectural-principles.md); the kernel side only sees cooperative interrupt-masking primitives (`sched::IrqGuard`) around context switches." (In the source doc-comment itself, wire `[P6]` to whatever intra-doc reference the crate uses for the BSP interrupt-handling principle.) + +- **[⚫ INFO]** Audit-log entry title for UNSAFE-2026-0030 still names the superseded primitive (`copy_nonoverlapping`) that this file no longer uses β€” `docs/audits/unsafe-log.md:676` (heading) vs. `kernel/src/syscall/user_access.rs:245,313` + **Action:** Update the UNSAFE-2026-0030 heading to say `core::ptr::copy` (or "byte move" without naming a specific primitive) to match the amended operation. + +- **[⚫ INFO]** Lint-provenance comment enumerates workspace lints but omits two that actually exist (`unreachable_pub`, `unused_must_use`) β€” `kernel/src/lib.rs:51-54` + **Action:** Either add the two missing lint names, or (more robust against future drift) replace the itemized list with a pointer: "see `[workspace.lints]` in the root Cargo.toml for the full list." + +- **[βšͺ LOW]** Module doc-comments in endpoint.rs / notification.rs / task.rs describe a design that never materialized and is now factually wrong β€” `kernel/src/obj/endpoint.rs:5-7,16-17`; `kernel/src/obj/notification.rs:5-7`; `kernel/src/obj/task.rs:5-7` + **Action:** Refresh all three module docs now that Phase B is closed: state plainly where the real state lives (`ipc::IpcQueues` for endpoint waiters; scheduler-side per-task arrays in `sched::Scheduler` for context; and for Notification, that blocking-wait remains unimplemented and is still an open roadmap item, not an A4-completed feature). + +- **[βšͺ LOW]** `SchedError::QueueFull` doc comment claims it is produced "only" by `add_task`, but `add_user_task` returns it through the identical path β€” `kernel/src/sched/mod.rs:178-186` (doc), `:493-497` (add_user_task's matching QueueFull path) + **Action:** Update the doc comment to "Produced by [`add_task`] or [`add_user_task`] at registration time..." (or link both). + +- **[🟑 MEDIUM]** `could_yield_pa_overlapping`'s doc-comment is stale: describes the ADR-0033 high-half migration as a future placeholder, but it landed at T-022 and the call site already uses it β€” `kernel/src/mm/pmm.rs:560-565` + **Action:** Update the doc-comment to state the migration is complete and name the actual helper (`tyrne_hal::kernel_va_to_phys`), matching the phrasing already used in mod.rs's `phys_frame_kernel_ptr` doc and in task_loader.rs's own comment at the call site. This is a security-relevant invariant description (it discharges UNSAFE-2026-0027's non-overlap precondition) β€” leaving it stale risks a future maintainer reasoning from a wrong premise. + +- **[βšͺ LOW]** `Pmm::new`'s inline "Validation (i)/(ii)/(iii)/(iv)" code-comment labels don't correspond to the doc-comment's 5 enumerated steps β€” `kernel/src/mm/pmm.rs:159-178` (docstring steps 1-5) vs. `:200,207-214,216,223,239` (inline validation labels) + **Action:** Renumber the inline roman-numeral labels to match the docstring's 1-5 ordering (or switch the docstring to reference the same non-sequential internal labels), so a maintainer cross-referencing the two doesn't have to reverse-engineer the correspondence. + +- **[βšͺ LOW]** Duplicate "Step 4" comment label in `cap_create_address_space` β€” the unsafe `create_address_space` block should be labeled Step 5 β€” `kernel/src/mm/address_space.rs:605` and `:611` + **Action:** Relabel the comment at line 611 from "Step 4" to "Step 5" so the code matches the docstring enumeration. + +- **[βšͺ LOW]** `load_image`'s public-fn contract omits that a mis-sourced image slice can panic the kernel via `kernel_va_to_phys`, not return `LoadError` β€” `kernel/src/obj/task_loader.rs:589-608` (row 4 preflight), `:604` (`kernel_va_to_phys` call site); `hal/src/mmu/mod.rs` `kernel_va_to_phys` + **Action:** Add a one-line `# Panics` section to `load_image`'s rustdoc cross-referencing `kernel_va_to_phys`'s own panic contract, so a future caller passing a non-kernel-owned image slice discovers the precondition from the doc rather than from a kernel abort. + +- **[βšͺ LOW]** console_write's doc comment implies a partial-write result but v1's kernel semantics are strictly all-or-nothing β€” `userland/tyrne-user/src/lib.rs:72-77` + **Action:** Tighten the doc to state the v1 guarantee explicitly, e.g. "Returns `buf.len()` on success (v1 is all-or-nothing: a partial write never happens) or the raw kernel status word on rejection." + +- **[βšͺ LOW]** `tyrne-user`'s `#![no_std]` is unconditional, contradicting its own doc comment's claim to mirror the kernel's `cfg_attr(not(test), no_std)` discipline β€” `userland/tyrne-user/src/lib.rs:21` and `:27` + **Action:** Either change line 21 to `#![cfg_attr(not(test), no_std)]` to actually match the kernel's discipline, or fix the comment to stop claiming a pattern the code doesn't use. + +- **[⚫ INFO]** hello's panic exit code (101) and the success exit code (0) are currently discarded by the kernel β€” not yet an observable distinction β€” `userland/hello/src/main.rs:44-58` + **Action:** Optional: add a one-line note to the panic handler's doc comment that the exit code is currently write-only from userspace's perspective, to set correct reader expectations until a future task surfaces task exit status. + +- **[βšͺ LOW]** `cap_create_address_space`'s doc comment claims it resolves the parent cap "via `resolve_address_space_cap`", but the function body reimplements the lookup+kind-check inline instead of calling that helper β€” `kernel/src/mm/address_space.rs:447` (doc claim) vs. `:547-553` (actual body) + **Action:** Either make `cap_create_address_space` actually call `resolve_address_space_cap` (extending it to also return the Capability/rights/depth it needs), or correct the doc comment. The former also resolves an identical `task_loader.rs:538-543` duplicate by giving all these sites one real shared implementation. + +- **[🟑 MEDIUM]** `TrapFrame` / `SyscallTrapFrame` lack the per-field `offset_of!` pinning the project itself identified as necessary for this exact class of asm/Rust layout drift β€” `bsp-qemu-virt/src/exceptions.rs:112` and `bsp-qemu-virt/src/syscall.rs:90` (contrast: `bsp-qemu-virt/src/cpu.rs:339-344`) + **Action:** Add `core::mem::offset_of!` const-asserts for every field of `TrapFrame` and `SyscallTrapFrame`, mirroring `cpu.rs:340-344`. Consider promoting the pattern into `unsafe-policy.md` as a companion to the existing S5a naked-fn rule so future trap-frame-shaped structs inherit the discipline by default β€” directly relevant as Phase C's SMP/preemption work introduces new trap-frame-shaped structs. + +- **[🟑 MEDIUM]** `FrameProvider::alloc_frame`'s load-bearing zero-initialised contract is documented in prose on a safe trait method, not enforced as an `unsafe fn` contract, unlike the structurally identical `Mmu::create_address_space` β€” `hal/src/mmu/mod.rs:349-355` (contrast `hal/src/mmu/mod.rs:481-488`; consumed unsafely at `bsp-qemu-virt/src/mmu.rs:622-631`) + **Action:** Either mark `FrameProvider::alloc_frame` `unsafe fn` with a `# Safety` section stating the zero-init and exclusive-ownership requirements, mirroring `Mmu::create_address_space`, or add a debug-only zero-scan assertion at the one production consumption point (`walk_or_alloc_table`'s allocate-new-table branch). + +- **[βšͺ LOW]** `add_user_task` hardens the `cap_table` null-pointer hazard for release builds but leaves the equally load-bearing `kernel_stack_top`/`user_sp` invariants debug-assert-only β€” `kernel/src/sched/mod.rs:455-480` and `:510` + **Action:** Defensible today since `add_user_task` is `unsafe fn` and its caller is in-tree BSP/loader code, so treat as polish. For uniform posture, consider the same release-mode degrade-safely pattern for `kernel_stack_top`/`user_sp`, or document in the function's `# Safety` section why `cap_table` alone warranted extra release-mode hardening. + +### Acceptance criteria + +- Every UNSAFE-2026-NNNN audit-log entry cited above matches its actual call site (Location/Invariants fields, Amendment cross-references, and heading text all consistent). +- All module/crate/function doc-comments identified above are corrected to describe the current, landed behavior. +- `could_yield_pa_overlapping`'s doc-comment states the ADR-0033 migration is complete and names the real helper. +- `TrapFrame`/`SyscallTrapFrame` gain `offset_of!` const-asserts for every field, mirroring `cpu.rs`. +- `FrameProvider::alloc_frame`'s zero-init contract is either an `unsafe fn` with a `# Safety` section, or backed by a debug-only zero-scan assertion. + +--- + +## Track B.2-6 β€” Performance micro-fixes + +Two small, low-severity performance findings surfaced by the review land here. Both are explicitly framed by the review itself as deliberate safety-over-performance tradeoffs rather than defects, so neither blocks this phase's exit bar β€” they're tracked so they aren't lost, and so the review's own recommendation to fix them "once the first non-console/production syscall lands" has a durable home. + +### Sub-breakdown + +- **[βšͺ LOW]** console_write's whole-range probe and per-chunk copy re-probe every page redundantly β€” `kernel/src/syscall/dispatch.rs:252-300` + **Action:** This is a deliberate, documented safety-over-performance tradeoff, not an oversight β€” treat as low priority. Once Track B.2-3's length cap lands (bounding worst-case iterations), consider whether a single probed-then-trusted copy pass (skip the chunk-level re-probe once the whole range already passed Gate 3, keeping only the chunk-level re-validate) would meaningfully cut per-page `Mmu::translate` calls without weakening the all-or-nothing guarantee β€” profile first. + +- **[βšͺ LOW]** `copy_from_user`/`copy_to_user` walk the page table twice for every page touched β€” `kernel/src/syscall/user_access.rs:150-154` (`probe_user_pages`), `:199` and `:213-215` (copy_from_user's second translate) + **Action:** Before the first production syscall that carries a real (non-console) buffer lands, restructure `probe_user_pages` to cache the resolved `PhysFrame` per page into a small fixed-size buffer so the copy pass consumes the cached frame instead of re-translating. Keeps the cheap USER-bit re-check while dropping the second 4-level table walk. Low urgency today since the only caller is debug-only; worth fixing opportunistically alongside whichever task adds the first real `copy_to_user` caller. + +### Acceptance criteria + +- Either fixed, or explicitly deferred with a one-line profiling note, once Track B.2-3's console_write length cap lands. + +--- + +## Polish & excellence opportunities + +None of the following are defects β€” they are the review's excellence-track observations, grouped by theme and condensed. They are not gating for Phase B.2's exit bar; pick them up opportunistically alongside the track work above, or leave them for a future phase's backlog. + +### Documentation + +- **Polish** β€” Add a dedicated architecture chapter for the syscall ABI / EL0 execution model, synthesizing the pipeline diagram, capability-gating discipline, audit-log surface, and rollback/error taxonomy that today are scattered across four inconsistently-tensed documents. +- **Polish** β€” Adopt a lightweight "last verified against" freshness marker on each architecture doc, tied to the phase-closure checklist β€” the same staleness root cause (task-loader.md, hal.md, scheduler.md, exceptions.md, overview.md all describing B6 as future work after B6 closed) repeated five times. +- **Polish** β€” Resync overview.md's boot-flow Mermaid diagram with boot.md's now much richer sequence, since overview.md is the first document a reader opens. +- **Polish** β€” Consolidate the scattered Phase-C (multi-core) readiness backlog into one index (in phase-c.md or a short new ADR) cross-linking every deferred single-core assumption. +- **Polish** β€” Close the two dangling append-only-correction gaps (ADR-0011 GICv3, ADR-0012 EL-drop) in the same sweep as the other ADR fixes in Track B.2-2 β€” the only two places the project's otherwise-working forward-pointer mechanism was applied inconsistently. +- **Polish** β€” Revisit the bitflags-crate open question now that two hand-rolled bitfields exist project-wide (Track B.2-4) β€” a short successor ADR/rider deciding "still no" or "yes, adopt now" closes the question before a third and fourth hand-rolled bitfield accrete. +- **Polish** β€” Ground the IrqGuard vtable-aliasing claim (ADR-0008) in rustc/LLVM evidence or retract it β€” the same rigor unsafe-policy demands of safety-relevant unsafe code should apply to safety-relevant ADR rationale duplicated into source rustdoc. +- **Polish** β€” console_write's production/debug gate rides on `cfg!(debug_assertions)` rather than a dedicated Cargo feature; ADR-0031 left this as an open implementation choice, but a dedicated `debug-console` feature (default off) would make the gate's intent explicit and independently auditable. +- **Polish** β€” Extract the `write-adr` skill's Β§Simulation + row-to-verification-mapping discipline into a lightweight linter/checklist, mechanising a check that is currently enforced entirely by careful re-reading. +- **Polish** β€” Automate the drift-detection this review had to do by hand (test-naming adoption, commit-scope drift, untagged fences, broken cross-references) as a standing ~50-line script rather than a periodic full manual pass. +- **Polish** β€” Mark each standard's volatile (CI/tooling-dependent) claims with an explicit enforcement-status tag, mirroring infrastructure.md's exemplary "Required (enforced today)" / "Planned (not yet enforced)" / "Advisory" split β€” the other five CI-referencing standards state their gates in flat prose, which is exactly where this review found staleness. +- **Polish** β€” Give logging-and-observability.md an explicit implementation-status banner β€” the single largest gap between prescriptive tone and implementation reality in the standards folder. +- **Polish** β€” Reconcile commit-style.md's scope enum with reality and add the CI gate the doc itself calls "planned", in one pass, so future scope drift doesn't re-accumulate the way it did between the list's original authoring and today's 38-commit `roadmap` usage. +- **Polish** β€” Institutionalize a roadmap/task-corpus consistency check (diff each phase README's task index against `ls`, flag any task current.md calls "Done" whose own Status isn't, collect tentative ADR numbers across all phase files and flag duplicates) instead of relying on periodic master-review sweeps. +- **Polish** β€” Formalize the `date_done` field in TEMPLATE.md β€” a template field is cheaper to keep correct than a prose convention that has to be independently rediscovered each time, and this exact gap let the Status-drift defect recur nine times in a row. +- **Polish** β€” Add a lightweight relative-link checker to the docs corpus (no network calls, repo-relative-only) β€” near-zero-cost and would have caught two of the broken-link findings above automatically. +- **Polish** β€” Add `` anchors immediately before each glossary headword so the 20 existing cross-references can land precisely, without changing the glossary's flat-list visual format. +- **Polish** β€” Automate the index-vs-disk reconciliation this review had to do by hand (a `sync-task-index`/`sync-review-index` skill, or a CI grep step comparing `ls ` against each README table). +- **Polish** β€” Add a one-line "status as of ``" stamp on high-churn status paragraphs, or have them defer to `docs/roadmap/current.md` as the mechanically-checkable source of truth instead of restating the phase number inline. +- **Polish** β€” Codify a "closure-batch index sweep" checklist item in the business master-plan, mirroring the existing "no closure-trio without recorded smoke" acceptance criterion β€” the performance master-plan's own "Index in README.md updated" AC already exists in principle but isn't enforced at closure time. +- **Polish** β€” Add a "Phase-C readiness sweep" cross-reference table before Phase C tasks are scoped, consolidating the security review, the business review's Adjustments, and phase-c.md's own carry-forwards into one artifact. +- **Polish** β€” Run a second whole-tree master review at the Phase B β†’ Phase C boundary β€” the prior master review's ROI was clearly high (4 Blockers + 18 Majors found and mostly fixed same-week), and a phase boundary is the textbook trigger point for the next run. + +### Build / infra / CI + +- **Polish** β€” Parameterize the QEMU core count (`QEMU_SMP` env var threaded through `tools/run-qemu.sh`, plus a documented cargo-runner override) ahead of Phase C, so `cargo run`/`cargo kernel-run` don't silently stay single-core once the scheduler grows SMP support. +- **Polish** β€” Consider migrating to Rust edition 2024 once Phase C stabilizes, making several hand-enforced lint levels the language default rather than a repo-local override a future crate could forget to inherit. +- **Polish** β€” Make the release profile self-documenting about panic strategy with a one-line comment directly above `[profile.release]` pointing at `.cargo/config.toml`'s rustflags. +- **Polish** β€” Wire `tools/smoke.sh` into CI as a real `qemu-smoke` job β€” the single largest gap between what the project's docs/README/CLAUDE.md claim is verified and what CI actually enforces (see Track B.2-1's findings; this is the same fix, framed as closing that trust gap). +- **Polish** β€” Close the userland-crate clippy gap (Track B.2-1) β€” the userland crates carry the actual EL0/EL1 unsafe syscall-trap boundary, exactly the code CLAUDE.md's rule #2 is most concerned with, yet are the only crates with zero clippy coverage in CI. +- **Polish** β€” Shellcheck the `tools/` scripts, especially `perf-harness.sh` (600 lines of bash with background-job watchdogs, signal traps, and awk pipelines β€” a script that already carries shellcheck-disable directives implying this was the original intent). +- **Polish** β€” Add `--smp` passthrough to `run-qemu.sh`/`smoke.sh` ahead of Phase C (Track B.2-1) β€” cheaper to do now than to retrofit once SMP tasks are already in flight and depend on the tooling. +- **Polish** β€” Add a build-time size guard (`ASSERT(__bss_end - KBASE <= 0x0800_0000 * 64, ...)`) bounding the kernel image against its mapped 128 MiB RAM window, moving a future overflow from a cryptic runtime translation fault to a clear build error. +- **Polish** β€” Add `OUTPUT_ARCH(aarch64)` to both linker scripts β€” a low-cost, purely defensive addition common in embedded/from-scratch OS linker scripts that gives the linker an extra independent cross-arch check. +- **Polish** β€” Extend the aarch64-host build-hazard fix (Track B.2-3) with an actual periodic or PR-gated arm64 host-check CI job β€” the bug class is otherwise structurally invisible to CI, since all runners are x86_64. + +### Kernel: capabilities & syscall + +- **Polish** β€” Document the `debug_assertions` ↔ release-profile dependency directly in Cargo.toml, next to the existing `overflow-checks` comment (see Track B.2-1's release-gate finding for the same underlying concern). +- **Polish** β€” Add a tiny `SyscallError -> &'static str`/`Display` helper for kernel-side diagnostics, kept close to the source of truth rather than letting `tyrne-user`'s error-rendering code re-derive a parallel string table. +- **Polish** β€” Make the `has_current_task` ↔ empty-table pairing self-checking (a `debug_assert`), not just correctly-constructed-by-convention β€” the file's own docstring calls this "the single most security-sensitive control-flow join in the kernel". +- **Polish** β€” Extract the duplicated per-page cursor-walk arithmetic into one shared helper (see Track B.2-4's `copy_from_user`/`copy_to_user` dedup finding) so a future fix to the cursor math automatically applies to both directions. +- **Polish** β€” Add a short worked example to `user_access.rs`'s module doc for future pointer-returning syscalls, lowering the chance a future syscall re-implements a weaker ad hoc validation. + +### Kernel: IPC / objects / scheduler + +- **Polish** β€” Make `SendOutcome` and `RecvOutcome` `#[non_exhaustive]`, matching `IpcError`, for a symmetric forward-compatibility stance across the whole public IPC outcome/error surface. +- **Polish** β€” Document the compound-failure error-priority in `ipc_send` explicitly, matching the treatment the module already gives the ADR-0030 K2-5 ordering commentary. +- **Polish** β€” Give `Task` a hazard doc-comment symmetric with `Endpoint`'s C3-001, pre-empting the same bug class at the same documentation bar. +- **Polish** β€” Add typed `get_*_mut` wrappers for symmetry with `get_task`/`get_endpoint`/`get_notification`, tightening the API surface so "typed handles are the only way in" stays visibly true rather than true only by convention. +- **Polish** β€” Make the `current_idx != next_idx` invariant self-defending instead of debug-only β€” the single invariant the entire ADR-0021 raw-pointer bridge depends on for its no-aliasing soundness claim; the file already has the idiom for this (the idle-filter guard) two dozen lines away. +- **Polish** β€” Amend the UNSAFE-2026-0008 / UNSAFE-2026-0014 audit entries to state the "no slot reuse" precondition explicitly, in the same spirit as ADR-0026's own postmortem lesson about writing an explicit state-machine simulation table. + +### Kernel: memory (PMM/address-space) + +- **Polish** β€” Round out reserved-range capacity boundary test coverage to mirror the existing N*8 exact-fit test, closing the same off-by-one risk class symmetrically. +- **Polish** β€” Collapse or type-distinguish `wrap_bootstrap` vs. `from_mmu_address_space` β€” the "call exactly once, only for the already-live bootstrap topology" invariant is currently documentation-only; a marker type or restricted visibility would make it compiler-checked. +- **Polish** β€” Assert the bootstrap-handle ordering invariant at the BSP call site (`debug_assert_eq!(bootstrap_as_handle, BOOTSTRAP_ADDRESS_SPACE_HANDLE)`), converting a silently-violable convention into a checked one (see Track B.2-3's related finding). + +### Test-HAL & userland + +- **Polish** β€” Clarify the unsafe-policy's "`#[cfg(test)]` modules" wording to explicitly cover dev-dependency-only crates, so SAFETY comments in `test-hal` are literal-compliant rather than compliant-by-charitable-reading. +- **Polish** β€” Document `set_now`'s monotonicity break explicitly, or split it from the monotonic-safe advance API, so a future test author doesn't unknowingly rely on backward-time behavior no real Tyrne target can produce. +- **Polish** β€” Give `tyrne-user`'s `SyscallError` decode helpers instead of a bare status word (see Track B.2-4's decode-API finding) β€” as soon as a second userspace program needs to branch on error kind, it would otherwise reverse-engineer the bit layout by hand. +- **Polish** β€” Wrap capability handles in a userland newtype instead of a bare `u64` (see Track B.2-4's `CapWord` finding) β€” cheap to prevent early, expensive to retrofit once more syscalls/caps exist. +- **Polish** β€” Extend `hello.ld`'s PIC-artifact ASSERTs to relocation sections β€” the one category of PIC artifact the current script's loud-build-time-failure philosophy doesn't yet cover. + +### Cross-cut: concurrency/SMP + +- **Polish** β€” Codify the "single-core cooperative" invariant as one greppable marker (a `const TYRNE_SINGLE_CORE_V1: bool = true;` or `cfg` feature referenced by the ~13 files/subsystems that key off it) instead of ~64 independent prose restatements, turning Phase C's kickoff into a literal, compiler-checkable worklist (`#[cfg(not(feature = "smp"))]` gating the current unsynchronized fast paths). Directly de-risks the Milestone C1/C2 migration. + +### Doc/code contradictions + +- **Polish** β€” Automate a doc/trace drift check in CI β€” this review found exactly one class of defect (a README trace line for a print statement that no longer exists) that a machine-checkable invariant would catch for free. +- **Polish** β€” Bind `docs/roadmap/current.md` updates to the merge commit itself, not a follow-up doc pass β€” the gap is process sequencing, not missing tooling, given the project's already-disciplined skill-driven procedures. +- **Polish** β€” Timestamp/pin the roadmap's top banner to a commit SHA, so a reader can tell at a glance whether it reflects HEAD without cross-referencing `git log` by hand. + +### Cross-cut: performance + +- **Polish** β€” Schedule the per-AS ASID allocator now that Phase C is starting and real per-task address spaces exist β€” `activate()`'s full flush is no longer purely theoretical now that T-028 landed the first real EL0 task with its own AS, and Phase C's cross-core `TLBI VMALLE1IS` broadcast would make the same-core cost far more expensive per switch. Relevant to Milestone C5 (TLB shootdown) β€” landing the already-forward-flagged `AddressSpace::asid` field before Phase C's scheduler work begins avoids compounding an expensive pattern across cores. +- **Polish** β€” Turn the IPC-delivery receiver lookup into O(1) by storing the blocked `TaskHandle` directly in the endpoint's `IpcQueues` waiter state, before Phase C's larger task counts make the already-documented O(N) scan both bigger and harder to retrofit. +- **Polish** β€” Reverse-index (or refcount) capabilityβ†’object references before Phase C destroy/revoke traffic grows, landing alongside the ADR-0023 cross-table-CDT work the in-code note already cross-references. +- **Polish** β€” Re-run and extend the T-029 perf harness after the SVC trampoline callee-saved-register trim (a performance fix tracked outside this phase's routing), adding a fourth measured primitive (`Mmu::activate`'s TLB-flush cost in isolation) to quantify the ASID-allocator opportunity above. + +### Cross-cut: quality/API/testing + +- **Polish** β€” Track `phys_to_kernel_va`/`kernel_va_to_phys`'s hard `assert!` (reachable transitively from the syscall-facing copy path, currently inert) as a named forward-flag for the Phase D (Pi 4) planning docs β€” fixing it now would mean plumbing a `Result` through a `const fn` for no present benefit. +- **Polish** β€” Give `PhysAddr`/`VirtAddr` a small, audited arithmetic/alignment API (see Track B.2-4's newtype findings β€” same fix, framed as excellence). +- **Polish** β€” Add `Nanos`/`Ticks` newtypes to `hal::timer`, turning the exact class of mistake the trait doc warns about into a compile error, at zero runtime cost. +- **Polish** β€” Reconcile `CapRights::from_raw` and `MappingFlags::from_raw`'s diverging unknown-bit semantics, or cross-reference the deliberate divergence explicitly in both doc comments. +- **Polish** β€” Promote `tools/smoke.sh` from a manual maintainer ritual to a first-class CI gate with a companion coverage-of-scenarios report (see Track B.2-1 β€” same fix, framed as rigor-parity with the host-test suite's invariant-based assertions). +- **Polish** β€” Extract a host-testable "resolve syscall context" pure function from `bsp-qemu-virt/src/syscall.rs::syscall_entry` (see Track B.2-4's BSP-trust-boundary finding β€” same fix). +- **Polish** β€” Bundle the recurring `(table, pmm, mmu, as_arena)` quadruple into a small context struct, mirroring `SyscallContext`'s existing pattern, and shrinking several already-flagged `too_many_arguments` sites. +- **Polish** β€” Let `task_loader`'s image-page loop and stack-page loop share their alloc+map+rollback shape β€” the function's own comment already flags this as intentional, acknowledged duplication. +- **Polish** β€” Centralize the u128-to-saturating-u64 cast used by `ticks_to_ns` and `ns_to_ticks` in one place, consistent with the module's own stated goal. + +### Cross-cut: unsafe audit + +- **Polish** β€” Codify the `offset_of!` pinning pattern (Track B.2-5) as a standing unsafe-policy rule, a companion to the existing S5a naked-fn rule, so it automatically covers the next trap-frame-shaped struct Phase C's SMP/preemption work introduces. +- **Polish** β€” Automate the audit-tag-to-source cross-reference the quarterly review currently does by hand β€” this pass found 33 of 33 tags matching bidirectionally, a genuinely strong result worth locking in mechanically as the ~250-site unsafe surface grows through Phase C. +- **Polish** β€” Extend the release-mode fail-closed degrade pattern uniformly within `add_user_task` (see Track B.2-5's related finding) β€” consistency of security posture across a single function's parameters is itself a readability and audit win. + +--- + +## Closing note on ADRs + +Introducing Phase B.2 is itself a **structural change to the roadmap** β€” inserting a phase between B and C β€” and therefore requires a governing ADR per [ADR-0013](../../decisions/0013-roadmap-and-planning.md) and the roadmap change-process (see [roadmap/README.md Β§"Changing the roadmap"](../../roadmap/README.md#changing-the-roadmap): "Adding or dropping a phase… requires an ADR that supersedes the affected statements"). That ADR β€” recording the decision to add a consolidation-bridge phase and superseding the phase-index statements it affects β€” is a **prerequisite for this phase**, tracked as a named-but-unallocated ADR placeholder (in the style of ADR-0034) until it is written via the [`write-adr`](../../../.agents/skills/write-adr/SKILL.md) skill and linked here. + +Beyond that structural ADR, the track work above changes no *other* Accepted architectural decision β€” every item is a documentation correction, a CI wiring change, or a code fix. If, in the course of executing a track, a fix turns out to imply an additional design decision β€” for example, choosing between the two `MappingFlags::DEVICE` remediation options in Track B.2-3, or deciding to add a scheduled (rather than PR-gated) perf-regression CI job in Track B.2-1 β€” write it up and route it through the same ADR governance rather than deciding it silently inside this phase's backlog. + +Covers all 141 review findings + 71 polish items routed to this phase. diff --git a/docs/roadmap/phases/phase-c.md b/docs/roadmap/phases/phase-c.md index 7a3cb37..591ff39 100644 --- a/docs/roadmap/phases/phase-c.md +++ b/docs/roadmap/phases/phase-c.md @@ -137,3 +137,245 @@ Items identified during Phase B closure that survive into Phase C: - Whether per-core queues with work-stealing justify their complexity in v1 (global queue with locking may suffice). - Real-time guarantees for the scheduler (probably "none beyond priority" in v1; richer RT is a later ADR). - Whether ADR-0034 (per-section permissions) should land as C-phase hardening before or concurrently with C1, given that C3's preemption makes the kernel W+X surface larger. + +--- + +## Review-derived work items (2026-07-15 full-repository review) + +The 2026-07-15 full-repository review examined the codebase at the Phase B/C boundary specifically through a concurrency/SMP-readiness lens. Every item below is grouped by which part of Phase C it gates or extends, cross-referencing Milestones C1-C5 and the existing "Carry-forwards from Phase B" list rather than restating their bodies. Severity tags: πŸ”΄ critical Β· 🟠 high Β· 🟑 medium Β· βšͺ low Β· ⚫ info. + +### 1. Entry gate β€” the two SMP-critical defects (must close before any secondary core boots) + +These two findings block Milestone C1's first secondary-core boot outright: C2's `PerCore` migration and C2/C3's frame-allocation work both implicitly assume neither defect still exists. Both are πŸ”΄. + +- πŸ”΄ **`StaticCell`'s blanket `unsafe impl Sync` erases all compiler protection against cross-core aliasing for every kernel global.** + `bsp-qemu-virt/src/main.rs:161-170`. + Action: before Milestone C1/C2 lands any secondary-core boot code, either (a) make `StaticCell: Sync` conditional on a project-defined marker implemented only for types that are genuinely safe to share (this defeats the purpose for most of today's uses), or (b) replace every write-once-then-shared-mutable-access pattern with the `PerCore` abstraction Milestone C2 already plans, wrapped where genuinely shared by the real spinlock / `Cpu::without_interrupts` primitive Milestone C3 introduces. Track this as an explicit **prerequisite gate on C1/C2**, not something C2's `PerCore` work will pick up incidentally β€” every existing `assume_init_mut()` call site in `main.rs` / `perf_bench.rs` / `syscall.rs` needs to be re-audited and re-wrapped, not just the ones a first pass happens to touch. + +- πŸ”΄ **`Pmm` bitmap read-modify-write (`set_bit`/`clear_bit`) is non-atomic; concurrent `alloc_frame` on two cores can double-allocate the same physical frame.** + `kernel/src/mm/pmm.rs:741-746` and `:348-486`. + Action: (1) fix the misleading comment at `pmm.rs:450-453` now β€” it currently asserts a "set_bit atomicity" invariant the code does not provide, which is exactly the class of stale/incorrect SAFETY reasoning that causes real bugs later. (2) **Serialize shared-`Pmm` frame allocation across cores before Milestone C1 brings a second core online.** Use a real cross-core spinlock **or** convert the bitmap to lock-free `AtomicU8`/`AtomicUsize` `fetch_or`/`fetch_and` (`Ordering::AcqRel`). The Milestone C3 `Cpu::without_interrupts` critical-section primitive alone is **not** sufficient here: masking interrupts prevents same-core IRQ-vs-thread reentrancy, not two cores racing on the bitmap β€” cross-core mutual exclusion requires an actual lock or lock-free atomics. **Secondary-core enablement (Milestone C1) must stay gated until one of these exists.** (3) **Put this in the C1/C2 milestone planning, not just this backlog:** as written, none of C1-C5's sub-breakdowns name the physical frame allocator at all, despite it being one of the two or three most safety-critical shared structures in the kernel β€” add an explicit "cross-core `Pmm` serialization" item to Milestone C1's sub-breakdown as a boot-order gate on `CPU_ON`, and/or to Milestone C2 alongside `PerCore`, so the secondary-core boot path cannot land before the allocator is cross-core-safe. + +### 2. Shared-state SMP-safety β€” concurrency contracts and locking discipline (ties to Milestone C1/C2) + +Twenty-eight findings, all variations on the same theme the review's cross-cut/security section names directly: capability-table, arena, PMM, and per-core-identity code is soundness-justified almost entirely by "v1 is single-core," and that premise expires the moment C1 boots a second core. + +**StaticCell audit & consolidation hygiene** (companions to Epic 1's `StaticCell` defect): + +- 🟑 **Multiple `StaticCell` single-write publishes are audited under the wrong UNSAFE-2026 ID.** + `bsp-qemu-virt/src/main.rs:1021,1026,1028,1045,1072,1091,1098,1363,1413,1531,1631` vs. `docs/audits/unsafe-log.md:14-25` and `:133-144`. + Action: sweep every `Audit: UNSAFE-2026-0001` citation in `main.rs` and re-point the plain `StaticCell`-write sites (not `Pl011Uart` construction) to UNSAFE-2026-0010, or add an Amendment to 0001 explicitly folding in the `StaticCell`-write pattern β€” but not leave both entries silently overlapping without a documented reason. +- βšͺ **CI's Miri gate explicitly excludes the `bsp-qemu-virt` crate, so the `StaticCell` aliasing discipline underlying the Epic-1 defect has zero automated soundness verification.** + `.github/workflows/ci.yml:224` (`cargo +$NIGHTLY_PIN miri test --workspace --exclude tyrne-bsp-qemu-virt`). + Action: extract the `StaticCell` aliasing pattern itself (`UnsafeCell>` + blanket `Sync` + `as_mut_ptr`/`assume_init_mut` discipline) into a small, no-asm, host-buildable module that CAN run under `cargo miri test` in isolation, so Miri's Stacked Borrows checker can catch the exact bug class Epic 1 describes before it manifests on real hardware. +- 🟑 **56 call sites in `bsp-qemu-virt` hand-inline the same unsafe `StaticCell` dereference instead of a shared accessor.** + `bsp-qemu-virt/src/main.rs` (31 sites), `perf_bench.rs` (11), `syscall.rs` (11), `exceptions.rs` (1), `cpu.rs` (2). + Action: add `unsafe fn get(&self) -> &T` / `unsafe fn get_mut(&mut self) -> &mut T` to `StaticCell` with one comprehensive `# Safety` doc, and have call sites write `unsafe { CPU.get() }` with a one-line SAFETY comment referencing the type-level contract instead of re-deriving it 56 times. This does not weaken any check β€” every call site is still individually `unsafe {}` β€” it only consolidates the reasoning into one audited definition. + +**Capability-table / Arena / Pmm concurrency contracts:** + +- βšͺ **`ContextSwitch: Send + Sync` ships without the ADR revision note the project's own process requires for post-Accept drift.** + `hal/src/context_switch.rs:32`. + Action: add a short revision-note rider to ADR-0020 recording that the shipped `ContextSwitch` trait carries `: Send + Sync`, matching the precedent the ADR itself already set for the d8-d15 correction. +- ⚫ **`Iommu` is the only HAL trait in the crate without a `Send + Sync` supertrait bound.** + `hal/src/lib.rs:62`. + Action: change to `pub trait Iommu: Send + Sync {}` now, matching the crate-wide convention, so the pinning ADR for its real method surface starts from the already-correct bound. +- βšͺ **Generation counter still has no overflow/poisoning guard.** + `kernel/src/cap/table.rs:642-654` (`free_slot`). + Action: not a blocker β€” the ADR already accepts this for v1. As a cheap proactive step ahead of Phase C, reserve `Generation::MAX` as a permanent-poison sentinel: `free_slot` stops advancing past it and marks the slot permanently unallocatable; `pop_free` never returns a slot at the sentinel. +- βšͺ **`SyscallContext` hands out unsynchronized `&mut` references to cross-task shared kernel objects β€” will not survive Phase C SMP unchanged.** + `kernel/src/syscall/dispatch.rs:78-111`. + Action: per project rule 5, land an ADR now specifying the SMP synchronization strategy for the shared IPC objects (spinlock-guarded arena/queues, per-core partitioning, or a dedicated IPC-server core) before the first multi-core syscall path is implemented, so `SyscallContext`'s shape can be deliberately redesigned rather than retrofitted under time pressure. +- 🟑 **Context-switch anti-aliasing invariant (`current_idx != next_idx`) is enforced only by `debug_assert_ne!`, and the documented proof omits the "no slot reuse" premise it silently depends on.** + `kernel/src/sched/mod.rs:686-689` (documented invariant), `:1179-1206` (`yield_now` switch window), `:1517-1548` (`ipc_recv_and_yield` switch window). + Action: two fixes β€” (1) promote the aliasing check to an unconditional `assert!`, or make aliasing structurally unreachable by comparing `current_idx == next_idx` before taking raw-pointer references (mirroring the existing pattern at line 1462); (2) update the "Shared safety contract" (lines 686-689) and UNSAFE-2026-0008/0014 audit-log entries with an explicit Amendment stating the invariant's true precondition ("no live `TaskHandle` anywhere in `current`/`ready`/`idle` names a slot whose Arena generation has since advanced"), and add that precondition as an acceptance-criterion line item to the future task/slot-reclamation lifecycle ADR (SEC-T024-01's successor). +- βšͺ **`ipc_cancel_recv` authorizes on RECV-right possession, not on caller identity β€” a co-holder of a RECV cap on the same endpoint can silently cancel a different task's pending receive.** + `kernel/src/ipc/mod.rs:563-580` (`fn ipc_cancel_recv`); `EndpointState::RecvWaiting` at `:195`; doc note at `:534-540`. + Action: (1) add an explicit SECURITY doc-comment stating the load-bearing invariant that `ipc_cancel_recv` must only ever be invoked with the exact `ep_cap`/table pair that performed the matching `ipc_recv` (self-rollback only) β€” currently true by convention only. (2) Preferably, thread a lightweight caller/task token alongside `RecvWaiting` now and have `ipc_cancel_recv` verify it before clearing the slot. ADR-0032 Β§Consequences already earmarks this function for the B2+ endpoint-destroy "drain receivers" sweep and for preemption-rollback β€” exactly the kind of caller that could plausibly pass a different task's context if the invariant stays convention-only. +- 🟑 **`alloc_frame`'s wrap-around scan branch (the SMP-forward-compat path) has zero test coverage, and it is exactly the path Phase C is about to make reachable.** + `kernel/src/mm/pmm.rs:383-390`. + Action: since tests in this module already reach into private fields (`pmm.hint`, `pmm.bitmap`), add a targeted white-box test that directly sets `pmm.hint` above a known-free lower index and asserts `alloc_frame` still finds it via the wrap pass. Pins the wrap-scan's correctness independent of whether the "hint <= lowest-free" invariant continues to hold once Phase C introduces concurrent allocators. +- βšͺ **No documented SMP/locking discipline for `AddressSpaceArena` consumers ahead of Phase C.** + `kernel/src/mm/address_space.rs:594-597, 653, 665-669` (repeated "single-core cooperative model" soundness arguments). + Action: when Phase C work begins on this module, reframe these comments around "the caller must hold both `table` and `arena` under a single lock (or single `&mut` borrow) for the duration of the call" rather than "single-core." Add a short module-level "## Concurrency" doc section now, naming the exact invariant (whole-function atomicity across `table` + `arena` + `pmm`) a future spinlock wrapper must preserve. +- 🟑 **`security-model.md`'s "authority dropped on termination" invariant is not upheld by the current implementation, with no caveat noted in the doc.** + `docs/architecture/security-model.md:301` vs. `kernel/src/sched/mod.rs:958-968` and `bsp-qemu-virt/src/syscall.rs:268-293`. + Action: add a "v1 scope limitation" sub-bullet to the "Fault containment does not leak authority" invariant, mirroring the existing revocation-transitivity pattern, pointing at SEC-T024-01/SEC-T028-01 and stating precisely what is/isn't true today. Impact is currently contained (no second/attacker-controlled task exists to receive a reused slot), but this should land before Phase C or before any multi-task, task-creation-from-userspace feature makes slot/table reuse reachable. +- 🟑 **Every capability-table / IPC-state unsafe access is soundness-justified solely by "v1 is single-core" β€” no synchronization design exists ahead of Phase C.** + `kernel/src/cap/table.rs` (whole file β€” no Sync/lock primitives); `kernel/src/ipc/mod.rs` (`EP_ARENA`/`IpcQueues`, no sync); `bsp-qemu-virt/src/syscall.rs:117-141`. + Action: before Phase C lands any second-core dispatch, write the ADR the open-question list already calls for ("Revocation semantics under concurrent use") and decide the concrete model: per-core capability-table ownership, a global lock around `EP_ARENA`/`IpcQueues`, or per-core endpoint sharding. Treat the ~15+ "v1 is single-core" SAFETY comments as a checklist to re-audit item-by-item once that ADR is accepted. + +**Per-core identity & boot topology (ties to Milestone C1/C2):** + +- 🟑 **`IrqGuard` is unconditionally `Send` despite wrapping per-core, execution-context-affine interrupt-mask state.** + `hal/src/cpu.rs:115-135`. + Action: add a `_not_send: core::marker::PhantomData<*const ()>` field (or equivalent) so `IrqGuard` is statically `!Send`, matching its actual core-affine semantics. Land this now, before Phase C introduces the first code path that could plausibly move a guard across cores β€” zero-cost, purely additive, and much cheaper before any caller depends on `IrqGuard: Send`. +- 🟑 **`current_core_id()` derives `CoreId` from `MPIDR_EL1.Aff0` only, with no check that Aff1/Aff2/Aff3 are actually zero.** + `bsp-qemu-virt/src/cpu.rs:226-239` (`Cpu::current_core_id`). + Action: add a `debug_assert!((mpidr >> 8) & 0xFF_FFFF == 0, "...")` documenting and enforcing the flat-topology assumption, so a future multi-cluster configuration fails loudly instead of silently aliasing cores. Note in the comment what changes when GICv3 / >8 cores land. +- 🟑 **No per-core stack mechanism in `_start`/`linker.ld` β€” every core that ever executes `_start` gets the identical `sp = __stack_top`.** + `bsp-qemu-virt/src/boot.s:104-107`; `bsp-qemu-virt/linker.ld:108-110`. + Action: before Milestone C1's secondary-core bring-up lands, extend `_start` to read `MPIDR_EL1` (or accept a core index via PSCI `CPU_ON`'s context-id argument) and index into a per-core stack array sized `NUM_CORES * STACK_SIZE`, reserved in `linker.ld`. Design together with the MPIDR aff-checking fix above so both use the same affinity-to-index mapping. +- 🟑 **`CoreId` is a bare `u32` type alias, not a newtype β€” zero compile-time distinctness, and Phase C (SMP) is the very next phase.** + `hal/src/cpu.rs:12`. + Action: before Phase C lands any per-core state, convert `CoreId` to a proper newtype (`pub struct CoreId(pub u32);` at minimum, matching the project's `IrqNumber` pattern). Zero-runtime-cost, low-effort now (one alias, three call sites); progressively more expensive to retrofit once C2's per-core scheduler code threads `CoreId` through array indices, atomics, and IPI routing. + +**Cross-core panic & fault propagation (ties to Milestone C4's IPI primitive):** + +- 🟑 **Panic handler halts without masking interrupts, so an armed timer IRQ can still execute code after a panic.** + `bsp-qemu-virt/src/main.rs:1666-1701` (`#[panic_handler] fn panic`). + Action: as the very first action of the panic handler β€” before reconstructing the UART β€” mask all four DAIF bits (`asm!("msr daifset, #0xf")`, mirroring `boot.s`'s idiom). Guarantees the diagnostic UART write and final halt loop are truly atomic and that no ISR runs in a kernel state the panic may have left inconsistent. +- 🟑 **Panic handler halts only the panicking core; no cross-core halt/IPI exists, so surviving cores keep mutating shared kernel state a panic may have left inconsistent.** + `bsp-qemu-virt/src/main.rs:1666-1701`. + Action: not called out anywhere in phase-c.md's carry-forwards or milestones today. Add an explicit "cross-core panic propagation" requirement to **Milestone C4** (which already introduces the IPI primitive for cross-core wakeup) β€” reuse that mechanism to broadcast a halt-all-cores signal from the panic handler once IPI support exists. Fine until then (v1 is genuinely single-core), but flag it now so ADR-0043's IPI design accounts for a "halt everyone" message type, not just wakeup. +- ⚫ **EL0 non-SVC synchronous faults (illegal instruction, unmapped deref, alignment fault) halt the entire kernel via `panic_entry` β€” correctly tracked as K3-4/Phase E, but the SMP angle needs a documentation addendum.** + `bsp-qemu-virt/src/vectors.s:60-77` (only `ESR_EL1.EC==SVC64` routes to dispatch); `bsp-qemu-virt/src/exceptions.rs:277-307` (`panic_entry`, unconditional `panic!`); `docs/roadmap/phases/phase-c.md:132` (K3-4 carry-forward). + Action: no change needed to close this item β€” it is already correctly tracked and deliberately deferred to Phase E. One addendum worth adding to the existing K3-4 carry-forward note now: under SMP, a panic on one core currently has no defined story for signalling/halting the *other* cores (today's single-core `loop { spin_loop() }` halts everything by being the only core). When Milestone C1/C2 land, the K3-4 write-up (or ADR-0041 per-core state) should say explicitly whether a same-core EL0 fault still takes the whole machine down post-SMP, or is scoped sooner. + +**Console / shared I/O state:** + +- 🟑 **`FakeConsole`'s `Mutex` fully serializes writes, eliminating the byte-interleaving the `Console` contract permits and the real UART driver explicitly produces.** + `test-hal/src/console.rs:8-11, 58-64`. + Action: either (a) add an explicit doc caveat that `FakeConsole` cannot reproduce byte-level interleaving and should not be used to assert "no corruption under concurrency," or (b) add an opt-in interleaving-capable variant (e.g. `FakeConsole::new_interleaved()` that releases the lock between bytes) so Phase C multi-core tests can actually exercise and catch missing synchronization around console/logging call sites. +- βšͺ **`Console`/`Pl011Uart` writes are unsynchronized across cores by explicit design, accepted as "best-effort" β€” a real but consciously-scoped risk for Phase C debuggability.** + `bsp-qemu-virt/src/console.rs:46-58` and `hal/src/console.rs:26-29`. + Action: consider a minimal per-line spinlock (or a per-message length/sequence-number prefix) around `write_bytes`, specifically for the boot-banner and panic paths, once Milestone C1 brings up secondary cores. Debuggability quality-of-life only, not a correctness requirement given the trait's explicit contract. + +**SMP-readiness testing debt:** + +- ⚫ **`Scheduler`'s single global `current`/`idle`/`ready`/`contexts` shape is the review's explicit SMP-readiness inventory item for the scheduler's core data layout.** + `kernel/src/sched/mod.rs:249-330` (Scheduler fields) and `:772-799` (`register_idle`). + Action: no action needed beyond what Milestones C2/C3 already plan. Recorded here to give the eventual ADR-0042 author a precise enumeration of exactly which `Scheduler` fields need per-core partitioning vs. a shared lock (ready queue and `contexts` are natural lock-protected-shared or per-core-with-work-stealing candidates; `task_cap_tables`/`task_address_space_handles`/`task_user_windows` are read-mostly per-task metadata a `RwLock`-shaped primitive would suit). +- 🟑 **Coverage-percentage and Miri baseline reports have not been refreshed since 2026-04-27, despite ~2.5x test growth and entirely new subsystems landing.** + `docs/analysis/reports/2026-04-27-coverage-rerun.md`; `docs/analysis/reports/2026-04-23-miri-validation.md`. + Action: produce a fresh `docs/analysis/reports/-coverage-post-phase-b.md` (and a paired Miri narrative update) now that Phase B is closed β€” reuse the existing template and per-file triage method. Establishes the coverage floor Phase C's SMP work will be measured against. +- 🟑 **Test harness (test-hal + kernel `#[cfg(test)]`) is entirely single-threaded; the raw-pointer scheduler/IPC aliasing discipline (ADR-0021) that Phase C's SMP work will directly stress has no multi-threaded test double.** + `docs/analysis/reports/2026-04-23-miri-validation.md` ("What this does NOT validate" + "Next measurement"); `test-hal/src/cpu.rs` (`FakeCpu`, single-threaded); `kernel/src/sched/mod.rs:2558-2570` (`ResetQueuesCpu`'s `unsafe impl Send/Sync`, never exercised across a real thread). + Action: before any Phase C PR lands cross-core scheduler/IPC/cap-table mutation, add a genuinely concurrent test-hal double (e.g. a `ThreadedCpu` running `context_switch` bodies via real `std::thread::spawn` + a barrier, or a loom-style model-checked harness) and re-run Miri/the aliasing tests under it. Track explicitly as a Phase C prerequisite, mirroring how ADR-0032's preemption forward-flags are tracked. +- ⚫ **`testing.md`'s "Layer 2: Integration tests" (per-crate `tests/` directory) is defined but entirely unused.** + `docs/standards/testing.md:21-26`; workspace-wide `find -type d -name tests` returns no matches. + Action: either (a) reconcile the doc β€” note that v1 satisfies the cross-module-contract goal via inline tests and that `tests/` is reserved for genuine cross-*crate* scenarios, or (b) start populating `kernel/tests/` with a true end-to-end scenario now that Phase C adds cross-crate SMP concerns inline unit tests are less suited to model. + +**Address-space preflight-then-commit family (same remediation family; needs Milestone C3's lock primitive):** + +- 🟑 **`cap_create_address_space`'s preflight-then-commit sequence is non-atomic across the whole function body, relying solely on "single-core cooperative" for correctness.** + `kernel/src/mm/address_space.rs:586-603` and `:649-679`. + Action: same remediation family as the Epic-1 `Pmm` finding β€” this function needs to run under whatever per-CPU-lock primitive Milestone C3's `Cpu::without_interrupts` establishes before any code path can call it from more than one core. Worth noting explicitly in ADR-0042 or a companion capability-subsystem note, since this "preflight then commit across N steps" pattern likely recurs elsewhere in the object layer. +- 🟑 **`task_loader.rs`'s `load_image` has the identical unlocked preflight-then-commit pattern as `cap_create_address_space`.** + `kernel/src/obj/task_loader.rs:579-587` (frame-budget preflight) and the subsequent commit steps later in `load_image`'s body. + Action: fold into the same remediation family as the finding above rather than a separate follow-up β€” both `cap_create_address_space` and `load_image` need to run under the Milestone C3 per-CPU-lock primitive before either can be called from more than one core concurrently. Name both call sites explicitly in the eventual capability-subsystem SMP-readiness ADR. + +### 3. Preemption & TOCTOU (gates Milestone C3; extends into C4/C5) + +- 🟠 **`ipc_send`'s `unreachable!()` is a self-documented ticking time bomb for Phase C preemption, and is not carried into the Phase C roadmap's tracked-items list.** + `kernel/src/ipc/mod.rs:357-371` (the `unreachable!()` arm); cross-ref `docs/roadmap/phases/phase-b.md:287` (flags it as "later-phase, tracked"); `docs/roadmap/phases/phase-c.md:128-132` ("Carry-forwards from Phase B" β€” **omits it**). + Action: two independent actions β€” (1) **add this item explicitly to this file's "Carry-forwards from Phase B" list**, alongside SEC-T028-01/ADR-0034/K3-4, so it cannot be lost between phase-closure documents; (2) land the fix the code itself already recommends (see finding directly below) as part of, or immediately before, Milestone C3, ideally pre-emptively rather than discovered via a crash under QEMU `-smp N` testing. +- 🟑 **`ipc_send`'s queue-full commit path panics via `unreachable!()` instead of erroring β€” cheap to defuse now.** + `kernel/src/ipc/mod.rs:330-373`. + Action: replace `unreachable!()` with `return Err(IpcError::QueueFull)` β€” a single-line, zero-risk change identical to what `ipc_recv` already does for its analogous state β€” rather than waiting for Milestone C3 preemption work to make the race live. `IpcError::QueueFull` and its `SyscallError` composition already exist and are already tested for the ordinary QueueFull path. +- 🟠 **`copy_from_user`/`copy_to_user`'s two-pass probe-then-copy is a TOCTOU window the moment address-space mutation can happen concurrently with a syscall.** + `kernel/src/syscall/user_access.rs:198-249` and `:280-317`. + Action: this is explicitly the concern **Milestone C5** (TLB shootdown) partially addresses, but C5's acceptance criteria ("cross-core unmap is safely observable on all cores before the next memory access") describe post-unmap visibility, not protection of an in-flight probe-then-copy window against a *concurrent* unmap. When C4/C5 land, either (a) re-translate-and-recheck per byte-run atomically with the unmap path (hold a per-AS lock across the whole copy, not just the probe), or (b) make unmap defer physical-frame reuse until an RCU-like grace period / IPI-synchronized quiescence, so a stale translation used mid-copy still points at valid (if logically-unmapped) memory. Flag explicitly in the ADR-0044 (TLB shootdown) design doc β€” this is narrower and more security-relevant than plain TLB staleness. +- βšͺ **`copy_from_user`/`copy_to_user`'s two-pass probe-then-copy correctness rests on an explicit single-core assumption with no test (or test harness) to catch a Phase C violation.** + `kernel/src/syscall/user_access.rs:201-203, 238-241, 308-309` (SAFETY comments). + Action: file this explicitly as a test-debt/forward-flag item (mirroring ADR-0032's preemption forward-flags) so whatever ADR introduces cross-core page-table mutation must either (a) prove single-writer-per-AS still holds under SMP, or (b) add a re-validation-on-copy (not just on-probe) discipline, with a test that models the race via test-hal (e.g. a decorator MMU that mutates a mapping between a probe and copy call) before Milestone C3's scheduler work ships. +- βšͺ **The file's single-core, DAIF-masked "no interleaving" soundness basis is not carried forward into the Phase C (SMP) plan.** + `kernel/src/syscall/user_access.rs:236-241, 303-310`. + Action: add this file's interleaving invariant as an explicit Phase C carry-forward item (alongside SEC-T028-01/ADR-0034/K3-4 in this file), or add a code-level forward marker pointing at ADR-0044, so whoever lands C3/C4/C5 is forced to re-examine and re-prove (or re-synchronize, via the TLB-shootdown protocol) this file's two-pass translate-then-copy design before SMP ships. +- βšͺ **Running-task syscall context is assembled from three separate, non-atomic scheduler reads.** + `bsp-qemu-virt/src/syscall.rs:169-171`. + Action: before Phase C introduces cross-core mutation of scheduler task-bindings, either add a single `Scheduler` accessor that returns all three pieces from one borrow of `current`, or add an explicit comment naming "no interleaving between these three reads" as a precondition to be re-verified when SMP scheduling design lands. +- βšͺ **Gate-#3 running-task context is exposed as three independently-read accessors, not a single atomic snapshot.** + `kernel/src/sched/mod.rs:530-550` (`current_user_table` / `current_address_space_handle` / `current_user_window`). + Action: add a single `Scheduler::current_syscall_context(&self) -> Option<(*mut CapabilityTable, AddressSpaceHandle, UserAccessWindow)>`-shaped accessor that reads `self.current` exactly once and returns all three, and have the BSP `syscall_entry` call that instead of the three separate methods. Closes the documented forward-flag at negligible cost today; same remediation family as the finding directly above (BSP-side vs. kernel-side of the same non-atomicity). +- βšͺ **Stale docstring in `vectors.s` claims "v1 has no userspace," contradicted by the project's shipped EL0 syscall boundary.** + `bsp-qemu-virt/src/vectors.s:285-291` (`tyrne_unhandled_irq_trampoline` docstring). + Action: update the comment to state the real, current invariant β€” an IRQ cannot currently be taken while at EL0 because `enter_el0` masks DAIF for the EL0 dispatch (cite ADR-0037 Β§Decision outcome) β€” and note that this trampoline must be replaced with a real EL0-aware IRQ path (saving `SP_EL0` at minimum) **before Milestone C3's preemptive EL0 lands**. +- 🟑 **SVC trampoline spills 11 GPRs the AAPCS64 ABI already guarantees preserved, on every single syscall.** + `bsp-qemu-virt/src/vectors.s:206-211,245-250` (`tyrne_sync_trampoline`); `bsp-qemu-virt/src/syscall.rs:73-79` (`SyscallTrapFrame`). + Action: trim `SyscallTrapFrame`/`tyrne_sync_trampoline` to the set that is *not* ABI-guaranteed: x0-x18, x30, plus SP_EL0/ELR_EL1/SPSR_EL1 β€” mirroring the IRQ trampoline's already-applied reasoning. Update the `#[repr(C)]` struct, its `size_of` guard, and doc comments (frame drops from 272 to ~184 bytes). Re-run the T-029 `perf-bench` build to confirm the round-trip cost drops. Relevant to Milestone C3 because a shorter, translate-once critical section is a tighter interrupt-latency bound once preemption is live; if a future preemptive-syscall design genuinely needs a complete register snapshot at this exact point, that should be re-justified by the ADR that introduces it. + +### 4. Multi-core MMU / GIC / TLB (gates Milestone C5) + +- 🟑 **TLBI mnemonics are local-PE-only, but comments and the unsafe-log audit entries claim (and rely on) cross-core broadcast semantics for future SMP.** + `bsp-qemu-virt/src/mmu.rs:235, 403-408, 422, 432-434, 442`; `docs/audits/unsafe-log.md:480,486,490,492,514`. + Action: switch the three call sites to the inner-shareable broadcast encodings (`tlbi vae1is, {reg}` / `tlbi vmalle1is` x2). The existing `dsb ish` immediately after each TLBI then correctly waits for the broadcast invalidation domain-wide, matching what the comments already claim. Update UNSAFE-2026-0023/0024's "Rejected alternatives"/"Invariants relied on" text to stop asserting no asm change is needed for SMP. This also simplifies **Milestone C5**: ARM's IS-suffixed TLBI gives hardware-broadcast invalidation for free within one inner-shareable domain, without requiring a software IPI-shootdown protocol for the common single-domain case. +- 🟑 **`Mmu::map`/`unmap`'s intermediate-table-allocation walk has no documented (or enforced) mutual-exclusion contract for concurrent callers on the same `AddressSpace`.** + `bsp-qemu-virt/src/mmu.rs:575-634` (`walk_or_alloc_table`), `:491-560` (`walk_and_install_leaf`). + Action: before Phase C's first SMP boot lands, (a) add an explicit "must not be called concurrently for the same `AddressSpace`; callers must hold a per-AddressSpace lock" clause to the `Mmu` trait doc-comment and to both functions' SAFETY sections now, and (b) introduce the actual per-`AddressSpace` lock at the kernel `cap_map`/`cap_unmap` layer (`kernel/src/mm/address_space.rs`) ahead of enabling a second core. Same class of latent, currently-invisible concurrency debt as the Pmm finding in Epic 1 β€” parallel in urgency, distinct in mechanism (data race on page-table mutation vs. missing cross-core cache invalidation). +- 🟑 **No secondary-core park guard: `_start` will race on every core the instant `-smp` is raised above 1.** + `bsp-qemu-virt/src/boot.s:45-62` (whole `_start`, no MPIDR_EL1 check anywhere in the file). + Action: add a cheap, permanent defensive guard at the very top of `_start` β€” read `MPIDR_EL1`, mask to AFF0 (bits 7:0), and if non-zero, `wfe`-park in a dedicated `secondary_park:` loop (mirroring the existing `halt_unsupported_el` pattern) instead of falling through. ~4 instructions; makes a future `-smp N>1` fail safe instead of racing, and gives **Milestone C1**'s PSCI `CPU_ON` work a natural place to wake secondaries later. Land now, independent of the Phase C SMP task, as forward-hardening. +- 🟑 **`QemuVirtGic::init()` conflates one-time distributor-wide programming with per-core CPU-interface bring-up.** + `bsp-qemu-virt/src/gic.rs:153-247`. + Action: before **Milestone C1** lands secondary-core start, split `init()` into `init_distributor()` (steps 1-6, called once by the primary core before any secondary comes up) and `init_cpu_interface()` (step 7, called by every core including the primary, once each). Small, low-risk refactor to do now (still single-core, zero behavioural change if the BSP calls both in sequence) that removes an obvious footgun for whoever writes the C1 secondary-core entry path. +- 🟑 **Entire timer-IRQ delivery path (trampoline round-trip, GIC acknowledge/EOI) has never actually fired in this codebase, as of "Phase B complete."** + `bsp-qemu-virt/src/exceptions.rs:186-275` (`irq_entry`); no runtime caller of `arm_deadline` anywhere in the tree. + Action: before or alongside Phase C work, add a minimal smoke path that actually exercises `irq_entry`'s timer arm β€” a boot-time `arm_deadline` call with a short deadline in the BSP demo, or a dedicated integration test asserting the demo observes the IRQ round-trip via a serial marker line. Converts the "Pending QEMU smoke verification" status in UNSAFE-2026-0019/0020/0021 into an actual pass and gives **Milestone C3**'s timer-tick wiring and **Milestone C4**'s IPI extensions a verified foundation to extend rather than an unverified one. +- βšͺ **`QemuVirtMmu::activate()`'s barrier order (ISB then DSB ISHST) is the reverse of the DSB-before-enable pattern used correctly elsewhere in the same file.** + `bsp-qemu-virt/src/mmu.rs:227-243` (`Mmu::activate`). + Action: align `activate()` with the break-before-make (**flush-then-install**) ordering the Phase D Epic 2 finding on this same function mandates β€” the TLB invalidation and its barriers must complete **before** the `TTBR0_EL1`/`EPD0` write, not after it: `dsb ishst` β†’ `tlbi vmalle1` β†’ `dsb ish` (drain/invalidate the outgoing context while `TTBR0` still names it), then `msr ttbr0_el1, {ttbr0}` + the `EPD0` clear, then a closing `isb`. Cross-reference the two sites (`activate` and `high_half_activate`) in a comment so a future editor who touches one updates the other, and document whether the caller must mask IRQ/FIQ around the call (the corrected ordering removes the CPU's-own-lookup race but not a genuinely concurrent access from another exception context). The authoritative ordering is the flush-then-install one in [Phase D](phase-d.md#review-derived-work-items-2026-07-15-full-repository-review)'s MMU-correctness epic β€” keep the two entries in sync. (This supersedes the earlier "publish-before-enable" phrasing.) +- ⚫ **`compiler_fence(SeqCst)` on the spurious-IRQ return path provides no actual guarantee and is labelled misleadingly as "defence-in-depth."** + `bsp-qemu-virt/src/exceptions.rs:211-222`. + Action: either remove the fence (it currently does nothing observable), or replace the comment with an honest note that a *future* SMP-visible write in this branch would need a real memory barrier (`core::sync::atomic::fence` backed by `dmb`, not just `compiler_fence`), not this one. + +### Polish & excellence (SMP prep) + +The 29 routed polish items are not defects; each is marked **Polish** and condensed into thematic groups without dropping any. + +**HAL / BSP / aarch64:** + +- **Polish** β€” Document the `Mmu` trait's concurrency contract ahead of Phase C: the trait's existing `Send + Sync` bound leaves the actual method-level safety obligation unstated. +- **Polish** β€” Build a consolidated "single-core assumptions" inventory: ~19 statics each individually document "single-core cooperative" as their soundness argument; one audit-ready inventory turns 19 scattered greps into a single SMP-transition checklist. +- **Polish** β€” Design fault isolation for non-SVC sync exceptions from EL0 before Phase C's multi-task/multi-core world makes a single-task fault routine rather than exceptional (touches P1/P2 in spirit). +- **Polish** β€” Make the SMP-broadcast TLBI change now, while cheap: no behaviour change for single-core v1 (IS-suffixed TLBI is a strict superset of local-only), high payoff for Milestone C5 planning. +- **Polish** β€” Split `QemuVirtGic::init()` into `init_distributor()`/`init_cpu_interface()` ahead of Milestone C1, even if `init()` stays a convenience wrapper for the still-single-core v1 boot path. +- **Polish** β€” Add a boot-time self-test that actually fires an IRQ through the trampoline before relying on the timer, converting UNSAFE-2026-0019/0020/0021's "Pending QEMU smoke verification" into an actual pass ahead of Milestone C3/C4. +- **Polish** β€” Expose `set_priority`/`set_target` on `QemuVirtGic` now, even if unused by v1, so Milestone C4's IPI-vs-device-IRQ priority and per-core SPI routing work lands as a consumer of an already-reviewed API. +- **Polish** β€” Log task identity and exit code on `task_exit` instead of a generic message β€” once Phase C brings concurrent tasks, a code-free exit message turns every multi-task failure into a guessing game. +- **Polish** β€” Bundle the fail-closed console-DoS and TOCTOU items into one pre-Phase-C hardening pass, resolved or explicitly accepted at Phase C kickoff per CLAUDE.md's phase-pause process. +- **Polish** β€” Signpost the local-vs-broadcast TLBI call sites for Phase C, keeping the well-tracked TODO discoverable where a future SMP author will look first. +- **Polish** β€” Land a trivial secondary-core park guard now, ahead of Phase C, converting "silent multi-core boot corruption during early Phase C spikes" into "secondaries park quietly." +- **Polish** β€” Consider a boot self-test/latch pattern for the handful of must-run-exactly-once routines, converting a future refactor mistake (e.g. SMP work accidentally calling `kernel_entry` per-core before the park guard lands) from silent page-table corruption into a loud, diagnosable panic. + +**Kernel: capabilities & syscall:** + +- **Polish** β€” State `CapabilityTable`'s concurrency contract explicitly on the type itself (not just in ADR-0021): "Not internally synchronized: every mutating method requires `&mut self`... Phase C's per-CPU or locked-table story will wrap this guarantee rather than change it." +- **Polish** β€” Cap `console_write`'s length and land the SMP synchronization ADR before Milestone C3's first multi-core syscall path, per rule 5. +- **Polish** β€” Fuse `copy_from_user`/`copy_to_user`'s probe-pass and copy-pass to translate each page once instead of twice, shortening the DAIF-masked critical section relevant once Milestone C3's `without_interrupts` discipline lands. +- **Polish** β€” Give `copy_from_user`/`copy_to_user` their own bounded-chunk discipline (not just `console_write`'s), so an unbounded interrupt-masked copy doesn't become a scheduling-latency outlier once Milestone C3's timer-tick preemption is live. +- **Polish** β€” Consider `clippy::indexing_slicing` (or equivalent) to close the one implicit-panic class the current lint set doesn't cover, proposed as its own small ADR/task rather than flipped on unilaterally, given a panic on one core is a bigger liveness problem under Phase C SMP than today. + +**Kernel: IPC / objects / scheduler:** + +- **Polish** β€” Document `Arena`'s single-core contract explicitly in code (a `# Concurrency` module-doc section citing ADR-0016 Β§Open questions), and weigh an explicit `!Sync` marker in the future multi-core ADR so cross-core sharing must opt in via a deliberate wrapper. +- **Polish** β€” Close the gate-#3 non-atomicity forward-flag now, while free: one struct and one field read today vs. re-auditing every call site after preemption lands. + +**Kernel: memory (PMM/address-space):** + +- **Polish** β€” Formalize the Phase C (SMP) synchronization plan for `Pmm` before multi-core lands β€” an ADR amendment to ADR-0035 or a fresh ADR committing to global spinlock, `AtomicU8`/CAS bitmap words, or per-core free-list caches, turning the "SMP extension keeps this invariant via set_bit atomicity" comment into a discharged, implemented guarantee. +- **Polish** β€” Add a debug-only `Pmm` self-check that cross-verifies bitmap, `reserved_ranges`, and counters in one pass, paying for itself once SMP or capability-driven `MemoryRegionCap` grants mutate the PMM from more call sites. +- **Polish** β€” Add an atomic `cap_protect` primitive for permission transitions, letting future loader/JIT code implement true write-XOR-execute transitions without the fault-prone unmap/remap window, strengthening the W^X story ahead of ADR-0034. + +**Test-HAL & userland:** + +- **Polish** β€” Give `FakeMmuState` a per-core dimension ahead of Phase C (key `activated_root` and TLB logs by `CoreId`/thread-id), avoiding a scramble once the first multi-core scheduler test needs it. +- **Polish** β€” Give `FakeConsole` an interleaving-capable mode ahead of Phase C, so higher-level logging code's tolerance of non-atomic multi-byte console writes can be host-tested before it's tested for the first time on real hardware. +- **Polish** β€” Add a strict/gated acknowledge mode to `FakeIrqController`, so a missed `enable()` (silently-dead timer preemption) or a double-EOI (desynchronized interrupt priority state) becomes catchable pre-Phase-C rather than a live-hardware surprise. + +**Cross-cut: concurrency/SMP:** + +- **Polish** β€” Add an explicit "PMM / capability-table SMP audit" task to Milestone C2 or C3's sub-breakdown: "Audit and lock/atomic-ify `Pmm` and `CapabilityTable`/`AddressSpaceArena` for cross-core access," closing the gap between the roadmap and the concrete inventory this review produced. + +**Cross-cut: quality/API/testing:** + +- **Polish** β€” Encode `SyscallContext`'s "current task" as a structural `Option` rather than four independently-set fields plus a bool, since Phase C and any second BSP will add more construction sites where `has_current_task = true` could be set without a resolved `task_as`/`caller_table`. +- **Polish** β€” Publish a Phase-C-opening coverage + Miri baseline as the SMP-readiness gate, giving a clean before/after line for whatever coverage regressions (or improvements) Phase C introduces. + +**Cross-cut: security:** + +- **Polish** β€” Write the ADR for capability/IPC-state concurrency control before Phase C's first second-core dispatch, per rule 5/6, turning a standing open question every future unsafe block can cite into a concrete pre-Phase-C gate. + +--- + +Covers all 46 review findings + 29 polish items routed to this phase. diff --git a/docs/roadmap/phases/phase-d.md b/docs/roadmap/phases/phase-d.md index e944882..b72f8b1 100644 --- a/docs/roadmap/phases/phase-d.md +++ b/docs/roadmap/phases/phase-d.md @@ -176,3 +176,155 @@ Numbers are tentative; final numbers are assigned when the ADR is actually writt ## Resolved - **SD-image composition including the Pi's closed-source firmware blobs.** Resolved per [ADR-0004](../../decisions/0004-target-platforms.md): closed-source blobs that sit *below* the kernel (e.g., the VC4 stage-0 firmware on Pi 4) are out of our blob-policy scope. The SD image may therefore include them when that is the only way to boot the hardware. + +--- + +## Review-derived work items (2026-07-15 full-repository review) + +The 2026-07-15 full-repository review turned up a cluster of aarch64 hardware-correctness findings in `hal/` and `bsp-qemu-virt/` that are, without exception, **silent on QEMU `virt`** β€” the emulator either doesn't model the trapping/timing/register behaviour precisely enough to expose them, or the current single-task/single-core demo workload never exercises the code path that would. None of them have caused an observed failure to date. All of them are the kind of gap that turns into a boot hang, a silent data leak across an address-space or privilege boundary, or a hard-to-diagnose corruption the first time the same code runs against real GIC-400/PL011/MMU silicon on a Pi 4 β€” several are explicitly inherited wholesale by `bsp-pi4` per Milestone D5's "inherits VMSAv8 from QEMU's impl" and Milestone D2's "differs from `bsp-qemu-virt`'s GICv2 only in base addresses and board specifics." They are grouped below into three epics and must be resolved as part of Phase D hardware bring-up, not deferred past it. A **Polish & excellence** subsection follows with the review's non-defect quality findings routed to this phase. + +### Epic 1 β€” Privilege-boundary register hygiene + +EL2β†’EL1 drop and EL0/EL1 trap-frame handling that omit or under-specify register scrubbing. QEMU's emulated core never faults on an unset CPTR_EL2 and never surfaces FP/SIMD state corruption across a trap in the current single-task demo; real silicon will not be as forgiving, and Phase C's preemption/multi-task work (validated on Pi 4 in Milestone D8) is exactly the workload that would first expose a leak here. + +- **[🟠 HIGH]** `ContextSwitch::init_user_context`'s trait-level contract omits the security-critical register-scrub obligation that ADR-0037 identifies as HIGH severity. + - Location: `hal/src/context_switch.rs:84-121` + - Action: Add an explicit bullet to `init_user_context`'s `# Safety` section (or a new normative `# Implementor contract` subsection, since this is an obligation on implementations, not callers) stating that implementations must scrub every EL0-readable register not part of the explicit `(user_entry, user_sp)` hand-off β€” GPRs, SIMD/FP registers, and FPCR/FPSR/TPIDR_EL0/TPIDRRO_EL0 β€” before dropping to EL0, so no EL1 kernel state (pointers, stack addresses, capability data) is disclosed to userspace. Today this fact is recoverable only by archaeology through ADR-0037's revision history and the BSP's unsafe-log entry; it needs to be durable and discoverable in the one contract every future BSP author (starting with whoever writes `bsp-pi4`'s context-switch code) is expected to read. + - Gates: Milestone D1 (`bsp-pi4` scaffolding mirrors `bsp-qemu-virt`'s context-switch structure) and Milestone D8 (preemption/multi-task validation on hardware). + +- **[🟠 HIGH]** CPTR_EL2 is never initialized before the EL2β†’EL1 drop, unlike HCR_EL2/SPSR_EL2/ELR_EL2, which are all explicitly pinned. + - Location: `bsp-qemu-virt/src/boot.s:64-98 (el2_to_el1)` + - Action: Add an explicit CPTR_EL2 write inside `el2_to_el1`, before the `eret` β€” e.g. `mov x0, #0x33ff` / `msr cptr_el2, x0` β€” clearing TFP so FP/SIMD is never trapped to EL2 (RES1-safe for the pre-SVE register format). Document the choice the same way HCR_EL2/SPSR_EL2 are documented, and add an ADR-0024 addendum or audit-log note since this sits in the same UNSAFE-2026-0017-audited block. + - Gates: Milestone D1 β€” `bsp-pi4/src/boot.s` is scaffolded by mirroring `bsp-qemu-virt/src/boot.s`; fix this in the source before it is copied into a second BSP. + +- **[🟠 HIGH]** The IRQ trap frame never saves/restores FP/SIMD register state, despite CPACR_EL1 explicitly enabling untrapped FP/SIMD and the codebase's own boot comment admitting the compiler emits NEON in ordinary (non-float) code. + - Location: `bsp-qemu-virt/src/exceptions.rs:47-112 (TrapFrame)`, `bsp-qemu-virt/src/vectors.s:115-160 (tyrne_irq_curr_el_trampoline)`; root cause enabled at `bsp-qemu-virt/src/boot.s:109-116`. + - Action: Pick one of: (a) add `-C target-feature=-neon,-fp-armv8` (or equivalent) to the `aarch64-unknown-none` rustflags in `.cargo/config.toml` so "no FP/SIMD anywhere in the kernel image" becomes a compiler-enforced invariant, matching the project's existing preference for compile-time guards (cf. the `size_of::() == 192` assert); (b) extend `TrapFrame`/the IRQ trampoline to save/restore the caller-saved FP/SIMD registers (V0-V7, V16-V31, FPSR, FPCR); or (c) set `CPACR_EL1.FPEN = 0` for kernel-mode execution so an inadvertent FP/SIMD access traps loudly instead of silently corrupting state, consistent with the fail-loud philosophy already used in `panic_entry`. Record the choice as an ADR/audit-log entry. + - Gates: Milestone D8 (preemption/multi-task IPC on hardware is the first workload likely to interleave FP/SIMD-touching code across an IRQ). + +- **[🟠 HIGH]** Exception trampolines never save/restore/scrub the SIMD-FP register file (V0-V7, V16-V31, FPCR/FPSR) across an EL1 trap, despite FP/SIMD being live-usable at EL0/EL1. + - Location: `bsp-qemu-virt/src/vectors.s:127-160 (tyrne_irq_curr_el_trampoline)` and `bsp-qemu-virt/src/vectors.s:195-253 (tyrne_sync_trampoline)`. + - Action: Either (a) save/restore the full (or at minimum caller-saved: V0-V7, V16-V31) SIMD/FP register file plus FPCR/FPSR in both trampolines; or (b) scrub V0-V31/FPCR/FPSR on every `tyrne_sync_trampoline` restore path (not just the one-shot `enter_el0`) to close the confidentiality gap even if (a) is deferred for cost reasons; or (c) structurally prevent the hazard via a codegen restriction (`-C target-feature=-neon` plus a reviewed escape hatch for the boot-time zero-init the project already relies on). At minimum, add a `# Safety`/audit-log note next to both trampolines recording this as a known, unaudited gap β€” today neither UNSAFE-2026-0020 nor UNSAFE-2026-0029 mentions FP/SIMD register scope. + - Gates: Milestone D8, same rationale as above; overlaps directly with the IRQ trap-frame finding. + +- **[βšͺ LOW]** `context_switch_asm` saves/restores d8-d15 (FP data) but never FPCR/FPSR (FP control/status). + - Location: `bsp-qemu-virt/src/cpu.rs:372-423 (context_switch_asm)`; cf. `Aarch64TaskContext` at `cpu.rs:298-329`. + - Action: Either add `fpcr`/`fpsr` fields to `Aarch64TaskContext` and save/restore them alongside d8-d15 (cheap: two more registers), or explicitly document in the struct's doc comment that kernel-level cooperative tasks are assumed never to modify FPCR/FPSR, so the omission is a documented invariant rather than an unexamined gap. + - Gates: Milestone D1/D8 β€” same register-hygiene sweep as the two HIGH trampoline findings above; fix together. + +### Epic 2 β€” MMU & TLB hardware correctness + +Descriptor construction, activation ordering, and walk/translate logic in the shared VMSAv8 code that `bsp-pi4`'s `Mmu` impl will inherit near-verbatim per Milestone D5. QEMU's TLB/MMU emulation tolerates orderings and omissions that real Cortex-A72 silicon may not. + +- **[🟠 HIGH]** `Mmu::activate` installs the new TTBR0 before invalidating stale TLB entries β€” break-before-make is inverted. + - Location: `bsp-qemu-virt/src/mmu.rs:227-243 (QemuVirtMmu::activate)` + - Action: Reorder to flush-then-install: `dsb ishst; tlbi vmalle1; dsb ish` first (draining/invalidating while TTBR0 still names the outgoing context), then `msr ttbr0_el1, {ttbr0}` plus the EPD0 clear and a closing `isb`. Additionally, document in this function's SAFETY comment (or the `Mmu::activate` trait contract) whether the caller is required to mask IRQ/FIQ around the call β€” the corrected ordering removes the CPU's-own-lookup race but not a genuinely concurrent access from another exception context landing mid-sequence. This flush-then-install sequence is the **authoritative** ordering; a low-severity companion barrier-order item in [Phase C](phase-c.md#review-derived-work-items-2026-07-15-full-repository-review)'s SMP MMU epic points here and must be kept in sync. + - Gates: Milestone D5 directly β€” `bsp-pi4`'s `Mmu` impl "inherits VMSAv8 from QEMU's impl," so this bug would otherwise ship into the second BSP unmodified. Fix before D5's implementation work begins. + +- **[🟑 MEDIUM]** Descriptor validity/table/AF bit constants are module-private in HAL, forcing a byte-for-byte duplicate copy in the BSP walker. + - Location: `hal/src/mmu/vmsav8.rs:221,229-233` (cf. `bsp-qemu-virt/src/mmu.rs:63-65,366,371,380,533,549,592,604`) + - Action: Make `DESC_VALID_BIT`, `DESC_TABLE_OR_PAGE_BIT`, and `DESC_AF_BIT` (and, for consistency, `DESC_NG_BIT`/`DESC_PXN_BIT`/`DESC_UXN_BIT`) `pub const` in `hal/src/mmu/vmsav8.rs`, matching the treatment already given to the OA masks. Delete the duplicate declarations in the BSP and import the HAL constants instead, so there is exactly one textual definition of every VMSAv8 descriptor-field bit position in the codebase β€” important before a second BSP (`bsp-pi4`) needs its own copy of the same walker. + - Gates: Milestone D5. + +- **[🟑 MEDIUM]** `flags_to_descriptor_bits` has no defense-in-depth guard against `USER | WRITE | EXECUTE` (a user-writable-and-executable mapping). + - Location: `hal/src/mmu/vmsav8.rs:342-350` (cf. `hal/src/mmu/mod.rs:559-564`) + - Action: Extend the documented `InvalidFlags` contract (and whichever layer enforces it β€” currently the BSP's `map()`) to also reject `USER | WRITE | EXECUTE`, or at minimum add a `debug_assert!` in `flags_to_descriptor_bits` flagging the combination, closing the gap before any syscall surface can reach it. Kernel-only WRITE+EXECUTE (the existing, ADR-0034-blessed bootstrap RWX block mapping) must remain unaffected β€” scope the new check to the USER-tagged case specifically. + - Gates: Milestone D5. + +- **[βšͺ LOW]** `DescriptorBits` has fully public, unvalidated fields; the silent-truncation hazard documented for `pa` is undocumented and untested for `attr_idx`/`ap`/`sh`. + - Location: `hal/src/mmu/vmsav8.rs:260-275 (struct)` and `:479-490 (page_descriptor masking)` + - Action: Either (a) make `DescriptorBits`'s fields non-`pub` with `flags_to_descriptor_bits` as the sole safe constructor, or (b) document the truncation hazard on the struct itself and add a pinning test analogous to `block_descriptor_drops_low_bits_for_unaligned_pa` for out-of-range `attr_idx`/`ap`/`sh`. + - Gates: Milestone D5. + +- **[βšͺ LOW]** `mmu_bootstrap` Step 3's SCTLR-enable asm block is marked `options(nomem)`, contradicting the project's own stated rationale for omitting `nomem` on barrier sequences. + - Location: `bsp-qemu-virt/src/mmu_bootstrap.rs:246-261 (mmu_bootstrap, Step 3 asm block, options at line 258)` + - Action: Drop `nomem` from Step 3's asm options (matching Step 2's already-justified pattern), or, if `nomem` is intentional, add a comment explaining why reordering across the SCTLR-enable point is safe here and why that reasoning differs from Step 2's. Treat as unverified-in-codegen (no cargo/objdump available for the static review) but apply the conservative fix per CLAUDE.md's "when in doubt, the conservative option wins." + - Gates: Milestone D5. + +- **[βšͺ LOW]** `docs/audits/unsafe-log.md` and `mmu.rs`'s own module doc describe an obsolete (pre-T-022) instruction sequence for `QemuVirtMmu::activate`. + - Location: `bsp-qemu-virt/src/mmu.rs:9-14 (module doc)`; `docs/audits/unsafe-log.md:473-501 (UNSAFE-2026-0023 entry)` + - Action: Append a dated Amendment to UNSAFE-2026-0023 documenting `activate()`'s EPD0-clear addition (new MRS/AND/MSR TCR_EL1 + DSB ISHST instructions) and its `options(nostack)` (no `nomem`) choice with rationale; update `mmu.rs:9-14`'s summary to match the current sequence. Do this alongside the break-before-make reorder above so the audit log describes the corrected sequence, not another stale one. + - Gates: Milestone D5. + +- **[βšͺ LOW]** `Mmu::translate` cannot resolve any VA inside a block-mapped region β€” it returns `MmuError::BlockMapped` instead of decoding the block. + - Location: `bsp-qemu-virt/src/mmu.rs:326-388 (translate)`, specifically the `walk_or_alloc_table(..., unmap=true)` reuse at lines 348-357. + - Action: Extend `translate()` to detect a block descriptor at L1/L2 and decode it directly using the appropriate `BLOCK_OA_MASK` (already exported from `vmsav8.rs`) plus the VA's sub-block offset, calling the existing `descriptor_bits_to_flags` (bit-position-compatible with both block and page descriptors). At minimum, document the current block-mapped-region limitation prominently in `translate()`'s doc comment. + - Gates: Milestone D5. + +- **[βšͺ LOW]** `unmap`'s L3-leaf validation checks only the Valid bit, not the reserved/page bit that `translate()` checks. + - Location: `bsp-qemu-virt/src/mmu.rs:529-544 (walk_and_install_leaf, unmap branch)` + - Action: Add the same `(existing & DESC_TABLE_OR_PAGE_BIT) == 0 -> NotMapped` check to the unmap branch of `walk_and_install_leaf` for defense-in-depth and consistency with `translate()`'s stricter validation. + - Gates: Milestone D5. + +### Epic 3 β€” Boot & GIC on real silicon + +The remaining HAL/BSP hardware findings: GIC-400 register-programming completeness, boot-time sanity checks that are compiled out in release, and console/build hygiene. Milestone D2's GIC-400 impl and Milestone D1/D7's boot path inherit these patterns directly. + +- **[🟑 MEDIUM]** `high_half_alias` (`main.rs:800-808`)'s boot-time sanity checks are `debug_assert!`s that release builds silently drop β€” both the migration-target validity check and the >4 GiB kernel-image PA-mask guard. *(The review filed the migration-target check and the >4 GiB PA-mask guard as two items against the same function; merged here.)* + - Location: `bsp-qemu-virt/src/main.rs:800-808 (fn high_half_alias)` (cross-ref `Cargo.toml:59-70` `overflow-checks`, `tools/smoke.sh:12-56`) + - Action: The workspace already treats `overflow-checks` as a security property that must not be dropped for performance; apply the same reasoning to this function's checks β€” either (a) promote both `debug_assert!`s to unconditional `assert!`s (the function runs exactly twice per boot β€” negligible cost), or (b) set `debug-assertions = true` in `[profile.release]` for the workspace, consistent with the `overflow-checks` precedent already set two lines above it in the same file. + - Gates: Milestone D1/D7 β€” this guards the target of an unconditional `br` mid-boot; Pi 4's boot flow (ADR-0042) must not carry a release-mode-silent version of the same hazard. Also Milestone D5 (its ADR-0045 Pi 4 memory layout): Pi 4's RAM base (`0x0000_0000`) and peripheral window differ from QEMU's, and the 4 GiB PA-mask assumption behind the PA-guard is flagged in `boot.md` as a "forward limit" needing re-validation on real hardware. + +- **[🟑 MEDIUM]** `GICD_IPRIORITYR` is never programmed for IRQ IDs 0-31 (SGIs/PPIs, including the timer's own PPI 27) during `init()`. + - Location: `bsp-qemu-virt/src/gic.rs:188-210 (Step 4, priority loop starts at FIRST_SPI = 32)` + - Action: Add a small separate loop (or four `write_distributor` calls) in `init()` that programs `GICD_IPRIORITYR0-7` (byte offsets 0..31) to `DEFAULT_PRIORITY_BYTE` as well, before the distributor is enabled β€” 8 extra word writes at boot, closing the gap between the doc comment's stated intent and actual behavior. + - Gates: Milestone D2 directly β€” Pi 4's GIC-400 `IrqController` differs from QEMU's GICv2 impl "only in base addresses and board specifics," so this omission would otherwise carry straight into `bsp-pi4/src/irq.rs`. + +- **[🟑 MEDIUM]** Unbounded, interrupt-masked UART TX-FIFO poll turns `console_write` into a system-wide stall vector. + - Location: `bsp-qemu-virt/src/console.rs:78-84` (root cause), compounded by the masked-IRQ invariant documented at `bsp-qemu-virt/src/syscall.rs:118-119`. + - Action: Bound the poll β€” add an iteration/time cap in `write_bytes` that drops remaining bytes once exceeded (consistent with ADR-0007's own "best-effort... dropping bytes... is preferable to deadlocking" philosophy) β€” and/or enforce a hard per-syscall byte cap before the chunk loop in `sys_console_write`. Currently Medium rather than High because `console_write` is debug-gated out of release builds and v1 ships one demo task, but this becomes reachable the moment a second task gets a console capability or a future ADR lifts the debug gate β€” and Pi 4's PL011 (Milestone D3) has different, board-specific FIFO drain timing than QEMU's pre-initialized model, so the stall duration is not validated by QEMU testing alone. + - Gates: Milestone D3. + +- **[🟑 MEDIUM]** The `perf-bench` feature has zero automated build/run coverage; existing tooling structurally cannot validate it. + - Location: `bsp-qemu-virt/Cargo.toml:24` (feature def); `.github/workflows/ci.yml:140-143` (kernel-build job); `tools/smoke.sh:110`. + - Action: Add a `kernel-build-perf-bench` CI step/job that runs `cargo +$NIGHTLY_PIN build --target aarch64-unknown-none -p tyrne-bsp-qemu-virt --features perf-bench` (and the matching clippy invocation) so a breaking change to the bench module goes red. Separately, give `tools/smoke.sh` a `--features ` passthrough and either an alternate `--done-marker` string or auto-detection of perf-bench builds. + - Gates: Milestone D8 (hardware-parity validation tooling should not have a blind spot the CI already has). + +- **[βšͺ LOW]** `init()`'s SPI priority/target loop bound trusts the hardware-reported `GICD_TYPER` line count unclamped, unlike `enable()`/`disable()`'s hard-asserted `GIC_MAX_IRQ` bound. + - Location: `bsp-qemu-virt/src/gic.rs:160-167 (irq_count derivation)` vs. `gic.rs:317-322 / 342-348 (enable/disable asserts against GIC_MAX_IRQ = 1020)` + - Action: Clamp `irq_count = irq_count.min(GIC_MAX_IRQ as usize)` right after deriving it from `GICD_TYPER`, so `init()` and `enable`/`disable` share the same upper bound. This matters more on real GIC-400 than on QEMU's emulated distributor, where `GICD_TYPER`'s reported line count is whatever the emulator author chose rather than a value that can genuinely vary by SoC revision. + - Gates: Milestone D2. + +- **[βšͺ LOW]** `TIMER_IRQ_ID` is duplicated between `exceptions.rs` and `cpu.rs` with no compile-time cross-check. + - Location: `bsp-qemu-virt/src/exceptions.rs:32-36 (TIMER_IRQ_ID: u32 = 27)` and `bsp-qemu-virt/src/cpu.rs:49 (TIMER_IRQ: IrqNumber = IrqNumber(27))` + - Action: Either define the constant once (e.g. in `gic.rs` or a small shared `irq_ids` module) and import it from both sites, or add `const _: () = assert!(TIMER_IRQ_ID == crate::cpu::TIMER_IRQ.0);` next to one of the two definitions. Pi 4's PPI routing for the generic timer (Milestone D4) is a different line number in principle, so this pattern needs to not silently drift a second time when `bsp-pi4` defines its own copy. + - Gates: Milestone D2/D4. + +- **[βšͺ LOW]** GICv2 `acknowledge()`/`end_of_interrupt()` mask away GICC_IAR's CPU-ID subfield (bits [12:10]), which GICv2 requires be echoed back verbatim in GICC_EOIR for SGIs. *(The review filed this against two slightly different line ranges in `gic.rs` as separate items; they are the same underlying gap, merged here.)* + - Location: `bsp-qemu-virt/src/gic.rs:364-376` / `:369-370` (`acknowledge`) and `:378-385` (`end_of_interrupt`); `hal/src/irq_controller.rs:13 (IrqNumber)`. + - Action: Zero-cost today; no action needed for v1 (single-core, timer-only β€” no SGI/IPI traffic). When the future SGI/IPI ADR is written, preserve the CPU-ID subfield explicitly: either widen `IrqNumber` (or add a parallel raw-acknowledge type / a dedicated `acknowledge_sgi()` path) to carry the untruncated GICC_IAR value through to `end_of_interrupt`, or scope a GIC-driver-private raw-EOI path for the SGI ID range. Leave a forward-note next to the relevant mask constant now so it is discoverable without re-deriving it later. + - Note: the review cites this as gated by "ADR-0043, roadmap C4" for the eventual SGI/IPI ADR β€” a number that now collides with Phase D's own ADR-0043 (GIC-400 register layout, per the ADR ledger below, renumbered 2026-05-22). Flag as an ADR-numbering item to reconcile when Phase C's SGI/IPI ADR is actually drafted (also tracked under Phase B.2 Track B.2-2's ADR-ledger cascade-renumbering finding). + - Gates: none directly in Phase D (single-core Pi 4 has no SGI/IPI traffic yet); tracked here so it is not lost before Phase C's SGI/IPI work is ported to Pi 4 in a later phase. + +- **[βšͺ LOW]** Reported per-op perf-bench numbers never disclose build profile, even though the default toolchain path is `dev` (opt-level=1), not `release`. + - Location: `bsp-qemu-virt/src/perf_bench.rs:367-374 (ctx-switch banner)`, `:390-397 (IPC banner)`, `:531-536 (EL0 banner)`. + - Action: Have `build.rs` forward Cargo's own `PROFILE`/`OPT_LEVEL` env vars via `println!("cargo:rustc-env=TYRNE_BUILD_PROFILE={}", std::env::var("PROFILE").unwrap())`, then have `perf_bench.rs`'s three banners include `env!("TYRNE_BUILD_PROFILE")` (or at minimum `cfg!(debug_assertions)`) in the printed line, mirroring `perf-harness.sh`'s existing convention. This matters for Phase D because Pi 4 vs. QEMU timing comparisons (Milestone D8) are meaningless if the profile isn't recorded alongside the number. + - Gates: Milestone D8. + +- **[βšͺ LOW]** `build.rs`'s userland-image existence check is unconditional even though it serves only the non-bench (production demo) path. + - Location: `bsp-qemu-virt/build.rs:28-35`; `bsp-qemu-virt/src/main.rs:375`. + - Action: Either (a) gate `USERSPACE_IMAGE` (and related constants, if perf-bench-dead too) behind `#[cfg(not(feature = "perf-bench"))]` in `main.rs` and make `build.rs`'s assert conditional on `CARGO_FEATURE_PERF_BENCH` being unset; or (b) if the coupling is intentional, say so explicitly in a doc comment. + - Gates: Milestone D1/D7 (bsp-pi4's own build.rs will need the same decision made, not re-derived from scratch). + +- **[⚫ INFO]** `boot.s` comment overstates that the BSS-zero loop involves NEON instructions. + - Location: `bsp-qemu-virt/src/boot.s:109-116`. + - Action: Tighten the comment to say "...before the first NEON instruction the compiler may emit in Rust code (`kernel_entry` and beyond)" and drop the "BSS zeroing" clause, since the explicit hand-written loop never uses NEON. + - Gates: Milestone D1 (boot.s is mirrored into `bsp-pi4`; fix the comment at the source). + +### Polish & excellence + +Non-defect quality findings from the 2026-07-15 full-repository review, condensed into grouped bullets. None are dropped from the source list of 28. + +- **HAL trait-contract precision** (documentation-only, zero runtime cost, all in `hal/`): state explicitly that `IrqController` performs no capability/authorization checking of its own (matches CLAUDE.md's no-ambient-authority rule); spell out that `IrqState` captures the full D/A/I/F mask, not just IRQ, ahead of a future RISC-V/PLIC HAL implementer who won't have aarch64's DAIF vocabulary to fall back on; add an explicit non-aliasing bullet to `ContextSwitch::context_switch`'s `# Safety` section; clarify `Console`'s "avoid blocking indefinitely" language against expected UART FIFO-full spin loops (directly relevant to Pi 4's PL011, which is flakier than QEMU's pre-initialized model); give BSPs a shared IRQ-range validation policy instead of each choosing independently (QEMU's GICv2 panics on out-of-range; nothing stops `bsp-pi4` from silently no-op'ing the same case); cross-reference ADR-0037's DAIF-at-EL0 masking directly from `vectors.s`; consider echoing FPCR/FPSR/NZCV-adjacent state explicitly in the `# Safety`/audit-log entries for the two live trampolines (would have caught the Epic 1 FP/SIMD gap by construction). + +- **Constants, duplication, and type-safety hygiene**: mark `DescriptorBits` `#[non_exhaustive]` (consistent with `MmuError`'s own treatment in the same crate); derive `TABLE_NLA_MASK` from `PAGE_OA_MASK_L3` instead of retyping the identical literal; use the workspace's `VirtAddr` newtype for `Pl011Uart::base` instead of raw `usize`, making the "must be mapped for kernel access" contract compiler-checked β€” directly useful when `bsp-pi4/src/console.rs` (Milestone D3) writes its own PL011 driver against the same trait; add `publish = false` to `bsp-qemu-virt/Cargo.toml` (and other bare-metal-only crates, including the forthcoming `bsp-pi4`); unify or cross-reference the two independent warm-up constants `WARMUP`/`EL0_WARMUP` (both = 256) with a one-line "kept equal by convention, not coupling" note; extend the compile-time layout-guard pattern (`const _: () = assert!(...)`) already used for `TrapFrame`/`SyscallTrapFrame`/`Aarch64TaskContext` to the four bootstrap page-table frame declarations, which are the one remaining ABI-shaped fact without one. + +- **Fail-stop verification and defense-in-depth polish**: add a `debug_assert!` second line of defense in `descriptor_bits_to_flags` for the caller-validated V/page/AF bits; promote the migration-target check to an unconditional `assert!` (echoes the Epic 3 finding above β€” same fix, same file); teach `translate()` to decode block descriptors (echoes the Epic 2 finding above); give `panic_entry` a small `ESR_EL1.EC` decode table (cheap, no allocation, already anticipated as a deferred follow-up in `unsafe-log.md:406` β€” pays for itself the first time a Phase C/D bring-up bug lands in this exact panic path); state explicitly that `CNTVCT_EL0` reads carry no ISB barrier and that the skew is bounded and negligible at N=50,000 round-trips; add a host-side decode assertion for `BENCH_EL0_IMAGE`'s hand-assembled bytes (a `const fn` compile-time assertion matching each 4-byte word against its documented encoding, in the style of the existing `N_*_ROUNDTRIPS` non-zero guard). + +- **Boot-path and audit-log bookkeeping**: run a dedicated audit-log sweep for the T-022 `kernel_entry`/`kernel_main_high` split β€” UNSAFE-2026-0028 is confirmed stale in its header fields, and 0001/0010 still say `kernel_entry` for what are now largely `kernel_main_high`-resident operations, a pattern that has recurred at least three times and is more efficiently fixed in one sweep; expand the crate-level doc comment to cover the T-028 EL0 task, since roughly half of `kernel_main_high`'s current logic (loader, AS/Task cap resolution, `add_user_task`, `USER_TASK_TABLE`) exists to run a real EL0 program and the module doc gives no signal of that; record the actual T-028-era boot dispatch order in `docs/architecture/boot.md` or the sched module doc, since it is an emergent, non-local result of three independently-reasonable pieces of code; convert the ASCII "Memory map at boot" diagram in `boot.md` to an inline Mermaid diagram (per [documentation-style.md](../../standards/documentation-style.md)'s Mermaid-only rule), reflecting the T-022/ADR-0033 link-high/load-low reality and **replacing** β€” not duplicating β€” the ASCII version (two sections of the same document currently disagree on the level of detail; also tracked under Phase B.2 Track B.2-2's `boot.md`-diagram finding); add a small script/lint that cross-checks `unsafe-log.md` entries against the asm/unsafe blocks they claim to cover (a grep-based CI check flagging an asm `!` line-count change with no corresponding new Amendment date) to preserve the audit trail's credibility going forward, including through Phase D's own new `bsp-pi4` unsafe blocks. + +- **Code-size and test-quality polish** (run-once boot paths, negligible runtime impact): the BSS-zero loop could use `DC ZVA` or a NEON pair-store for boot-time code-size/perf polish; `enter_el0`'s manual 61-instruction GPR/vector-register zeroing sweep could shrink via a small loop; fix the break-before-make ordering in `activate()` before it gets a second real caller (echoes the Epic 2 HIGH finding β€” flagged again here because `bsp-pi4`'s `Mmu` impl is precisely that "second real caller"); extract `syscall_entry`'s frame-to-struct marshalling into small, pure, host-testable functions (e.g. `args_from_words`/`write_resume_words`) so a register-order transposition is caught at `cargo test` time rather than by reading a QEMU trace β€” the highest-value item in this group given it covers the widest untrusted-input surface in the system. + +--- + +Covers all 25 routed review findings (23 after merging 2 near-duplicate pairs β€” the GICv2 CPU-ID/EOI items and the `high_half_alias` release-`debug_assert!` items) + 28 polish items routed to this phase. diff --git a/docs/roadmap/phases/phase-e.md b/docs/roadmap/phases/phase-e.md index 67de9f7..32c0297 100644 --- a/docs/roadmap/phases/phase-e.md +++ b/docs/roadmap/phases/phase-e.md @@ -122,3 +122,111 @@ Numbers are tentative; final numbers are assigned when the ADR is actually writt - Whether a unified "service interface" pattern emerges that multiple services share, or each service designs its own interface. - Sync vs. async driver model. - Where smoltcp fits in licensing and `cargo-vet` posture. + +--- + +## Review-derived work items (2026-07-15 full-repository review) + +The 2026-07-15 full-repository review surfaced a cluster of capability-enforcement and object-lifecycle gaps in the current `kernel/src/cap`, `kernel/src/mm`, `kernel/src/obj/task_loader`, `kernel/src/ipc`, and `kernel/src/syscall` code. None of these is exploitable today: `grep` confirms `cap_map`, `cap_create_address_space`, and the other affected primitives have **zero syscall wiring** β€” every live caller is trusted kernel-internal code (`task_loader.rs`, boot sequencing) or test code, and no EL0 task can reach any of them. That is precisely why they belong on the Phase E backlog rather than an incident report: Phase E is where "userspace drivers as first-class tasks" stops being an architectural claim and starts being real syscall-reachable code (`MemoryRegionCap` for MMIO in Milestone E1, block/network device drivers in E4/E6, task-spawning services throughout). **Each item below MUST be closed in the same change that first exposes its path to a syscall or to a less-trusted caller** β€” not tracked as a follow-up, not merged as "we'll harden it later." Landing the syscall wiring first and the check second is exactly the ambient-authority window CLAUDE.md rule 1 forbids. + +### Epic 1 β€” Capability-ownership enforcement (close before the syscall wires the path) + +These gate Milestone E1: a driver's `MemoryRegionCap` for its device's MMIO is backed by the same `cap_map`/`cap_create_address_space`/`cap_derive` machinery audited here, and by extension every later milestone that maps device memory into a task (E4 storage driver, E6 network stack integration). + +- 🟠 **HIGH** β€” `cap_map`/`cap_create_address_space` accept a caller-chosen `PhysFrame` with no memory-ownership capability check β€” an ambient-authority gap: any caller that reaches these functions can map arbitrary physical memory into an address space regardless of who owns it. + - **Location:** `kernel/src/mm/address_space.rs:728-746` (`cap_map`); also `kernel/src/cap/mod.rs:85-86` (`CapKind::MemoryRegion` reserved but unimplemented). + - **Action:** Treat as a hard architectural precondition, not a follow-up. Land the `MemoryRegion` capability / Untyped-style frame-ownership discipline (already tracked against [ADR-0028](../../decisions/0028-address-space-data-structure.md) for B4+) and thread a frame-ownership check through `cap_map` **before** any syscall exposes it to userspace β€” concretely, before Milestone E1's driver template can hand a real `MemoryRegionCap` to a driver task. Add a CI or review gate (a comment-linked TODO/ADR condition) that blocks a syscall-dispatch-wiring PR for `cap_map` from merging until the ownership check exists. + +- 🟠 **HIGH** β€” `cap_derive` lets the caller mint a capability naming an arbitrary, unrelated kernel object β€” there is no check that `new_object` relates to `src` at all, and the [ADR-0014](../../decisions/0014-capability-representation.md) src↔object correlation was never implemented. + - **Location:** `kernel/src/cap/table.rs:258-320` (`cap_derive`; doc at 243-247). + - **Action:** Split the kind check out from the existing rights validation (which stays unchanged: DERIVE-present, then no-widening). Add the kind check using the accessors that actually exist β€” there is **no** `rights_and_kind_of` helper: the `entry_of(src)` lookup already inside `cap_derive` resolves the source capability, so compare its kind (`entry.capability.kind()`, i.e. `Capability::kind()` β†’ `self.object.kind()`) against `new_object.kind()` (`CapObject::kind() -> CapKind`), returning `CapError::WrongKind` on mismatch. Minimal, safe, zero-cost. This does not break the one legitimate current call site (`cap_create_address_space` in `kernel/src/mm/address_space.rs:674-678`, which already independently checks `parent_cap.kind() == CapKind::AddressSpace` at line 551 before calling `cap_derive` with a fresh `CapObject::AddressSpace(handle)`; kind always matches there). For the deeper same-kind/different-instance case (e.g. an AddressSpace-DERIVE holder minting a cap to a *different* AddressSpace it was never granted), write a short ADR-0014 revision note (or successor ADR) formalizing that `cap_derive` is a "mint-new-object under creation-authority" primitive, not a same-object narrowing primitive β€” this was explicitly promised in ADR-0014 and never delivered. + +- βšͺ **LOW** β€” `cap_derive`'s object-identity behavior is completely untested β€” no test pins whether a kind- or instance-mismatched `new_object` is accepted or rejected. + - **Location:** `kernel/src/cap/table.rs:1123-1317` (test module). + - **Action:** Add at least two tests once the kind-check above lands: (1) a positive/negative pin for cross-kind derivation (`cap_derive` from a `Task`-kind source with `new_object = CapObject::Endpoint(...)`); (2) if same-kind-different-instance derivation is intended to stay permitted (the `cap_create_address_space` "mint new" pattern), a test that documents this by name β€” e.g. `cap_derive_same_kind_different_object_is_permitted_by_design` β€” so the behavior is pinned rather than accidental. + +- 🟑 **MEDIUM** β€” `link_child`'s (believed-unreachable) `InvalidHandle` path permanently leaks the already-popped free-list slot, with no `debug_assert` unlike the file's other "should never happen" branches. + - **Location:** `kernel/src/cap/table.rs:613-628` (`link_child`); call sites at 220-227 (`cap_copy`) and 300-306 (`cap_derive`). + - **Action:** Either (a) push `new_index` back onto the free list before returning the error (mirror `free_slot`'s prepend), or (b) reorder so the parent's liveness is validated *before* `pop_free()` is called β€” `link_child` could take an already-resolved `&mut SlotEntry` for the parent instead of an `Option`, eliminating the ordering hazard entirely. Either way, add `debug_assert!(false, "link_child: parent slot is empty")` matching the file's existing convention so a future invariant violation is caught loudly in tests rather than silently shrinking table capacity in production. + +- 🟑 **MEDIUM** β€” W^X is unenforced by the mapping primitives: flags flow unchecked from caller through `cap_map` to `Mmu::map`, and neither `cap_map` nor the encoder rejects WRITE+EXECUTE. + - **Location:** `kernel/src/mm/address_space.rs:728-746` (`cap_map`); cross-checked against `hal/src/mmu/vmsav8.rs:342-350` (`flags_to_descriptor_bits`). + - **Action:** Enforce the invariant at the **shared, lower layer** β€” `Mmu::map` or a new shared `MappingFlags::validate()` β€” by rejecting `WRITE && EXECUTE && USER` (the userspace W^X case) with `InvalidFlags`, *not* at `cap_map`. Scope the check to the USER-tagged case specifically: kernel-only `WRITE && EXECUTE` β€” the existing ADR-0034-blessed bootstrap RWX block mapping β€” stays permitted until the ADR-0034 per-section kernel-image remap lands, so the guard must not reject it. `cap_map` is **not** the universal chokepoint: the task loader and bootstrap map pages by calling the mapper directly, bypassing `cap_map` entirely (this is exactly the direct-caller path the companion finding immediately below covers). Landing the check in the shared validation point closes the gap for every caller by construction. Keep the **same** `WRITE && EXECUTE && USER` rejection at `cap_map` too, framed as **defense-in-depth** at the capability layer β€” so the capability path still fails closed even if a future refactor moves the lower-level check. Both are independent of, and cheaper than, the broader ADR-0034 per-section kernel-image remap work. + +- βšͺ **LOW** β€” `Mmu::map`/`flags_to_descriptor_bits` do not structurally reject WRITE+EXECUTE+USER β€” W^X is upheld only by loader-call-site discipline, not by the primitive itself. **This is the primary, shared enforcement point** the finding above defers to (it covers the direct loader/bootstrap callers that bypass `cap_map`); the `cap_map` guard is the defense-in-depth companion, not the authoritative check. + - **Location:** `bsp-qemu-virt/src/mmu.rs:259-271` (`Mmu::map`'s only flag-combination rejection today is DEVICE+EXECUTE); `hal/src/mmu/vmsav8.rs:319-350` (`flags_to_descriptor_bits`). + - **Action:** Make the shared MMU validation β€” `Mmu::map`, or a shared `MappingFlags::validate()` it calls β€” the **authoritative** rejection point for `WRITE && EXECUTE && USER`, returning `InvalidFlags`: it is the one point every direct mapper caller (`cap_map`, the task loader, bootstrap) passes through, so the userspace-W^X gap closes there by construction. Keep **matching** defense-in-depth checks at the two layers bracketing it β€” the VMSAv8 descriptor encoder (`flags_to_descriptor_bits`, the last line before the raw descriptor bits; mirror its existing DEVICE+EXECUTE guard) and `cap_map` (the capability layer, per the finding above) β€” so all direct callers stay covered even if one layer is later refactored. Continue to permit kernel-only `WRITE && EXECUTE` (the ADR-0034-blessed bootstrap RWX) at every layer until the per-section remap lands. Cheap now, before any userspace-reachable map-style syscall exists; expensive to retrofit once multiple callers depend on the permissive behavior. + +- βšͺ **LOW** β€” `resolve_address_space_cap` checks capability kind only, never rights β€” `CapRights::EMPTY` still grants full map/unmap/activate authority. + - **Location:** `kernel/src/mm/address_space.rs:430-441` (`resolve_address_space_cap`), used by `cap_map:738` and `cap_unmap:782`. + - **Action:** Before any syscall wiring exposes `cap_map`/`cap_unmap` to userspace, land the per-operation rights bits (MAP/UNMAP/ACTIVATE) the doc comment already anticipates for "B5+", and have `resolve_address_space_cap` take an expected-rights parameter so `cap_map` and `cap_unmap` can each demand the specific bit they need instead of sharing one kind-only check. + +- βšͺ **LOW** β€” `AddressSpaceCap` authority is kind-only (any AS cap of the right kind grants full map/unmap/activate), not rights-gated per operation, and this gap has no entry in `security-model.md`'s open-questions list (unlike the kernel-image W^X gap, which does). + - **Location:** `kernel/src/mm/address_space.rs:408-441` (`resolve_address_space_cap` doc + body). + - **Action:** Add an entry to the security model document's Open Questions recording that `AddressSpaceCap` authority is currently kind-gated rather than rights-gated, and that this must close before `cap_map`/`cap_unmap` become syscall-reachable β€” at which point a holder of any narrowed/derived AS cap (e.g. one meant only to `activate`) could `unmap` arbitrary pages in that AS. Companion documentation fix to the finding directly above; do not merge one without the other. + +- βšͺ **LOW** β€” `cap_map`/`cap_unmap` discharge `MapperFlush` tokens without documenting reliance on `activate`'s full-TLB-flush as the compensating control for non-active address spaces. + - **Location:** `kernel/src/mm/address_space.rs:741-744` (`cap_map`'s `token.flush(mmu)`) and `:785-788` (`cap_unmap`'s `token.flush(mmu)`); cross-referenced against `hal/src/mmu/mod.rs:376-386`. + - **Action:** Add a short doc note at `cap_map`/`cap_unmap` cross-referencing `hal::MapperFlush`'s future-soundness-cliff paragraph, stating explicitly that correctness for non-active-AS mutation currently depends on `Mmu::activate`'s unconditional full flush β€” so a future change to `activate`'s flush policy (e.g. when ASIDs land) must revisit both call sites in lockstep. + +### Epic 2 β€” Object lifecycle & reclaim + +These gate Milestone E3 (service manager/supervisor): crash-and-restart of a supervised task is the first realistic trigger for reclaiming task-owned kernel resources at runtime rather than only at boot. They also connect directly to the existing Phase-C carry-forward **EL0 fault containment (K3-4)** β€” already tracked in `phase-c.md`'s carry-forward list and targeted at Phase E ("first real driver task") rather than Phase C. K3-4's supervisor-endpoint fault delivery is exactly Milestone E3's "Fault-endpoint plumbing" sub-item; closing K3-4's *fault-reporting* path and closing this epic's *resource-reclamation* path are two halves of the same restart story and should land together β€” a supervisor that can observe a crashed task's fault but cannot reclaim its address-space arena slot, frames, or capabilities has only solved half the problem Milestone E3 exists to solve. + +- 🟑 **MEDIUM** β€” every failed `load_image` call permanently burns one of only 8 total address-space arena slots β€” no reclaim path exists. + - **Location:** `kernel/src/obj/task_loader.rs:468-479` (rollback discipline doc), `:810-823` (`rollback()` doc), `:849-851` (`rollback()`'s cap-drop-only cleanup). + - **Action:** With only 8 total slots, this is not a distant concern β€” the moment any code path calls `load_image` more than a handful of times without treating every failure as fatal (spawning several userspace tasks in a loop, or retrying after a transient frame-budget miss), the kernel permanently loses its ability to create new address spaces for its entire remaining uptime. Prioritize landing the forward-flagged `MemoryRegionCap`/per-AS-destroy mechanism (or at minimum a `Pmm`-and-arena-level rollback that also frees the AS arena slot) before `load_image` is called more than once per boot in any production path β€” which Milestone E3's restart loop will do routinely. + +- 🟑 **MEDIUM** β€” `rollback()` leaks the AS arena slot despite an already-reachable, ADR-free function (`destroy_address_space` + `AddressSpace::root_frame`) that could reclaim it and the L0 root frame today. + - **Location:** `kernel/src/obj/task_loader.rs:824-852` (`rollback()` body); `kernel/src/mm/address_space.rs:306-313` (`destroy_address_space`), `kernel/src/mm/address_space.rs:123-131` (`AddressSpace::root_frame`). + - **Action:** Wire `rollback()` to resolve `loaded_as_cap` β†’ `AddressSpaceHandle` (mirroring `task_create_from_image`'s own step-1 lookup), call `destroy_address_space(as_arena, handle)`, and `pmm.free_frame(freed.root_frame(mmu))` on the result before (or after) `cap_drop`. This closes two of the three documented leaks (arena slot + L0 frame) immediately, without waiting on the forward-flagged `MemoryRegionCap`/per-AS-destroy ADR β€” only the intermediate-table leak genuinely needs that future work. Update the `rollback()`/`load_image` doc comments to stop implying the whole leak set is ADR-blocked. + +- βšͺ **LOW** β€” `reset_if_stale_generation`'s capability-leak guard is a debug-only `debug_assert` with no release-mode enforcement. + - **Location:** `kernel/src/ipc/mod.rs:254-271` (`reset_if_stale_generation`); cross-ref `kernel/src/obj/endpoint.rs:78-114` (`destroy_endpoint`, C3-001 note). + - **Action:** Track this as a hard precondition for the Phase B.2+ endpoint-destroy ADR ([ADR-0032](../../decisions/0032-endpoint-rollback-and-cancel-recv.md) already references it, and it is the same destroy/drain primitive Milestone E3's task-restart path will exercise): the destroy/drain primitive must not be mergeable until the cap-bearing-slot case is a release-mode-enforced refusal (typed error) rather than a `debug_assert!` β€” a silent authority leak in release is exactly the failure mode CLAUDE.md rule 1 rules out. No code change is required in `kernel/src/ipc/mod.rs` itself yet, but the tracking issue must say explicitly "promote `debug_assert!` to a release-enforced check," not just "add a drain step." + +### Epic 3 β€” Userland runtime maturation + +Findings that don't sit at a capability-mint chokepoint or an object-destroy path, but harden the syscall/IPC/task-creation surface every Phase E service will exercise routinely once real drivers and services (E1 onward) make live syscalls instead of being test/boot-only callers. + +- βšͺ **LOW** β€” `SyscallError::Ipc(ReceiverTableFull)` discloses a different task's private resource state to the sender, outside ADR-0030's stated security justification. + - **Location:** `kernel/src/syscall/error.rs:145` (`IpcError::ReceiverTableFull => 6,` inside `ipc_error_code`, composed into the stable status `0x206`). + - **Action:** Either (a) extend [ADR-0030](../../decisions/0030-syscall-abi.md)'s security argument with an explicit rider covering `ReceiverTableFull`/`QueueFull`, documenting why disclosing a communicating peer's transient resource state to a party that already holds a SEND capability to it is an acceptable, bounded leak, or (b) if the leak isn't worth the diagnostic value, collapse `ReceiverTableFull` into a less specific code (e.g. fold it into `QueueFull` or a generic "transfer could not complete, retry" code) that does not name which side's table caused it. Given this is genuinely low-severity and low-likelihood, (a) β€” an explicit ADR amendment closing the analysis gap β€” is the proportionate fix, and should land before Milestone E2's log service and E3's supervisor put IPC error codes in front of routine cross-task traffic. + +- βšͺ **LOW** β€” the `has_current_task` / fail-closed-table pairing is enforced only by caller discipline, not asserted inside `dispatch()`. + - **Location:** `kernel/src/syscall/dispatch.rs:97-110, 144-161`. + - **Action:** Add a cheap `debug_assert!` at the top of `dispatch` (or a constructor-time check on `SyscallContext`) asserting that `has_current_task == false` implies the caller table cannot resolve any handle β€” e.g. expose/require a cheap `CapabilityTable::is_empty()` the test-only assertion can call. Converts an implicit, externally-verified contract into a self-checking one, matching the module's stated "single most security-sensitive control-flow join" bar, before Phase E's services turn this join into a hot path. + +- βšͺ **LOW** β€” `task_create_from_image` trusts a caller-constructed `LoadedImage` with no consistency check against the named address space. + - **Location:** `kernel/src/obj/task_loader.rs:194-229` (`LoadedImage` field-invariants doc), `:917-936` (`task_create_from_image` step 1). + - **Action:** Either (a) make `LoadedImage`'s fields private with construction restricted to `load_image`'s success path β€” removing the public-struct-literal convention now that a real consumer (`task_create_from_image`) exists β€” or (b) have `task_create_from_image` perform a cheap runtime check (e.g. via the [ADR-0038](../../decisions/0038-mmu-translate-and-user-access.md) `Mmu::translate` read-only walk once it lands) that `entry_va` resolves to an EXECUTE|USER mapping inside the AS named by `as_cap` before minting the Task cap. Permissive construction was safe when `LoadedImage` was purely a descriptor (pre-T-024); it is materially riskier now that it seeds a runnable EL0 execution context β€” and Milestone E1's driver template will be the first non-boot code constructing these routinely. + +- βšͺ **LOW** β€” `task_create_from_image` mints a live task but leaves the source AS capability's unrestricted map/unmap authority untouched. + - **Location:** `kernel/src/obj/task_loader.rs:892-903` (v1 cap-rights model doc), `:917-967` (`task_create_from_image` body). + - **Action:** Not currently exploitable β€” the cap lives only in the fully-trusted `BOOTSTRAP_AS_TABLE`, unreachable from EL0 β€” but the gap is structural and belongs to the trust-boundary story this module owns. Document explicitly in `task_create_from_image`'s doc comment what the caller is expected to do with `loaded.as_cap` after minting a task (drop it, or keep only a documented-safe use), and/or forward-flag that per-operation AS rights (MAP/UNMAP, already a named B5+ TODO elsewhere in this file β€” see the `resolve_address_space_cap` finding in Epic 1) must close this gap before `task_create_from_image` is reachable from anything less than fully-trusted kernel-boot code, which Milestone E1's driver spawning will be. + +- βšͺ **LOW** β€” `task_create_from_image` mints a Task cap from an AS cap with zero rights check β€” not even DERIVE β€” unlike `cap_create_address_space`'s explicit DERIVE-gated mint. + - **Location:** `kernel/src/obj/task_loader.rs:924-936` (step 1, no rights check on the resolved AS cap). + - **Action:** If the intent is genuinely "any AS cap of the right kind authorizes task creation, mirroring the AS kind-only model" (a defensible v1 choice given the project's own documented kind-only AS baseline), say so explicitly in `task_create_from_image`'s doc comment β€” the same way `resolve_address_space_cap`'s doc explicitly calls out its own kind-only contract β€” so a future reader doesn't mistake the omission for an oversight. Otherwise, gate on `CapRights::DERIVE` (or a purpose-built right) to match `cap_create_address_space`'s discipline before this function becomes reachable from anything less trusted than boot code. + +### Polish & excellence + +- **Polish** β€” Bake rights/kind correlation into `CapRights` or `Capability::new` as defense-in-depth, complementing the `cap_derive` fix above: catches a nonsensical rights/kind combination at construction time rather than relying on every downstream syscall handler to independently check both kind and the relevant right bit. Zero runtime cost in release builds (`debug_assert`); turns a class of "the two are just conventionally correlated" bugs into compile-adjacent, test-caught ones. + +- **Polish** β€” Surface the destroy-drain / cap-leak forward-reference from `obj::endpoint.rs` inside `ipc/mod.rs`'s own module doc, so a reader auditing `ipc/mod.rs` in isolation doesn't have to cross into `obj/endpoint.rs` to get the full picture of an already well-documented hazard β€” keeps the two halves co-located for the next auditor, and for whoever lands Milestone E3's fault/restart path. + +- **Polish** β€” Track `ObjError::StillReachable`/`references_object` as unenforced before the first destroy-capable syscall lands. A short pre-flight checklist item β€” "the first live `task_destroy`/`endpoint_destroy`/`notification_destroy` syscall must wire `references_object` across every live `CapabilityTable`, not just one" β€” prevents this deferred invariant from being forgotten when Milestone E3's supervisor restart path authors that syscall (related tracking: SEC-T028-01 in `sched/mod.rs:958-964`). + +- **Polish** β€” Surface a per-AS live-mapping/frame-count diagnostic accessor (mirroring `root_frame()`). Would help the eventual B4+/E3 per-AS destroy path verify it has fully drained an AS's mappings before freeing the root, and make future OOM/leak debugging on real hardware substantially easier. + +- **Polish** β€” Reserve VA 0 as an unmapped guard page. High-assurance kernels commonly refuse to map the zero page (Linux's `mmap_min_addr`, OpenBSD's default) as defense-in-depth against a kernel NULL-pointer-dereference bug turning into userspace-controlled arbitrary-address kernel memory access. Low-cost, high-signal hardening to consider once image placement becomes policy-configurable rather than a fixed linker-script constant β€” worth doing before `copy_from_user`/`copy_to_user` (ADR-0038) starts dereferencing user-supplied pointers in earnest for Phase E's driver/service IPC traffic. + +- **Polish** β€” Enforce or forward-flag task-id uniqueness. Currently harmless (`Task::id()` is used only for diagnostics/tests, not as an addressing key), but worth a one-line doc note or a `debug_assert`-level uniqueness check now, before scheduler diagnostics, IPC addressing, or debugger attach start trusting task ids as unique β€” all plausible needs once Milestone E3's supervisor is watching multiple live tasks. + +- **Polish** β€” Cross-reference the `WidenedRights`/intermediate-frame-count forward-flags already sitting in the task-loader docstrings back to their originating review item (`docs/analysis/reviews/master-review/2026-05-22-152729/tracks/C4-kernel-task-loader.md`), the same discipline already applied to the unsafe-log cross-references, so a future reader doesn't have to independently rediscover which review round motivated each comment. + +- **Polish** β€” Add a structural WRITE^EXECUTE guard to `Mmu::map`/`flags_to_descriptor_bits` mirroring the existing DEVICE^EXECUTE guard (see the matching Epic 1 finding). Converts today's "W^X holds because the one caller behaves" into "W^X holds because the primitive enforces it" β€” the difference matters precisely because Phase C and beyond add more mapping call sites (per-region `MemoryRegionCap`, a future mmap-equivalent syscall), and each new caller currently inherits zero protection from the trait layer. + +--- + +Covers all 17 review findings + 8 polish items routed to this phase. diff --git a/docs/roadmap/phases/phase-g.md b/docs/roadmap/phases/phase-g.md index b0c131a..2aeedc5 100644 --- a/docs/roadmap/phases/phase-g.md +++ b/docs/roadmap/phases/phase-g.md @@ -130,3 +130,63 @@ Numbers are tentative; final numbers are assigned when the ADR is actually writt - Whether verification is a standing practice after G4 or a one-off pilot. - How deeply to audit crypto dependencies we do not own. - Whether TLS lives in the network service or in its own crypto service (likely its own service for capability-scope reasons). + +--- + +## Review-derived work items (2026-07-15 full-repository review) + +The 2026-07-15 full-repository review found Phase G's scope largely in the "prove-it" category: the review turned up only one documentation-accuracy finding, and its remaining observations are excellence/polish items that push already-correct code and already-documented gaps toward the exemplary standard a high-assurance kernel should hold itself to. Items are grouped into the three epics below, followed by the one stray finding and a Polish & excellence subsection. + +### Epic G-R1 β€” Prove-it: property & fuzz testing of invariants + +This phase's "prove-it" posture (Milestone G4 in particular) is best served by turning today's hand-picked example tests into property/fuzz tests that defend the codebase's stated invariants across their full input space rather than at a few chosen boundary points. Four independent opportunities converge on this theme: + +- **[Polish]** Property-test the capability derivation-tree invariants (parent-liveness, no-cycles, BFS-completeness). + - Location: `kernel/src/cap/table.rs` β€” `cap_derive` (table.rs:258); the derivation-tree size-and-shape reasoning at table.rs:341-359; the cycle-detection `debug_assert!`s at table.rs:369-372 and table.rs:393-396, which today only fire incidentally during ordinary unit tests; the ADR-0014 cross-reference at table.rs:428. + - Action: add a `proptest`/`quickcheck`-style test module (the crate already runs a `no_std`-with-`std`-test-harness setup, so this is a dependency-light addition) that generates randomized sequences of `cap_derive` / `cap_copy` / `cap_revoke` calls against `CapabilityTable` and asserts, after every step: (1) every live slot's parent chain terminates at a root within `MAX_DERIVATION_DEPTH` (parent-liveness); (2) the `first_child`/`next_sibling` linked structure never revisits a node (no-cycles) β€” the property the existing `debug_assert!`s only catch by accident; (3) a BFS from any node visits exactly its live descendant set, each exactly once (BFS-completeness), checked against a reference `HashSet`-based oracle. This is a precursor deliverable for Epic G-R2 below β€” it independently re-derives, and directly precedes, the "prior review already flagged this as a strong fit" conclusion for the same invariants. + +- **[Polish]** Add a property/fuzz test asserting panic-freedom over the full syscall register space. + - Location: `kernel/src/syscall/dispatch.rs` β€” the module's stated hard rule at dispatch.rs:9-11 ("No path may `panic!` / `unwrap` / `expect` on any register-supplied input"); `dispatch()` itself at dispatch.rs:129. + - Action: add a property/fuzz test (a `proptest` strategy or a `cargo-fuzz` target, whichever the crate's existing test tooling favors) that calls `dispatch` with randomized `SyscallNumber` and `x1`–`x7` register patterns drawn from the full `u64` space and asserts no panic occurs for any input. This turns the module doc's strongest stated guarantee from "true for tested inputs" into "defended by construction of the test," the natural next step from correct to exemplary. + +- **[Polish]** Property-test `SchedQueue`'s wraparound arithmetic. + - Location: `kernel/src/sched/mod.rs` β€” `SchedQueue::enqueue`'s `wrapping_add`/`% N` tail computation (mod.rs:104-119) and the mirrored `dequeue` head advance (mod.rs:133); the existing hand-picked FIFO/full/empty/one-wraparound tests (mod.rs:1737-1780 area). + - Action: add a reference-model fuzz/property test that runs `SchedQueue` against a `std::collections::VecDeque` oracle over randomized enqueue/dequeue sequences at several `N` values, with generation biased toward the `N`-1 boundary and repeated-wrap sequences β€” exactly the region where an off-by-one is easy to miss by hand and cheap to catch by construction, given the crate is already `no_std`-with-`std`-test-harness. + +- **[Polish]** Exhaustive pairwise-disjointness test across the full status-code space. + - Location: `kernel/src/syscall/error.rs` β€” the blocked status-code layout documented at error.rs:1-27 (`0` = Ok, `1`–`3` top-level, `0x101`–`0x1FF` = Cap block, `0x201`–`0x2FF` = Ipc block); `SyscallError::as_status` (error.rs:95); the existing `cap_and_ipc_status_blocks_do_not_collide` test (error.rs:241 area), which checks only block-level, not full pairwise, disjointness. + - Action: add a host test that enumerates every `CapError`, `IpcError`, and top-level `SyscallError` variant's `as_status()` output into one collection (e.g. inserting into a `BTreeSet` and asserting every insert returns `true`) and asserts all values are pairwise distinct. This closes the gap left by the existing block-collision test without requiring a `const`-fn rewrite, and fits the module's already-strong culture of pinning ABI-stability invariants with host tests (the exhaustive-without-wildcard match technique cited in error.rs's own module doc, error.rs:24-27). + +### Epic G-R2 β€” Formal-verification pilot candidates + +Milestone G4 names its target only tentatively ("probably the capability table's derivation/revocation logic"). Based on this review, three primitives are strong, concretely-scoped candidates for the ADR-0061 tool pilot (gates Milestone G4's Specification sub-step): + +- **Capability derivation/revocation logic** β€” `kernel/src/cap/table.rs`'s `cap_derive` (table.rs:258) and `cap_revoke` (table.rs:329), including the BFS descendant-collection loops at table.rs:341-410. This is the primitive G4 already names as the likely target, and it is the highest-value candidate: it is security-critical, its invariants are genuinely multi-step (spanning several functions, per Epic G-R1 above), and Epic G-R1's property tests are a direct, low-cost precursor to G4's Specification sub-step β€” the same parent-liveness / no-cycles / BFS-completeness invariants that a property test defends empirically are exactly what a machine-checkable specification needs to state formally. +- **`SchedQueue`'s bounded-FIFO wraparound arithmetic** β€” `kernel/src/sched/mod.rs:104-134`. Small, self-contained, free of external dependencies or `unsafe`, and already isolated behind a `const`-generic capacity (mod.rs:91). A good lower-risk warm-up candidate for exercising whichever tool ADR-0061 selects before committing to the higher-stakes capability-table primitive. +- **Syscall status-code encoding** β€” `kernel/src/syscall/error.rs`'s `as_status` (error.rs:95) and `kernel/src/syscall/abi.rs`'s `encode_cap_handle`/`decode_cap_handle` (abi.rs:213 area). Purely combinatorial, `unsafe`-free code with an existing exhaustive-match discipline (error.rs:24-27) β€” well suited to a tool that specializes in bounded model-checking over a finite state space, and complementary to the pairwise-disjointness property test in Epic G-R1. + +### Epic G-R3 β€” Decisions to formalize + +- **[Polish]** Make an explicit ADR call on zero-on-free vs. zero-on-alloc-only before capability revocation lands. + - Location: [ADR-0035](../../decisions/0035-physical-memory-manager.md):33 ("Capability-aware extension point... the future capability-system grants will sit on top of `Pmm::alloc_frame`"); `kernel/src/mm/pmm.rs` β€” the module doc's "zero-fill every allocated frame" statement (pmm.rs:18) confirms the current posture is zero-on-alloc-only; `alloc_frame`'s doc (pmm.rs:327-335). + - Action: write an ADR making the zero-on-free vs. zero-on-alloc-only decision explicit before B5+'s `MemoryRegionCap` revocation model creates the first real reader of freed-but-unreused physical frames. Per CLAUDE.md's security-first rule ("when in doubt, choose the more conservative option"), the current zero-on-alloc-only behavior should be either upgraded to zero-on-free or explicitly justified against a stated threat model β€” not inherited silently as the default. Number per the Phase G ADR ledger's normal allocation process. + +- **Measured-boot items** β€” cross-checked against Milestone G1, not new scope. The review found no measured-boot decision point that Milestone G1's ADR-0058 sub-breakdown (item 1: "PCR-like registers, event log, chaining algorithm (SHA-256 vs. -384), where measurements are stored") does not already track. This bullet records that cross-check for completeness; ADR-0058 remains the single place these decisions land, and no additional decision item is proposed here. + +### HAL/BSP: documentation accuracy + +- **[⚫ INFO]** Wrong unsafe-policy section cited for `create_address_space`'s "no separate audit entry" claim. + - Location: `bsp-qemu-virt/src/mmu.rs:174-186` (the `create_address_space` `SAFETY` comment), specifically the citation "per unsafe-policy Β§4" at mmu.rs:180. + - Action: correct the citation β€” [docs/standards/unsafe-policy.md](../../standards/unsafe-policy.md) Β§4 covers `unsafe impl` / `unsafe trait` discipline, not this case (an unsafe-body-free `unsafe fn` implementing a trait method's contract). Cite Β§2 ("`unsafe fn` requires a `# Safety` section") and/or Β§3 ("every `unsafe` block has an audit entry") instead, whose reasoning is what the comment actually relies on. Alternatively, if the project wants to formally recognize "unsafe-body-free `unsafe fn` implementing a safe trait method" as its own named exemption class, add that language explicitly to unsafe-policy.md rather than leaving the reasoning implicit in a single call-site comment. + +### Polish & excellence + +- **[Polish]** `cap_revoke`'s release-build fallback silently reports `Ok(())` on a (currently provably unreachable) invariant violation, rather than failing closed. + - Location: `kernel/src/cap/table.rs:369-374` and the mirrored branch at table.rs:393-398, inside `cap_revoke`'s BFS descendant-collection loop (`cap_revoke` starts at table.rs:329; the size proof backing "provably unreachable" is at table.rs:353-359). + - Action: on the `desc_len >= CAP_TABLE_CAPACITY` branch, return a typed `CapError` (or escalate to a hard `panic!`/`assert!`, matching the precedent `enqueue_ready` already sets for an analogous "should never happen" condition at `kernel/src/sched/mod.rs:605-608`) instead of `break`-ing out to a silent `Ok(())`. Speculative defense-in-depth rather than an active bug β€” the current size proof is sound today β€” but a few lines of change make `cap_revoke` fail-closed under future corruption instead of fail-open. + +- **[Polish]** Promote the two v1 rights-model gaps from source docstrings to `security-model.md`'s tracked Open Questions list. + - Location: `kernel/src/mm/address_space.rs:416-421` (`resolve_address_space_cap`'s "v1 rights model" docstring: an `AddressSpaceCap` grants full mapping authority by kind alone, not by rights bits β€” deferred to B5+); `hal/src/mmu/mod.rs`'s `Mmu::map` flags contract (mod.rs:565-573), which documents only the `DEVICE | EXECUTE` rejection and has no `WRITE ^ EXECUTE` guard β€” confirmed absent by `hal/src/mmu/vmsav8.rs`, whose `WRITE | EXECUTE` flag combination (vmsav8.rs:595, vmsav8.rs:628) is exercised without any rejection path, unlike the `DEVICE | EXECUTE` combination it does reject (vmsav8.rs:671-692). Target: [`docs/architecture/security-model.md`](../../architecture/security-model.md)'s `## Open questions` section (currently ending around line 331). + - Action: add two bullets to security-model.md's Open Questions list: (1) `AddressSpaceCap`'s kind-only authority check, and (2) the absence of a W^X (`WRITE` ^ `EXECUTE`) guard in `Mmu::map`'s flags validation. Centralizing every known authority-model gap in the document a security reviewer reads first β€” human or AI β€” is what turns "correct" into "exemplary" for a project whose stated goal is to be the reference for how a capability kernel documents itself. + +Covers all 1 review finding + 7 polish items routed to this phase. diff --git a/docs/roadmap/phases/phase-h.md b/docs/roadmap/phases/phase-h.md index 27d90d1..a61bad4 100644 --- a/docs/roadmap/phases/phase-h.md +++ b/docs/roadmap/phases/phase-h.md @@ -73,3 +73,71 @@ Business review. The HAL abstraction has been tested by three architecturally di - Whether any HAL trait needs a v2 to accommodate architectural differences that Phase A could not foresee. - The degree to which BSPs should share helper code (e.g., a `bsp-arm-gic` crate between Pi 4 / Pi 5 / Jetson) vs. remaining independent. - Whether to target a specific RISC-V profile (e.g., RVA22) or stay minimal. + +--- + +## Review-derived work items (2026-07-15 full-repository review) + +Phase H's exit bar depends on the HAL abstraction being real: `bsp-pi5`, `bsp-jetson`, and the first RISC-V BSP are each validated, before any hardware is in hand, against the host-side HAL doubles in `test-hal/`, and each new BSP author leans directly on the trait contracts in `hal/`. The two epics below matter precisely *because* of that dependency β€” a test double that diverges from real BSP behavior lets a new-BSP implementation pass host tests while being wrong on hardware, and a HAL trait contract that is ambiguous or under-specified gets a different (and possibly incompatible) reading from each new BSP author who has to interpret it. Both epics are therefore work that should land before or alongside H1 (`bsp-pi5`), so that `bsp-jetson` (H2) and the RISC-V BSP (H3) inherit a truthful test-HAL and a tightened trait surface rather than each rediscovering the same gaps. + +### Epic 1 β€” Test-HAL fidelity & parity + +Keeps the host-side HAL doubles in `test-hal/` bit-faithful to real BSP behavior, so that a new BSP validated against these doubles (gating Milestone H1, and by inheritance H2/H3) is being validated against a truthful model rather than a convenient one. + +- βšͺ **LOW** β€” `BlockMappedMmu` checks block-membership before alignment/flags validation in `test-hal`, inverting the real walker's precedence. + - Location: `test-hal/src/mmu.rs:533-547` and `test-hal/src/mmu.rs:549-558` + - Action: Factor the shared map-precondition validation (alignment + `DEVICE`/`EXECUTE` checks) that `FakeMmu::map` performs into one small helper, and have `BlockMappedMmu::map`/`unmap` (and any other decorator) call it **before** consulting `is_blocked`, so the decorator's check-order is structurally tied to the base fake's (and the real BSP's) precedence instead of relying on convention. This turns the file's own documented claim that "the injecting decorators … add exactly one failure mode each, delegating the success path unchanged" (`test-hal/src/mmu.rs:107-109`) into an enforced invariant rather than a per-decorator duplication. + +- βšͺ **LOW** β€” `FakeUserMem`'s size arithmetic is unchecked in `test-hal`, unlike the identical pattern in sibling kernel test fixtures. + - Location: `test-hal/src/mmu.rs:621` and `test-hal/src/mmu.rs:697` + - Action: Use `npages.checked_mul(PAGE_SIZE).expect("test math")` in both `new()` and `region_len()` (or compute the byte length once and store it), matching the defensive-arithmetic convention already established in `kernel/src/obj/task_loader.rs`'s equivalent fixture. + +- βšͺ **LOW** β€” `FakeUserMem::write`/`read` bound checks use unchecked addition, the same overflow-then-OOB shape as the `npages * PAGE_SIZE` finding above, at a different site. + - Location: `test-hal/src/mmu.rs:663` and `test-hal/src/mmu.rs:679` + - Action: Use `off.checked_add(bytes.len()).is_some_and(|end| end <= self.region_len())` (and the `read` equivalent) so an overflowing offset/length combination fails the guard instead of silently wrapping past it. + +- βšͺ **LOW** β€” `FakeIrqController::acknowledge` does not gate on `IrqController::enable`, unlike the real GICv2, so a kernel bug that forgets to enable an IRQ line before expecting delivery is invisible to host tests. + - Location: `test-hal/src/irq_controller.rs:45-55, 124-126, 217-226` + - Action: Keep the loose `inject`/`acknowledge` for tests that deliberately want to bypass gating, but add a second, stricter helper (e.g. `acknowledge_enabled_only`, or a `debug_assert` behind a `strict` flag set at construction) that mirrors the real GIC by only returning an IRQ from `acknowledge` if `is_enabled` is true for it. Route any future scheduler/timer-IRQ integration tests through the strict variant β€” relevant as H1–H3 each bring up interrupt handling on new hardware β€” so a missing `enable()` call is caught on the host rather than on real silicon. + +- βšͺ **LOW** β€” `FakeIrqController::end_of_interrupt` does not validate the acknowledge/EOI pairing invariant its own trait documents. + - Location: `test-hal/src/irq_controller.rs:128-130` + - Action: Track an "active" set populated by `acknowledge` and cleared by `end_of_interrupt`; have `end_of_interrupt` panic (matching this crate's existing style of enforcing architectural invariants via `assert!`, e.g. the `FAKE_MAX_IRQ` guard) when called with an IRQ that is not currently active. Low priority since `FakeIrqController` is not yet consumed by any kernel test (confirmed via repo-wide search), but worth doing before it is wired into any Phase H BSP's IRQ-handling tests. + +- βšͺ **LOW** β€” `FakeTimer::set_now` silently permits moving the clock backward, violating the `Timer` trait's monotonicity contract, and this is already exercised unflagged by the crate's own tests. + - Location: `test-hal/src/timer.rs:48-55, 131-137` + - Action: Document on `set_now` that it intentionally bypasses the trait's monotonicity guarantee and is for test setup only (e.g. seeding the clock before other calls), not for simulating time reversal mid-scenario; consider renaming to make the escape hatch obvious (e.g. `reset_now_unchecked`) or adding a `debug_assert` that most call sites only use it before any `advance`/read has occurred. + +- βšͺ **LOW** β€” `FakeTimer::new` accepts `resolution_ns == 0`, a value no real `Timer` implementation can produce. + - Location: `test-hal/src/timer.rs:24-36` + - Action: Add `assert!(resolution_ns > 0, "FakeTimer: resolution_ns must be > 0 β€” no real Timer implementation can report 0")` in `new`, mirroring the production invariant, so the fake cannot model an impossible timer β€” one that a new BSP's real timer driver (H1–H3) could never actually exhibit. + +- βšͺ **LOW** β€” `FakeContextSwitch::init_context` resets `is_user`/`user_sp`/`entry_addr` on slot reuse but leaves the `switched` marker stale. + - Location: `test-hal/src/context_switch.rs:170-185` (contrast with the reuse test at 278-299) + - Action: Add `ctx.switched = false;` to both `init_context` and `init_user_context` for full re-seed consistency, and extend `init_context_clears_prior_user_markers_on_reuse` (or add a sibling test) to assert `switched` is also cleared on reuse. + +- βšͺ **LOW** β€” `FakeIrqController::inject` lacks the `FAKE_MAX_IRQ` range guard that `enable`/`disable` enforce, letting tests inject architecturally-impossible `IrqNumber`s. + - Location: `test-hal/src/irq_controller.rs:53-55` + - Action: Add the same `assert!(irq.0 < FAKE_MAX_IRQ, ...)` guard to `inject` (or explicitly document why injection is allowed to model impossible INTIDs, if that is ever intentionally useful for a specific negative test). This keeps the fake's three interrupt-numbering entry points (enable/disable/inject) consistently bounded to what real GICv2 hardware can actually present, so a test author's typo or miscomputed constant surfaces as a host-side panic instead of silently exercising the kernel against a state hardware can never produce. + +### Epic 2 β€” HAL abstraction hardening + +Trait-bound items in `hal/` itself that stress the abstraction surface Phase H's exit bar depends on ("the HAL abstraction has been tested by three architecturally distinct targets"). Unlike Epic 1, these are contract gaps in the trait definitions, not in the test doubles that model them β€” left unresolved, each new BSP author in H1–H3 answers the same open question independently, risking divergent (and possibly incompatible) behavior across `bsp-pi5`, `bsp-jetson`, and the RISC-V BSP. + +- 🟑 **MEDIUM** β€” `Timer::resolution_ns()`'s trait doc says deadlines round "to nearest," which contradicts the ceiling-rounding rationale that the trait's own `arm_deadline` contract actually requires. + - Location: `hal/src/timer.rs:50-54` + - Action: Reword `Timer::resolution_ns`'s doc to match the contract `arm_deadline`/`ns_to_ticks` actually enforce, e.g.: "Deadlines round *up* to the next multiple of this value (ceiling), never down, so `arm_deadline`'s 'fires at-or-after deadline_ns' guarantee holds; finer precision at the call site is silently lost." Left as-is, a future BSP author (e.g. for the RISC-V target in Milestone H3, or a Pi 5/Jetson-style target that arms its hardware comparator directly rather than reusing `ns_to_ticks`) could implement a literal "round to nearest" reading and produce deadlines that fire early, silently breaking `sleep_until`-class correctness. + +- βšͺ **LOW** β€” `IrqController::enable`/`disable`'s contract is silent on out-of-range `IrqNumber` behavior; the one shipped implementation diverges from the ADR's own stated expectation by panicking instead of no-op'ing. + - Location: `hal/src/irq_controller.rs:37-59` + - Action: Settle the out-of-range behavior in an **ADR before any H1–H3 implementation assumes a particular contract** β€” changing a HAL trait signature is a structural decision (this file's own Out-of-scope calls out "rework of HAL traits … resolved via ADR"), and moving `enable`/`disable` to `Result<(), IrqError>` is exactly such a change, with migration impact on every existing and future BSP impl plus all kernel call sites. Extend or supersede [ADR-0011](../../decisions/0011-irq-controller-trait.md)'s open question with an ADR that (a) chooses one required behavior for an out-of-range `IrqNumber` β€” `Result<(), IrqError>`, clamp/no-op, or a documented panic β€” weighing error-handling.md's rule that `panic!` is reserved for broken invariants and is not a substitute for `Result` once callers are less trusted (the `IrqCap` capability layer will feed capability-derived values in), and (b) records the trait-signature migration impact. **Until that ADR is approved, H1–H3 must not assume a `Result`-based signature** β€” pin the decision down before the GICv2-derived (H1, H2) and PLIC-derived (H3, per Milestone H3's sub-breakdown item 2) `IrqController` impls each have to guess. + +This epic's companion constants-duplication concern β€” the GICv2 architectural max-IRQ value living independently in `test-hal` and in the real driver β€” is not a defect but is carried below as a polish item, since it is currently harmless drift risk rather than an observed inconsistency. + +### Polish & excellence + +- **Polish** β€” Tighten the few `test-hal` SAFETY comments that don't yet name the rejected safer alternative, bringing them up to the same letter-of-policy standard the rest of the crate already sets (it cross-references PR review rounds and ADRs for its fidelity gaps). +- **Polish** β€” De-duplicate the GICv2 architectural max-IRQ constant between `FakeIrqController` and the real driver **without** hoisting it into the generic `tyrne_hal::irq_controller` trait module: that module is architecture-neutral and must stay so for the RISC-V PLIC path (Milestone H3), whose interrupt numbering has no ARM GIC bound β€” an ARM-only constant does not belong in the generic HAL API. Share it as an **implementation-local** constant instead (e.g. a GICv2-specific `hal` submodule or a small shared `gic` constants module that both `bsp-qemu-virt`'s driver and `test-hal`'s GICv2-modelling `FakeIrqController` import), so the fake and the real driver track one GICv2 bound β€” and `bsp-pi5`/`bsp-jetson` (H1/H2) reuse that GIC value while the PLIC BSP (H3) is free to define its own β€” rather than a future GICv3/LPI extension or differing board max-IRQ silently drifting the fake out of sync with the bound it models. +- **Polish** β€” Note in `test-hal`'s `lib.rs` Status section that `FakeIrqController` and `FakeTimer` are not yet consumed by any kernel test (confirmed via repo-wide search), turning "this fidelity gap doesn't matter yet" into a documented, trackable fact ahead of Phase H wiring these fakes into new-BSP-adjacent test suites. + +Covers all 11 review findings + 3 polish items routed to this phase (one map-validation polish item folded into Epic 1's `BlockMappedMmu` check-order finding).