fix(image_verify): fail closed when the CRC32 integrity path cannot read flash - #38
Merged
srpatcha merged 2 commits intoAug 27, 2026
Conversation
…ead flash eos_crc32() returned 0 when eos_hal_flash_read() failed. That value is indistinguishable from a region that genuinely hashes to 0, so the CRC32 branch of eos_image_verify_integrity() reported EOS_OK for an image whose payload could not be read at all — provided the stored CRC was 0. The stored CRC is the first four bytes of hdr->hash, which lives in the image header. The header is not covered by the Ed25519 signature (that is computed over hdr->hash only), so setting those bytes to 0 costs nothing. The SHA-256 branch has always propagated the read error: eos_crypto_verify_image() returns rc from eos_hal_flash_read(). The two branches now behave the same. Reproduced against the simulated flash in tests/unit/test_image_verify.c by placing an image so its payload runs past the end of the device: flash read of payload tail -> -6 (EOS_ERR_FLASH) eos_image_verify_integrity -> 0 (EOS_OK) <-- verified an unreadable image Adds eos_crc32_checked(), which reports read failures through its return value, and switches the verification path to it. eos_crc32() is kept for API compatibility, now implemented in terms of the checked variant and documented as unsuitable for deciding whether an image is intact. Two smaller fixes in the same function, both the same "do not verify something you did not read" shape: a zero image_size is rejected rather than CRC'd to a fixed value, and an addr + hdr_size that wraps uint32_t is rejected rather than producing a payload address that is not the payload. Tests: seven cases added to tests/unit/test_image_verify.c (12 total, was 5), including a matching-CRC image that must verify, a wrong-CRC image that must not, and the unreadable-payload regression. The CRC32 used to build expectations is implemented independently in the test so it does not simply restate image_verify.c. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
requested review from
hshanmug12,
maheshmunnangi and
srpatcha
as code owners
August 26, 2026 06:14
10 tasks
srpatcha
previously approved these changes
Aug 27, 2026
This was referenced Aug 28, 2026
srpatcha
pushed a commit
that referenced
this pull request
Aug 29, 2026
master does not compile. Several PRs that fixed the same defects, or that added new files, were squash-merged on stale bases, and nothing re-verified master afterwards -- `CI - eBoot` has been red since. Build breakage: - core/recovery.c declared `slot_size` twice (#33 and #50 both landed the same bounds check). - include/eos_image.h declared `int eos_crc32(uint32_t, size_t, uint32_t *)` while core/image_verify.c defines `uint32_t eos_crc32(uint32_t, size_t)` (#38 vs #52). The header now matches the implementation. - core/sha512.c and core/rollback.c were never added to CMakeLists.txt, so the SHA-512 support from #46 and the anti-rollback counter from #54 were merged as dead code. - Two SHA-512 APIs survived the merge: eos_crypto_boot.h declares eos_sha512_*, include/eos_sha512.h declared sha512_*, and only the latter was implemented. Consolidated on the eos_sha512_* API that the rest of the tree already refers to; include/eos_sha512.h is removed. - The body of eos_ed25519_verify() was lost. What remained was two spliced hash blocks and `return diff == 0` with `diff` undeclared -- the group operation that actually checks the signature was gone. Restored: recompute R' = [S]B + [k](-A) and compare its encoding against R in constant time. - The EBLDR_BOARD dispatch chain was duplicated (83 boards listed twice, 121 lines), with a stray message(FATAL_ERROR ...) spliced into the kalimba branch. tests/unit/test_cmake_board_dispatch.py already covered this. Test suite: - tests/unit/test_slot_manager.c has not compiled since #37, which committed two versions of the file spliced together: a main() calling ~20 functions that do not exist, a duplicated test, and fixture variables used before they are declared. Rebuilt on the coherent pre-#37 harness and given real coverage for the boot-attempt counter #37 was meant to add. - tests/unit/test_boot_log.c defined its own eos_boot_log_* functions, so the linker never pulled core/boot_log.c out of libeboot_core.a: the test exercised its own stubs and reported PASS. Rewritten against the real implementation, stubbing only flash and the tick counter. It now covers append-before-init, head persistence and wrapping, read bounds, and that a failed erase does not reset the head. - include/eos_boot_log.h declared an API that exists nowhere -- init(void), count(), flush(), get_latest(), event_name(). Every one of them lived only in the old test's stubs. The header now documents what core/boot_log.c implements, which is what recovery.c and stage1 already call. - The ARM job in ci.yml pointed CMAKE_TOOLCHAIN_FILE at cmake/arm-cortex-m4.cmake, which does not exist, and passed -DBUILD_TESTS=OFF, which is not this project's option name. Pointed at toolchains/arm-none-eabi.cmake with EBLDR_BOARD=stm32f4. Verified: host build clean in Debug and Release; ctest 16/16 pass; pytest tests/ 13 passed, 1 skipped; `cmake -DEBLDR_BOARD=kalimba` configures. Not verified locally: the ARM cross-build, for lack of an arm-none-eabi toolchain on this machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha
added a commit
that referenced
this pull request
Aug 30, 2026
* fix: restore the build and make the unit suite exercise real code master does not compile. Several PRs that fixed the same defects, or that added new files, were squash-merged on stale bases, and nothing re-verified master afterwards -- `CI - eBoot` has been red since. Build breakage: - core/recovery.c declared `slot_size` twice (#33 and #50 both landed the same bounds check). - include/eos_image.h declared `int eos_crc32(uint32_t, size_t, uint32_t *)` while core/image_verify.c defines `uint32_t eos_crc32(uint32_t, size_t)` (#38 vs #52). The header now matches the implementation. - core/sha512.c and core/rollback.c were never added to CMakeLists.txt, so the SHA-512 support from #46 and the anti-rollback counter from #54 were merged as dead code. - Two SHA-512 APIs survived the merge: eos_crypto_boot.h declares eos_sha512_*, include/eos_sha512.h declared sha512_*, and only the latter was implemented. Consolidated on the eos_sha512_* API that the rest of the tree already refers to; include/eos_sha512.h is removed. - The body of eos_ed25519_verify() was lost. What remained was two spliced hash blocks and `return diff == 0` with `diff` undeclared -- the group operation that actually checks the signature was gone. Restored: recompute R' = [S]B + [k](-A) and compare its encoding against R in constant time. - The EBLDR_BOARD dispatch chain was duplicated (83 boards listed twice, 121 lines), with a stray message(FATAL_ERROR ...) spliced into the kalimba branch. tests/unit/test_cmake_board_dispatch.py already covered this. Test suite: - tests/unit/test_slot_manager.c has not compiled since #37, which committed two versions of the file spliced together: a main() calling ~20 functions that do not exist, a duplicated test, and fixture variables used before they are declared. Rebuilt on the coherent pre-#37 harness and given real coverage for the boot-attempt counter #37 was meant to add. - tests/unit/test_boot_log.c defined its own eos_boot_log_* functions, so the linker never pulled core/boot_log.c out of libeboot_core.a: the test exercised its own stubs and reported PASS. Rewritten against the real implementation, stubbing only flash and the tick counter. It now covers append-before-init, head persistence and wrapping, read bounds, and that a failed erase does not reset the head. - include/eos_boot_log.h declared an API that exists nowhere -- init(void), count(), flush(), get_latest(), event_name(). Every one of them lived only in the old test's stubs. The header now documents what core/boot_log.c implements, which is what recovery.c and stage1 already call. - The ARM job in ci.yml pointed CMAKE_TOOLCHAIN_FILE at cmake/arm-cortex-m4.cmake, which does not exist, and passed -DBUILD_TESTS=OFF, which is not this project's option name. Pointed at toolchains/arm-none-eabi.cmake with EBLDR_BOARD=stm32f4. Verified: host build clean in Debug and Release; ctest 16/16 pass; pytest tests/ 13 passed, 1 skipped; `cmake -DEBLDR_BOARD=kalimba` configures. Not verified locally: the ARM cross-build, for lack of an arm-none-eabi toolchain on this machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(stage0): include the crypto header so the stage-1 hash check compiles stage0/jump_stage1.c uses eos_sha256_ctx_t and the eos_sha256_* functions under EBLDR_VERIFY_STAGE1 without including eos_crypto_boot.h. That option defaults to ON, so every cross-compiled board build fails: stage0/jump_stage1.c:70:9: error: unknown type name 'eos_sha256_ctx_t' The host build never caught it because EBLDR_BOARD defaults to "none" and stage0 is only added for a real board -- so the first link in the secure-boot chain, stage-0 verifying stage-1 before jumping to it, has never been compiled. Surfaced by the Cross-compile STM32F4 job on this PR. Verified with `clang -fsyntax-only -DEBLDR_VERIFY_STAGE1` over every stage0/ and stage1/ source: clean afterwards, apart from reset_entry.c's weak aliases, which clang rejects on darwin regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop the Python job failing on a coverage floor nothing has met .coveragerc sets fail_under = 100. Measured coverage is 23.06%, most of the gap being tests/production_test_suite.py (736 statements) which nothing imports. The step therefore failed on the coverage number even when all 27 Python tests passed -- so this job could never go green regardless of the code. ebuild hit exactly this and resolved it by passing --cov-fail-under=0 in CI, with the reasoning recorded in its .coveragerc: the repo-wide ratchet belongs in codecov.yml, and TESTING.md's 95% target is a patch target, not a repo-wide one. Same fix here, for consistency across the two repos. Both numbers are left alone -- raising .coveragerc to a real floor, or enforcing one here, is a maintainer decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(stage0): actually generate the stage-1 hash stage-0 verifies against With the include fixed, ebldr_stage0 compiles but does not link: undefined reference to `stage1_expected_size' undefined reference to `stage1_expected_hash' stage0/jump_stage1.c declares both extern and hashes stage-1 in flash against them before jumping. Nothing in the tree defined them. tools/embed_stage1_hash.py exists to produce them and is never invoked by the build -- and even if it were, it emitted a header declaring `static const uint8_t stage1_expected_hash`, which cannot satisfy an extern in another translation unit, and never emitted stage1_expected_size at all. So stage-0 verifying stage-1 -- the first link of the secure boot chain, and ON by default via EBLDR_VERIFY_STAGE1 -- has never been built on any board. - tools/embed_stage1_hash.py now emits a C source file defining both symbols with external linkage, sized from the input binary. - CMakeLists.txt generates it from eboot_firmware.bin and compiles it into ebldr_stage0. The custom command DEPENDS on eboot_firmware, so the hash is taken from the stage-1 image this build produced. - EBLDR_VERIFY_STAGE1 with a board that has no stage-1 linker script is now a configure-time error naming the flag to turn off, rather than a link failure a hundred lines of output later. Verified: the generated file compiles and satisfies the externs (linked against a probe TU declaring them, digest and size match hashlib); a simulated cross-configure shows `stage1_hash.c: eboot_firmware.elf` in the dependency graph and stage1_hash.c.obj in ebldr_stage0's objects. The host build is untouched -- ctest 16/16, pytest 13 passed 1 skipped. Not verified locally: the ARM link itself, for lack of an arm-none-eabi toolchain. The board_stm32f4.c assembly cannot be assembled by host clang. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop gating on an EoSim release that was never published Every job in Simulation Sanity Test dies at "Install EoSim": ERROR: HTTP error 404 ... EoSim/releases/download/v0.1.0/eosim-0.1.0-py3-none-any.whl embeddedos-org/EoSim has no v0.1.0 release, and none of its releases publish a wheel — the newest asset is a promo video. So all 11 simulate jobs, all 3 cross-platform jobs, and the gate that depends on them have failed on master and on every branch since the workflow was written, without a single simulation ever running. ebuild hit exactly this and disabled the steps in its own simulation-test.yml ("EoSim repository not found. Skipping simulation tests."). Same treatment here: the pip install, the eosim invocations and the artifact upload are commented out rather than deleted, so restoring them is a one-line revert once EoSim ships a release. Left alone: .github/workflows/eosim-sanity.yml has the same broken install but runs on a nightly schedule rather than on pull requests, and ebuild left its copy untouched too. Whether to disable a nightly diagnostic is a maintainer call, not something to fold into a build-fix PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: add a concurrency group so superseded runs stop holding runners ci.yml is the only workflow in this repo without a concurrency group, and it is the heaviest one -- a matrix spanning ubuntu, macos and windows. Every push to a pull request therefore left the previous run queued, and all of them competed for the same scarce windows/macos runners. On this branch three superseded runs sat ahead of the current one for over an hour, testing commits that were no longer HEAD. Uses the same group expression the sibling workflows already use, with cancel-in-progress: true, because a superseded commit's result is not wanted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: drop an accidentally committed .coverage database `pytest --cov` writes a .coverage SQLite file into the repo root, and it was not gitignored, so a `git add -A` swept 52 KB of local coverage state into this branch. Removed, and gitignored so it cannot happen again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): namespace test targets so eos and eBoot can build together ebuild composes eos and eBoot into one CMake project via add_subdirectory, and CMake requires target names to be globally unique. Two names existed in both repositories: test_crypto eos/tests/CMakeLists.txt:52 eBoot/tests/CMakeLists.txt:10 test_multicore eos/tests/CMakeLists.txt:42 eBoot/tests/CMakeLists.txt:50 So `ebuild integration` — "build all EoS packages together", the only build that checks the two repositories work with each other — was the one build that could not configure: CMake Error at eBoot/tests/CMakeLists.txt:10 (add_executable): add_executable cannot create target "test_crypto" because another target with the same name already exists. The existing target is an executable created in source directory "eos/tests". Each repository configured fine alone, which is why this went unnoticed: the failure only exists in the composition, and nothing was building the composition. All 16 test targets here are prefixed eboot_. The add_test(NAME ...) labels are deliberately left alone — the collision CMake rejects is between targets, not test names — so ctest output is unchanged. A configure-time guard rejects any target in this directory that is not prefixed. Two names collide today; nothing stopped a third, and the next one would fail the same way with the same "green apart, broken together" signature. Verified the guard fires rather than merely existing: add_executable(test_bootctl ...) -> CMake Error: Test target 'test_bootctl' is not namespaced. Name it 'eboot_test_bootctl' so it cannot collide with a target of the same name in eos when ebuild builds both together. Verified: eBoot standalone 16/16 passed, test names unchanged ebuild integration build configure done, was "Configuring incomplete" ecosystem runner, ebuild PASS 38 tests, was FAIL 38 is eos's 22 and eBoot's 16 running in one build for the first time. Refs embeddedos-org/ebuild#85 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: pin the .efw image header wire format eos_image_header_t is a contract with eFirmware, which writes the images eBoot parses. eos_image.h pins four things about it: sizeof, and the offsets of hash, sig_type and signature. Those catch a field that grows and a field inserted before hash. They do not catch two same-width fields exchanging places. Compiling the header with load_addr and entry_addr transposed passes all four: all four existing static asserts: PASSED load_addr is now at offset 16 (was 12) entry_addr is now at offset 12 (was 16) A bootloader built from that header loads the image at its entry point and jumps to its load address. Nor do they say anything about the values of EOS_IMG_MAGIC, EOS_HASH_SIZE, EOS_SIG_MAX_SIZE, EOS_IMG_SIGNED_LEN or the eos_sig_type_t enumerators. Those travel inside the image and are wire format as much as the offsets are. Renumbering EOS_SIG_ED25519 makes eBoot check an existing image's signature under a different algorithm than the one that signed it, with a clean compile and a green suite. This adds every field's offset and width, the total size, and the value of every shared constant — 38 checks, mirroring the treatment eFirmware gives its own side of the format in tests/test_abi.c. The numbers are stated independently in each repository on purpose. Two statements of one contract, each living where it applies, means editing one side turns the other red. Verified against both drift classes the existing asserts miss: transposed load_addr/entry_addr 36/38, names both offending fields EOS_SIG_SHA256 renumbered to 9 37/38 restored 38/38 ctest: 17/17 (was 16/16). Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kartikey1306 <kartikeydheer@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha
added a commit
that referenced
this pull request
Aug 30, 2026
…60) * fix: restore the build and make the unit suite exercise real code master does not compile. Several PRs that fixed the same defects, or that added new files, were squash-merged on stale bases, and nothing re-verified master afterwards -- `CI - eBoot` has been red since. Build breakage: - core/recovery.c declared `slot_size` twice (#33 and #50 both landed the same bounds check). - include/eos_image.h declared `int eos_crc32(uint32_t, size_t, uint32_t *)` while core/image_verify.c defines `uint32_t eos_crc32(uint32_t, size_t)` (#38 vs #52). The header now matches the implementation. - core/sha512.c and core/rollback.c were never added to CMakeLists.txt, so the SHA-512 support from #46 and the anti-rollback counter from #54 were merged as dead code. - Two SHA-512 APIs survived the merge: eos_crypto_boot.h declares eos_sha512_*, include/eos_sha512.h declared sha512_*, and only the latter was implemented. Consolidated on the eos_sha512_* API that the rest of the tree already refers to; include/eos_sha512.h is removed. - The body of eos_ed25519_verify() was lost. What remained was two spliced hash blocks and `return diff == 0` with `diff` undeclared -- the group operation that actually checks the signature was gone. Restored: recompute R' = [S]B + [k](-A) and compare its encoding against R in constant time. - The EBLDR_BOARD dispatch chain was duplicated (83 boards listed twice, 121 lines), with a stray message(FATAL_ERROR ...) spliced into the kalimba branch. tests/unit/test_cmake_board_dispatch.py already covered this. Test suite: - tests/unit/test_slot_manager.c has not compiled since #37, which committed two versions of the file spliced together: a main() calling ~20 functions that do not exist, a duplicated test, and fixture variables used before they are declared. Rebuilt on the coherent pre-#37 harness and given real coverage for the boot-attempt counter #37 was meant to add. - tests/unit/test_boot_log.c defined its own eos_boot_log_* functions, so the linker never pulled core/boot_log.c out of libeboot_core.a: the test exercised its own stubs and reported PASS. Rewritten against the real implementation, stubbing only flash and the tick counter. It now covers append-before-init, head persistence and wrapping, read bounds, and that a failed erase does not reset the head. - include/eos_boot_log.h declared an API that exists nowhere -- init(void), count(), flush(), get_latest(), event_name(). Every one of them lived only in the old test's stubs. The header now documents what core/boot_log.c implements, which is what recovery.c and stage1 already call. - The ARM job in ci.yml pointed CMAKE_TOOLCHAIN_FILE at cmake/arm-cortex-m4.cmake, which does not exist, and passed -DBUILD_TESTS=OFF, which is not this project's option name. Pointed at toolchains/arm-none-eabi.cmake with EBLDR_BOARD=stm32f4. Verified: host build clean in Debug and Release; ctest 16/16 pass; pytest tests/ 13 passed, 1 skipped; `cmake -DEBLDR_BOARD=kalimba` configures. Not verified locally: the ARM cross-build, for lack of an arm-none-eabi toolchain on this machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(stage0): include the crypto header so the stage-1 hash check compiles stage0/jump_stage1.c uses eos_sha256_ctx_t and the eos_sha256_* functions under EBLDR_VERIFY_STAGE1 without including eos_crypto_boot.h. That option defaults to ON, so every cross-compiled board build fails: stage0/jump_stage1.c:70:9: error: unknown type name 'eos_sha256_ctx_t' The host build never caught it because EBLDR_BOARD defaults to "none" and stage0 is only added for a real board -- so the first link in the secure-boot chain, stage-0 verifying stage-1 before jumping to it, has never been compiled. Surfaced by the Cross-compile STM32F4 job on this PR. Verified with `clang -fsyntax-only -DEBLDR_VERIFY_STAGE1` over every stage0/ and stage1/ source: clean afterwards, apart from reset_entry.c's weak aliases, which clang rejects on darwin regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop the Python job failing on a coverage floor nothing has met .coveragerc sets fail_under = 100. Measured coverage is 23.06%, most of the gap being tests/production_test_suite.py (736 statements) which nothing imports. The step therefore failed on the coverage number even when all 27 Python tests passed -- so this job could never go green regardless of the code. ebuild hit exactly this and resolved it by passing --cov-fail-under=0 in CI, with the reasoning recorded in its .coveragerc: the repo-wide ratchet belongs in codecov.yml, and TESTING.md's 95% target is a patch target, not a repo-wide one. Same fix here, for consistency across the two repos. Both numbers are left alone -- raising .coveragerc to a real floor, or enforcing one here, is a maintainer decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(stage0): actually generate the stage-1 hash stage-0 verifies against With the include fixed, ebldr_stage0 compiles but does not link: undefined reference to `stage1_expected_size' undefined reference to `stage1_expected_hash' stage0/jump_stage1.c declares both extern and hashes stage-1 in flash against them before jumping. Nothing in the tree defined them. tools/embed_stage1_hash.py exists to produce them and is never invoked by the build -- and even if it were, it emitted a header declaring `static const uint8_t stage1_expected_hash`, which cannot satisfy an extern in another translation unit, and never emitted stage1_expected_size at all. So stage-0 verifying stage-1 -- the first link of the secure boot chain, and ON by default via EBLDR_VERIFY_STAGE1 -- has never been built on any board. - tools/embed_stage1_hash.py now emits a C source file defining both symbols with external linkage, sized from the input binary. - CMakeLists.txt generates it from eboot_firmware.bin and compiles it into ebldr_stage0. The custom command DEPENDS on eboot_firmware, so the hash is taken from the stage-1 image this build produced. - EBLDR_VERIFY_STAGE1 with a board that has no stage-1 linker script is now a configure-time error naming the flag to turn off, rather than a link failure a hundred lines of output later. Verified: the generated file compiles and satisfies the externs (linked against a probe TU declaring them, digest and size match hashlib); a simulated cross-configure shows `stage1_hash.c: eboot_firmware.elf` in the dependency graph and stage1_hash.c.obj in ebldr_stage0's objects. The host build is untouched -- ctest 16/16, pytest 13 passed 1 skipped. Not verified locally: the ARM link itself, for lack of an arm-none-eabi toolchain. The board_stm32f4.c assembly cannot be assembled by host clang. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop gating on an EoSim release that was never published Every job in Simulation Sanity Test dies at "Install EoSim": ERROR: HTTP error 404 ... EoSim/releases/download/v0.1.0/eosim-0.1.0-py3-none-any.whl embeddedos-org/EoSim has no v0.1.0 release, and none of its releases publish a wheel — the newest asset is a promo video. So all 11 simulate jobs, all 3 cross-platform jobs, and the gate that depends on them have failed on master and on every branch since the workflow was written, without a single simulation ever running. ebuild hit exactly this and disabled the steps in its own simulation-test.yml ("EoSim repository not found. Skipping simulation tests."). Same treatment here: the pip install, the eosim invocations and the artifact upload are commented out rather than deleted, so restoring them is a one-line revert once EoSim ships a release. Left alone: .github/workflows/eosim-sanity.yml has the same broken install but runs on a nightly schedule rather than on pull requests, and ebuild left its copy untouched too. Whether to disable a nightly diagnostic is a maintainer call, not something to fold into a build-fix PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: add a concurrency group so superseded runs stop holding runners ci.yml is the only workflow in this repo without a concurrency group, and it is the heaviest one -- a matrix spanning ubuntu, macos and windows. Every push to a pull request therefore left the previous run queued, and all of them competed for the same scarce windows/macos runners. On this branch three superseded runs sat ahead of the current one for over an hour, testing commits that were no longer HEAD. Uses the same group expression the sibling workflows already use, with cancel-in-progress: true, because a superseded commit's result is not wanted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: drop an accidentally committed .coverage database `pytest --cov` writes a .coverage SQLite file into the repo root, and it was not gitignored, so a `git add -A` swept 52 KB of local coverage state into this branch. Removed, and gitignored so it cannot happen again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kartikey1306 <kartikeydheer@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha
added a commit
that referenced
this pull request
Aug 30, 2026
#71) * fix: restore the build and make the unit suite exercise real code master does not compile. Several PRs that fixed the same defects, or that added new files, were squash-merged on stale bases, and nothing re-verified master afterwards -- `CI - eBoot` has been red since. Build breakage: - core/recovery.c declared `slot_size` twice (#33 and #50 both landed the same bounds check). - include/eos_image.h declared `int eos_crc32(uint32_t, size_t, uint32_t *)` while core/image_verify.c defines `uint32_t eos_crc32(uint32_t, size_t)` (#38 vs #52). The header now matches the implementation. - core/sha512.c and core/rollback.c were never added to CMakeLists.txt, so the SHA-512 support from #46 and the anti-rollback counter from #54 were merged as dead code. - Two SHA-512 APIs survived the merge: eos_crypto_boot.h declares eos_sha512_*, include/eos_sha512.h declared sha512_*, and only the latter was implemented. Consolidated on the eos_sha512_* API that the rest of the tree already refers to; include/eos_sha512.h is removed. - The body of eos_ed25519_verify() was lost. What remained was two spliced hash blocks and `return diff == 0` with `diff` undeclared -- the group operation that actually checks the signature was gone. Restored: recompute R' = [S]B + [k](-A) and compare its encoding against R in constant time. - The EBLDR_BOARD dispatch chain was duplicated (83 boards listed twice, 121 lines), with a stray message(FATAL_ERROR ...) spliced into the kalimba branch. tests/unit/test_cmake_board_dispatch.py already covered this. Test suite: - tests/unit/test_slot_manager.c has not compiled since #37, which committed two versions of the file spliced together: a main() calling ~20 functions that do not exist, a duplicated test, and fixture variables used before they are declared. Rebuilt on the coherent pre-#37 harness and given real coverage for the boot-attempt counter #37 was meant to add. - tests/unit/test_boot_log.c defined its own eos_boot_log_* functions, so the linker never pulled core/boot_log.c out of libeboot_core.a: the test exercised its own stubs and reported PASS. Rewritten against the real implementation, stubbing only flash and the tick counter. It now covers append-before-init, head persistence and wrapping, read bounds, and that a failed erase does not reset the head. - include/eos_boot_log.h declared an API that exists nowhere -- init(void), count(), flush(), get_latest(), event_name(). Every one of them lived only in the old test's stubs. The header now documents what core/boot_log.c implements, which is what recovery.c and stage1 already call. - The ARM job in ci.yml pointed CMAKE_TOOLCHAIN_FILE at cmake/arm-cortex-m4.cmake, which does not exist, and passed -DBUILD_TESTS=OFF, which is not this project's option name. Pointed at toolchains/arm-none-eabi.cmake with EBLDR_BOARD=stm32f4. Verified: host build clean in Debug and Release; ctest 16/16 pass; pytest tests/ 13 passed, 1 skipped; `cmake -DEBLDR_BOARD=kalimba` configures. Not verified locally: the ARM cross-build, for lack of an arm-none-eabi toolchain on this machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(stage0): include the crypto header so the stage-1 hash check compiles stage0/jump_stage1.c uses eos_sha256_ctx_t and the eos_sha256_* functions under EBLDR_VERIFY_STAGE1 without including eos_crypto_boot.h. That option defaults to ON, so every cross-compiled board build fails: stage0/jump_stage1.c:70:9: error: unknown type name 'eos_sha256_ctx_t' The host build never caught it because EBLDR_BOARD defaults to "none" and stage0 is only added for a real board -- so the first link in the secure-boot chain, stage-0 verifying stage-1 before jumping to it, has never been compiled. Surfaced by the Cross-compile STM32F4 job on this PR. Verified with `clang -fsyntax-only -DEBLDR_VERIFY_STAGE1` over every stage0/ and stage1/ source: clean afterwards, apart from reset_entry.c's weak aliases, which clang rejects on darwin regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop the Python job failing on a coverage floor nothing has met .coveragerc sets fail_under = 100. Measured coverage is 23.06%, most of the gap being tests/production_test_suite.py (736 statements) which nothing imports. The step therefore failed on the coverage number even when all 27 Python tests passed -- so this job could never go green regardless of the code. ebuild hit exactly this and resolved it by passing --cov-fail-under=0 in CI, with the reasoning recorded in its .coveragerc: the repo-wide ratchet belongs in codecov.yml, and TESTING.md's 95% target is a patch target, not a repo-wide one. Same fix here, for consistency across the two repos. Both numbers are left alone -- raising .coveragerc to a real floor, or enforcing one here, is a maintainer decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(stage0): actually generate the stage-1 hash stage-0 verifies against With the include fixed, ebldr_stage0 compiles but does not link: undefined reference to `stage1_expected_size' undefined reference to `stage1_expected_hash' stage0/jump_stage1.c declares both extern and hashes stage-1 in flash against them before jumping. Nothing in the tree defined them. tools/embed_stage1_hash.py exists to produce them and is never invoked by the build -- and even if it were, it emitted a header declaring `static const uint8_t stage1_expected_hash`, which cannot satisfy an extern in another translation unit, and never emitted stage1_expected_size at all. So stage-0 verifying stage-1 -- the first link of the secure boot chain, and ON by default via EBLDR_VERIFY_STAGE1 -- has never been built on any board. - tools/embed_stage1_hash.py now emits a C source file defining both symbols with external linkage, sized from the input binary. - CMakeLists.txt generates it from eboot_firmware.bin and compiles it into ebldr_stage0. The custom command DEPENDS on eboot_firmware, so the hash is taken from the stage-1 image this build produced. - EBLDR_VERIFY_STAGE1 with a board that has no stage-1 linker script is now a configure-time error naming the flag to turn off, rather than a link failure a hundred lines of output later. Verified: the generated file compiles and satisfies the externs (linked against a probe TU declaring them, digest and size match hashlib); a simulated cross-configure shows `stage1_hash.c: eboot_firmware.elf` in the dependency graph and stage1_hash.c.obj in ebldr_stage0's objects. The host build is untouched -- ctest 16/16, pytest 13 passed 1 skipped. Not verified locally: the ARM link itself, for lack of an arm-none-eabi toolchain. The board_stm32f4.c assembly cannot be assembled by host clang. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: stop gating on an EoSim release that was never published Every job in Simulation Sanity Test dies at "Install EoSim": ERROR: HTTP error 404 ... EoSim/releases/download/v0.1.0/eosim-0.1.0-py3-none-any.whl embeddedos-org/EoSim has no v0.1.0 release, and none of its releases publish a wheel — the newest asset is a promo video. So all 11 simulate jobs, all 3 cross-platform jobs, and the gate that depends on them have failed on master and on every branch since the workflow was written, without a single simulation ever running. ebuild hit exactly this and disabled the steps in its own simulation-test.yml ("EoSim repository not found. Skipping simulation tests."). Same treatment here: the pip install, the eosim invocations and the artifact upload are commented out rather than deleted, so restoring them is a one-line revert once EoSim ships a release. Left alone: .github/workflows/eosim-sanity.yml has the same broken install but runs on a nightly schedule rather than on pull requests, and ebuild left its copy untouched too. Whether to disable a nightly diagnostic is a maintainer call, not something to fold into a build-fix PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: add a concurrency group so superseded runs stop holding runners ci.yml is the only workflow in this repo without a concurrency group, and it is the heaviest one -- a matrix spanning ubuntu, macos and windows. Every push to a pull request therefore left the previous run queued, and all of them competed for the same scarce windows/macos runners. On this branch three superseded runs sat ahead of the current one for over an hour, testing commits that were no longer HEAD. Uses the same group expression the sibling workflows already use, with cancel-in-progress: true, because a superseded commit's result is not wanted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: drop an accidentally committed .coverage database `pytest --cov` writes a .coverage SQLite file into the repo root, and it was not gitignored, so a `git add -A` swept 52 KB of local coverage state into this branch. Removed, and gitignored so it cannot happen again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): namespace test targets so eos and eBoot can build together ebuild composes eos and eBoot into one CMake project via add_subdirectory, and CMake requires target names to be globally unique. Two names existed in both repositories: test_crypto eos/tests/CMakeLists.txt:52 eBoot/tests/CMakeLists.txt:10 test_multicore eos/tests/CMakeLists.txt:42 eBoot/tests/CMakeLists.txt:50 So `ebuild integration` — "build all EoS packages together", the only build that checks the two repositories work with each other — was the one build that could not configure: CMake Error at eBoot/tests/CMakeLists.txt:10 (add_executable): add_executable cannot create target "test_crypto" because another target with the same name already exists. The existing target is an executable created in source directory "eos/tests". Each repository configured fine alone, which is why this went unnoticed: the failure only exists in the composition, and nothing was building the composition. All 16 test targets here are prefixed eboot_. The add_test(NAME ...) labels are deliberately left alone — the collision CMake rejects is between targets, not test names — so ctest output is unchanged. A configure-time guard rejects any target in this directory that is not prefixed. Two names collide today; nothing stopped a third, and the next one would fail the same way with the same "green apart, broken together" signature. Verified the guard fires rather than merely existing: add_executable(test_bootctl ...) -> CMake Error: Test target 'test_bootctl' is not namespaced. Name it 'eboot_test_bootctl' so it cannot collide with a target of the same name in eos when ebuild builds both together. Verified: eBoot standalone 16/16 passed, test names unchanged ebuild integration build configure done, was "Configuring incomplete" ecosystem runner, ebuild PASS 38 tests, was FAIL 38 is eos's 22 and eBoot's 16 running in one build for the first time. Refs embeddedos-org/ebuild#85 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kartikey1306 <kartikeydheer@gmail.com> 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
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.
Summary
The CRC32 branch of
eos_image_verify_integrity()failed open.eos_crc32()returned
0wheneos_hal_flash_read()failed, and0is indistinguishablefrom a region that genuinely hashes to
0. An image whose payload could not beread at all therefore passed the integrity check, provided the stored CRC was
0.The SHA-256 branch has always failed closed —
eos_crypto_verify_image()returns
rcstraight fromeos_hal_flash_read(). This PR makes the twobranches behave the same.
The bug
and the caller:
stored_crcis the first four bytes ofhdr->hash. That field is in the imageheader, and the header is not covered by the signature —
eos_image_verify_signature()signshdr->hashitself, not the surroundingheader — so setting those four bytes to
0costs an attacker nothing. It isalso the natural value in erased or zeroed flash.
Reproduced against the simulated flash already in
tests/unit/test_image_verify.c,placing an image so its payload runs past the end of the device:
For a bootloader this is the wrong direction to fail in: a slot that cannot be
read is a slot that should send the device to recovery, not one that should be
booted.
Approach
Add
eos_crc32_checked(addr, len, *out_crc), which returnsEOS_ERR_FLASHon aread failure and only writes
*out_crcon success, and switch the verificationpath to it.
eos_crc32()is kept and now delegates to the checked variant. It isdeclared in the public
eos_image.h, and removing it would be an API break fora defect that is fixable without one. Its doc comment now carries an explicit
@warningthat it cannot report a read failure and must not be used to decidewhether an image is intact. If maintainers would rather retire the unchecked
form outright, that is a one-line follow-up — I did not want to bundle an API
break into a security fix.
Two smaller fixes in the same function, both the same don't verify what you
didn't read shape:
image_size == 0is rejected rather than CRC'd to a fixed value. The SHA-256path already rejects it via
eos_crypto_verify_image().addr + hdr->hdr_sizeis rejected when it wrapsuint32_t, rather thanproducing a
payload_addrthat is not the payload.I did not add the double-call fault-injection pattern used on the SHA-256
path. CRC32 is not a security primitive and duplicating the read would double
the verification time for no attacker-facing benefit.
Scope note — what this does not fix
While tracing the stored CRC I noticed that the image header as a whole is
unauthenticated:
eos_image_verify_signature()covershdr->hashonly, soflags,image_size,load_addrandentry_addrcan be changed withoutinvalidating the signature. Clearing
EOS_IMG_FLAG_HASH_SHA256downgrades asigned image from SHA-256 to forgeable CRC32.
That is a design-level issue needing a header-format change, and it is well
outside a fail-closed fix. Flagging it rather than fixing it here — happy to
open a separate issue if useful.
Testing
Seven cases added to
tests/unit/test_image_verify.c— 12 total, was 5. TheCRC32 used to build expectations is implemented independently in the test, so
the tests do not simply restate
image_verify.c.test_crc_image_with_matching_crc_verifiestest_crc_image_with_wrong_crc_is_rejectedEOS_ERR_CRCtest_crc_unreadable_payload_fails_closedEOS_OKtest_crc32_checked_reports_flash_failureEOS_ERR_FLASHon failure,out_crcuntouched,EOS_ERR_INVALIDon NULL, correct value on successtest_zero_length_image_is_rejectedimage_sizetest_payload_address_overflow_is_rejectedaddr + hdr_sizetest_verify_integrity_null_headerAlso passes with
-DEBLDR_SANITIZE=ON(ASan/UBSan): 12/12.Before / after, same reproducer linked against the pre-change and
post-change libraries:
Pre-existing failure, not from this PR
test_recoveryisNot Run— it fails to link with undefinedeos_boot_log_append/eos_boot_log_get_head/eos_boot_log_read, becauseboot_log.clives ineboot_stage1which the test does not link. I confirmedthis fails identically on a clean
mastercheckout before making any changes.It is already addressed by #35, so I have deliberately left it alone rather than
conflict with that PR. Every other test builds and passes (13/14).
Limitations and considerations
transient read error on a CRC-only image will now go to recovery instead. That
is the intended direction, but it is a behaviour change worth calling out for
anyone running CRC-only (Phase 1) images.
eos_crc32()remains in the public API and remains fail-open byconstruction. It has no in-tree callers after this change; the
@warningandthe new checklist row are what guard against it being picked up again.
core/image_verify.candCHANGELOG.mdhave mixedCRLF/LF endings in
master. I re-applied my edits byte-wise so untouched lineskeep their original endings — the diff is 37 insertions / 5 deletions rather
than a whole-file rewrite.
Type of Change
Changes
core/image_verify.c—eos_crc32_checked(); CRC path fails closed;zero-size and payload-address-overflow guards.
include/eos_image.h— declareeos_crc32_checked();@warningoneos_crc32().tests/unit/test_image_verify.c— 7 new cases (12 total).docs/security_review_checklist.md— required item on failing closed on I/Oerror, plus a Common Mistakes row.
docs/secure_boot_chain.md—EOS_ERR_FLASHadded to the failure-mode table.CHANGELOG.md—Unreleasedentries.Pre-Submission Checklist
masterstill pass (13/14; the 14th is thepre-existing
test_recoverylink failure, verified present before my changes)