diff --git a/MEMORY_AUDIT.md b/MEMORY_AUDIT.md index 69ee06d..1c084d6 100644 --- a/MEMORY_AUDIT.md +++ b/MEMORY_AUDIT.md @@ -4,6 +4,14 @@ > point-in-time review of the `unsafe`/FFI surface; line numbers and the > soundness arguments below are only valid against that tree. Re-run it after > any change to `usb_kbd.rs`, the SD/git FFI, or an `esp-idf-sys` bump. +> +> **Remediation (2026-07-10).** Findings #1 and #3 are addressed in follow-up +> commits, and the decode logic is now the host-tested `keymap` crate (14 tests, +> including an ASCII-invariant sweep over all 256 usage IDs and a never-panics +> fuzz). The `usb_kbd.rs` line numbers below predate that refactor. The fixes are +> verified by inspection plus the `keymap` tests, but still need an on-device +> hot-plug run to confirm — #1/#3 live in FFI that can't run on the host. +> Per-finding status is inline. ## Scope & method @@ -39,6 +47,10 @@ operation. - Closing #1 so the in-flight invariant is explicit → 9. A 10 on FFI this heavy needs the structural guards in the "Regression testing" section. +**Update (2026-07-10):** #1 and #3 are now addressed in code and the decode path +is host-tested (see per-finding status). Once the hot-plug bench run confirms #1 +on hardware this becomes a 9; #2 and #4 are the remaining gap to 10. + This is a *memory-safety* score. Robustness (leaks on hot-plug) and correctness would score separately and slightly lower. @@ -84,6 +96,13 @@ before freeing — or track an in-flight flag set on submit and cleared in minimum, check the return value of `usb_host_transfer_free` and don't null the pointer / proceed to `device_close` while it reports the transfer busy. +**Status: fixed in code.** A `REPORT_INFLIGHT` flag is set on submit and cleared +first thing in every `report_cb`. The teardown moved into a `close_device` +helper that pumps client events (bounded) until the transfer quiesces before +freeing it — and *leaks it rather than frees* if it never does, since a leak is +recoverable and a UAF is not. `usb_host_transfer_free`'s return value is now +checked. Needs the hot-plug bench run below to confirm on hardware. + ### 2. `mem::zeroed()` / `MaybeUninit::zeroed().assume_init()` on bindgen structs is a latent footgun — `usb_kbd.rs:110,143`, `sd_fat.rs:138,173,192` **Sound today**: every field of the zeroed descriptors is valid at all-zero (C @@ -101,6 +120,12 @@ Prefer keeping them as `MaybeUninit` and writing fields via `addr_of_mut!`, or at least add a static/compile-time assertion (or a test) that pins the zero-is-valid assumption so a dependency bump fails loudly. +**Status: left as-is deliberately** (sound today, low severity). There is no +clean *stable* compile-time "all-zero is a valid bit pattern" assertion, and a +runtime canary can't fire before the UB it would guard (`assume_init` is the UB +site). The `addr_of_mut!` rewrite is the real fix but churns five call sites for +a latent-only risk; deferred until an `esp-idf-sys` bump makes it worth it. + ### 3. Resource leaks on re-attach and on submit error — not UB — `usb_kbd.rs:163-168, 417/436, 449` - A second keyboard attaching while one is open makes `setup_keyboard` overwrite @@ -116,6 +141,10 @@ which matters for an always-powered appliance. Guard the re-attach case (`if !open_dev.is_null()` → tear down first) and free-on-error in `control_request`. +**Status: fixed in code.** A new attach while a device is still open now runs +`close_device` on the old one first. `control_request` and `start_report_polling` +free the transfer they allocated on a submit error before returning. + ### 4. USB thread stack sizing is unverified — `usb_kbd.rs:121,132` Daemon thread = 4096 B, client thread = 8192 B. The client thread runs @@ -175,13 +204,18 @@ split by what's reachable where, ranked by leverage. `handle_report`'s decode, the editor text ops, `changed_rows` / `only_adds_ink` in `main.rs`, the `epd` row math. Pull them into a no-esp-deps module/crate (workspace member or `#[cfg]`-gated) so `cargo test` - runs on host. Then: - - **Fuzz `handle_report` on host under Miri or ASAN** — the single most - valuable test. It's the exact path where a broken/malicious keyboard's - bytes meet `from_raw_parts` + slicing; feed arbitrary `&[u8]` and Miri - catches any OOB the clamp fails to prevent. Guards finding #5. - - Unit-test that `translate` never emits a non-ASCII `char`, pinning the - invariant `editor.rs` byte-indexing depends on. + runs on host. + - **Done for the keyboard decode.** `translate` + the report edge-detection + are now the `../keymap` crate (`#![no_std]`, `#![forbid(unsafe_code)]`, zero + deps), with 14 host tests including a `translate` **ASCII-invariant sweep** + over all 256 usage IDs × modifiers and a **never-panics fuzz** feeding + arbitrary-length/content byte streams. The slice itself is bounds-clamped in + the FFI layer (finding #5), and once it's a safe `&[u8]` the decode is + `forbid(unsafe_code)` — so Miri adds nothing over the panic-freedom the fuzz + test already proves; the plain `cargo test` run is the guard. + - **Still TODO:** the editor text ops and `changed_rows`/`only_adds_ink` are + also FFI-free and worth the same treatment, but stay coupled to + `embedded-graphics` / `epd` constants for now. 2. **Compile-time guards for the `zeroed()` assumption (#2).** Static assertions (or a test constructing the struct and checking a sentinel field is `None`/`0`) so an `esp-idf-sys` bump fails loudly instead of going silently @@ -189,7 +223,9 @@ split by what's reachable where, ranked by leverage. 3. **`clippy` as a ratchet.** `#![warn(clippy::undocumented_unsafe_blocks)]` + `clippy::multiple_unsafe_ops_per_block`, deny-warnings in CI. Forces every new `unsafe` to carry a SAFETY comment — keeps the existing discipline from - eroding. + eroding. **Deferred:** turning it on crate-wide today floods warnings on the + many existing bare-FFI `unsafe` one-liners, which trains people to ignore it. + Worth doing *after* a pass that adds `// SAFETY:` to the existing blocks. 4. **On-device tests for what only exists on device (#1, #3, #4).** - **Hot-plug stress loop**: attach/detach ~100× on a bench script, log `esp_get_free_heap_size` each cycle. A downward trend proves the leaks diff --git a/README.md b/README.md index c5f9cf6..64cb944 100644 --- a/README.md +++ b/README.md @@ -144,8 +144,9 @@ package.json pnpm + oxfmt — formatting toolchain for docs/JSON ≤32 GB card ([postmortem](docs/postmortems/2026-07-05-spike3-sd-cmd59.md)). - [ ] Heap fragmentation over a long writing session with the PSRAM allocator. - [ ] Real-world e-ink ghosting with the current partial-refresh cadence. -- [ ] Possible use-after-free freeing the in-flight USB transfer on keyboard - unplug ([`MEMORY_AUDIT.md`](MEMORY_AUDIT.md) finding #1). +- [~] Use-after-free freeing the in-flight USB transfer on keyboard unplug — + fixed in code, pending an on-device hot-plug run to confirm + ([`MEMORY_AUDIT.md`](MEMORY_AUDIT.md) finding #1). Retired risks ([gix push](docs/postmortems/2026-07-05-spike7-gix-https-push.md), TinyUSB HID stability, TLS heap, libgit2-on-xtensa) and how they died: