fix(secure-boot): a debug lock that failed must not report a successful boot - #82
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
69a2f65 to
998035f
Compare
998035f to
c5bb181
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#82 "fix(secure-boot): a debug lock that failed must not report a successful boot"
head: c5bb181 author: Kartikey1306 ci: 24 checks pass
Verdict: The fix is right and the reasoning about EOS_ERR_NOT_SUPPORTED being the
common case, not the edge case, is the valuable part. The tests do not cover the thing the
PR changed, and the function being hardened is not reachable from the boot path.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_secure_boot_policy.c:88-121 |
The behavioural change is untested. All three tests call eos_secure_boot_lock_debug() directly and assert its return value. None calls eos_secure_boot() with cfg.lock_debug = true, so the new Step-7 early return — the attest_record(..., EOS_SBOOT_ERR_POLICY) and return EOS_SBOOT_ERR_POLICY at core/secure_boot.c:190-194 — never executes. The file's own docblock says "the debug-lock policy must be enforced, not merely attempted"; what it tests is that the helper reports, which is the "attempted" half. |
Add a fourth test: build a cfg with lock_debug = true on a board with no otp_write, run a boot that would otherwise succeed, and assert eos_secure_boot(...) == EOS_SBOOT_ERR_POLICY. That is the regression that would catch a future revert. |
| 2 | Medium | core/secure_boot.c:182 |
eos_secure_boot() has no production caller. git grep eos_secure_boot across master, excluding its own file and tests/, returns one hit — a filename in README.md:29. The live path is stage1/main.c:75 → eboot_jump_to_app() (stage1/jump_app.c), which parses, verifies integrity, verifies signature, checks rollback, applies the MPU and jumps — and never locks the debug port. So this PR closes a fail-open in a module nothing runs, while the path that does run has no debug-lock step at all. docs/secure_boot_chain.md:143 documents eboot_jump_to_app() as step 12 of the chain, which confirms which one is intended to be live. |
Merge this anyway — it is correct and cheap. Then open an issue: either wire eos_secure_boot() into Stage 1 or fold its policy steps into eboot_jump_to_app(). Two parallel orchestrators for the same responsibility is how one of them silently rots, which is what happened here. |
| 3 | Low | PR body, "Why this was never observable" | Stale. It says core/secure_boot.c is not in CMakeLists.txt and that this PR adds the line. #72 landed that (2955938), and files.txt confirms this PR no longer touches the top-level CMakeLists.txt at all. The grep -c "core/secure_boot.c" CMakeLists.txt -> 0 evidence block no longer reproduces. |
Update the body so the record is accurate; the finding it supports is still real. |
| 4 | Low | tests/CMakeLists.txt:86 |
Same insertion anchor as #80 — both add their triple immediately after add_test(NAME test_keystore ...). Same author, both open, guaranteed conflict for whichever lands second. |
Re-anchor one of them. |
On the fix itself
The defect is exactly as described. eos_hal_otp_write() returns EOS_ERR_NOT_SUPPORTED
when the board supplies no otp_write hook, and the old void eos_secure_boot_lock_debug(void)
discarded it, so lock_debug: true was a silent no-op on every such board while
attestation recorded EOS_SBOOT_OK. Reusing EOS_SBOOT_ERR_POLICY (include/eos_secure_boot.h:45,
"Boot policy violation") rather than inventing a code is right. Recording the failed
attestation before returning matches how every other failure in the function behaves.
void → int on a public header is source- and ABI-compatible for existing callers, so
§23.2's compatibility contract is not engaged.
Worth noting for whoever wires finding 2: this fix means a board with lock_debug: true
and no otp_write will refuse to boot. That is the correct fail-closed behaviour and I am
not arguing against it — but today it is unreachable, and the day eos_secure_boot()
becomes live is the day it becomes an availability decision. lock_debug is set true
nowhere in the tree at present (git grep lock_debug finds only the struct member, the
two call sites, and test_secure_boot.c:140 setting it false), so there is time to
decide whether the right response is a hard stop or a route to recovery per §8.1.
Architecture conformance
Conforms. §21 Tier 1 (eBoot, Foundation). §5.1 — "eBoot keeps the trusted computing base
minimal and auditable": a policy step that reports success without having taken effect is
the opposite of auditable, and this removes one. §8.1 requires "Explicit separation between
implemented, experimental and planned security features", and §28 grades Implemented as
requiring "Code and functional tests" — finding 1 is that gap, finding 2 is a sharper
version of the same one. No dependency edges added.
Proposed changes
core/secure_boot.c keep as-is — the fix is correct
tests/unit/test_secure_boot_policy.c
+ test_secure_boot_returns_policy_error_when_lock_fails()
driving eos_secure_boot() end to end
tests/CMakeLists.txt re-anchor away from #80's insertion point
PR body drop the CMakeLists claim (#72 landed it)
follow-up issue reconcile eos_secure_boot() with eboot_jump_to_app()
Not checked
- Nothing was built or run.
build clean,ctest 20/20,pytest 30 passedare the
author's figures; I did not reproduce them, and I did not confirm that the new test
fails against unmodifiedsecure_boot.cas claimed. checks.txtshows 24 green checks on this head — CI's signal, not mine.- I did not review
tests/unit/test_secure_boot.c(added by #72) for overlap with the new
file beyond the naming collision the author already avoided. - Whether any out-of-tree board sets
lock_debug = trueis unknown; finding 2's
availability note covers in-tree configuration only.
Automated architecture review of c5bb18192bfd — 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.
…rged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — embeddedos-org#93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. embeddedos-org#87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) embeddedos-org#93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — embeddedos-org#86 and embeddedos-org#57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only embeddedos-org#57's public_key_is_valid_subgroup() is wired to the call site, so embeddedos-org#86's key_has_prime_order() was dead. Kept the live function, folded embeddedos-org#86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on embeddedos-org#86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight.
… clash Three findings from the review on embeddedos-org#80, all in files this branch already owns. Finding 2 (Medium) -- core/fw_decrypt.c's file header still read "Falls back to HAL hw_aes_decrypt if available." The diff started at line 192, so that line survived and directly contradicted the twenty-line rationale this PR installs below it. A self-contradictory file is worse than the original. Finding 1 (Medium) -- after the removal, hw_aes_decrypt has zero call sites while eos_hal.h still advertises it under "software fallback used if NULL", which is now false in the other direction: a board author who implements the hook gets nothing, silently, with no diagnostic. That is the same class of failure this PR is fixing. Kept the member rather than deleting it -- removing it from a public struct is an ABI change for out-of-tree boards, and the hook is worth having once it can express the operation -- and documented it as reserved and currently unconsumed, with the reason and with what a streaming-capable replacement would need. Finding 3 (Low) -- this PR and embeddedos-org#82 both inserted their add_executable/add_test triple immediately after add_test(NAME test_keystore ...), so whichever landed second would have conflicted for no reason but placement. Re-anchored this one to the end of the registrations, with a comment saying why. Also rebased: this branch was 7 commits behind and its diff against current master would have reverted the test_recovery link-line fix. It is now stacked on embeddedos-org#94, which repairs master -- without that, every PR that builds the test suite is red on include/eos_image.h and core/ed25519_verify.c. Verified: cmake --build (EBLDR_BUILD_TESTS=ON) clean ctest 22/22 PASS test_fw_decrypt 8/8 PASS git grep hw_aes_decrypt header + this file's comment only Refs embeddedos-org#80
…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>
… it calls Answers finding 1 (Medium) from the review on embeddedos-org#82. All three tests called eos_secure_boot_lock_debug() directly and asserted its return value. None drove eos_secure_boot() with cfg.lock_debug = true, so the step-7 early return this PR adds -- the attest_record + return EOS_SBOOT_ERR_POLICY -- never executed under test. The file's docblock says "the debug-lock policy must be enforced, not merely attempted"; what it covered was the attempted half. Adds three end-to-end cases through eos_secure_boot(): - lock_debug: true on a board with no otp_write -> EOS_SBOOT_ERR_POLICY - the same image with lock_debug: false -> EOS_SBOOT_OK, and the recorded entry point is the header's, which is the counter-check: without it the first test would also pass if steps 1-6 were failing for an unrelated reason and never reaching step 7 - lock_debug: true with a working fuse -> EOS_SBOOT_OK, one write Reaching step 7 needs an image that clears steps 1, 2 and 5, so the fixture gains a simulated flash and stages an unsigned, unencrypted image whose SHA-256 matches. EOS_IMG_FLAG_HASH_SHA256 is load-bearing there: without it verify_integrity takes the CRC32 branch, reads a CRC out of hash[], and step 2 fails before the policy step is ever reached. Finding 4 (Low), the tests/CMakeLists.txt anchor shared with embeddedos-org#80, is resolved on embeddedos-org#80's side -- its block moved to the end of the registrations, so this one keeps its position and the two no longer collide. Verified: ctest 22/22 PASS test_secure_boot_policy 6/6 PASS discrimination: with step 7 reverted to `(void)eos_secure_boot_lock_debug();` test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken FAILS the three original helper tests still PASS which is the gap the finding described, reproduced. Refs embeddedos-org#82
c5bb181 to
eafe67c
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#82 "fix(secure-boot): a debug lock that failed must not report a successful boot"
head: eafe67c author: Kartikey1306 ci: 23 pass, 1 skipped (Create GitHub Release)
Verdict: The fix is right and the follow-up commit closes the coverage gap properly —
including the counter-check that stops the new test passing for the wrong reason. One new
High finding, in the same function and of the same class the PR exists to fix.
What I ran
Built refs/pull/82/head from a clean export, host gcc,
-DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug:
cmake exit 0 build exit 0
ctest --no-tests=error --output-on-failure --timeout 120 -> exit 0, 22/22 passed
test_secure_boot_policy Passed (6/6 within it)
origin/master (22d8f8b) under the same procedure fails to build and runs 0 of 21 tests —
include/eos_image.h:135 references a removed reserved member and
core/ed25519_verify.c:338 redefines point_is_identity. This branch is stacked on #94,
which repairs both; that is why gh pr diff shows ed25519_verify.c, eos_image.h and
test_ed25519.c here. This PR's own delta is 0886ac0 + eafe67c, over
core/secure_boot.c, include/eos_secure_boot.h, tests/CMakeLists.txt and
tests/unit/test_secure_boot_policy.c.
Previous findings (head c5bb1819)
| Was | Now |
|---|---|
M1 the behavioural change was untested — every test drove the helper, none drove eos_secure_boot() |
Fixed, and better than asked. test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken drives the whole function to EOS_SBOOT_ERR_POLICY, and test_the_same_image_boots_when_no_debug_lock_is_asked_for is the counter-check that proves steps 1–6 actually reach step 7 rather than the first test passing off an unrelated failure. test_secure_boot_proceeds_when_the_debug_lock_succeeds closes the third corner. The stage_bootable_image() comment about EOS_IMG_FLAG_HASH_SHA256 being required or step 2 takes the CRC32 branch is the sort of thing that is usually rediscovered rather than written down. |
M2 eos_secure_boot() has no production caller |
Still open. git grep 'eos_secure_boot(' on this head returns the definition and the declaration, nothing else. Restated below, not re-argued. |
L3 PR body claims core/secure_boot.c is not in CMakeLists.txt |
Still stale. origin/master:CMakeLists.txt:110 already lists it, and this PR does not touch that file. |
L4 tests/CMakeLists.txt anchor clash with #80 |
Resolved — #80 moved its block to after test_ecc; this one keeps the test_keystore anchor. No conflict. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | core/secure_boot.c:104-118 |
Step 4 is the same defect this PR is fixing, two steps earlier, and is untouched. "Verify signing key against OTP root-of-trust" reads the OTP hash, and then: if eos_hal_otp_read() returns anything but EOS_OK the failure is discarded and boot continues; if it succeeds and the hash is provisioned, the body of the if is /* In a full implementation, extract key hash from TLV and compare */ — no comparison happens. So a device whose root of trust is provisioned boots an image signed by any key the image itself carries, and attest_record(..., EOS_SBOOT_OK) records a clean secure boot. .ai/security.md: "A verification step that cannot run must fail, not pass"; "Attestation must not record OK for a boot in which a policy step was skipped." This is pre-existing, not introduced here — but it is in the function this PR edits, and the PR's own thesis is the argument for fixing it. |
Not this PR's scope to implement TLV key-hash extraction. Do two things that are: fail on a non-EOS_OK otp_read, and make the unimplemented branch return EOS_SBOOT_ERR_SIGNATURE when OTP is provisioned, so a provisioned device fails closed instead of booting on an unverified key. If that is judged too large, the minimum is that step 4 stops being labelled a verification — §8.1 requires "explicit separation between implemented, experimental and planned security features", and a comment inside an if is not that separation. |
| 2 | Medium | core/secure_boot.c:71 |
Carried forward from c5bb1819, still true at this head: eos_secure_boot() has no production caller. The live path is stage1/main.c:75 → eboot_jump_to_app(), which never locks the debug port. Two orchestrators for one responsibility is how one of them rots — which is what produced both this fail-open and finding 1. |
Merge this anyway; then open an issue to either wire eos_secure_boot() into Stage 1 or fold its policy steps into eboot_jump_to_app(). Note that finding 1 is what makes this urgent rather than tidy: the unreachable orchestrator is the one that would be reached first if anyone wired it up as-is. |
| 3 | Low | tests/unit/test_secure_boot_policy.c:30-37, 232 |
TEST() does not increment tests_run and main() hardcodes tests_run = 6, so a test added to this file but not wired into main() is skipped silently with a zero exit. #94 — the base commit of this branch — removed exactly this pattern from tests/unit/test_ed25519.c, where a hardcoded tests_run = 11 was masking two uncalled tests. #80 has the same finding against its new file. |
tests_run++; inside the TEST() macro, drop the assignment in main(). Matches tests/unit/test_ed25519.c:29-36 on this branch. |
| 4 | Low | tests/CMakeLists.txt:93-97 |
test_secure_boot_policy is not in the Valgrind foreach(TEST_NAME ...) list at tests/CMakeLists.txt:121-131, so it gets no memory-safety run. The list is hand-maintained and now misses 6 of 22 registered tests. |
Add it. Separately, derive the list from the registered tests — nothing fails today when a name is forgotten. |
| 5 | Low | PR body | Two claims no longer reproduce: grep -c "core/secure_boot.c" CMakeLists.txt -> 0 (it is 1 on master, landed by #72) and "Added the one line that builds it". The verification table also predates the follow-up commit — it says ctest 20/20, this head is 22/22. |
Refresh the body. The finding it supports is real; the evidence quoted for it is not. |
On the fix itself
Correct, and I confirmed the mechanism rather than taking the body's word. eos_hal_otp_write()
returns EOS_ERR_NOT_SUPPORTED when the board supplies no otp_write, the old
void eos_secure_boot_lock_debug(void) discarded it, so lock_debug: true was a silent no-op
on every such board while attestation recorded EOS_SBOOT_OK. Reusing the existing
EOS_SBOOT_ERR_POLICY rather than inventing a code is right, and recording the failed
attestation before returning matches every other failure path in the function.
void → int on the public prototype is source- and ABI-compatible for existing callers, so
§23.2's compatibility contract is not engaged. lock_debug is still set true nowhere in the
tree (git grep lock_debug on this head: the struct member, the two call sites, and
test_secure_boot.c:140 setting it false), so the availability consequence of failing closed
is not yet live on any in-tree board.
Architecture conformance
Conforms. §21 Tier 1 (eBoot, Foundation) — right repo. §5.1 "eBoot keeps the trusted
computing base minimal and auditable": a policy step reporting success without having taken
effect is the opposite of auditable, and this removes one of two — finding 1 is the other.
§28 grades Implemented as requiring "code and functional tests"; the debug-lock policy now
meets that bar and step 4 does not. No dependency edges added.
The master design has no gap here that I can see: §8.1 and §14.1 already require what
findings 1 and 2 ask for. Nothing appended to proposals/.
Proposed changes
core/secure_boot.c:104-118 fail on non-EOS_OK otp_read; fail closed when OTP is
provisioned and the key-hash comparison is not implemented
tests/unit/test_secure_boot_policy.c:30-37,232
tests_run++ in TEST(); drop tests_run = 6
tests/CMakeLists.txt:126 add test_secure_boot_policy to the valgrind foreach
PR body drop the CMakeLists claim, refresh 20/20 -> 22/22
follow-up issue reconcile eos_secure_boot() with eboot_jump_to_app()
Only finding 1 is worth blocking on, and only if you judge it in scope; the other four are
one-liners.
Not checked
- Host build and
ctestonly — gcc, x86-64, Debug. No cross builds, no sanitizer build, no
valgrind, nopytest tests/. The 23 green checks inchecks.txtcover those; that is CI's
evidence, not mine. - I did not verify the claim that the new tests fail to compile against unmodified
secure_boot.c("invalid operands to binary expression ('void' and 'int')"). It is
plausible from the signature change, but I did not run it. - I did not review
tests/unit/test_secure_boot.c(from #72) for behavioural overlap with the
new file, only for the naming collision the author already avoided. eos_secure_boot_lock_debug()does not read the fuse back after writing it; whether a
successfulotp_writeimplies a blown fuse on real silicon is a HAL contract question I
cannot settle from the tree.- Step 4's
secure_compare()usage and the attestation chain hash were read, not tested.
Automated architecture review of eafe67c5d3c5 — 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.
… makes Answers the second review on embeddedos-org#82. Finding 1 (High) -- step 4, "Verify signing key against OTP root-of-trust", was the same fail-open this PR removes from step 7, two steps earlier. An `eos_hal_otp_read()` failure was discarded and boot continued; and when the read succeeded and the anchor was provisioned, the body of the `if` was `/* In a full implementation, extract key hash from TLV and compare */`. So a device whose root of trust *is* provisioned booted an image signed by any key the image carried, and step 8 recorded EOS_SBOOT_OK. Implementing the TLV comparison is out of scope, as the review says. The two things that are in scope are done: a non-EOS_OK otp_read now fails EOS_SBOOT_ERR_SIGNATURE, and a provisioned anchor that nothing compares against refuses the boot rather than proceeding. An unprovisioned board (all-zero anchor) is deliberately unchanged -- there is nothing to check against, and refusing would brick every board that has not been provisioned. The comment now states plainly that the step is planned rather than implemented, which §8.1 asks for and a comment inside an `if` was not. No test for those two refusals, deliberately, and the file says why rather than leaving it to be discovered. Reaching step 4 requires passing step 3 -- a real Ed25519 signature checked against the keystore. I wrote the obvious test first and it was worthless: with require_signature = true and an unsigned fixture the boot fails at step 3 and returns the same EOS_SBOOT_ERR_SIGNATURE step 4 returns, so it passed against the unfixed code too. I confirmed that by reverting step 4 and watching it still pass. A test that cannot fail is worse than none. What is there instead is the counter-check that the change does not refuse a boot it should allow. The fixture is buildable -- the keystore ships RFC 8032 TEST 1's public key and the matching private key is in the RFC -- but that machinery is embeddedos-org#88's (tools/gen_signed_image_fixture.py). Worth doing once embeddedos-org#88 lands. Finding 3 (Low) -- `TEST()` did not increment `tests_run` and `main()` hardcoded `tests_run = 6`, so a test added to the file but not wired into `main()` would have been skipped with a zero exit. embeddedos-org#94 removed exactly this from tests/unit/test_ed25519.c, where a hardcoded 11 was masking two uncalled tests. Same fix here. Finding 4 (Low) -- the Valgrind `foreach` is hand-maintained and missed 6 of 22 registered tests. Added test_secure_boot_policy, test_fdt_loader and test_fw_decrypt. The list being hand-maintained at all is the real defect and is not fixed here -- it is the same class as the hardcoded count, one level up. Finding 2 (Medium), that eos_secure_boot() has no production caller, stands and is not addressed here; finding 1 makes it sharper rather than resolving it. Finding 5 (Low) is a PR-body correction. Verified: ctest 22/22 PASS test_secure_boot_policy 7/7 PASS step 7 discrimination, still: reverting the step-7 branch fails test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken Refs embeddedos-org#82
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#82 "fix(secure-boot): a debug lock that failed must not report a successful boot"
head: 81bfe0b author: Kartikey1306 ci: pass
Verdict: The core change is right and well tested — a discarded eos_hal_otp_write() result meant lock_debug: true booted with SWD/JTAG open and recorded EOS_SBOOT_OK, and step 7 now fails closed. But the PR also carries an unannounced step-4 change that makes every OTP-provisioned device refuse to boot, and a two-name addition to the Valgrind test list that breaks CMake generation wherever valgrind is installed.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | tests/CMakeLists.txt:127-128 |
test_fdt_loader and test_fw_decrypt are added to the Valgrind foreach, but no eboot_test_fdt_loader / eboot_test_fw_decrypt target exists in this repo. $<TARGET_FILE:...> then fails at generate time, so cmake -B build aborts on any host that has valgrind. Reproduced against this head with -DVALGRIND=/usr/bin/true: CMake Error at tests/CMakeLists.txt:129 (add_test): No target "eboot_test_fdt_loader" … CMake Generate step failed. This is invisible in the PR checks because no required job installs valgrind — but .github/workflows/weekly.yml:59-73 ("Valgrind Memcheck") installs valgrind and then runs cmake -B build, so that job breaks on merge. |
Drop the two names. test_secure_boot_policy is the only new target this PR adds and it is correct to list. |
| 2 | High | core/secure_boot.c:123-140 |
Step 4 now returns EOS_SBOOT_ERR_SIGNATURE unconditionally whenever require_signature is set and the OTP anchor is non-zero. A device whose root of trust is provisioned — the intended production configuration — can no longer boot any image at all. The same holds for rc != EOS_OK, which includes EOS_ERR_NOT_SUPPORTED on a board with no otp_read hook. Failing closed is the right direction (.ai/security.md, "Fail closed"), but this is a device-bricking behaviour change that is not in the title, not in the PR body's "The change" section, appears only inside a code comment, and by the author's own note has no test. |
Split it into its own PR. It needs an explicit decision, a release note, and either the TLV comparison it is standing in for or a config gate. Landing it inside a debug-lock fix hides it. |
| 3 | Medium | core/secure_boot.c:126,136 |
Step 4's two refusals record and return EOS_SBOOT_ERR_SIGNATURE, which is exactly what step 3 returns for a genuinely bad signature. The condition is therefore unobservable from the result code and, as the note at tests/unit/test_secure_boot_policy.c:669-687 says, untestable — the obvious test passed against the unfixed code too. EOS_SBOOT_ERR_KEY_MISMATCH (include/eos_secure_boot.h:42, "Key hash doesn't match OTP") already exists and is the code for this. |
Return EOS_SBOOT_ERR_KEY_MISMATCH from both step-4 refusals. The test that could not be written then becomes writable with the existing unsigned fixture. |
| 4 | Medium | PR body | Two claims are not supported. (a) "core/secure_boot.c is not in CMakeLists.txt … grep -c → 0 … Added the one line that builds it" — #72 merged as 2955938, core/secure_boot.c is at CMakeLists.txt:110 on origin/master, and this PR does not touch the root CMakeLists.txt at all (files.txt confirms). (b) "ctest --no-tests=error 20/20 pass" — this head actually runs 22 tests. Per the brief, an assertion carried without matching output is itself the finding. |
Re-run against current origin/master and restate. The stale #72 paragraph should be deleted, not corrected in place. |
| 5 | Low | core/ed25519_verify.c, include/eos_image.h, tests/unit/test_ed25519.c |
Three changes unrelated to the debug lock ride along. They are not wrong — see "Not checked"/evidence below, origin/master does not compile without them — but nothing in the PR says the PR is also the master-build fix, so a reviewer looking only at the title will not know that reverting it re-reds the tree. |
One line in the body naming the two origin/master compile errors this also fixes. |
Architecture conformance
Conforms. Master design §8.1 requires "explicit separation between implemented, experimental and planned security features"; the step-4 comment does exactly that, and §5.1's "eBoot keeps the trusted computing base minimal and auditable" is served by making a policy step's failure visible rather than discarded. No dependency direction is touched: core/secure_boot.c calls only eos_hal_* and core/ peers, all at or below its own tier. Tier placement is correct — eBoot is Tier 1 Foundation (§21) and secure-boot policy is eBoot's own responsibility per §8.
The .ai/architect.md rule "restructure and change behaviour in the same commit" is stretched by finding 2, not by findings 5: the ed25519 and eos_image.h edits are build repair, the step-4 edit is new behaviour.
Proposed changes
Smallest sequence that keeps everything building:
tests/CMakeLists.txt— revert the Valgrind list to add one name only:Verify with- test_tlv_auth test_secure_boot_policy test_fdt_loader - test_fw_decrypt) + test_tlv_auth test_secure_boot_policy)
cmake -B build -DEBLDR_BUILD_TESTS=ON -DVALGRIND=/usr/bin/true, which currently fails and must configure clean.core/secure_boot.c— change both step-4EOS_SBOOT_ERR_SIGNATUREreturns toEOS_SBOOT_ERR_KEY_MISMATCH, then add the test that was previously impossible: provisioned anchor +require_signature = true+ valid unsigned fixture → expectEOS_SBOOT_ERR_KEY_MISMATCH, which fails against the unfixed code because step 3 returns-5.- Move the step-4 hunk to its own PR, or keep it and say in the body that it stops provisioned devices booting until TLV extraction lands.
- Correct the two PR-body claims.
Findings 1 and 3 are the only ones that need to land before merge; 1 because it breaks a scheduled job, 3 because it is two token changes and it unblocks the test the author wanted to write.
Not checked
- Not verified: nothing was run on hardware.
eos_hal_otp_write()failure andEOS_ERR_NOT_SUPPORTEDwere exercised only through the simulated board intest_secure_boot_policy.c; whether a real eFuse write reports failure the way the test assumes is untested here. - Not verified: the Cortex-M
#if defined(__ARM_ARCH)block ineos_secure_boot_lock_debug()is still entirely commented out, so on ARM the function reportsEOS_OKafter writing the OTP fuse without having touched DHCSR/DBGMCU. Whether the OTP fuse alone actually closes SWD/JTAG on the supported boards is a hardware question I cannot answer from here, and no test covers it. - Not verified: the eight low-order encodings in
k_low_order[]and the three ink_non_canonical[]were not independently recomputed; the suite passing only shows they are all rejected, not that they are the points the comments name. - Not verified: the claim "Measured against 13a7a02 … 16 of the 64 (key, R) pairs were accepted" — I did not reproduce it.
- Not checked: interaction with #72 and #88 beyond confirming #72 is already merged and does not collide with this diff.
Evidence
Run locally against origin/master (22d8f8b) and this head, from read-only git archive extractions — no repository was modified:
origin/master : cmake -B build -DCMAKE_BUILD_TYPE=Debug -DEBLDR_BUILD_TESTS=ON -> OK
cmake --build build -> FAIL, 12 errors
include/eos_image.h:135:23: error: 'eos_image_header_t' has no member named 'reserved'
include/eos_image.h:142:55: error: 'eos_image_header_t' has no member named 'reserved'
gcc -c -Iinclude core/ed25519_verify.c -> FAIL
core/ed25519_verify.c:338:12: error: redefinition of 'point_is_identity'
(previous definition at core/ed25519_verify.c:281)
81bfe0b7 : cmake -B build ... -> OK
cmake --build build -j4 -> OK
ctest --no-tests=error -> 22/22 passed
cmake -B build-vg ... -DVALGRIND=/usr/bin/true -> FAIL (finding 1)
So origin/master is red on two counts today and this branch is the thing that makes it green again — worth saying plainly, because finding 1 is the only reason it cannot land as it stands.
Separately: tests/unit/test_ed25519.c on origin/master hard-codes tests_run = 11 while main() makes 13 run_* calls (one of them duplicated), and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery is defined but never called. This PR fixes both by counting in the TEST macro. That is a real repair, not bookkeeping — a suite that reports its own total cannot detect a test it stopped running.
Automated architecture review of 81bfe0b76410 — 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.
Correcting the body — two claims no longer reproduceFinding 5 is right, and both are my own stale evidence rather than a change in the code. " "Added the one line that builds it" — likewise no longer this PRs doing. " The current state, run just now on this head: On finding 1 — step 4Fixed in the latest commit, and it is the more important half of this PR now. A non- There is no test for those two refusals, and the file says so rather than leaving it to be found. Reaching step 4 requires passing step 3, which is a real Ed25519 signature checked against the keystore. I wrote the obvious test first and it was worthless: with The fixture is buildable — the keystore ships RFC 8032 TEST 1s public key and the matching private key is in the RFC — but that machinery is #88s ( Findings 3 and 4 are done here ( |
3 participants
cfg.lock_debugasks for SWD/JTAG to be closed before the verified image runs. Step 7 ofeos_secure_boot()honoured that request like this:eos_secure_boot_lock_debug()returned void and discarded the result of the OTP write that actually blows the fuse. When that write failed, boot continued, attestation recordedEOS_SBOOT_OK, and the device ran the image with its debug port open — the exact condition the policy exists to prevent, reported as a clean secure boot.The interesting case is not a flaky fuse
eos_hal_otp_write()returnsEOS_ERR_NOT_SUPPORTEDwhen the board provides nootp_writehook at all. So on every such board,lock_debug: truewas silently a no-op — that is the default configuration, not an edge case.The change
eos_secure_boot_lock_debug()returnsint, and a caller that asked for the lock and did not get it now fails withEOS_SBOOT_ERR_POLICY— a code that already existed for exactly this ("Boot policy violation") — with the failure recorded in the attestation log instead of a success.Why this was never observable
core/secure_boot.cis not inCMakeLists.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 #72 adds, written identically, so whichever lands first leaves the other a trivial rebase.
Tests
tests/unit/test_secure_boot_policy.ccovers the three outcomes: fuse written, write failing, and a board with nootp_write. Kept in its own file so it does not collide with thetest_secure_boot.cthat #72 introduces.Against
masterthe new test does not compile:because there is no result to check. That is the defect stated as a compile error rather than a runtime one.
Verification
ctest --no-tests=errorpytest tests/Stacked on #77 — master's test suite does not compile without it (
test_image_verify.clost a line continuation in #70).Relationship to #72
Complementary, not competing. #72 builds the module and stops it booting plaintext; this fixes a separate fail-open inside the same function's policy handling. The only textual overlap is the one-line
CMakeLists.txtaddition, deliberately identical.🤖 Generated with Claude Code