fix(recovery): INFO sent three bytes of stack to whoever asked, and the client printed them as the flash size by Kartikey1306 · Pull Request #139 · embeddedos-org/eBoot

Kartikey1306 · 2026-09-15T06:31:38Z

Stacked on #131 (→ #127 → #115); review 00283e8..55a39d0 for this change alone — three commits, seven files (corrected from 00283e8..2693259, before that 00283e8..e14456f; the second commit takes findings 1–5 of the 07:11Z review, the third takes finding 6 and in doing so replaces the finding-3 fix with a stronger one): core/recovery.c, tools/uart_recovery.py, tests/unit/test_recovery.c, tests/unit/test_uart_recovery.py. Merge order #115 → #127 → #131 → this; do not merge without #131, whose commit this carries.

Closes #140.

Takes finding 1 (P1, pre-existing) and finding 2 (P2) from the review of #131, in one change because both are in the same struct.

The leak

recovery_handle_info() built its response in a struct that was not packed — unlike rcvr_packet_t fifteen lines above it — and never zeroed. On the wire that was 24 bytes: ack, three bytes of padding nothing wrote, then the five geometry fields. RCVR_CMD_INFO needs no authentication (cmd_requires_auth() does not list it), so any client on the UART could ask, repeatedly, and receive three bytes of whatever the previous call had left on the stack.

Reproduced with the struct poisoned to 0xA5 and filled exactly as the firmware fills it:

sizeof(info)=24  offsetof(flash_size)=4        (client expects 21, packed)
wire[0..8]: AA A5 A5 A5 00 00 10 00
client decodes flash_size=0x00A5A5A5 slot_a_addr=0x00001000   (actual 0x00100000 / 0x08010000)

tools/uart_recovery.py has always parsed the packed 21-byte layout, so it printed the leaked bytes as "Flash size", every slot address one word off, and left three surplus bytes in the serial buffer to desynchronise the next command. grep -rn "CMD_INFO\|handle_info" tests/ was empty: nothing had ever exercised it.

The fix

The struct is packed and memset before any field is written. That makes the firmware send what the client always read — a repair, not a format change — and means a future field cannot open a new hole.

One byte is appended: caps, RCVR_CAP_RNG | RCVR_CAP_OTP, derived from the ops table. This is finding 2: #131 makes authenticated recovery unreachable by construction on a board with no entropy source, and the event it logs is readable only over RCVR_CMD_LOG, which needs the authentication the missing entropy prevents. What an integrator saw was a bare NACK — indistinguishable from a wrong secret — after 0+1+2+4+8 = 15 s of backoff. The unauthenticated INFO response answers that before the first AUTH is sent, and reveals nothing an attacker could not learn by trying.

The client reads 22 bytes, decodes caps, and says in words when a board has no authenticated recovery. Correction (finding 2, 07:11Z review): this paragraph originally said "21 is the old length". No firmware ever sent 21 bytes — 21 was only ever what the client read. The legacy length is 24, and the first version of this client decoded it: read(22) of 24 bytes succeeds, so it printed the leaked padding as the flash size with a fabricated "no entropy" line and left two bytes to desync the next command — the PR's own defect, still shown against every unpatched device. 2693259 fixes that: after read(22) the client probes for surplus bytes (0.25 s timeout of its own), and any surplus means pre-fix firmware — it says so, drains, and returns False. A truncated read is still refused too.

Tests, and what each is for

test	pins
`test_info_sends_the_packed_layout_the_client_parses_and_nothing_else`	`out_buf` poisoned `0xA5` first; INFO via `sim_ops`; `out_len == 22`; every field at the client's offsets; the byte after the response still `0xA5`
`test_info_reports_a_board_without_an_entropy_source`	ops with `rng_get = NULL` → `caps` has RNG clear, OTP set
`test_info_decodes_the_packed_22_byte_response`	a 22-byte response decodes to the printed geometry and leaves the next byte unread
`test_info_names_a_board_with_no_entropy_source`	OTP-only `caps` → the message names the missing entropy
`test_info_refuses_the_24_byte_response_of_pre_fix_firmware`	the reviewer's reproduction byte for byte: `info()` is `False`, "pre-fix firmware"/"24 bytes" printed, `10601K` and `Entropy:` not printed, buffer empty, port timeout restored
`test_info_probe_restores_the_timeout_on_the_good_path`	the 0.25 s probe timeout is restored after a good response
`test_info_refuses_a_short_response`	a truncated (21-byte) read is refused — kept, docstring corrected

