From f704d87fd445e4d61db00bc302e78660fc99f02f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:06:59 +0530 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20repair=20master=20=E2=80=94=20the=20?= =?UTF-8?q?ABI=20asserts=20and=20the=20Ed25519=20verifier=20both=20merged?= =?UTF-8?q?=20broken?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — #93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. #87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) #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 — #86 and #57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only #57's public_key_is_valid_subgroup() is wired to the call site, so #86's key_has_prime_order() was dead. Kept the live function, folded #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 #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. --- core/ed25519_verify.c | 56 ++++------------- include/eos_image.h | 16 +++-- tests/unit/test_ed25519.c | 126 ++++++++++++++++++++++++++++---------- 3 files changed, 116 insertions(+), 82 deletions(-) diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index d36cc7c..34aca77 100644 --- a/core/ed25519_verify.c +++ b/core/ed25519_verify.c @@ -289,6 +289,16 @@ static int point_is_identity(gf p[4]) return diff == 0; } +static void scalarbase(gf r[4], const uint8_t *s) +{ + gf q[4]; + fe_copy16(q[0], BX); + fe_copy16(q[1], BY); + fe_copy16(q[2], gf1); + fe_mul(q[3], BX, BY); + scalarmult(r, q, s); +} + /* Reject a public key outside the prime-order subgroup. * * Decoding a point is not enough. Ed25519 has eight points of low order, and @@ -304,51 +314,9 @@ static int point_is_identity(gf p[4]) * so there is no separate constant to transcribe wrongly: a mistyped L would * reject valid keys, and only in the field. * - * A arrives negated from unpackneg(). [L](-A) = -[L]A and the identity is its - * own negation, so neither condition is affected by the sign. - * - * Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached - * this before I did and states both conditions in one expression. + * The key arrives negated from unpackneg(). [L](-A) = -[L]A and the identity + * is its own negation, so neither condition is affected by the sign. */ -static int key_has_prime_order(gf A[4]) -{ - uint8_t order_l[32]; - gf q[4], multiple[4]; - int i; - - for (i = 0; i < 32; i++) - order_l[i] = (uint8_t)ORDER_L[i]; - for (i = 0; i < 4; i++) - fe_copy16(q[i], A[i]); - - scalarmult(multiple, q, order_l); - return point_is_identity(multiple) && !point_is_identity(A); -} - -static void scalarbase(gf r[4], const uint8_t *s) -{ - gf q[4]; - fe_copy16(q[0], BX); - fe_copy16(q[1], BY); - fe_copy16(q[2], gf1); - fe_mul(q[3], BX, BY); - scalarmult(r, q, s); -} - -static int point_is_identity(gf p[4]) -{ - uint8_t encoded[32]; - point_pack(encoded, p); - - uint8_t diff = (uint8_t)(encoded[0] ^ 1U); - for (int i = 1; i < 32; i++) - diff |= encoded[i]; - return diff == 0; -} - -/* Public keys must be non-identity points in Ed25519's prime-order subgroup. - * Merely decoding a point is insufficient: an identity or torsion key can - * make the verification equation true without knowledge of a private key. */ static int public_key_is_valid_subgroup(gf public_key[4]) { uint8_t order_l[32]; diff --git a/include/eos_image.h b/include/eos_image.h index 62744d0..24cbb16 100644 --- a/include/eos_image.h +++ b/include/eos_image.h @@ -108,7 +108,8 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) + /* Every remaining field, pinned. * - * Four of the fourteen fields were asserted. Transposing two adjacent + * Three of the thirteen field offsets were asserted (the fourth pre-existing + * assert is sizeof, which is not a field). Transposing two adjacent * same-width fields moves neither sizeof nor any of those four offsets, so it * compiled clean: with load_addr and entry_addr swapped, all four existing * asserts still passed and the bootloader would load an image at its entry @@ -132,15 +133,18 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, flags) == 24, "flags must stay at offset 24"); EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, sig_len) == 61, "sig_len must stay at offset 61"); -EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, reserved) == 62, - "reserved[] must stay at offset 62"); +/* tlv_len and tlv_hash are asserted above, where #93 introduced them; the + * 30 bytes they occupy are the ones this block used to pin as reserved[]. */ /* Field widths. An offset assert cannot see a field growing into padding that - * happens to keep every later offset -- reserved[] absorbs exactly that. */ + * happens to keep every later offset -- the 30 bytes at 62 absorb exactly + * that, which is why both halves of that span carry a width assert. */ EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->hash) == 32, "hash[] is 32 bytes on the wire"); -EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->reserved) == 30, - "reserved[] is 30 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_len) == 2, + "tlv_len is 2 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_hash) == 28, + "tlv_hash is 28 bytes on the wire"); EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->signature) == 64, "signature[] is 64 bytes on the wire"); diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f78012d..9ea837c 100644 --- a/tests/unit/test_ed25519.c +++ b/tests/unit/test_ed25519.c @@ -30,6 +30,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -211,29 +212,105 @@ TEST(test_ed25519_identity_key_forgery_rejected) msg, sizeof(msg) - 1) != EOS_OK); } +/* The eight canonical low-order point encodings. + * + * Every order was computed rather than copied: decoding each y, recovering x, + * and repeatedly adding the point until it reached the identity gives + * 1, 2, 4, 4, 8, 8, 8, 8 for the entries below in order. An earlier revision + * of this array held only five of them -- it omitted y=0 with the sign bit + * set and both sign-flipped order-8 encodings -- while its comment claimed to + * hold "the eight". [L](-A) = -[L]A, so the guard rejects a sign variant + * whether or not it is listed; the reason to list them is that this is the + * regression record for a secure-boot bypass, and a claimed class has to be + * the class it claims. */ +static const uint8_t k_low_order[8][32] = { + /* order 1: the identity, y = 1 */ + {0x01}, + /* order 2: y = -1 */ + {0xEC,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* order 4: y = 0, sign bit clear */ + {0x00}, + /* order 4: y = 0, sign bit set -- the encoding the earlier array missed */ + {0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x80}, + /* order 8 */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x05}, + /* order 8 */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0x7A}, + /* order 8: sign flip of the first order-8 entry -- also missing before */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x85}, + /* order 8: sign flip of the second -- also missing before */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0xFA}, +}; + +/* Not low-order points, and refused earlier and by a different mechanism: + * unpackneg() rejects them on canonicality or because no x exists. Kept + * separate so the array above means what its name says -- an earlier revision + * spent one of its eight slots on D9FF..FF, which does not decode at all. */ +static const uint8_t k_non_canonical[3][32] = { + /* y = p: reduces to 0, decodes as an order-4 point but is not canonical */ + {0xED,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* y = p + 1: reduces to the identity, likewise not canonical */ + {0xEE,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* no x satisfies the curve equation for this y: unpackneg() fails */ + {0xD9,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF}, +}; + +/* A low-order key forges for roughly one message in n, where n is its order, + * so a single fixed message would let a real bypass pass this suite. */ +static const char *const messages[] = { + "untrusted firmware", "v1.0.0", "", "a", "boot", "eos", "1234", "payload", +}; + TEST(test_ed25519_low_order_keys_rejected) { /* zero_pubkey covers one encoding; Ed25519 has eight low-order points and * the family is what matters. A subgroup test alone is not enough either: * the identity has order 1, which divides L, so [L]identity = identity and - * it passes. Both checks are required. */ - static const uint8_t low_order[4][32] = { - {0}, - {1}, - {0x26,0xe8,0x95,0x8f,0xc2,0xb2,0x27,0xb0,0x45,0xc3,0xf4,0x89,0xf2,0xef,0x98,0xf0, - 0xd5,0xdf,0xac,0x05,0xd3,0xc6,0x33,0x39,0xb1,0x38,0x02,0x88,0x6d,0x53,0xfc,0x05}, - {0xc7,0x17,0x6a,0x70,0x3d,0x4d,0xd8,0x4f,0xba,0x3c,0x0b,0x76,0x0d,0x10,0x67,0x0f, - 0x2a,0x20,0x53,0xfa,0x2c,0x39,0xcc,0xc6,0x4e,0xc7,0xfd,0x77,0x92,0xac,0x03,0x7a}, - }; - const uint8_t msg[] = "untrusted firmware"; + * it passes. Both checks are required. + * + * The sweep is every low-order encoding as the key against every one as R, + * over eight messages, because a low-order key of order n forges for + * roughly one message in n -- a single fixed message would let a genuine + * bypass through this test. Measured against 13a7a02, the last commit + * before the subgroup check: 16 of the 64 (key, R) pairs were accepted by + * at least one message. Here: none. */ + for (size_t k = 0; k < sizeof(k_low_order) / sizeof(k_low_order[0]); k++) { + for (size_t r = 0; r < sizeof(k_low_order) / sizeof(k_low_order[0]); r++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { + uint8_t sig[64]; + memset(sig, 0, sizeof(sig)); + memcpy(sig, k_low_order[r], 32); + ASSERT(eos_ed25519_verify(sig, k_low_order[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); + } + } + } +} - for (int k = 0; k < 4; k++) { - for (int r = 0; r < 4; r++) { +TEST(test_ed25519_non_canonical_encodings_rejected) +{ + /* These are refused before the subgroup check ever runs -- unpackneg() + * rejects them on canonicality, or because no x satisfies the curve + * equation. Pinned separately so that nobody deletes that path on the + * grounds that the subgroup test now covers it. It does not. */ + for (size_t k = 0; k < sizeof(k_non_canonical) / sizeof(k_non_canonical[0]); k++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { uint8_t sig[64]; memset(sig, 0, sizeof(sig)); - memcpy(sig, low_order[r], 32); - ASSERT(eos_ed25519_verify(sig, low_order[k], - msg, sizeof(msg) - 1) != EOS_OK); + memcpy(sig, k_non_canonical[k], 32); + ASSERT(eos_ed25519_verify(sig, k_non_canonical[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); } } } @@ -260,21 +337,6 @@ TEST(test_ed25519_zero_signature_rejected) ASSERT(eos_ed25519_verify(sig, pk, msg, 1) != EOS_OK); } -TEST(test_ed25519_identity_key_forgery_rejected) -{ - /* The identity point has compressed encoding 01 00...00. With both the - * public key and R set to the identity and S set to zero, the verification - * equation is true for every message unless low-order keys are rejected. */ - uint8_t identity_pub[32] = {1}; - uint8_t identity_sig[64] = {1}; - const uint8_t msg[] = "untrusted firmware"; - - ASSERT(eos_ed25519_verify(identity_sig, identity_pub, - msg, sizeof(msg) - 1) != EOS_OK); -} - -/* ---- SHA-512, the hash Ed25519 is defined over (FIPS 180-4) ---- */ - TEST(test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery) { /* The subgroup check guards the public key, not R, and that is @@ -367,13 +429,13 @@ int main(void) run_test_ed25519_null_args(); run_test_ed25519_identity_key_forgery_rejected(); run_test_ed25519_low_order_keys_rejected(); + run_test_ed25519_non_canonical_encodings_rejected(); + run_test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery(); run_test_ed25519_zero_pubkey_rejected(); run_test_ed25519_zero_signature_rejected(); - run_test_ed25519_identity_key_forgery_rejected(); run_test_sha512_known_answers(); run_test_sha512_streaming_matches_one_shot(); - tests_run = 11; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } From 93632f33b9190e7201638e6b7932dec90384f5e5 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 18:08:46 +0530 Subject: [PATCH 2/2] test: derive the suite totals and the Valgrind list instead of restating them Two findings arrived one after another in review, on different PRs, with the same shape underneath: a number or a list that describes what the tests do, written out separately from the thing it describes, with nothing checking the two agree. Fixing them one instance at a time was going to keep producing the same finding, so this fixes the class and adds the guard. **1. Suites stated a total instead of counting one.** The `TEST()` macro incremented `tests_passed`; `main()` assigned `tests_run = `. A test defined but never wired into `main()` was skipped with a zero exit, and the suite still reported "N/N passed" for an N that was a claim. 18 of 21 suites carried it. It was not theoretical: - `test_ed25519.c` had a hardcoded 11 that masked one test called twice and two never called at all (repaired in #94, which is where this started). - `test_tlv_auth.c` assigns `tests_run` twice in one function -- 8, then 7. The stale 8 survives only because the later assignment wins. Its suite reports 7/7 today by luck. - `test_secure_boot.c` counted nothing and ended `return 0`, so a suite that ran none of its cases still reported success. The ASSERT macro exits on failure, so that return could only ever have signalled the one case it ignored. `TEST()` now increments `tests_run` where it invokes the test, every literal assignment and every `%d/` in a summary is gone, and `test_secure_boot.c` compares and returns accordingly. **2. The Valgrind list named its suites again by hand**, and had drifted to 17 of 21 -- `test_ecc`, `test_rollback`, `test_secure_boot` and `test_storage` got no memory-safety run, and nothing failed when a name was forgotten. Each `add_test()` now appends to `EBLDR_UNIT_TESTS` and the `foreach` iterates that, so a suite added without touching the block still gets a Valgrind target. Confirmed with a stub `valgrind` on PATH: 21 `valgrind_*` tests are generated, up from 17. **3. tests/unit/test_suite_bookkeeping.py** keeps both from returning. Four checks, no C toolchain needed: no suite hardcodes its own total; every `TEST()` macro counts the test it runs; every defined test is actually called; and the Valgrind list is derived rather than repeated. Three suites with no `TEST()` macro are listed in `NO_TEST_MACRO` with a reason each. Verified: ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 42 passed valgrind targets, with a stub on PATH 21 (was 17) every suite's reported total now equals what it ran -- e.g. test_bootctl 12/12, test_tlv_auth 7/7, test_secure_boot 4/4 Each of the four guards was probed against the defect it is written for: reintroduce `tests_run = 12` -> test_bootctl.c: tests_run = 12 remove `tests_run++` from the macro -> FAIL stop calling a defined TEST() -> FAIL drop a suite from EBLDR_UNIT_TESTS -> FAIL and all four pass again on restore. Refs #94, #80, #82 --- tests/CMakeLists.txt | 41 ++++++++-- tests/unit/test_board_config.c | 2 +- tests/unit/test_board_registry.c | 2 +- tests/unit/test_boot_log.c | 2 +- tests/unit/test_bootctl.c | 2 +- tests/unit/test_crypto.c | 2 +- tests/unit/test_device_table.c | 2 +- tests/unit/test_fw_transport.c | 2 +- tests/unit/test_image_verify.c | 2 +- tests/unit/test_keystore.c | 2 +- tests/unit/test_multicore.c | 2 +- tests/unit/test_rollback.c | 2 +- tests/unit/test_runtime_svc.c | 2 +- tests/unit/test_secure_boot.c | 10 ++- tests/unit/test_slot_manager.c | 2 +- tests/unit/test_slot_size_bounds.c | 2 +- tests/unit/test_suite_bookkeeping.py | 112 +++++++++++++++++++++++++++ tests/unit/test_tlv_auth.c | 3 +- 18 files changed, 170 insertions(+), 24 deletions(-) create mode 100644 tests/unit/test_suite_bookkeeping.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..4f61cad 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,27 +4,36 @@ # --- test_bootctl: Boot control block --- add_executable(eboot_test_bootctl unit/test_bootctl.c) target_link_libraries(eboot_test_bootctl PRIVATE eboot_core) +# Every unit suite registered below appends its ctest name here, so the +# Valgrind block at the bottom derives its list instead of repeating it. +set(EBLDR_UNIT_TESTS "") + add_test(NAME test_bootctl COMMAND eboot_test_bootctl) +list(APPEND EBLDR_UNIT_TESTS test_bootctl) # --- test_crypto: SHA-256 against known vectors --- add_executable(eboot_test_crypto unit/test_crypto.c) target_link_libraries(eboot_test_crypto PRIVATE eboot_core) add_test(NAME test_crypto COMMAND eboot_test_crypto) +list(APPEND EBLDR_UNIT_TESTS test_crypto) # --- test_image_verify: Image header parse bounds --- add_executable(eboot_test_image_verify unit/test_image_verify.c) target_link_libraries(eboot_test_image_verify PRIVATE eboot_core) add_test(NAME test_image_verify COMMAND eboot_test_image_verify) +list(APPEND EBLDR_UNIT_TESTS test_image_verify) # --- test_image_abi: pins the .efw image header wire format against eFirmware --- add_executable(eboot_test_image_abi unit/test_image_abi.c) target_link_libraries(eboot_test_image_abi PRIVATE eboot_core) add_test(NAME test_image_abi COMMAND eboot_test_image_abi) +list(APPEND EBLDR_UNIT_TESTS test_image_abi) # --- test_secure_boot: Secure boot policy gates --- add_executable(eboot_test_secure_boot unit/test_secure_boot.c) target_link_libraries(eboot_test_secure_boot PRIVATE eboot_core) add_test(NAME test_secure_boot COMMAND eboot_test_secure_boot) +list(APPEND EBLDR_UNIT_TESTS test_secure_boot) # --- test_recovery: UART recovery write range --- # eboot_stage1 links eboot_core PUBLIC, so naming eboot_core here too put it on @@ -34,92 +43,112 @@ add_test(NAME test_secure_boot COMMAND eboot_test_secure_boot) add_executable(eboot_test_recovery unit/test_recovery.c) target_link_libraries(eboot_test_recovery PRIVATE eboot_stage1) add_test(NAME test_recovery COMMAND eboot_test_recovery) +list(APPEND EBLDR_UNIT_TESTS test_recovery) # --- test_fw_transport: UART raw/XMODEM/YMODEM firmware transport --- add_executable(eboot_test_fw_transport unit/test_fw_transport.c) target_link_libraries(eboot_test_fw_transport PRIVATE eboot_core) add_test(NAME test_fw_transport COMMAND eboot_test_fw_transport) +list(APPEND EBLDR_UNIT_TESTS test_fw_transport) # --- test_slot_size_bounds: verify_slot() must reject image_size > slot capacity --- add_executable(eboot_test_slot_size_bounds unit/test_slot_size_bounds.c) target_link_libraries(eboot_test_slot_size_bounds PRIVATE eboot_core) add_test(NAME test_slot_size_bounds COMMAND eboot_test_slot_size_bounds) +list(APPEND EBLDR_UNIT_TESTS test_slot_size_bounds) # --- test_device_table: UEFI-style device table --- add_executable(eboot_test_device_table unit/test_device_table.c) target_link_libraries(eboot_test_device_table PRIVATE eboot_core) add_test(NAME test_device_table COMMAND eboot_test_device_table) +list(APPEND EBLDR_UNIT_TESTS test_device_table) # --- test_runtime_svc: Runtime variable store --- add_executable(eboot_test_runtime_svc unit/test_runtime_svc.c) target_link_libraries(eboot_test_runtime_svc PRIVATE eboot_core) add_test(NAME test_runtime_svc COMMAND eboot_test_runtime_svc) +list(APPEND EBLDR_UNIT_TESTS test_runtime_svc) # --- test_board_config: Declarative hardware config --- add_executable(eboot_test_board_config unit/test_board_config.c) target_link_libraries(eboot_test_board_config PRIVATE eboot_core) add_test(NAME test_board_config COMMAND eboot_test_board_config) +list(APPEND EBLDR_UNIT_TESTS test_board_config) # --- test_multicore: Multicore boot management --- add_executable(eboot_test_multicore unit/test_multicore.c) target_link_libraries(eboot_test_multicore PRIVATE eboot_core) add_test(NAME test_multicore COMMAND eboot_test_multicore) +list(APPEND EBLDR_UNIT_TESTS test_multicore) # --- test_board_registry: Runtime board selection --- add_executable(eboot_test_board_registry unit/test_board_registry.c) target_link_libraries(eboot_test_board_registry PRIVATE eboot_core) add_test(NAME test_board_registry COMMAND eboot_test_board_registry) +list(APPEND EBLDR_UNIT_TESTS test_board_registry) # --- test_slot_manager: Firmware slot management --- add_executable(eboot_test_slot_manager unit/test_slot_manager.c) target_link_libraries(eboot_test_slot_manager PRIVATE eboot_core) add_test(NAME test_slot_manager COMMAND eboot_test_slot_manager) +list(APPEND EBLDR_UNIT_TESTS test_slot_manager) # --- test_boot_log: Boot log subsystem --- add_executable(eboot_test_boot_log unit/test_boot_log.c) target_link_libraries(eboot_test_boot_log PRIVATE eboot_core) add_test(NAME test_boot_log COMMAND eboot_test_boot_log) +list(APPEND EBLDR_UNIT_TESTS test_boot_log) # --- test_ed25519: Ed25519 signature verification --- add_executable(eboot_test_ed25519 unit/test_ed25519.c) target_link_libraries(eboot_test_ed25519 PRIVATE eboot_core) add_test(NAME test_ed25519 COMMAND eboot_test_ed25519) +list(APPEND EBLDR_UNIT_TESTS test_ed25519) # --- test_keystore: Key management --- add_executable(eboot_test_keystore unit/test_keystore.c) target_link_libraries(eboot_test_keystore PRIVATE eboot_core) add_test(NAME test_keystore COMMAND eboot_test_keystore) +list(APPEND EBLDR_UNIT_TESTS test_keystore) # --- test_rollback: Anti-rollback security counter --- add_executable(eboot_test_rollback unit/test_rollback.c) target_link_libraries(eboot_test_rollback PRIVATE eboot_core) add_test(NAME test_rollback COMMAND eboot_test_rollback) +list(APPEND EBLDR_UNIT_TESTS test_rollback) # --- test_tlv_auth: TLV area must be bound to the signed header --- add_executable(eboot_test_tlv_auth unit/test_tlv_auth.c) target_link_libraries(eboot_test_tlv_auth PRIVATE eboot_core) add_test(NAME test_tlv_auth COMMAND eboot_test_tlv_auth) +list(APPEND EBLDR_UNIT_TESTS test_tlv_auth) # --- test_storage: Unified storage abstraction --- add_executable(eboot_test_storage unit/test_storage.c) target_link_libraries(eboot_test_storage PRIVATE eboot_core) add_test(NAME test_storage COMMAND eboot_test_storage) +list(APPEND EBLDR_UNIT_TESTS test_storage) # --- test_ecc: ECC memory range validation --- add_executable(eboot_test_ecc unit/test_ecc.c) target_link_libraries(eboot_test_ecc PRIVATE eboot_core) add_test(NAME test_ecc COMMAND eboot_test_ecc) +list(APPEND EBLDR_UNIT_TESTS test_ecc) # --- Valgrind test targets --- +# +# The list is derived from the suites registered above, not written out again. +# Hand-maintained, it drifted: it had 17 of the 21 registered tests, missing +# test_ecc, test_rollback, test_secure_boot and test_storage -- and nothing +# failed when a name was forgotten, because a missing entry is simply a test +# that never gets a memory-safety run. +# +# EBLDR_UNIT_TESTS is appended by each add_test() above, so a suite added +# without touching this block still gets a Valgrind target. find_program(VALGRIND valgrind) if(VALGRIND) set(VALGRIND_OPTS --leak-check=full --error-exitcode=1 --quiet) - foreach(TEST_NAME test_bootctl test_crypto test_ed25519 test_keystore - test_device_table test_runtime_svc test_board_config - test_multicore test_board_registry test_slot_manager - test_boot_log test_image_verify test_image_abi - test_recovery test_slot_size_bounds test_fw_transport - test_tlv_auth) + foreach(TEST_NAME ${EBLDR_UNIT_TESTS}) add_test( NAME valgrind_${TEST_NAME} COMMAND ${VALGRIND} ${VALGRIND_OPTS} $ diff --git a/tests/unit/test_board_config.c b/tests/unit/test_board_config.c index 3096e03..64cf84e 100644 --- a/tests/unit/test_board_config.c +++ b/tests/unit/test_board_config.c @@ -20,6 +20,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -188,7 +189,6 @@ int main(void) run_test_total_ram(); run_test_total_flash(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_board_registry.c b/tests/unit/test_board_registry.c index 991b28b..a70ffe2 100644 --- a/tests/unit/test_board_registry.c +++ b/tests/unit/test_board_registry.c @@ -21,6 +21,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -134,7 +135,6 @@ int main(void) run_test_get_by_index(); run_test_register_null(); - tests_run = 8; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_boot_log.c b/tests/unit/test_boot_log.c index 5770780..0bde90f 100644 --- a/tests/unit/test_boot_log.c +++ b/tests/unit/test_boot_log.c @@ -300,6 +300,6 @@ int main(void) RUN(test_clear_reports_erase_failure); RUN(test_append_does_not_advance_head_when_write_fails); RUN(test_entry_layout_is_stable); - printf("\n%d/11 tests passed\n", tests_passed); + printf("\n%d/%d tests passed\n", tests_passed); return 0; } diff --git a/tests/unit/test_bootctl.c b/tests/unit/test_bootctl.c index 7c56d7f..f561b09 100644 --- a/tests/unit/test_bootctl.c +++ b/tests/unit/test_bootctl.c @@ -109,6 +109,7 @@ static int tests_passed = 0; static void run_##name(void) { \ setup(); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -292,7 +293,6 @@ int main(void) run_test_version_encoding(); run_test_validate_null(); - tests_run = 12; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } \ No newline at end of file diff --git a/tests/unit/test_crypto.c b/tests/unit/test_crypto.c index 279bcd1..59d1ff5 100644 --- a/tests/unit/test_crypto.c +++ b/tests/unit/test_crypto.c @@ -20,6 +20,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -118,7 +119,6 @@ int main(void) run_test_sha256_incremental(); run_test_crypto_null_args(); - tests_run = 5; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_device_table.c b/tests/unit/test_device_table.c index 01a39cd..6daf412 100644 --- a/tests/unit/test_device_table.c +++ b/tests/unit/test_device_table.c @@ -20,6 +20,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -145,7 +146,6 @@ int main(void) run_test_corrupt_crc_fails(); run_test_oversized_counts_fail(); - tests_run = 8; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_fw_transport.c b/tests/unit/test_fw_transport.c index 61a43ac..d22aadb 100644 --- a/tests/unit/test_fw_transport.c +++ b/tests/unit/test_fw_transport.c @@ -163,6 +163,7 @@ static int tests_passed = 0; static void run_##name(void) { \ setup(); \ printf(" %-56s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[OK]\n"); \ @@ -555,7 +556,6 @@ int main(void) run_test_raw_oversized_length_is_rejected(); run_test_raw_zero_length_is_rejected(); - tests_run = 12; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_image_verify.c b/tests/unit/test_image_verify.c index c275a4f..f714dd7 100644 --- a/tests/unit/test_image_verify.c +++ b/tests/unit/test_image_verify.c @@ -95,6 +95,7 @@ static int tests_passed = 0; sim_tick = 0; \ eos_hal_init(&sim_ops); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -481,7 +482,6 @@ int main(void) run_test_unsigned_signature_types_are_rejected(); run_test_header_version_is_validated(); run_test_tlv_unreadable_entry_fails_closed(); - tests_run = 17; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_keystore.c b/tests/unit/test_keystore.c index 7236bef..1e1960d 100644 --- a/tests/unit/test_keystore.c +++ b/tests/unit/test_keystore.c @@ -82,6 +82,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -276,7 +277,6 @@ int main(void) run_test_revocation_is_persisted_without_clobbering_other_slots(); run_test_revoke_reports_a_failed_persist(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_multicore.c b/tests/unit/test_multicore.c index 0355860..adeb405 100644 --- a/tests/unit/test_multicore.c +++ b/tests/unit/test_multicore.c @@ -21,6 +21,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -211,7 +212,6 @@ int main(void) run_test_invalid_core_id(); run_test_ipi_mailbox_fallback(); - tests_run = 10; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_rollback.c b/tests/unit/test_rollback.c index 328eb86..88304a5 100644 --- a/tests/unit/test_rollback.c +++ b/tests/unit/test_rollback.c @@ -22,6 +22,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -233,7 +234,6 @@ int main(void) run_test_commit_unsupported_without_hardware(); run_test_clear_staged_prevents_commit(); run_test_downgrade_blocked_after_confirmed_update(); - tests_run = 14; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_runtime_svc.c b/tests/unit/test_runtime_svc.c index 2e59c6b..30ca23b 100644 --- a/tests/unit/test_runtime_svc.c +++ b/tests/unit/test_runtime_svc.c @@ -21,6 +21,7 @@ static int tests_passed = 0; static void run_##name(void) { \ eos_rtsvc_init(); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -140,7 +141,6 @@ int main(void) run_test_next_boot_slot(); run_test_time(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_secure_boot.c b/tests/unit/test_secure_boot.c index 7358178..5f9160a 100644 --- a/tests/unit/test_secure_boot.c +++ b/tests/unit/test_secure_boot.c @@ -84,6 +84,7 @@ static const eos_board_ops_t sim_ops = { .deinit_peripherals = sim_noop, }; +static int tests_run = 0; static int tests_passed = 0; #define TEST(name) \ @@ -93,6 +94,7 @@ static int tests_passed = 0; sim_tick = 0; \ eos_hal_init(&sim_ops); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -203,6 +205,10 @@ int main(void) run_test_encrypted_image_rejected_while_decrypt_unimplemented(); run_test_plaintext_image_boots_when_encryption_not_required(); run_test_decrypt_failure_is_attested(); - printf("%d passed\n", tests_passed); - return 0; + /* Compare, and let the exit code carry it. `return 0` meant a suite that + * ran nothing at all still reported success -- the ASSERT macro exits on + * failure, so the only thing this return could ever have signalled is + * exactly the case it ignored. */ + printf("\n%d/%d passed\n", tests_passed, tests_run); + return tests_passed == tests_run ? 0 : 1; } diff --git a/tests/unit/test_slot_manager.c b/tests/unit/test_slot_manager.c index 5bca305..ebb70d1 100644 --- a/tests/unit/test_slot_manager.c +++ b/tests/unit/test_slot_manager.c @@ -171,6 +171,7 @@ static int tests_passed = 0; erased_size = 0; \ eos_hal_init(&sim_ops); \ printf(" %-55s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -338,7 +339,6 @@ int main(void) run_test_boot_attempts_drive_rollback(); run_test_boot_attempts_reject_invalid_slot(); run_test_erase_resets_boot_attempts(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_slot_size_bounds.c b/tests/unit/test_slot_size_bounds.c index c8acc8d..9efe5ad 100644 --- a/tests/unit/test_slot_size_bounds.c +++ b/tests/unit/test_slot_size_bounds.c @@ -119,6 +119,7 @@ static int tests_passed = 0; payload_bytes_read = 0; \ eos_hal_init(&sim_ops); \ printf(" %-55s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -190,7 +191,6 @@ int main(void) printf("=== eBootloader: Slot-Size Bounds Regression Tests ===\n\n"); run_test_oversized_image_rejected_before_reading_payload(); run_test_in_bounds_image_is_not_over_rejected(); - tests_run = 2; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_suite_bookkeeping.py b/tests/unit/test_suite_bookkeeping.py new file mode 100644 index 0000000..afa59bb --- /dev/null +++ b/tests/unit/test_suite_bookkeeping.py @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project +"""Two guards against counts and lists that are maintained by hand. + +Both patterns below were found in review, one after another, and both have the +same shape: a number or a list that describes what the suite does, written out +separately from the thing it describes, with nothing checking the two agree. + + 1. `main()` assigned `tests_run = ` while the `TEST()` macro + incremented only `tests_passed`. A test defined but never wired into + `main()` was then skipped with a zero exit -- the suite reported + "N/N passed" for an N that was a claim, not a count. + + Not hypothetical: `tests/unit/test_ed25519.c` carried a hardcoded 11 that + masked one test called twice and two never called at all, and + `tests/unit/test_tlv_auth.c` assigned `tests_run` twice in one function + (8, then 7) with the stale value surviving only because the later + assignment won. + + 2. The Valgrind `foreach` named its suites again by hand, and had drifted to + 17 of 21 -- test_ecc, test_rollback, test_secure_boot and test_storage + got no memory-safety run, and nothing failed when a name was forgotten. + +These tests are cheap and need no C toolchain. +""" + +import re +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +UNIT = REPO / "tests" / "unit" +CMAKE = REPO / "tests" / "CMakeLists.txt" + +#: Suites with no TEST() macro -- they count differently or not at all. A name +#: may sit here only with a reason about the suite itself. +NO_TEST_MACRO = { + "test_boot_log.c": "prints its own summary and has no TEST() macro", + "test_ecc.c": "single-scenario suite; no per-test harness", + "test_image_abi.c": "compile-time _Static_asserts; nothing runs per test", +} + + +def _suites(): + found = sorted(UNIT.glob("test_*.c")) + assert found, f"no unit suites under {UNIT}" + return found + + +def _macro_block(source): + m = re.search(r"#define TEST\(name\)(?:[^\n]*\\\n)*[^\n]*\n", source) + return m.group(0) if m else None + + +def test_no_suite_hardcodes_its_own_total(): + """`tests_run = ` is a claim about the suite, not a measurement.""" + offenders = [] + for path in _suites(): + source = path.read_text(encoding="utf-8") + for literal in re.findall(r"tests_run\s*=\s*([1-9]\d*)\s*;", source): + offenders.append(f"{path.name}: tests_run = {literal}") + # `printf("%d/12 tests passed")` is the same claim in another place. + for literal in re.findall(r"%d\s*/\s*(\d+)\s*(?:tests )?passed", source): + offenders.append(f"{path.name}: literal total {literal} in the summary") + assert not offenders, ( + "these suites state a total instead of counting one; a test that is " + "defined but never called is then invisible:\n " + "\n ".join(offenders) + ) + + +def test_every_test_macro_counts_the_test_it_runs(): + offenders = [] + for path in _suites(): + if path.name in NO_TEST_MACRO: + continue + block = _macro_block(path.read_text(encoding="utf-8")) + if block is None: + offenders.append(f"{path.name}: has no TEST() macro and is not in NO_TEST_MACRO") + elif "tests_run++" not in block: + offenders.append(f"{path.name}: TEST() does not increment tests_run") + assert not offenders, "\n ".join([""] + offenders) + + +def test_every_defined_test_is_actually_called(): + """A TEST() nobody calls is a test that silently does not run.""" + offenders = [] + for path in _suites(): + source = path.read_text(encoding="utf-8") + defined = set(re.findall(r"^TEST\((\w+)\)", source, re.M)) + if not defined: + continue + called = set(re.findall(r"run_(\w+)\s*\(\s*\)\s*;", source)) + for name in sorted(defined - called): + offenders.append(f"{path.name}: {name}() is defined but never called") + assert not offenders, "\n ".join([""] + offenders) + + +def test_the_valgrind_list_is_derived_not_repeated(): + """Every registered suite gets a Valgrind run, by construction.""" + cmake = CMAKE.read_text(encoding="utf-8") + + registered = re.findall(r"add_test\(NAME (\w+) COMMAND", cmake) + appended = re.findall(r"list\(APPEND EBLDR_UNIT_TESTS (\w+)\)", cmake) + + assert "foreach(TEST_NAME ${EBLDR_UNIT_TESTS})" in cmake, ( + "the Valgrind foreach must iterate the accumulated list rather than " + "name its suites again; a hand-written list drifts and nothing fails" + ) + missing = sorted(set(registered) - set(appended)) + assert not missing, ( + f"these suites are registered with ctest but never appended to " + f"EBLDR_UNIT_TESTS, so they get no Valgrind run: {missing}" + ) diff --git a/tests/unit/test_tlv_auth.c b/tests/unit/test_tlv_auth.c index 28c0e41..de46f5f 100644 --- a/tests/unit/test_tlv_auth.c +++ b/tests/unit/test_tlv_auth.c @@ -99,6 +99,7 @@ static int tests_passed = 0; sim_counter = 0; \ eos_hal_init(&sim_ops); \ printf(" %-58s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -294,7 +295,6 @@ int main(void) { printf("TLV authentication (anti-rollback counter)\n\n"); - tests_run = 8; run_test_authenticated_tlv_counter_is_read(); run_test_tampered_tlv_counter_is_rejected(); run_test_tamper_is_invisible_to_signature_and_integrity(); @@ -303,7 +303,6 @@ int main(void) run_test_tlv_binding_fields_are_inside_the_signed_prefix(); run_test_oversized_tlv_len_is_rejected(); - tests_run = 7; printf("\n%d/%d passed\n", tests_passed, tests_run); return tests_passed == tests_run ? 0 : 1; }