typewriter/MEMORY_AUDIT.md

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.

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

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.

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.

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:

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.

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 fn pointers are Option<extern fn> → 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.

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 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.

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 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

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<RefCell<…>> 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.
   • 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 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. 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 (#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.