diff --git a/MEMORY_AUDIT.md b/MEMORY_AUDIT.md new file mode 100644 index 0000000..69ee06d --- /dev/null +++ b/MEMORY_AUDIT.md @@ -0,0 +1,208 @@ +# Memory-safety audit — firmware Rust + +> **Snapshot.** Audited at commit `371b542` on 2026-07-10. This is a +> 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. + +## Scope & method + +Memory-unsafety in Rust can only originate in `unsafe`, so the audit focuses +there. The entire `unsafe` surface is FFI into ESP-IDF / libgit2, concentrated +in: + +- `firmware/src/usb_kbd.rs` — by far the largest, and the only place with C + callbacks, raw transfer buffers, and `slice::from_raw_parts`. **Highest + risk.** +- `firmware/src/bin/{sd_fat,git_push,git_sync,wifi_tls}.rs` — descriptor-struct + zeroing plus simple FFI calls. +- `firmware/src/epd.rs`, `firmware/src/editor.rs`, + `spikes/spike7-git-push/` — **100% safe Rust, zero `unsafe`.** They cannot + cause UB; their failure mode is panic→abort, not corruption. + +Bottom line: the FFI code is careful, with real SAFETY reasoning and one +genuinely good defensive clamp. The audit found **one plausible true-UB path** +(conditional, ordering-dependent) plus a set of latent footguns and non-UB +robustness gaps. Nothing looks like a slam-dunk exploitable bug in normal +operation. + +## Score: 8 / 10 (memory safety only) + +- The design leans on safe Rust — only ~40 lines of genuine `unsafe`, all thin + FFI wrappers, and the safe core (editor, framebuffer, layout) can't produce + UB at all. Right architecture; most of the score. +- Every `unsafe` site carries real SAFETY reasoning, and the one place + untrusted device data sizes a raw slice (`report_cb`) is correctly clamped. +- Not a 9–10 because finding #1 is closed by an *assumed* event ordering rather + than by construction, and findings #2–#3 are latent, dependency-sensitive + risks. Real memory safety means the invariant is *enforced*, not hoped for. +- 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. + +This is a *memory-safety* score. Robustness (leaks on hot-plug) and correctness +would score separately and slightly lower. + +## Findings + +### 1. Possible use-after-free freeing the interrupt transfer on unplug — `usb_kbd.rs:176-182` (highest) + +On `DEV_GONE`, `client_loop` frees `report_xfer` and closes the device: + +```rust +if !report_xfer.is_null() { + unsafe { usb_host_transfer_free(report_xfer) }; // line 177 + report_xfer = ptr::null_mut(); +} +``` + +The interrupt-IN transfer is resubmitted on every completion (`report_cb`, +line 224), so it is **in-flight** most of the time. `report_cb` only fires from +inside `usb_host_client_handle_events` (line 159); the free happens *after* that +call returns. The code implicitly relies on the transfer's final +canceled-completion callback having already run in the same `handle_events` +batch that delivered `DEV_GONE`. + +If the library delivers the `DEV_GONE` client event **before** the transfer's +cancellation callback, then either: + +- `usb_host_transfer_free` refuses an in-flight transfer + (`ESP_ERR_INVALID_STATE` — its return value is **ignored** here → silent + leak), or +- a later `usb_host_client_handle_events` iteration invokes `report_cb` on the + freed transfer → `let t = unsafe { &mut *transfer };` (line 219) is a + **use-after-free**. + +Ordering-dependent, so medium confidence rather than a definite always-fires +bug — but it's the one path in the codebase that can reach real UB, and it's +exactly the teardown race ESP-IDF's USB Host contract warns about (free only +when not in-flight). Verify against the library semantics rather than assuming +the batch ordering holds. + +**Fix.** Halt/dequeue the endpoint and wait for the last completion callback +before freeing — or track an in-flight flag set on submit and cleared in +`report_cb`, and only free once it's clear (loop `handle_events` until then). At +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. + +### 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 +fn pointers are `Option` → `None`; floats → `0.0`; enums are `u32` +aliases; bools → `false`). + +The risk is that this soundness is **invisible and unenforced**. `esp-idf-sys` +is pinned to a git branch (`[patch.crates-io]` in `Cargo.toml`), so a bindgen +regen that introduces a field where zero is an invalid bit pattern (a reference, +`NonNull`, or a niche enum) turns `assume_init()` into instant UB with no +compiler warning — the classic `zeroed`-on-FFI trap. + +**Fix.** These structs are fully overwritten in their meaningful fields anyway. +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. + +### 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 + `open_dev`/`report_xfer` (lines 164-167) without freeing/closing the previous + ones → leaked transfer + device handle. +- `control_request`: if `usb_host_transfer_submit_control` errors, the `?` at + line 430 returns before `usb_host_transfer_free(xfer)` (line 436) → leaked + 64-byte transfer. A submit failure in `start_report_polling` (line 458) leaks + similarly. + +Not memory-unsafe — worst case is heap exhaustion over many hot-plug cycles, +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`. + +### 4. USB thread stack sizing is unverified — `usb_kbd.rs:121,132` + +Daemon thread = 4096 B, client thread = 8192 B. The client thread runs +`report_cb → handle_report → enqueue → log::info!`, and formatting/logging is +stack-hungry; a FreeRTOS stack overflow is silent memory corruption unless the +canary/MPU check catches it. `git_push.rs` already reasons carefully about this +(96 KB, with a comment block on why); the USB threads deserve the same +measured-headroom treatment. Low confidence it's actually too small — measure +the high-water mark, don't change blindly. + +### 5. `report_cb` bounds clamp — done right (noted, not a defect) — `usb_kbd.rs:221-222` + +```rust +let n = (t.actual_num_bytes as usize).min(BOOT_REPORT_LEN); +let report = unsafe { core::slice::from_raw_parts(t.data_buffer, n) }; +``` + +The one place device-controlled data sizes a raw slice. `.min(BOOT_REPORT_LEN)` +correctly clamps even a negative/garbage `actual_num_bytes` (the `i32 as usize` +blows up huge, `.min(8)` reins it back), and `handle_report` re-guards with +`report.len() < 3`. A malicious/broken keyboard can't overread the 8-byte +buffer here. + +## Safe modules — no UB possible by construction + +`editor.rs`, `epd.rs`, and the spike are safe Rust. Two invariants confirmed +rather than assumed: + +- **`editor.rs` byte-indexing invariant holds.** It slices the buffer by byte + offset treating it as a char index (`self.text[..self.caret]`, + `text.as_bytes()[..]`). Valid only because the buffer is pure ASCII — and it + is: the only source of `Key::Char` is `translate()` (`usb_kbd.rs:298`), which + emits ASCII exclusively, and every internal insert (TAB, list markers, table + formatting) is ASCII too. So byte == char holds and those slices can't hit a + char-boundary panic. **When the v0.2 UTF-8 work lands, this invariant breaks + into panics** — add a `debug_assert`/comment at the insert boundary then. +- **`epd.rs` slicing is bounded by its asserts.** `display_frame*` assert + `fb.len() == FB_BYTES` and `y0 + h <= HEIGHT`, the row math stays within + `FB_BYTES`, and the u16 arithmetic (`x+w-1`, `y0+h`) doesn't overflow given + those bounds. + +`git_push.rs`/`git_sync.rs`: the `RemoteCallbacks` closures capture +`Rc>` and run synchronously on the git thread during `remote.push` — +never sent across threads, so no `Send`/aliasing hazard. +`git2::opts::set_ssl_cert_file` (line 267) is `unsafe` because it sets a +process-global; called once in single-threaded setup before the git thread +spawns — sound. + +## Regression testing + +The honest constraint first: **the on-target binary can't be run under +Miri/ASAN** (`target_os = "espidf"`, all `unsafe` is FFI). So the strategy is +split by what's reachable where, ranked by leverage. + +1. **Make the pure logic host-testable (highest leverage).** The functions that + take untrusted input or do the slicing are FFI-free: `translate`, + `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. +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 + UB. +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. +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 + (#3); a crash/`LoadProhibited` on the freed transfer proves the UAF (#1). + - **Stack high-water mark**: `uxTaskGetStackHighWaterMark` on the USB + threads, asserted in a debug build, guards #4. +5. **Fix #1 by making it impossible, not just tested.** The best defense is an + in-flight flag set on submit and cleared in `report_cb`, with a + `debug_assert!(!in_flight)` before the free. Any future change that + reintroduces the race trips the assert in the hot-plug loop instead of + corrupting memory in the field. + +Priority if you only do some: **1 (fuzz `handle_report` under Miri) + 5 +(in-flight flag)** cover the two real memory-safety concerns; 2 and 3 are cheap +insurance against dependency bumps; 4 is the only way to regression-test the +on-device races and is worth it for an always-powered appliance. diff --git a/README.md b/README.md index 817f6c3..c5f9cf6 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,8 @@ transport — has an ADR in [`docs/adr.md`](docs/adr.md), including the rejected alternatives (Ratatui, Gleam + Shore on AtomVM, C/Arduino — ADR-001/002). How each decision is weighted against the user-facing requirements lives in [`docs/qfd.md`](docs/qfd.md); the ontology those docs use is defined in -[`GLOSSARY.md`](GLOSSARY.md). +[`GLOSSARY.md`](GLOSSARY.md). A memory-safety review of the Rust `unsafe`/FFI +surface (mostly `usb_kbd.rs`) is in [`MEMORY_AUDIT.md`](MEMORY_AUDIT.md). | Layer | Choice | Notes | | ------------- | --------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | @@ -143,6 +144,8 @@ 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). 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: