fix(fw_decrypt): drop a HW-crypto shortcut that cannot express streaming GCM

[Kartikey1306]

core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot path, with no tests at all. It has an opt-in fast path:

if (ops && ops->hw_aes_decrypt) {
    int rc = ops->hw_aes_decrypt(ctx->key, EOS_AES_KEY_SIZE,
                                 ctx->iv, data, data, len);
    if (rc == EOS_OK) { ctx->bytes_processed += len; return EOS_OK; }
}

The hook is (key, key_len, iv, in, out, len). That signature cannot carry streaming GCM, and taking the path breaks decryption two ways.

1. No counter position → CTR keystream reuse

ctx->iv is passed unchanged on every call, and nothing communicates bytes_processed. A second chunk restarts the keystream at block 0 and is decrypted against the same keystream as the first. Reusing a CTR keystream across two plaintexts is the one thing the mode must never do.

2. No GHASH → genuine images rejected

The hook returns plaintext only, so ctx->ghash_acc is never fed. eos_fw_decrypt_final() then computes the tag over an empty accumulator and rejects the image. A board with an AES engine could not install a correctly encrypted firmware update at all.

(2) is why this was never noticed: it fails closed, and no board in-tree implements the hook yet. (1) is why it cannot simply be patched by also feeding GHASH — the plaintext would still be wrong past the first chunk.

Re-enabling this needs a hook that takes a block offset and either exposes the GHASH state or performs the whole GCM operation including the tag. Removed for now, with that reasoning written where the next person will look.

The software GCM itself is correct

I checked before changing anything, against vectors from an independent implementation (Python cryptography, i.e. OpenSSL):

1 block, one call      plaintext=OK   tag=ACCEPTED
2 blocks, one call     plaintext=OK   tag=ACCEPTED
2 blocks, 16B chunks   plaintext=OK   tag=ACCEPTED
20B (partial tail)     plaintext=OK   tag=ACCEPTED
32B as [8,24] [10,22] [1,31] · 20B as [5,15] [16,4]   all OK

That is a clean negative result and worth stating: the arithmetic is fine, the integration is not.

First tests for this file

tests/unit/test_fw_decrypt.c — vectors come from OpenSSL rather than from this code, so they pin behaviour rather than record it:

• whole blocks, and a 20-byte payload with a partial trailing block
• the same ciphertext split [32] / [16,16] / [10,22] / [1,31] / [5,15] — GCM is a stream, so the result must not depend on how the caller sliced it
• a board advertising a working AES engine must reach the same answer as one without — the case that fails on master
• every single-bit flip in the tag (128 of them) and in each ciphertext byte must be rejected
• unprovisioned/unreadable OTP keys and uninitialised contexts are refused

A detail that matters: the simulated engine succeeds. My first version returned EOS_ERR_NOT_SUPPORTED, which made the caller fall through to the software path — so the test passed against the broken code and proved nothing. The bug only appears when the engine returns EOS_OK.

Verification

with this change	8/8 pass
against unmodified fw_decrypt.c	fails on test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it exists to make

