review: iteration-2 fixes (fail-closed loader whole-name match + doc accuracy)
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/<name>/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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wB8KB3MMQztRDXCMCkPrf
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
c23de76ccc
commit
5a8b60ba3a
@@ -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
|
- **`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
|
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
|
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
|
recovery maneuver). The matching firmware-side `Bus` seam on flared's `devmem` — so
|
||||||
that assert the shipped ladder pokes the confirmed offset, never the wrong-SoC one.
|
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
|
- **`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,
|
(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
|
and fault injection — silent-drop and forced-NAK) so `warden-modbus`'s master can be
|
||||||
|
|||||||
@@ -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`,
|
- **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
|
`relays.c`, `warden_rga.c` (wrapper), `hpmcu.rs`, `devmem.rs`, `freshness.c`, plus
|
||||||
the two smallest near-mainline drivers where feasible. Enforced in CI
|
the two smallest near-mainline drivers where feasible. Enforced in CI
|
||||||
(`gcc-14 -fcondition-coverage` + `gcov-14 --conditions`; `cargo-llvm-cov --mcdc`
|
(`gcc-14 -fcondition-coverage` + `gcov-14 --conditions`). Rust uses
|
||||||
for Rust).
|
`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**
|
- **Tier 2 — ported/vendor drivers → fault-injection + branch coverage + benchmarks**
|
||||||
against the simulator, behind a stable seam. Explicitly NOT literal MC/DC.
|
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."
|
rest, a very serious testing and benchmarking system."
|
||||||
- The seam is the shared object: the same thing MC/DC is measured against and the
|
- The seam is the shared object: the same thing MC/DC is measured against and the
|
||||||
simulator implements — the two goals reinforce, not duplicate.
|
simulator implements — the two goals reinforce, not duplicate.
|
||||||
- Every Tier-1 file gets a `drivers/<name>-mcdc/` host harness with an
|
- Every Tier-1 file gets a `drivers/<name>/test/` host harness (a `Makefile` +
|
||||||
`enforce-mcdc.sh` that fails CI below 100%.
|
`test_<name>.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%.
|
||||||
|
|||||||
+1
-1
@@ -31,7 +31,7 @@ is already modelled and tested here** in `../sim/`:
|
|||||||
|
|
||||||
| Driver | Serious-testing status | SDK model |
|
| 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 |
|
| `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 |
|
| HPMCU supervisor (`hpmcu.rs`) | arm/beat/fire + boot-grace safety property | `sim::hpmcu` (7 tests) + `hpmcu_tick` benchmark |
|
||||||
|
|
||||||
|
|||||||
@@ -54,7 +54,7 @@ fn parse_addr(s: &str) -> Option<u64> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// 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
|
/// 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
|
/// 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
|
/// reserved. Anything NOT on this allowlist that declares a LOAD_ADDR is treated as
|
||||||
@@ -62,17 +62,33 @@ fn parse_addr(s: &str) -> Option<u64> {
|
|||||||
/// "Rtos"/"Bl32"/"M0" must not slip through an MCU-*name* allowlist the way the
|
/// "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
|
/// original `hpmcu`/`mcu`/`amp` substring test would have — that is exactly the
|
||||||
/// c8a3 0x40000-brick class this tool exists to catch.
|
/// 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 {
|
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] = &[
|
const SAFE: &[&str] = &[
|
||||||
"flashdata",
|
"flashdata",
|
||||||
"flashboot",
|
"flashboot",
|
||||||
|
"nandboot",
|
||||||
|
"nandboot1",
|
||||||
|
"nandboot2",
|
||||||
|
"sdboot",
|
||||||
|
"emmcboot",
|
||||||
|
"spinorboot",
|
||||||
"ddr",
|
"ddr",
|
||||||
"usbplug",
|
"usbplug",
|
||||||
"spl",
|
"spl",
|
||||||
"uboot",
|
"uboot",
|
||||||
"u-boot",
|
|
||||||
"loader",
|
"loader",
|
||||||
|
"miniloader",
|
||||||
"trust",
|
"trust",
|
||||||
"tee",
|
"tee",
|
||||||
"atf",
|
"atf",
|
||||||
@@ -80,7 +96,7 @@ fn is_known_safe_loader(name: &str) -> bool {
|
|||||||
"fsbl",
|
"fsbl",
|
||||||
"idblock",
|
"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=<name>`
|
/// Extract MCU firmware loads from an rkbin loader `.ini`: for each `LOADERn=<name>`
|
||||||
@@ -360,4 +376,37 @@ LOAD_ADDR=0x40000
|
|||||||
// and it's caught when no reserved-memory node covers it:
|
// and it's caught when no reserved-memory node covers it:
|
||||||
assert_eq!(check(&loads, &parse_reserved_ranges(DT_NO_RTOS)).len(), 1);
|
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<u64> = loads.iter().map(|l| l.load_addr).collect();
|
||||||
|
addrs.sort();
|
||||||
|
assert_eq!(addrs, vec![0x50000, 0x60000, 0x70000], "loads: {loads:?}");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user