fix(confirm): a screen will not accept the previous hold's release bounce

docs/release/rc30-evidence/T5-cutoff-findings.md

@@ -0,0 +1,91 @@
+# T5 — Cut Off pre-screen (#428/#481/#482), hardware round
+
+Device: 7.14.2, variant KeepKey, device_id 39353036114736342A004600, AdvancedMode=False.
+Vehicle: `Ping` with `button_protection` — `fsm_msgPing` -> `confirm(..., "Ping", "%s", msg->message)`.
+
+## PASS — boundary and wire protocol
+
+| body | ButtonRequests | result |
+|---|---|---|
+| 100 ch | 1 | one screen, no warning |
+| 117 ch | 1 | three full rows, no warning |
+| 118 ch | 2 | CUT OFF — boundary is 118 |
+| 119 ch | 2 | CUT OFF |
+| 255 ch | 2 | CUT OFF |
+
+- **#481 confirmed on hardware.** Boundary is 118, not 119. A clipped final glyph
+  no longer reports as fitting (`draw.c:213-219`).
+- **#482 confirmed on hardware.** The Cut Off screen emits its own ButtonRequest
+  (`code=1`, ButtonRequest_Other). An auto-approving host can no longer deadlock.
+- No false positives at 100 or 117.
+
+## FINDING 1 — "Hold to view it anyway" discloses nothing
+
+`confirm_sm.c:441` re-draws the SAME truncated body after the warning:
+
+    return confirm_screen(request_title, request_body, ...);
+
+`request_body` is unchanged and the generic `confirm()` path has no pager. The
+byte-exact pager (`confirm_bytes()`, with n/m counters) exists only for the three
+SignMessage handlers. So the hidden text stays hidden and the second hold buys
+the user nothing.
+
+The screen tells the user it is about to disclose the remainder, and does not.
+Either the copy is wrong or the pager is missing. Wrong copy on a consent screen.
+
+## FINDING 2 — the second consent is satisfied by the RELEASE of the first
+
+Three runs, varying only when the host's ButtonAck for BR2 lands:
+
+| run | ack timing | hands off after hold #1 | result |
+|---|---|---|---|
+| carry-over | immediate | yes | **Success @ 1.602s** |
+| no-ack control | never sent | yes | silence 15s (screen is gated) |
+| delayed-ack | +5s | yes | silence 15s (screen waits) |
+
+Immediate ack completes; ack delayed past the bounce window does not. The press
+edge therefore arrives shortly after the user releases hold #1.
+
+**Mechanism.** `keepkey_button.c` has no debounce. EXTI is `EXTI_TRIGGER_BOTH`
+and `buttonisr_usr()` decides press vs release from the GPIO level at interrupt
+time. A mechanical release bounces low and dispatches `on_press_handler`.
+`confirm_screen()` resets `state_info` to HOME and re-registers handlers
+(`confirm_sm.c:198-220`), so screen 2 is armed and accepts that bounce as a
+fresh press out of HOME, then runs its own hold timer to completion.
+
+**Consequence.** #482 added a second ButtonRequest so the user would be asked to
+consent to a body the device admits it cannot fully display. On hardware that
+second consent can be satisfied by the physical release of the first hold. The
+user holds once and both screens pass.
+
+**Why CI cannot see this.** The emulator has no bounce and no physical button;
+`keepkey_button_up()` is `return false` under EMULATOR. Only a hardware round
+finds it — this is the case for keeping the hardware gate.
+
+## Proposed fix (not yet applied)
+
+Require the button to be observed UP before a confirm screen accepts a press.
+Local to confirm_sm.c, no driver change, no timing constant:
+
+- at `confirm_screen()` init: `state_info.armed = keepkey_button_up();`
+- in `handle_screen_press()`: ignore the press unless `si->armed`
+- in `handle_screen_release()`: `si->armed = true;`
+
+A screen that opens while the button is still down (or bouncing) refuses presses
+until a genuine release is seen. Costs one bool and two branches.
+
+## Reproduce
+
+    cd deps/python-keepkey/tests
+    PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428_carryover.py   # Success ~1.6s
+    PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428_noack.py       # silence
+    PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428_delayack.py    # silence
+
+Replug between runs; `hwpreflight.idle_or_die()` refuses to start on a dirty device.
+
+## Trap recorded
+
+An aborted run leaves a ButtonRequest queued that SURVIVES into the next session,
+and the next run answers it silently — observed live as BR1 returning code=4
+instead of the Ping's own code=23. Every test in this plan needs the first-code
+assertion or it can be invalidated by whatever ran before it.