Negative controls

Each file reverted to #131's version with the tests kept, touch + rebuild both ways, recompilation confirmed in the build log:

core/recovery.c at #131         ->  fails at tests/unit/test_recovery.c:578: out_len == INFO_WIRE_LEN   (sends 24)
tools/uart_recovery.py at #131  ->  3 of 7 fail

For 2693259:

tools/uart_recovery.py at e14456f, tests kept          ->  1 of 9 fails (the 24-byte case)
__attribute__((packed)) and the _MSC_VER pragma stripped ->  _Static_assert fires: "INFO response must be 22 bytes packed"

Also in `2693259`

Finding 3, then finding 6 (55a39d0): 2693259 mirrored the _MSC_VER pragma and asserted sizeof == 22. 55a39d0 removes the struct entirely: the response is built into uint8_t[RCVR_INFO_LEN] with an explicit little-endian store and the fill is checked to be exactly 22 bytes. That makes both findings true by construction — a byte buffer has no padding on any toolchain, and the words are little-endian regardless of the target — so the pragma and the assert from 2693259 are gone with the struct they guarded. rcvr_packet_t's own packing is untouched.
Finding 4: ADR-021 — INFO stays unauthenticated deliberately (everything it discloses an attacker learns by trying; the integrator needs it before AUTH), LOG stays authenticated. T-404 records caps, narrows its mitigation to LOG, cites the ADR.
Finding 5: docs/architecture.md's command table gives the 22-byte layout field by field.

At `55a39d0`

ctest            100% tests passed, 0 tests failed out of 31
test_recovery    9/9
pytest tests/    93 passed

core/recovery.c is CRLF and stays CRLF (545 → 574 lines). Attribution grep empty.

NOT RUN: a real UART session. The sim_ops harness carries the same bytes through eos_hal_uart_send that a board would; the client tests use a FakeSerial with the byte-exact response.

Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/scorecard-action@v2.4.0 pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.

… -1.0.0 as an option test_out_of_range_version_is_rejected[-1.0.0] passed locally (Python 3.14) and failed in CI (ubuntu-22.04, Python 3.10) with argparse's own "expected one argument": the older negative-number matcher does not accept -1.0.0, so the token was read as an unknown option and imgpack.py's range check -- the thing under test -- never ran. The joined form is unambiguous on every interpreter and the test now reaches the tool's message.

tests/vectors/fw_update_test_sigs.h is the output of tools/gen_fw_update_test_sigs.py, committed because eBoot has no Ed25519 signer in C. Nothing checked that the two agree: a generator edit without a regeneration leaves test_fw_update and test_fw_transport verifying against stale signatures, failing with EOS_ERR_SIGNATURE and nothing to say why. tests/unit/test_fw_update_test_sigs.py runs the generator with the test's own interpreter and compares its stdout to the committed header byte for byte, so a line-ending change counts too. It follows the same dependency rule as test_eos_sign_payload_offset.py: with EOS_REQUIRE_SIGNING_TESTS set (the CI workflow sets it before the pytest step) a missing cryptography module fails the job instead of skipping. Negative control: one flipped hex byte in the header fails the test with a unified diff naming the line. build_image() and build_container() now carry a comment naming the coupling: the signed prefix is assembled both there and in the generator, and changing any field in it means changing the generator's copy and regenerating the header.

…epair embeddedos-org#103 and embeddedos-org#104 were both merged and disagree on whether the anti-rollback counter or the signature is checked first in eos_fw_update_finalize(). The master design orders boot as verify image, then version policy (section 8.1), but its update flow (section 15) never places the anti-rollback check, so the order the install path uses existed only in a PR body. ADR-020 records it: the signature over the signed header prefix is verified first, the TLV counter is read only after the prefix that binds it is authenticated, and an image that fails verification is refused as EOS_ERR_SIGNATURE without its counter being consulted. docs/adr/README.md is added in the shape of the eos repository's index; 020 avoids reusing 001 through 019. CHANGELOG.md gains the Unreleased entries for the repair: the configure, compile and link breakage after the 09-07 batch merge, the settled check ordering with the suites streaming signed images, the re-derived Valgrind list, fuzz-build in the CI gate, counted tests_run, the EoSim Windows legs running under bash, and the Scorecard action on its ghcr.io-hosted release.

