fix(tests): restore the line continuation that unterminates TEST() in test_image_verify - #77
Closed
Kartikey1306 wants to merge 1 commit into
Closed
Conversation
… test_image_verify
master does not build. The TEST() macro in tests/unit/test_image_verify.c lost
the backslash on one line:
#define TEST(name) \
static void name(void); \
static void run_##name(void) { \
memset(sim_flash, 0xFF, sizeof(sim_flash)); \
sim_unreadable_from = UINT32_MAX; <-- no continuation
sim_tick = 0; \
...
The macro therefore ends at that line, and every line after it -- the tick
reset, eos_hal_init(), the printf, the call, the closing brace -- is parsed as
file-scope code instead of macro body. That produces 24 errors that all look
unrelated to the cause:
error: redefinition of 'sim_tick' with a different type: 'int' vs 'uint32_t'
error: conflicting types for 'eos_hal_init'
error: conflicting types for 'printf'
error: extraneous closing brace ('}')
Restoring the backslash is the whole fix.
before: 24 errors, ctest 13 of 19 failing
after: 0 errors, ctest 19/19 pass
Introduced by embeddedos-org#70 (881f00c). It also means every TEST() in that file was
running without its per-test reset of sim_unreadable_from and sim_tick, so the
fixture state leaked between cases -- the file has not compiled since, so this
never showed up as a wrong result, only as a build failure.
`CI — eBoot` was already red on master for exactly this before I started.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 1, 2026
…ing GCM
core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot
path with no tests. 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 broke decryption two ways:
1. No counter position. ctx->iv is passed unchanged on every call, so a second
chunk restarts the CTR 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. The hook returns plaintext only, so ctx->ghash_acc is never fed.
eos_fw_decrypt_final() 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 be patched by also feeding
GHASH: the plaintext would still be wrong past the first chunk. Re-enabling
needs a hook that takes a block offset and either exposes GHASH state or does
the whole GCM operation including the tag. Removed, with that written down
where the next person will look.
Also adds tests/unit/test_fw_decrypt.c -- the first tests this file has had.
Vectors come from an independent implementation (Python cryptography, i.e.
OpenSSL) rather than from this code, so they pin behaviour rather than
recording 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 -- this is 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
Verified: 8/8 pass with this change; against the unmodified fw_decrypt.c the
suite fails on
test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it
exists to make. The software GCM itself is correct -- I checked it against the
reference vectors before changing anything, including every chunk split above.
Note: the full test build on master is currently broken by test_image_verify.c
(fixed in embeddedos-org#77), so this target was built directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 1, 2026
Contributor
Author
|
Closing — this landed. sim_unreadable_from = UINT32_MAX; \Merging current master into this branch leaves an empty diff, so there is nothing here to review. Glad it is in either way: it was blocking every PR that builds the test suite. #80, #81 and #82 were stacked on this only because master would not compile; I have rebased them onto master directly. |
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 1, 2026
…ing GCM
core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot
path with no tests. 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 broke decryption two ways:
1. No counter position. ctx->iv is passed unchanged on every call, so a second
chunk restarts the CTR 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. The hook returns plaintext only, so ctx->ghash_acc is never fed.
eos_fw_decrypt_final() 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 be patched by also feeding
GHASH: the plaintext would still be wrong past the first chunk. Re-enabling
needs a hook that takes a block offset and either exposes GHASH state or does
the whole GCM operation including the tag. Removed, with that written down
where the next person will look.
Also adds tests/unit/test_fw_decrypt.c -- the first tests this file has had.
Vectors come from an independent implementation (Python cryptography, i.e.
OpenSSL) rather than from this code, so they pin behaviour rather than
recording 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 -- this is 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
Verified: 8/8 pass with this change; against the unmodified fw_decrypt.c the
suite fails on
test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it
exists to make. The software GCM itself is correct -- I checked it against the
reference vectors before changing anything, including every chunk split above.
Note: the full test build on master is currently broken by test_image_verify.c
(fixed in embeddedos-org#77), so this target was built directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 1, 2026
…ul boot
cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:
if (cfg->lock_debug) {
eos_secure_boot_lock_debug();
}
/* ---- Step 8: Record successful attestation ---- */
attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
return EOS_SBOOT_OK;
eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.
The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.
eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.
Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line embeddedos-org#72 adds, written
identically so whichever lands first leaves the other a trivial rebase.
tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c embeddedos-org#72 introduces.
Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.
Verified on this branch: build clean, ctest 20/20, pytest 30 passed.
Stacked on embeddedos-org#77 (master's test suite does not compile without it).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 1, 2026
…ul boot
cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:
if (cfg->lock_debug) {
eos_secure_boot_lock_debug();
}
/* ---- Step 8: Record successful attestation ---- */
attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
return EOS_SBOOT_OK;
eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.
The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.
eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.
Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line embeddedos-org#72 adds, written
identically so whichever lands first leaves the other a trivial rebase.
tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c embeddedos-org#72 introduces.
Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.
Verified on this branch: build clean, ctest 20/20, pytest 30 passed.
Stacked on embeddedos-org#77 (master's test suite does not compile without it).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 1, 2026
…ing GCM
core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot
path with no tests. 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 broke decryption two ways:
1. No counter position. ctx->iv is passed unchanged on every call, so a second
chunk restarts the CTR 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. The hook returns plaintext only, so ctx->ghash_acc is never fed.
eos_fw_decrypt_final() 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 be patched by also feeding
GHASH: the plaintext would still be wrong past the first chunk. Re-enabling
needs a hook that takes a block offset and either exposes GHASH state or does
the whole GCM operation including the tag. Removed, with that written down
where the next person will look.
Also adds tests/unit/test_fw_decrypt.c -- the first tests this file has had.
Vectors come from an independent implementation (Python cryptography, i.e.
OpenSSL) rather than from this code, so they pin behaviour rather than
recording 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 -- this is 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
Verified: 8/8 pass with this change; against the unmodified fw_decrypt.c the
suite fails on
test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it
exists to make. The software GCM itself is correct -- I checked it against the
reference vectors before changing anything, including every chunk split above.
Note: the full test build on master is currently broken by test_image_verify.c
(fixed in embeddedos-org#77), so this target was built directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 3, 2026
…ing GCM
core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot
path with no tests. 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 broke decryption two ways:
1. No counter position. ctx->iv is passed unchanged on every call, so a second
chunk restarts the CTR 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. The hook returns plaintext only, so ctx->ghash_acc is never fed.
eos_fw_decrypt_final() 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 be patched by also feeding
GHASH: the plaintext would still be wrong past the first chunk. Re-enabling
needs a hook that takes a block offset and either exposes GHASH state or does
the whole GCM operation including the tag. Removed, with that written down
where the next person will look.
Also adds tests/unit/test_fw_decrypt.c -- the first tests this file has had.
Vectors come from an independent implementation (Python cryptography, i.e.
OpenSSL) rather than from this code, so they pin behaviour rather than
recording 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 -- this is 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
Verified: 8/8 pass with this change; against the unmodified fw_decrypt.c the
suite fails on
test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it
exists to make. The software GCM itself is correct -- I checked it against the
reference vectors before changing anything, including every chunk split above.
Note: the full test build on master is currently broken by test_image_verify.c
(fixed in embeddedos-org#77), so this target was built directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 3, 2026
…ul boot
cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:
if (cfg->lock_debug) {
eos_secure_boot_lock_debug();
}
/* ---- Step 8: Record successful attestation ---- */
attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
return EOS_SBOOT_OK;
eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.
The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.
eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.
Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line embeddedos-org#72 adds, written
identically so whichever lands first leaves the other a trivial rebase.
tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c embeddedos-org#72 introduces.
Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.
Verified on this branch: build clean, ctest 20/20, pytest 30 passed.
Stacked on embeddedos-org#77 (master's test suite does not compile without it).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
masterdoes not build. One backslash.The macro ends at that line, so everything after it — the tick reset,
eos_hal_init(), theprintf, the call, the closing brace — is parsed as file-scope code rather than macro body. The 24 errors that fall out all point somewhere other than the cause:ctestpytest tests/Worth knowing beyond the fix
Introduced by #70 (
881f00c). Because the macro was unterminated, everyTEST()in that file was defined without its per-test reset ofsim_unreadable_fromandsim_tick— fixture state would have leaked between cases. That never produced a wrong result only because the file has not compiled since, so the tests have not run at all.CI — eBootwas already red onmasterfor this before I opened anything, so this is not a regression from another in-flight PR.🤖 Generated with Claude Code