docs/release/rc30-evidence/artifact-0-device.md

@@ -0,0 +1,24 @@
+# Artifact #0 — device identity, rc30 hardware round
+
+Captured before any test, over WebUSB, Vault stopped.
+
+| field | value |
+|---|---|
+| version | 7.14.2 |
+| firmware_variant | KeepKey (NOT KeepKeyBTC — handler-dependent tests are valid) |
+| device_id | 39353036114736342A004600 |
+| label | rc30 |
+| initialized | True |
+| pin_protection | False |
+| passphrase_protection | False |
+| bootloader_mode | False |
+| firmware_hash (first 16B) | 933dc8ff10c7861ab2b25d67e4cc86dd |
+| policies | ShapeShift=False, Pin Caching=True, Experimental=False, AdvancedMode=False |
+
+AdvancedMode reads False at rest, matching the compiled default
+(include/keepkey/firmware/policy.h). Every phase-B test re-asserts it anyway.
+
+Artifact was UNSIGNED: flashing wiped storage. Seed present at capture time is
+disposable and will be wiped again by T1.
+
+TODO: sha256 of the flashed .bin (host-side, not device-readable).

include/keepkey/board/confirm_sm.h

@@ -79,6 +79,11 @@ typedef struct {
   DisplayState display_state;
   ActiveLayout active_layout;
   bool immediate;
+  /* False until the button has been observed released while this screen owned
+     it. The button driver has no debounce, so releasing a hold bounces the line
+     and dispatches a press; a screen armed a moment earlier would otherwise
+     accept that bounce as consent. See #484. */
+  bool armed;
 } StateInfo;
 
 #define isprint(c) ((c) >= 0x20 && (c) < 0x7f)

lib/board/confirm_sm.c

@@ -68,6 +68,14 @@ static void handle_screen_press(void* context) {
 
   StateInfo* si = (StateInfo*)context;
 
+  /* A screen that opened while the button was still down -- or still bouncing
+     from the previous screen's release -- has not seen a genuine press yet.
+     The driver has no debounce and reports a release bounce as a press, so
+     without this the release of one hold satisfies the NEXT screen. #484. */
+  if (!si->armed) {
+    return;
+  }
+
   if (button_request_acked) {
     switch (si->display_state) {
       case HOME:
@@ -88,6 +96,10 @@ static void handle_screen_release(void* context) {
 
   StateInfo* si = (StateInfo*)context;
 
+  /* The button is up: any bounce from a previous screen has settled and the
+     next press on this screen is the user's own. #484. */
+  si->armed = true;
+
   switch (si->display_state) {
     case CONFIRM_WAIT:
       si->active_layout = LAYOUT_REQUEST_NO_ANIMATION;
@@ -200,6 +212,15 @@ static bool confirm_screen(const char* request_title_param,
   state_info.immediate = immediate;
   state_info.display_state = HOME;
   state_info.active_layout = LAYOUT_REQUEST;
+  /* Arm immediately only if the button is already up. If it is down, this
+     screen waits for the release before it will accept anything. #484.
+     There is no button under EMULATOR -- keepkey_button_up() is a constant
+     false there -- so arm unconditionally or emulated presses never land. */
+#ifdef EMULATOR
+  state_info.armed = true;
+#else
+  state_info.armed = keepkey_button_up();
+#endif
 
   /* Request */
   state_info.lines[LAYOUT_REQUEST].request_title = request_title;