Note: the full test build on master is currently broken by test_image_verify.c (a lost line continuation — fixed in #77), so this target was built directly. This PR is independent of #77 and does not touch that file.

core/fw_decrypt.c is CRLF; the diff is 26/10 lines with endings preserved, not a whole-file rewrite.

🤖 Generated with Claude Code

[Kartikey1306]

Now stacked on #77 — merge that first.

The three red checks here were master's, not this PR's: tests/unit/test_image_verify.c fails to compile because its TEST() macro lost a line continuation in #70, so any PR that builds the test suite goes red on it.

test_image_verify.c:95:9: error: conflicting types for 'sim_tick'; have 'int'
test_image_verify.c:97:28: error: stray '#' in program

Rather than leave this PR permanently red on someone else's breakage, it is rebased onto #77 (the one-character fix). Once #77 merges this collapses back to the two files it actually changes.

On the stacked branch, the whole suite builds and runs:

cmake --build	0 errors
ctest --no-tests=error --timeout 120	20/20 pass
pytest tests/	16 passed, 1 skipped

eboot_test_fw_decrypt is 8/8 within that, and still fails on test_board_with_aes_engine_still_accepts_a_genuine_image if core/fw_decrypt.c is reverted to master's version.

[codecov-commenter]

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

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[srpatcha]

Review — eBoot#80 "fix(fw_decrypt): drop a HW-crypto shortcut that cannot express streaming GCM"

head: c657bff author: Kartikey1306 ci: 24 checks pass

Verdict: Correct, well-evidenced, and the test design is the best thing in it. Two
loose ends the removal leaves behind, both in files the PR did not touch.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	include/eos_hal.h:90-92	After this merges, hw_aes_decrypt has zero call sites — git grep hw_aes_decrypt returns only this header, the removed block, and the stale doc comment in finding 2. The header still advertises it under /* HW-accelerated crypto (optional, software fallback used if NULL) */, which is now false in the other direction: a board author who implements the hook gets nothing, silently, and no diagnostic tells them. That is the same class of failure the PR is fixing.	Either delete the member, or keep it and change the comment to say it is reserved and currently unconsumed pending a streaming-capable contract, pointing at the rationale block this PR added.
2	Medium	core/fw_decrypt.c:11	The file header still reads "Falls back to HAL hw_aes_decrypt if available." The PR's diff starts at line 192, so this line survives and directly contradicts the twenty-line comment the PR installs below it.	Update line 11 in this PR — it is one line, and leaving the file self-contradictory is worse than the original.
3	Low	tests/CMakeLists.txt:86	Textual conflict with #82. Both PRs insert a new add_executable/add_test triple immediately after add_test(NAME test_keystore ...), at the same anchor. Same author, both open.	Whichever lands second, move the insertion point. Trivial, but it will surprise someone at merge time.

On the analysis

Both failure modes check out against the code being removed. The hook signature at
include/eos_hal.h:92 is (const void *key, size_t key_len, const void *iv, ...) with no
block offset and no output state, so:

• ctx->iv was passed unchanged on every eos_fw_decrypt_update() call and ctx->bytes_processed
  was never communicated — a second chunk restarts the CTR keystream at block 0. Reusing a
  CTR keystream across two plaintexts is the failure the mode cannot survive.
• The hook returns plaintext only, so ctx->ghash_acc is never fed and
  eos_fw_decrypt_final() computes the tag over an empty accumulator.

The second point is why this is a latent defect rather than a live one: it fails closed,
and no in-tree board provides the hook. The body says this plainly instead of inflating
the severity, which is the right call. The reasoning that it cannot be repaired by also
feeding GHASH — because the plaintext is already wrong past the first chunk — is correct.

The test file is the strongest part. Vectors from OpenSSL rather than from this code
pin behaviour rather than record it; the chunk-boundary matrix ([32], [16,16],
[10,22], [1,31], [5,15]) is the right way to test a streaming AEAD; and the note
that a simulated engine returning EOS_ERR_NOT_SUPPORTED made the test pass against
broken code — so it had to return EOS_OK — is exactly the kind of negative result that
usually goes unrecorded. test_board_with_aes_engine_still_accepts_a_genuine_image is
the assertion that carries the PR.

Architecture conformance

Deviates, pre-existingly, and this PR narrows the deviation.

Master design §14.1: "Use reviewed cryptographic libraries; do not invent cryptographic
primitives." core/fw_decrypt.c is a hand-written AES-256-GCM in the boot path, which
the PR body itself names. This PR does not introduce that and cannot resolve it; it adds
the first vector-based tests the file has ever had, which is the right move short of
replacement.

§14.1 also says "Keep target-specific hardware security adapters behind stable
interfaces." That is the rule eos_board_ops_t::hw_aes_decrypt broke — the interface was
stable and simply could not express the operation. The master design gives no contract
for what a hardware crypto adapter must be able to carry, which is why a one-shot
signature was a plausible thing to write. Proposal appended to
.ai/autoreview/proposals/2026-09.md.

§21 Tier 1 (eBoot, Foundation) — correct repo. §5.1 untouched; removing a call site adds
no dependency edge.

Proposed changes

core/fw_decrypt.c:11   - " * Falls back to HAL hw_aes_decrypt if available."
                       + " * Software-only: see eos_fw_decrypt_update() for why the
                       +    HAL hw_aes_decrypt hook is not used."
include/eos_hal.h:90-92  mark hw_aes_decrypt reserved/unconsumed, or remove it
tests/CMakeLists.txt     re-anchor whichever of #80/#82 lands second

Merge after #77 as the author stacked it. Nothing here should hold it up.

Not checked

• Nothing was built or run. 8/8, 20/20 and 16 passed, 1 skipped are taken from the
  PR body and the author's comment; I did not reproduce them, and I did not re-derive the
  OpenSSL vectors in tests/unit/test_fw_decrypt.c.
• checks.txt shows 24 green checks on this head, which is CI's signal, not mine.
• I did not audit the software AES-GCM implementation itself. The PR asserts it is
  correct against independent vectors; that assertion is untested here, and §14.1's
  objection to a hand-written primitive in the TCB stands regardless of whether these
  particular vectors pass.

---

Automated architecture review of c657bff65f79 — 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.

[srpatcha]

Review — eBoot#80 "fix(fw_decrypt): drop a HW-crypto shortcut that cannot express streaming GCM"

head: 04b7b74 author: Kartikey1306 ci: 24 checks pass

Verdict: Conforms. All three findings from the review of c657bff6 are addressed at
04b7b74, and this time I built and ran it rather than taking the numbers on trust.
Two new points, both Low, both in the test scaffolding rather than the fix.

What I ran

Built refs/pull/80/head from a clean export of the tree, host toolchain, gcc
-DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug:

cmake        exit 0
cmake --build exit 0
ctest --no-tests=error --output-on-failure --timeout 120
             exit 0    22/22 tests passed
             #22 test_fw_decrypt  Passed

For contrast, the same procedure on origin/master (22d8f8b):

cmake --build exit 2
include/eos_image.h:135: error: 'eos_image_header_t' has no member named 'reserved'
core/ed25519_verify.c:338: error: redefinition of 'point_is_identity'
ctest exit 8    0/21 tests run — every one "Not Run"

So the green CI on this PR is real and it is not master's. That is because this branch is
stacked on #94, which is what repairs those two. gh pr diff shows #94's commit as
part of this PR because the base is master; this PR's own delta is the two commits
fb3afdb + 04b7b74, touching core/fw_decrypt.c, include/eos_hal.h,
tests/CMakeLists.txt and tests/unit/test_fw_decrypt.c. Nothing below refers to the
ed25519_verify.c / eos_image.h / test_ed25519.c hunks — those are #94's and are
reviewed there.

Previous findings

Was	Now
M1 eos_hal.h:90-92 — hw_aes_decrypt left advertised with zero consumers	Fixed. include/eos_hal.h:92-107 now marks it reserved and unconsumed, says a board implementing it gets nothing, and names the contract change required before a caller may be added.
M2 core/fw_decrypt.c:11 — file header still said "Falls back to HAL hw_aes_decrypt"	Fixed at core/fw_decrypt.c:11-14.
L3 tests/CMakeLists.txt — same insertion anchor as #82	Fixed: re-anchored after test_ecc at tests/CMakeLists.txt:113, with the reason in a comment. Confirmed against #82's head — it inserts after test_keystore, so the two no longer collide.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Low	tests/CMakeLists.txt:121-131	test_fw_decrypt is not in the Valgrind foreach(TEST_NAME ...) list, so the one new test in the boot-crypto path does not get the memory-safety run the others do — and it is the test with a fixed uint8_t buf[64], a caller-driven chunk splitter, and 128 tag-mutation iterations, i.e. the one most likely to reward it. The list is hand-maintained and has already drifted: 5 of the 22 registered tests are absent from it (test_ecc, test_rollback, test_secure_boot, test_storage, and now test_fw_decrypt).	Add test_fw_decrypt to the foreach list. Separately — not this PR's job — the list should be derived from the registered tests rather than retyped, since nothing fails when a name is forgotten.
2	Low	tests/unit/test_fw_decrypt.c:24-32, 262	The new file hardcodes tests_run = 8; in main() and its TEST() macro does not increment tests_run. A test function added to this file but never wired into main() leaves the count at 8, tests_passed == tests_run, and the binary exits 0 having silently skipped it. The base commit of this very branch — #94, f704d87 — removed exactly this pattern from tests/unit/test_ed25519.c, where a hardcoded tests_run = 11 had been masking two tests that main() never called.	Copy the fixed form: put tests_run++; in the TEST() macro before name(); and delete the tests_run = 8; line, matching tests/unit/test_ed25519.c:29-36 on this same branch.

On the fix itself

The reasoning holds and I re-checked it against the removed code rather than the PR body.
eos_board_ops_t::hw_aes_decrypt is (key, key_len, iv, in, out, len) — no counter
position and no output state — so the removed block passed ctx->iv unchanged on every
eos_fw_decrypt_update() call and never fed ctx->ghash_acc. Keystream restart at block 0
on the second chunk, and a tag computed over an empty accumulator. The claim that it cannot
be repaired by feeding GHASH alone is right: the plaintext is already wrong past the first
chunk, so there is nothing correct to hash.

The chunk-boundary matrix is the part that earns its keep, and I confirmed it is doing real
work rather than passing trivially. [5,15] on the 20-byte vector splits inside a block and
still has to produce OpenSSL's tag — that only works if the implementation buffers the
partial block across calls instead of zero-padding each chunk, which is the single most
common way a hand-rolled streaming GCM is wrong. It passes.

test_board_with_aes_engine_still_accepts_a_genuine_image now passes because the call site
is gone rather than because the engine is exercised — sim_hw_aes_decrypt is installed and
never invoked. That is fine and is the point: it is a tripwire against reintroduction, and
it fails to compile if the struct member is deleted. Worth knowing when reading a green run.

Architecture conformance

Deviates pre-existingly; this PR narrows the deviation. Repo placement is correct.

• §14.1 "Use reviewed cryptographic libraries; do not invent cryptographic primitives."
  core/fw_decrypt.c remains a hand-written AES-256-GCM in the TCB. This PR neither
  introduces nor can resolve that; it gives the file its first vector-based tests, which is
  the right move short of replacement.
• §14.1 "Keep target-specific hardware security adapters behind stable interfaces."
  The interface was stable and could not express the operation. Proposal already appended
  for c657bff6 ("Hardware security adapters need a stated contract, not just a stable
  interface", proposals/2026-09.md); the header comment this PR adds at
  include/eos_hal.h:92-107 is a good local statement of the same gap and nothing further
  is proposed here.
• §21 Tier 1 (Foundation) — eBoot is the right repo. §5.1 untouched: removing a
  call site adds no dependency edge, and the new test links eboot_core only.

Proposed changes

tests/CMakeLists.txt:126     add test_fw_decrypt to the valgrind foreach list

tests/unit/test_fw_decrypt.c:24-32
    #define TEST(name) \
        static void name(void); \
        static void run_##name(void) { \
            printf("  %-52s ", #name); \
    +       tests_run++; \
            name(); \
tests/unit/test_fw_decrypt.c:262
    -   tests_run = 8;

Both are one-liners and neither blocks the merge. Merge order: #94 first, since this is
stacked on it and master does not build without it.

Not checked

• I did not audit the software AES-256-GCM implementation itself. The eight tests pass
  against OpenSSL-derived vectors for 16, 20 and 32 bytes; that says nothing about lengths
  past 32 bytes, non-empty AAD, IV lengths other than 96 bits, or the length-block encoding
  near 2^32 blocks. §14.1's objection to a hand-written primitive in the TCB stands
  regardless.
• I did not re-derive the vectors in tests/unit/test_fw_decrypt.c from an independent
  implementation; I confirmed the suite passes, not that the constants are OpenSSL's.
• Host build and ctest only — gcc, x86-64, Debug. I did not run the cross builds, the
  sanitizer build, valgrind, or pytest tests/. The 24 green checks in checks.txt are
  CI's evidence for those, not mine.
• ctx->bytes_processed is uint32_t; I did not check what happens to an image at or past
  4 GiB, which is out of scope for the targets in configs/.

---

Automated architecture review of 04b7b74c75ed — 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.