The changelog said eleven suites had no Valgrind run. Eleven were missing from EBLDR_UNIT_TESTS, but four of those were named in the hand-written foreach and did run; seven had no run at all. Say which. ADR-020: its design-document citation now says where it comes from (the architecture review of embeddedos-org#115), and the sentence about the two PRs' bases is replaced with what the history shows -- embeddedos-org#103's commits predate embeddedos-org#104's merge, and embeddedos-org#104 was written without embeddedos-org#103's check in place.

The regeneration test's failure path -- the unified diff under the regenerate command -- ran only when the header was stale, so a green run never executed it and the coverage report said so. The report is now a helper the match asserts with, and a second test drives the helper with two byte strings that differ in one byte and checks the command and both sides of the changed line appear.

…mapped slot Two wire-facing checks in core/recovery.c were weaker than the code around them said. recovery_handle_auth() read the shared secret from OTP and compared the client's response against SHA-256(challenge || secret) with no look at what the secret was. Unprogrammed fuses read back as all zeros or all ones, and both are public: a device whose recovery secret was never provisioned authenticated any client that sent SHA-256(challenge || 00..00). The keystore already refuses an all-zero key for exactly this reason; the recovery path now refuses both patterns before comparing, branch-free so the check does not leak which value the fuses hold, and logs event 0x22. No shipped board port provides otp_read today, so on current boards authentication always failed and this was latent; it is the first board with OTP that would have shipped it. recovery_handle_write() had stopped calling eos_recovery_write_in_range() -- the rule tests/unit/test_recovery.c exercises, which refuses a slot the board leaves unmapped -- and checked offset + len against the slot size inline. embeddedos-org#55's replay did that while repairing a duplicate declaration. For a slot whose base is 0 the write then landed at flash address `offset`, which in the test layout is between the boot-control block and its backup. The handler uses the shared rule again, so the wire has one definition. Tests: an all-zero and an all-ones secret are refused and the write that follows is refused too; the same exchange with a provisioned secret still authenticates and writes (the control); a write into an unmapped slot B is refused and flash at the would-be address is untouched. Each new test was run against the unfixed handler: the all-zero secret authenticated (out_buf[33] was ACK) and the unmapped write was accepted (out_buf[34] was ACK).

…ode them core/recovery.c logged its authentication outcomes as bare 0x20, 0x21 and 0x22 with a comment for a name, while include/eos_types.h defines EOS_LOG_* for every other event and tools/uart_recovery.py maps codes to names for its `log` command. That table ended at 0x21, so the event this branch adds -- "this device was never provisioned", the one condition the fix exists to distinguish from a guessed secret -- printed as UNKNOWN(0x22) on the client that exists to read it. EOS_LOG_AUTH_SUCCESS, EOS_LOG_AUTH_FAIL and EOS_LOG_AUTH_UNPROVISIONED now sit beside the other EOS_LOG_* codes and recovery.c uses them by name; the client's BOOT_LOG_EVENT_NAMES gains 0x22. The new guard tests/unit/test_boot_log_event_names.py holds the three lists together: every eos_boot_log_append() in core/, stage0/ and stage1/ passes an EOS_LOG_* name, every code the header defines is in the client's table under the same name, and the client names nothing the header does not define. Against the previous commit it fails on the four bare literals and on the two codes the client named without a definition. The CHANGELOG entry also records the wire change the shared range rule brings with it: a WRITE with len == 0 is refused where the inline check had ACKed it and written nothing.

…omment header_events() anchored its pattern on end of line, so an EOS_LOG_* define written in this header's own style -- a trailing comment, as EOS_LOG_MAGIC already has -- dropped out of the client cross-check without a trace. Demonstrated by the review with the very next code: EOS_LOG_AUTH_NO_ENTROPY 0x23 with a comment, no client entry, 4 passed. Under-collection is the failure mode a parser cannot report on its own. The match now stops at the value. Two tests pin it: a sample header with C and C++ trailing comments parses to the expected map, and the strict pattern is cross-checked against a looser count of the same lines in the real header, so a style it misses shows up as a mismatch. With the review's probe re-applied, the guard now fails where it should: "BOOT_LOG_EVENT_NAMES lacks: ['EOS_LOG_AUTH_NO_ENTROPY = 0x23']".

recovery_handle_auth() fell back to a linear congruential generator seeded with eos_hal_get_tick_ms() whenever eos_hal_rng_get() failed. The challenge is what stops a captured (challenge, response) pair from being replayed, and with that fallback it was a function of the millisecond at which the AUTH command was handled: a few thousand reachable values on a freshly reset board, shown to the client before it has to answer, retryable after a reset at no cost in failure count. No board port provides rng_get (0 of the 83 ops tables under boards/), so the fallback was the challenge on every board. None provides otp_read either, so no board authenticates anyone today and the defect was latent; a port that adds OTP without an RNG would have gone live with it. The AUTH is now refused outright: NACK, boot-log event EOS_LOG_AUTH_NO_ENTROPY (0x23, defined beside the other EOS_LOG_* codes and decoded by tools/uart_recovery.py), and counted as a failure so the existing backoff and the RCVR_MAX_AUTH_FAILS cap apply. Pinned by test_auth_refuses_when_the_board_has_no_entropy_source, which fails against the old code at the first byte of the reply.

…he client printed them as the flash size Finding 1 (P1, pre-existing) and finding 2 (P2) from the review of embeddedos-org#131, in one change because they touch the same struct. recovery_handle_info() built its response in a struct that was not packed -- unlike rcvr_packet_t fifteen lines above it -- and never zeroed. On the wire that was 24 bytes: ack, three bytes of padding nothing wrote, then the five geometry fields. RCVR_CMD_INFO needs no authentication, so any client on the UART could ask, repeatedly, and receive three bytes of whatever the previous call had left on the stack. Reproduced with the struct poisoned to 0xA5 and filled exactly as the firmware fills it: sizeof(info)=24 offsetof(flash_size)=4 wire[0..8]: AA A5 A5 A5 00 00 10 00 client decodes flash_size=0x00A5A5A5 slot_a_addr=0x00001000 tools/uart_recovery.py has always parsed the packed 21-byte layout, so it printed the leaked bytes as "Flash size" and every slot address one word off, and the three surplus bytes stayed in the serial buffer to desynchronise the next command. RCVR_CMD_INFO had no test anywhere. The struct is now packed and memset before any field is written, which makes the firmware match what the client always read -- a repair, not a format change -- and means a future field cannot open a new hole. One byte is appended: caps, with RCVR_CAP_RNG and RCVR_CAP_OTP derived from the ops table. embeddedos-org#131 makes authenticated recovery unreachable by construction on a board with no entropy source; the event it logs (EOS_LOG_AUTH_NO_ENTROPY) is readable only over RCVR_CMD_LOG, which needs the authentication the missing entropy prevents. What an integrator saw was a bare NACK, indistinguishable from a wrong secret, after 0+1+2+4+8 = 15 s of backoff. The unauthenticated INFO response is where that question is answered, before the first AUTH is sent. It reveals nothing an attacker could not learn by trying. The client reads 22 bytes, decodes caps, and says in words when a board has no authenticated recovery. It refuses a shorter response rather than decoding it -- 21 is the old length, and a firmware still sending 24 would otherwise be parsed one byte wrong forever. Tests: test_recovery.c out_buf poisoned 0xA5; INFO through sim_ops; out_len == 22; every field at the client's offsets; the byte after the response untouched. Second test: ops without rng_get -> caps has RNG clear, OTP set. test_uart_recovery.py a 22-byte response decodes to the printed geometry and leaves the next byte unread; an OTP-only caps byte names the missing entropy; a 21-byte response is refused. Negative controls (file reverted, tests kept, touch + rebuild both ways, recompilation confirmed): core/recovery.c at embeddedos-org#131 -> fails at out_len == INFO_WIRE_LEN tools/uart_recovery.py at embeddedos-org#131 -> 3 of 7 fail At this head, on embeddedos-org#131 at 00283e8 on embeddedos-org#127 at a9c6a2e: ctest 31/31, pytest 91. core/recovery.c is CRLF and stays CRLF.

codecov-commenter · 2026-09-15T06:33:17Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.51163% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tests/unit/test_boot_log_event_names.py	97.14%	1 Missing and 1 partial ⚠️
tests/unit/test_fw_update_test_sigs.py	93.33%	1 Missing and 1 partial ⚠️
tools/uart_recovery.py	90.90%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

srpatcha

Review — eBoot#139 "fix(recovery): INFO sent three bytes of stack to whoever asked, and the client printed them as the flash size"

head: e14456f author: Kartikey1306 ci: pass

Reviewed the incremental commit only, as the PR body asks: 00283e8..e14456f
(core/recovery.c, tools/uart_recovery.py, tests/unit/test_recovery.c,
tests/unit/test_uart_recovery.py). First look by this pipeline.

Verdict: The firmware-side fix is correct and well-tested — the struct is packed, zeroed before first write, and the new test_recovery.c case pins 22 bytes with the trailing byte still poisoned. The client-side half does not hold: against a device running today's unpatched firmware the new client still prints leaked padding as the flash size, now with a false "no entropy source" diagnostic attached, and the PR body states this case is refused when it is not.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	`tools/uart_recovery.py:128-131`	`read(self.INFO_LEN)` against pre-fix firmware returns 22 of its 24 bytes, so `len(response) < self.INFO_LEN` is false and the response is decoded. Verified by running this PR's `uart_recovery.py` against a 24-byte unpacked response with the padding poisoned `0xA5`: `info()` returned `True` and printed `Flash size: 10601K` (`0x00A5A5A5`), `Slot A: 0x00001000`, `Slot B: 0x00000600`, `Entropy: NO -- authenticated recovery is unavailable on this board`, leaving `0600` in the buffer. That is the defect in the PR title, still present on every device not yet reflashed, plus a false hardware diagnostic and a two-byte desync of the next command.	Reject an over-long response, not only a short one. After `response = self.ser.read(self.INFO_LEN)`, do a further short read; if it returns anything, print that the device is running pre-fix firmware (24-byte unpacked INFO), drain the input buffer and `return False`. Add the 24-byte case to `test_uart_recovery.py`.
2	High	PR body, "The fix"	"It refuses a shorter response rather than decoding it: 21 is the old length, and a firmware still sending the unpacked 24 would otherwise be parsed one byte wrong forever." No firmware ever emitted 21 bytes — the old firmware emitted 24, which is the case the guard does not catch (finding 1). `test_info_refuses_a_short_response` therefore pins a length nothing produces while the real legacy length is untested. Per the review brief an unsupported claim is itself the finding.	Correct the claim, or make it true by implementing finding 1. Keep the 21-byte test and add a 24-byte one.
3	Medium	`core/recovery.c:265-277`	The packed attribute is applied only under `#if defined(__GNUC__) \|\| defined(__clang__)`. `rcvr_packet_t` at `core/recovery.c:53-68` — the struct this one is explicitly modelled on — also carries `#ifdef _MSC_VER / #pragma pack(push, 1) … pop`; the copy dropped it. On any toolchain outside that `#if`, `sizeof(info)` is 24 again, the padding leak returns, and nothing fails: there is no assertion pinning the size. "Cross-Platform (windows-latest)" does not catch it — `simulation-test.yml:88` only echoes a skip message.	Mirror the `_MSC_VER` pragma from `rcvr_packet_t`, and add `_Static_assert(sizeof(info) == 22, "INFO wire layout");` (or the repo's equivalent) so a toolchain that will not pack the struct breaks the build instead of shipping the leak.
4	Medium	`docs/threat_model.md:142`	T-404 records INFO as an information-disclosure surface whose planned mitigation is "Restrict INFO/LOG to authenticated sessions (Phase 3)". This PR adds a new field to that unauthenticated response and builds a diagnostic workflow on its being reachable before AUTH — the whole point of `caps`. The threat model is not updated, and the two are now in direct conflict: implementing T-404's mitigation would remove the capability this PR relies on.	Update T-404 in the same PR: record `caps` as disclosed, and either narrow the planned mitigation to LOG or state why INFO is now deliberately exempt. This is a design decision worth an ADR alongside ADR-020, not a silent row.
5	Low	`docs/architecture.md:99`	The command table describes INFO as "Flash layout and slot addresses". The response now also carries a capability bitfield, and its length is part of the contract between firmware and `tools/uart_recovery.py`.	Add `caps` and the 22-byte packed layout to the table, so the wire format has one written definition.
6	Low	`core/recovery.c:270-274`	The five `uint32_t` go on the wire in target byte order; `uart_recovery.py` unpacks `<IIIII`. Correct on every current eBoot target and pre-existing, but the struct is now the declared wire format rather than an accident, so the assumption should be written down rather than inferred.	One comment line stating the protocol is little-endian, or explicit `htole32` conversions.

Architecture conformance

Conforms. core/recovery.c is shared boot logic in eBoot's core/ per .ai/architect.md's eBoot layout, and tools/uart_recovery.py is host-side tooling — neither introduces a dependency pointing up a tier. Master design §8.1 requires a factory/recovery image strategy; hardening the recovery protocol's only unauthenticated diagnostic is squarely inside that. No change to the trusted computing base boundary: INFO is not in cmd_requires_auth() (core/recovery.c:107-120) before or after. eBoot is Tier 1 (§21) and the change stays in it.

Against §14.1 and .ai/security.md, the direction of the fix is right — the struct now fails closed on a field nobody wrote, and memset makes a future field safe by construction. The one security-relevant judgement call, widening an unauthenticated response, is argued in the PR body but contradicts a documented planned control (finding 4).

Proposed changes

Smallest sequence that keeps things working:

tools/uart_recovery.py, in info(), after the 22-byte read:

if self.ser.read(1):
    print("Failed to get device info: device sent more than "
          f"{self.INFO_LEN} bytes — pre-fix firmware sends a 24-byte "
          "unpacked INFO. Reflash before trusting this output.")
    try:
        self.ser.reset_input_buffer()
    except AttributeError:
        pass
    return False

FakeSerial already supports read; the reset_input_buffer guard keeps the
existing tests working unchanged.

tests/unit/test_uart_recovery.py: add test_info_refuses_a_legacy_unpacked_response
feeding bytes([0xAA]) + b"\xA5\xA5\xA5" + struct.pack("<IIIII", …) and asserting
info() is False. Negative control: the test fails against this PR's client.
core/recovery.c: wrap the struct in the same #ifdef _MSC_VER / #pragma pack
pair rcvr_packet_t uses, and add a static assertion on sizeof(info) == 22.
docs/threat_model.md T-404 and docs/architecture.md:99 updated in the same
change, per the project rule that a change making existing documentation wrong
is not finished.

Steps 1–2 are independent of 3–4 and are the ones that matter for anyone holding an
unpatched board.

Not checked

A real UART session against real hardware. The PR says so itself; so do I.
I did not build or run ctest for this branch. The PR reports 100% tests passed, 0 of 31 and 91 passed at e14456f, and CI is green on all 26 reported checks; I did not re-run either. NOT RUN, not verified.
The negative controls described in the PR body (reverting each file to #131 and confirming the new tests fail) were not reproduced.
Findings 1 and 2 were verified by executing this PR's tools/uart_recovery.py against a synthetic 24-byte legacy response outside the repo tree; the output quoted above is real. Finding 3 is read from the source, not demonstrated on MSVC — no MSVC toolchain was available and CI does not compile there.
The rest of the stack (#115 → #127 → #131) was not re-reviewed here; only 00283e8..e14456f was judged. The 31-file diff in the bundle is the cumulative stack, not this change.
mergeStateStatus is BLOCKED — that is the stacked-merge-order requirement the PR describes, not a failing check, but I did not confirm which branch protection is holding it.

Automated architecture review of e14456f72d39 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

…tead of decoding the leak All five findings from the review of e14456f. 1. (High) The client half did not hold against an unpatched device. Firmware before the packed-struct fix sends 24 bytes; read(22) of those succeeds, so `len(response) < INFO_LEN` was false and the response was decoded. Reproduced exactly as the review did, padding poisoned 0xA5: info() returned True and printed "Flash size: 10601K" (0x00A5A5A5), every address one word off, "Entropy: NO -- authenticated recovery is unavailable" invented from a slot-size byte, and two bytes left in the buffer to desynchronise the next command. The defect in this PR's title, still shown by this PR's client, on every device not yet reflashed -- a fix that left its twin. After read(22) the client now probes for surplus bytes with a short timeout of its own (0.25 s; the port's 5 s is for flash work). Two bytes at any baud arrive well inside that; a short read rather than in_waiting because on a real UART they may still be on the wire when read(22) returns. Anything surplus means pre-fix firmware: say so, name the length, drain the buffer, return False. The timeout is restored on every path. 2. (High) The body said "21 is the old length". No firmware ever sent 21 bytes; 21 was only ever what the client read. The 24-byte case is now the test, the 21-byte test is kept as the truncated-read case with its docstring corrected, and the body is corrected. 3. (Medium) __attribute__((packed)) was under #if GNUC/clang only; the struct this one is modelled on also carries the _MSC_VER pragma pair and the copy dropped it. Mirrored. And sizeof(struct rcvr_info) is now _Static_assert'ed at 22, so a toolchain that honours neither turns the returning padding into a compile error. Negative control: with both guards stripped the assert fires ("INFO response must be 22 bytes packed"). 4. (Medium) T-404's planned mitigation -- restrict INFO/LOG to authenticated sessions -- would remove the capability byte this PR relies on being readable before AUTH. That is a design decision, so it is ADR-021: INFO stays unauthenticated, deliberately, because everything it discloses an attacker learns by trying and the integrator needs it before AUTH; LOG stays authenticated, because boot history is the material T-404 is right to protect. T-404 now records caps as disclosed, narrows the mitigation to LOG, and cites the ADR. The ADR index has the row. 5. (Low) docs/architecture.md's command table now gives INFO's 22-byte packed layout field by field, so the wire format has one written definition alongside the struct's assert and the client's parser. Tests: test_uart_recovery.py the reviewer's 24-byte reproduction, byte for byte: info() False, "pre-fix firmware" and "24 bytes" printed, "10601K" and "Entropy:" NOT printed, buffer empty, timeout restored; plus a good-path test that the timeout is restored there too. The 22-byte test no longer plants a stray trailing byte, since a trailing byte is now by design a rejection. Negative controls: tools/uart_recovery.py at e14456f, tests kept -> 1 of 9 fails (the 24-byte case) packing guards stripped from core/recovery.c -> _Static_assert fires At this head: ctest 31/31, test_recovery 9/9, pytest tests/ 93 passed. core/recovery.c is CRLF and stays CRLF (574 -> 585).

…struct Finding 6 (Low) from the review of e14456f, and it supersedes the finding-3 fix from 2693259 with a stronger one. The five uint32 fields went on the wire in target byte order while tools/uart_recovery.py unpacks '<IIIII'. Correct on every current target and pre-existing -- but the struct is now the declared wire format, and a wire format that depends on the target's endianness is an assumption, not a contract. The review offered a comment line or htole32; this does the second properly. The response is now built into a uint8_t[RCVR_INFO_LEN] with an explicit little-endian store, and the fill is checked to have written exactly RCVR_INFO_LEN bytes before sending. That makes three things true by construction that 2693259 had to assert or guard: - no padding, on any toolchain: a byte buffer has none, so the __attribute__((packed)) / _MSC_VER pragma pair and the _Static_assert on sizeof(struct rcvr_info) from 2693259 are gone -- there is no struct left to pack. rcvr_packet_t's own packing is untouched. - little-endian on the wire regardless of the target's byte order, which is what the client has always assumed. - the length is the contract, not a sizeof. test_recovery.c's INFO tests decode each field with an explicit le32() and pass unchanged: 9/9. ctest 31/31, pytest 93 at this head. The serialiser was checked on a value whose bytes differ by order (0x08010000 -> 00 00 01 08). core/recovery.c is CRLF and stays CRLF.

Kartikey1306 · 2026-09-15T08:07:38Z

All six taken, in two pushes. Head is 55a39d0; every number below was measured there.

1 (High) — the client decoded pre-fix firmware's 24 bytes. You were right, and it is the worst kind of wrong: the PR's own defect, still shown by the PR's own client, against every device not yet reflashed, with a fabricated "no entropy" line on top. Reproduced first, exactly your way — info() returned True, printed Flash size: 10601K, left 0600 in the buffer. Now: after read(22) the client probes for surplus bytes with a 0.25 s timeout of its own (the port's 5 s is for flash work; two bytes at any baud arrive well inside a quarter second), and any surplus means pre-fix firmware — it names the length, says the bytes after ACK are uninitialised stack, drains, returns False, restores the timeout on every path. A short read rather than in_waiting, because on a real UART those two bytes may still be on the wire when read(22) returns.

2 (High) — "21 is the old length." Wrong, and the body now says so where it said it: no firmware ever sent 21; 21 was only ever what the client read. The legacy length is 24, and your reproduction is now the test — test_info_refuses_the_24_byte_response_of_pre_fix_firmware, 0xA5 padding, asserting info() is False, "pre-fix firmware" and "24 bytes" printed, 10601K and Entropy: not printed, buffer empty, timeout restored. The 21-byte test is kept as the truncated-read case with its docstring corrected. My earlier 22-byte test planted a stray trailing byte and asserted it stayed unread — that premise belonged to the check-less client and is gone.

3 (Medium) and 6 (Low), together. 2693259 did what you asked for 3: the _MSC_VER pragma pair mirrored from rcvr_packet_t, plus _Static_assert(sizeof(struct rcvr_info) == 22) — and I confirmed the assert fires with both guards stripped. Then finding 6 made me look again: a wire format whose byte order depends on the target's is an assumption, not a contract, and the right shape for a declared wire format is bytes, not a struct. 55a39d0 builds the response into uint8_t[RCVR_INFO_LEN] with an explicit little-endian store and checks the fill is exactly 22. That makes 3 and 6 both true by construction — a byte buffer has no padding on any toolchain and no byte order of its own — so the pragma and the assert are gone with the struct they guarded. The serialiser was checked on a value whose bytes differ by order (0x08010000 → 00 00 01 08). If you would rather keep the belt with the braces I will put the length assert back, but there is no struct left to pack.

4 (Medium) — T-404. This one is a design decision and I have treated it as one: ADR-021 (Proposed, deciders as ADR-020). INFO stays unauthenticated deliberately — everything it discloses an attacker learns by trying, and the integrator needs it before AUTH to diagnose a board with no entropy source — and LOG stays authenticated, because boot history is the material T-404 is right to protect. T-404 now records caps as disclosed, narrows the mitigation to LOG, cites the ADR, and its status reads "Partial (INFO decided; LOG planned)". The ADR index has the row.

5 (Low) — docs/architecture.md. The command table gives INFO's 22-byte layout field by field, byte order stated, with a pointer to the ADR.

Negative controls

tools/uart_recovery.py at e14456f, tests kept              ->  1 of 9 fails   (the 24-byte case)
__attribute__((packed)) + _MSC_VER pragma stripped (2693259) ->  _Static_assert fires
   (moot at 55a39d0: no struct)

At `55a39d0`

ctest            100% tests passed, 0 tests failed out of 31
test_recovery    9/9   (le32() decode of every field, unchanged by the serialiser)
pytest tests/    93 passed

core/recovery.c is CRLF and stays CRLF. Range in the body is 00283e8..55a39d0 with both corrections visible. Merge order #115 → #127 → #131 → #139. No approval existed at e14456f; could you re-review at 55a39d0.

Kartikey1306 added 11 commits September 15, 2026 11:56

Kartikey1306 requested review from hshanmug12 and srpatcha as code owners September 15, 2026 06:31

Kartikey1306 mentioned this pull request Sep 15, 2026

fix(recovery): issue no challenge on a board without an entropy source #131

Open

This was referenced Sep 15, 2026

RCVR_CMD_INFO sends three bytes of uninitialised stack to an unauthenticated caller; the client prints them as the flash size #140

Open

fix: repair master after the 09-07 batch merge #115

Open

srpatcha reviewed Sep 15, 2026

View reviewed changes

Kartikey1306 added 2 commits September 15, 2026 13:34

This was referenced Sep 17, 2026

ci: pin every action to a Node 24 major, by commit SHA #144

Open

fix(multicore): an AMP core starts only on an image that verified #146

Open

Navigation Menu

fix(recovery): INFO sent three bytes of stack to whoever asked, and the client printed them as the flash size - #139

fix(recovery): INFO sent three bytes of stack to whoever asked, and the client printed them as the flash size#139
Kartikey1306 wants to merge 13 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/recovery-info-leaks-stack-padding

Kartikey1306 commented Sep 15, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 15, 2026 •

edited

Loading

Uh oh!

srpatcha left a comment

Uh oh!

Kartikey1306 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Kartikey1306 commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The leak

The fix

Tests, and what each is for

Negative controls

Also in 2693259

At 55a39d0

Uh oh!

codecov-commenter commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#139 "fix(recovery): INFO sent three bytes of stack to whoever asked, and the client printed them as the flash size"

Findings

Architecture conformance

Proposed changes

Not checked

Uh oh!

Kartikey1306 commented Sep 15, 2026

Negative controls

At 55a39d0

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kartikey1306 commented Sep 15, 2026 •

edited

Loading

Also in `2693259`

At `55a39d0`

codecov-commenter commented Sep 15, 2026 •

edited

Loading

At `55a39d0`