From 5a8b60ba3a6c8b49c163680131e1ce5900cc7258 Mon Sep 17 00:00:00 2001 From: BFE Engineering Date: Tue, 25 Aug 2026 16:02:21 -0600 Subject: [PATCH] review: iteration-2 fixes (fail-closed loader whole-name match + doc accuracy) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second recursive pass: two adversarial re-reviewers verified the iteration-1 fixes. Fuzz/empirical checks cleared the freshness min-budget rewrite (200k random trials), the config-lint reg-token scanner, its UTF-8 boundary safety, and the build-kernel.sh trap (5 exit scenarios) — no defects. Three items corrected here: - config-lint is_known_safe_loader: match the WHOLE normalized loader name, not an unanchored substring. The iteration-1 allowlist swap kept `contains()`, so a future coprocessor whose name merely contained a boot word ("AudioLoader" ⊃ "loader", "SplRtos" ⊃ "spl", "Bl32" ≠ "bl31") would have been waved through — reopening the 0x40000-brick false-negative the fail-closed change exists to prevent. Regression test added with those exact adversarial names. - docs/architecture.md §3: the `cru` bullet no longer claims flared's devmem `Bus` seam is shipped — it lands when flare-edge consumes warden-sdk ([maintainer]-gated), which is what §7 item 3 already said. Resolves an in-document contradiction. - drivers/README.md: modbus "11 pty scenarios" -> "8 pty scenarios + 3 wire/daemon checks (11 total)", matching flare-edge tools/modbus-sim's actual SCENARIOS list. - docs/decisions/0002-mcdc-tiering.md: Consequences now describe the shared drivers/enforce-mcdc.sh + drivers//test/ layout actually built (not the per-driver dirs the ADR first anticipated); Rust MC/DC tooling reality noted. config-lint: 9 tests pass; clippy clean under -D warnings; gitleaks clean. C drivers untouched (still relays 40/40, freshness 66/66 MC/DC). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017wB8KB3MMQztRDXCMCkPrf --- docs/architecture.md | 6 ++- docs/decisions/0002-mcdc-tiering.md | 13 +++++-- drivers/README.md | 2 +- tools/config-lint/src/lib.rs | 57 +++++++++++++++++++++++++++-- 4 files changed, 67 insertions(+), 11 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 0089902..d3c79e7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -63,8 +63,10 @@ supervisor logic runs in CI with no panel. - **`cru` — reset ladder.** Done. `CruSim` on `MemBus` (so `flared::devmem::hard_reset`'s ladder is host-tested against the known glb_srst_fst / DW-watchdog registers), plus the boot-mode register's survives-warm-reset / cleared-by-POR behaviour (the MaskRom - recovery maneuver). flared's `devmem` now has a matching `Bus` seam and unit tests - that assert the shipped ladder pokes the confirmed offset, never the wrong-SoC one. + recovery maneuver). The matching firmware-side `Bus` seam on flared's `devmem` — so + the shipped ladder can be asserted to poke the confirmed offset, never the wrong-SoC + one — lands when flare-edge consumes warden-sdk (§7 item 3, [maintainer]-gated), not yet on + flare-edge `main`. - **`modbus` — RS-485 device end.** Done. `ModbusSlave`: a byte-in/byte-out RTU slave (CRC16 byte-identical to the master, FC 0x01–0x06/0x0F/0x10/0x11, exception replies, and fault injection — silent-drop and forced-NAK) so `warden-modbus`'s master can be diff --git a/docs/decisions/0002-mcdc-tiering.md b/docs/decisions/0002-mcdc-tiering.md index b9e9ffa..a97ff3f 100644 --- a/docs/decisions/0002-mcdc-tiering.md +++ b/docs/decisions/0002-mcdc-tiering.md @@ -14,8 +14,10 @@ A **two-tier** policy, measured against the Hardware Abstraction Seam: - **Tier 1 — our own hardware-facing code → real 100% MC/DC.** `modbus_engine.c`, `relays.c`, `warden_rga.c` (wrapper), `hpmcu.rs`, `devmem.rs`, `freshness.c`, plus the two smallest near-mainline drivers where feasible. Enforced in CI - (`gcc-14 -fcondition-coverage` + `gcov-14 --conditions`; `cargo-llvm-cov --mcdc` - for Rust). + (`gcc-14 -fcondition-coverage` + `gcov-14 --conditions`). Rust uses + `cargo-llvm-cov` line/region coverage for now — true `--mcdc` needs a nightly + toolchain (`-Z coverage-options=condition`) and is deferred on that tooling skew; + the C drivers carry the literal MC/DC gate. - **Tier 2 — ported/vendor drivers → fault-injection + branch coverage + benchmarks** against the simulator, behind a stable seam. Explicitly NOT literal MC/DC. @@ -24,5 +26,8 @@ A **two-tier** policy, measured against the Hardware Abstraction Seam: rest, a very serious testing and benchmarking system." - The seam is the shared object: the same thing MC/DC is measured against and the simulator implements — the two goals reinforce, not duplicate. -- Every Tier-1 file gets a `drivers/-mcdc/` host harness with an - `enforce-mcdc.sh` that fails CI below 100%. +- Every Tier-1 file gets a `drivers//test/` host harness (a `Makefile` + + `test_.c`) that calls the one shared `drivers/enforce-mcdc.sh` — which derives + the driver name from the `.gcov` file, so there is a single gate to maintain, not a + per-driver copy. The CI `mcdc` job auto-discovers any `drivers/*/test/Makefile` and + fails below 100%. diff --git a/drivers/README.md b/drivers/README.md index 011d193..b197586 100644 --- a/drivers/README.md +++ b/drivers/README.md @@ -31,7 +31,7 @@ is already modelled and tested here** in `../sim/`: | Driver | Serious-testing status | SDK model | |---|---|---| -| `modbus_engine.c` (RS485 master) | 11 pty scenarios + fault-injection + a compiled corpus walk (flare-edge `tools/modbus-sim/`, green) | `sim::modbus` RTU slave (11 tests, silent-drop/forced-NAK faults) + `modbus_read_holding` benchmark | +| `modbus_engine.c` (RS485 master) | 8 pty scenarios + 3 wire/daemon checks (11 total) + fault-injection (flare-edge `tools/modbus-sim/`, green) | `sim::modbus` RTU slave (11 tests, silent-drop/forced-NAK faults) + `modbus_read_holding` benchmark | | `warden_rga.c` (RGA offload) | offload-dispatch + CPU-fallback logic | `sim::rga` recording `improcess` fake (programmable IM_STATUS) + `rga_improcess` benchmark | | HPMCU supervisor (`hpmcu.rs`) | arm/beat/fire + boot-grace safety property | `sim::hpmcu` (7 tests) + `hpmcu_tick` benchmark | diff --git a/tools/config-lint/src/lib.rs b/tools/config-lint/src/lib.rs index fadefe1..8da09c2 100644 --- a/tools/config-lint/src/lib.rs +++ b/tools/config-lint/src/lib.rs @@ -54,7 +54,7 @@ fn parse_addr(s: &str) -> Option { } } -/// Loaders that are part of the normal RK boot chain — DDR init, SPL/loader, the +/// Loaders that are part of the normal RK boot chain — DDR init, SPL/miniloader, the /// secure monitor, U-Boot — and are NOT boot-loaded coprocessor firmware, so their /// LOAD_ADDR (if any) is the vendor boot flow, not a DRAM carve-out that must be /// reserved. Anything NOT on this allowlist that declares a LOAD_ADDR is treated as @@ -62,17 +62,33 @@ fn parse_addr(s: &str) -> Option { /// "Rtos"/"Bl32"/"M0" must not slip through an MCU-*name* allowlist the way the /// original `hpmcu`/`mcu`/`amp` substring test would have — that is exactly the /// c8a3 0x40000-brick class this tool exists to catch. +/// +/// Matched as a WHOLE normalized name (separators stripped), never a substring, so a +/// coprocessor whose vendor name merely *contains* a boot-chain word — "AudioLoader" +/// ⊃ "loader", "SplRtos" ⊃ "spl", "Bl32" (≠ "bl31") — is still checked, not waved +/// through. Over-inclusion here would only over-report (the safe direction); a missed +/// coprocessor is the dangerous one. fn is_known_safe_loader(name: &str) -> bool { - let n = name.to_ascii_lowercase(); + let n: String = name + .chars() + .filter(|c| c.is_ascii_alphanumeric()) + .map(|c| c.to_ascii_lowercase()) + .collect(); const SAFE: &[&str] = &[ "flashdata", "flashboot", + "nandboot", + "nandboot1", + "nandboot2", + "sdboot", + "emmcboot", + "spinorboot", "ddr", "usbplug", "spl", "uboot", - "u-boot", "loader", + "miniloader", "trust", "tee", "atf", @@ -80,7 +96,7 @@ fn is_known_safe_loader(name: &str) -> bool { "fsbl", "idblock", ]; - SAFE.iter().any(|s| n.contains(s)) + SAFE.contains(&n.as_str()) } /// Extract MCU firmware loads from an rkbin loader `.ini`: for each `LOADERn=` @@ -360,4 +376,37 @@ LOAD_ADDR=0x40000 // and it's caught when no reserved-memory node covers it: assert_eq!(check(&loads, &parse_reserved_ranges(DT_NO_RTOS)).len(), 1); } + + // Coprocessor names that merely *contain* a safe boot-chain word, each with a + // LOAD_ADDR: "AudioLoader" ⊃ "loader", "SplRtos" ⊃ "spl", "Bl32" (≠ "bl31"). + const SUBSTRING_TRAP_INI: &str = r#" +[LOADER_OPTION] +NUM=4 +LOADER1=FlashData +LOADER2=AudioLoader +LOADER3=SplRtos +LOADER4=Bl32 +FlashData=bin/ddr.bin +AudioLoader=bin/audio.bin +SplRtos=bin/rtos.bin +Bl32=bin/bl32.bin +[LOADER2_PARAM] +LOAD_ADDR=0x50000 +[LOADER3_PARAM] +LOAD_ADDR=0x60000 +[LOADER4_PARAM] +LOAD_ADDR=0x70000 +"#; + + /// Regression (2nd-pass correctness review): the safe allowlist matches a WHOLE + /// normalized name, never a substring — a coprocessor whose name contains a + /// boot-chain word must NOT be waved through. FlashData (a real boot component, + /// no LOAD_ADDR here) stays safe; the three coprocessors are all checked. + #[test] + fn safe_loader_is_whole_name_not_substring() { + let loads = parse_ini_mcu_loads(SUBSTRING_TRAP_INI); + let mut addrs: Vec = loads.iter().map(|l| l.load_addr).collect(); + addrs.sort(); + assert_eq!(addrs, vec![0x50000, 0x60000, 0x70000], "loads: {loads:?}"); + } }