From f704d87fd445e4d61db00bc302e78660fc99f02f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:06:59 +0530 Subject: [PATCH 1/9] =?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 a1103a370cd41d205fd37637d530f2f798621bf7 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 1 Sep 2026 16:01:54 +0530 Subject: [PATCH 2/9] fix(fdt): bound a device tree parser that was never compiled core/fdt_loader.c is in no source list, so it has never been built. The header offers it to callers, rtos_boot.c is named as the consumer, and nothing catches what is in it -- not the compiler, not ctest, not the sanitizer job. What is in it is a parser for a blob that comes out of flash, whose every header field it uses as an offset or a length without checking any of them. eos_fdt_validate() looks at magic and version only, so it accepts a blob whose off_dt_struct points anywhere: hdr.totalsize = 40 (the header alone) hdr.off_dt_struct = 0x100000 eos_fdt_validate(blob) -> 0 eos_fdt_get_prop(...) -> AddressSanitizer: BUS, READ at fdt_loader.c:25 in fdt_read_u32 Five more, all reachable the same way: * the tag read at the top of the loop takes 4 bytes where the loop condition guarantees 1; * strlen() on a node name reads until it finds a zero, which for a name running to the end of the block is past it; * nameoff indexes the strings block unchecked, so strcmp() reads from an arbitrary address; * a property len is clamped to the caller's buffer before the memcpy, which bounds the write but not the read -- an oversized len copies whatever follows the blob out to the caller; * FDT_END_NODE decrements depth with no floor. Bound them. validate() is the gate every path goes through, so the block offsets are checked against totalsize there, and get_prop() calls it before trusting the header. Inside the loop each read is checked for the width it takes, the name and property-name scans are bounded by memchr within their blocks, and the padded advances are re-checked for overrun. Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest: ten cases, one well-formed tree that must still parse and nine malformed ones. Against the unfixed parser the suite fails on the third; with the bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE. Not fixed here, because it is a behaviour change rather than a safety one: node paths below the root do not resolve. _get_prop derives the depth to match from a slash count, so "/chosen" looks for depth 1, which is the root -- "chosen" is at depth 2. Only "/" resolves today. Co-Authored-By: Claude Opus 5 (1M context) --- CMakeLists.txt | 1 + core/fdt_loader.c | 63 +++++++- tests/CMakeLists.txt | 6 + tests/unit/test_fdt_loader.c | 286 +++++++++++++++++++++++++++++++++++ 4 files changed, 354 insertions(+), 2 deletions(-) create mode 100644 tests/unit/test_fdt_loader.c diff --git a/CMakeLists.txt b/CMakeLists.txt index f8fe6eb..cfe5d46 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -90,6 +90,7 @@ add_library(eboot_core STATIC core/fw_transport_uart.c core/rtos_boot.c core/rtos_params.c + core/fdt_loader.c core/boot_menu.c core/device_table.c core/runtime_services.c diff --git a/core/fdt_loader.c b/core/fdt_loader.c index 1be0175..bbd1220 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -26,12 +26,36 @@ static uint32_t fdt_read_u32(const uint8_t *ptr) return fdt32_to_cpu(val); } +/* Does [off, off+len) sit inside a blob of totalsize bytes, without the + * addition wrapping? Every offset in the header is attacker-controlled. */ +static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize) +{ + if (off < sizeof(fdt_header_t)) return false; + if (len > totalsize) return false; + return off <= totalsize - len; +} + int eos_fdt_validate(const void *fdt_blob) { if (!fdt_blob) return -1; const fdt_header_t *hdr = (const fdt_header_t *)fdt_blob; if (fdt32_to_cpu(hdr->magic) != FDT_MAGIC) return -2; if (fdt32_to_cpu(hdr->version) < 16) return -3; + + /* magic and version alone say nothing about where the blob claims its + * blocks are. The struct and string offsets are read straight out of + * flash and then used as pointers, so a blob that clears the two checks + * above could still point them anywhere; eos_fdt_get_prop walked off the + * end of a 40-byte allocation and took a bus fault. Reject the blob here + * instead, because this is the gate every path goes through. */ + uint32_t totalsize = fdt32_to_cpu(hdr->totalsize); + if (totalsize < sizeof(fdt_header_t)) return -6; + if (!fdt_block_in_bounds(fdt32_to_cpu(hdr->off_dt_struct), + fdt32_to_cpu(hdr->size_dt_struct), totalsize)) + return -6; + if (!fdt_block_in_bounds(fdt32_to_cpu(hdr->off_dt_strings), + fdt32_to_cpu(hdr->size_dt_strings), totalsize)) + return -6; return 0; } @@ -49,6 +73,10 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) const fdt_header_t *hdr = (const fdt_header_t *)dest; uint32_t total = fdt32_to_cpu(hdr->totalsize); if (total > max_size) return -4; + /* validate() has already rejected a totalsize below the header, but the + * header copied above is all that was read: re-check against what the + * caller actually offered before the full copy. */ + if (total < sizeof(fdt_header_t)) return -4; /* Copy full DTB */ memcpy(dest, src, total); @@ -60,10 +88,17 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path, { if (!fdt || !node_path || !prop_name || !buf || !buf_len) return -1; + /* The blob is whatever was in flash. Every offset below comes out of its + * header, so none of them can be trusted until validate() has bounded + * them against totalsize. */ + int vrc = eos_fdt_validate(fdt); + if (vrc != 0) return vrc; + const fdt_header_t *hdr = (const fdt_header_t *)fdt; const uint8_t *dt_struct = (const uint8_t *)fdt + fdt32_to_cpu(hdr->off_dt_struct); const char *dt_strings = (const char *)fdt + fdt32_to_cpu(hdr->off_dt_strings); uint32_t struct_size = fdt32_to_cpu(hdr->size_dt_struct); + uint32_t strings_size = fdt32_to_cpu(hdr->size_dt_strings); /* Simple linear search through struct block */ uint32_t offset = 0; @@ -78,15 +113,22 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path, } if (path_depth == 0) path_depth = 1; - while (offset < struct_size) { + /* offset < struct_size only guarantees one byte; every read below wants + * four or more, so each is checked for the width it actually takes. */ + while (offset + 4 <= struct_size) { uint32_t tag = fdt_read_u32(dt_struct + offset); offset += 4; switch (tag) { case FDT_BEGIN_NODE: { + /* strlen() here read until it happened to find a zero, which + * for a name running to the end of the block is past it. */ const char *name = (const char *)(dt_struct + offset); - uint32_t name_len = (uint32_t)strlen(name) + 1; + const void *nul = memchr(name, '\0', struct_size - offset); + if (!nul) return -6; + uint32_t name_len = (uint32_t)((const char *)nul - name) + 1; offset += (name_len + 3) & ~3U; + if (offset > struct_size) return -6; depth++; /* Check if this node matches the target path */ @@ -103,13 +145,28 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path, } case FDT_END_NODE: if (in_target && depth == target_depth) in_target = false; + /* An unbalanced blob would drive depth negative and let a later + * BEGIN_NODE match path_depth at the wrong nesting level. */ + if (depth == 0) return -6; depth--; break; case FDT_PROP: { + if (offset + 8 > struct_size) return -6; uint32_t len = fdt_read_u32(dt_struct + offset); uint32_t nameoff = fdt_read_u32(dt_struct + offset + 4); offset += 8; + + /* nameoff indexes the strings block; unchecked it named any + * address, and strcmp then read from it. */ + if (nameoff >= strings_size) return -6; const char *pname = dt_strings + nameoff; + if (!memchr(pname, '\0', strings_size - nameoff)) return -6; + + /* The value has to be inside the struct block before it is read: + * copy_len was clamped to the caller's buffer, which bounded the + * write but not the read, so an oversized len leaked whatever + * followed the blob into buf. */ + if (len > struct_size - offset) return -6; if (in_target && strcmp(pname, prop_name) == 0) { uint32_t copy_len = len < *buf_len ? len : *buf_len; @@ -117,7 +174,9 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path, *buf_len = copy_len; return 0; } + /* len is bounded above, so the pad cannot wrap. */ offset += (len + 3) & ~3U; + if (offset > struct_size) return -6; break; } case FDT_END: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..9ffffd7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -110,6 +110,12 @@ 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) +# --- test_fdt_loader: device tree parsing against malformed blobs --- +# core/fdt_loader.c was in no source list, so it had never been compiled. +add_executable(eboot_test_fdt_loader unit/test_fdt_loader.c) +target_link_libraries(eboot_test_fdt_loader PRIVATE eboot_core) +add_test(NAME test_fdt_loader COMMAND eboot_test_fdt_loader) + # --- Valgrind test targets --- find_program(VALGRIND valgrind) if(VALGRIND) diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c new file mode 100644 index 0000000..10df768 --- /dev/null +++ b/tests/unit/test_fdt_loader.c @@ -0,0 +1,286 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project +// ISO/IEC 25000 | ISO/IEC/IEEE 15288:2023 + +/** + * @file test_fdt_loader.c + * @brief Unit tests for the FDT loader against malformed device trees + * + * core/fdt_loader.c parses a blob that comes out of flash, so every offset + * and length in its header is attacker-controlled. The file was in no + * CMakeLists, so none of this was ever compiled, let alone run. + * + * Each negative case below is a blob that eos_fdt_validate() used to accept + * -- it checked only magic and version -- and that then drove + * eos_fdt_get_prop() off the end of the allocation. The positive case is + * here to keep the bounds checks from simply rejecting everything. + */ + +#include "eos_fdt_loader.h" +#include +#include +#include +#include + +static int tests_passed = 0; + +#define ASSERT(condition) \ + do { \ + if (!(condition)) { \ + fprintf(stderr, "[FAIL] %s:%d: %s\n", \ + __FILE__, __LINE__, #condition); \ + exit(1); \ + } \ + } while (0) + +#define RUN(test) \ + do { \ + test(); \ + tests_passed++; \ + printf("[PASS] %s\n", #test); \ + } while (0) + +/* ---- DTB construction helpers ---------------------------------------- */ + +#define BLOB_MAX 512 + +typedef struct { + unsigned char bytes[BLOB_MAX]; + uint32_t len; +} blob_t; + +static void put_be32(unsigned char *p, uint32_t v) +{ + p[0] = (unsigned char)(v >> 24); p[1] = (unsigned char)(v >> 16); + p[2] = (unsigned char)(v >> 8); p[3] = (unsigned char)v; +} + +static void append_be32(blob_t *b, uint32_t v) +{ + ASSERT(b->len + 4 <= BLOB_MAX); + put_be32(b->bytes + b->len, v); + b->len += 4; +} + +/* Append a NUL-terminated string padded to a 4-byte boundary. */ +static void append_padded(blob_t *b, const char *s) +{ + uint32_t n = (uint32_t)strlen(s) + 1; + ASSERT(b->len + ((n + 3) & ~3U) <= BLOB_MAX); + memcpy(b->bytes + b->len, s, n); + b->len += n; + while (b->len & 3U) b->bytes[b->len++] = 0; +} + +/* A minimal well-formed tree: / { bootargs = "ro"; chosen { }; } + * + * Layout is header, then the struct block, then the strings block, so the + * offsets the header carries are the real ones. + * + * The property sits on the root and the tests query "/". Node paths below + * the root do not resolve: _get_prop derives the depth to match at by + * counting slashes, so "/" looks for depth 1 -- which is the root + * itself, the node "chosen" being at depth 2. That is a separate defect in + * the same never-compiled file and is not what these tests are about, so + * they stay on the one path form the matcher handles. + */ +static void build_valid(blob_t *b, const char *value) +{ + memset(b, 0, sizeof(*b)); + b->len = sizeof(fdt_header_t); + uint32_t off_struct = b->len; + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, ""); + + uint32_t vlen = (uint32_t)strlen(value) + 1; + append_be32(b, FDT_PROP); + append_be32(b, vlen); + append_be32(b, 0); /* nameoff: "bootargs" at strings[0] */ + append_padded(b, value); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "chosen"); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END); + uint32_t size_struct = b->len - off_struct; + + uint32_t off_strings = b->len; + append_padded(b, "bootargs"); + uint32_t size_strings = b->len - off_strings; + + fdt_header_t *h = (fdt_header_t *)b->bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b->len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); +} + +static void set_field(blob_t *b, size_t field_offset, uint32_t v) +{ + put_be32(b->bytes + field_offset, v); +} + +#define FIELD(name) offsetof(fdt_header_t, name) + +/* Run get_prop on a heap copy sized exactly to the blob, so an + * out-of-bounds read lands in a sanitizer's redzone rather than in + * whatever the test's own stack happens to hold. */ +static int get_prop_exact(const blob_t *b, const char *node, const char *prop, + void *out, uint32_t *out_len) +{ + unsigned char *heap = malloc(b->len); + ASSERT(heap != NULL); + memcpy(heap, b->bytes, b->len); + int rc = eos_fdt_get_prop(heap, node, prop, out, out_len); + free(heap); + return rc; +} + +/* ---- Tests ------------------------------------------------------------ */ + +static void test_valid_tree_round_trips(void) +{ + blob_t b; build_valid(&b, "ro"); + ASSERT(eos_fdt_validate(b.bytes) == 0); + + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) == 0); + ASSERT(len == 3); + ASSERT(strcmp(out, "ro") == 0); +} + +static void test_missing_property_is_reported(void) +{ + blob_t b; build_valid(&b, "ro"); + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "nonesuch", out, &len) != 0); +} + +static void test_struct_offset_past_the_blob_is_rejected(void) +{ + blob_t b; build_valid(&b, "ro"); + set_field(&b, FIELD(off_dt_struct), 0x100000); + + ASSERT(eos_fdt_validate(b.bytes) != 0); + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); +} + +static void test_strings_offset_past_the_blob_is_rejected(void) +{ + blob_t b; build_valid(&b, "ro"); + set_field(&b, FIELD(off_dt_strings), 0x100000); + + ASSERT(eos_fdt_validate(b.bytes) != 0); + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); +} + +static void test_struct_size_running_past_the_blob_is_rejected(void) +{ + blob_t b; build_valid(&b, "ro"); + set_field(&b, FIELD(size_dt_struct), 0xFFFFFF00U); + + ASSERT(eos_fdt_validate(b.bytes) != 0); + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); +} + +static void test_totalsize_below_the_header_is_rejected(void) +{ + blob_t b; build_valid(&b, "ro"); + set_field(&b, FIELD(totalsize), 8); + ASSERT(eos_fdt_validate(b.bytes) != 0); +} + +static void test_property_length_past_the_struct_block_is_rejected(void) +{ + /* The write was clamped to the caller's buffer, so this never + * overflowed buf -- it read past the blob and copied what followed it + * out to the caller. */ + blob_t b; build_valid(&b, "ro"); + + /* Find the FDT_PROP tag and enlarge its length field. */ + for (uint32_t i = sizeof(fdt_header_t); i + 8 <= b.len; i += 4) { + if (b.bytes[i] == 0 && b.bytes[i+1] == 0 && + b.bytes[i+2] == 0 && b.bytes[i+3] == FDT_PROP) { + set_field(&b, i + 4, 0xFFFF0000U); + break; + } + } + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); +} + +static void test_property_nameoff_past_the_strings_block_is_rejected(void) +{ + blob_t b; build_valid(&b, "ro"); + for (uint32_t i = sizeof(fdt_header_t); i + 12 <= b.len; i += 4) { + if (b.bytes[i] == 0 && b.bytes[i+1] == 0 && + b.bytes[i+2] == 0 && b.bytes[i+3] == FDT_PROP) { + set_field(&b, i + 8, 0xFFFF0000U); /* nameoff */ + break; + } + } + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); +} + +static void test_unterminated_node_name_is_rejected(void) +{ + /* A node name that runs to the end of the struct block with no NUL: + * strlen() used to read straight past it. */ + blob_t b; memset(&b, 0, sizeof b); + b.len = sizeof(fdt_header_t); + uint32_t off_struct = b.len; + append_be32(&b, FDT_BEGIN_NODE); + for (int i = 0; i < 8; i++) b.bytes[b.len++] = 'A'; + uint32_t size_struct = b.len - off_struct; + uint32_t off_strings = b.len; + b.bytes[b.len++] = 0; b.len += 3; + + fdt_header_t *h = (fdt_header_t *)b.bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->totalsize, b.len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, 4); + + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); +} + +static void test_null_arguments_are_rejected(void) +{ + blob_t b; build_valid(&b, "ro"); + char out[16]; uint32_t len = sizeof out; + ASSERT(eos_fdt_validate(NULL) != 0); + ASSERT(eos_fdt_get_prop(NULL, "/", "bootargs", out, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, NULL, "bootargs", out, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, "/", NULL, out, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, "/", "bootargs", NULL, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, "/", "bootargs", out, NULL) != 0); +} + +int main(void) +{ + printf("=== eBootloader FDT Loader Tests ===\n"); + RUN(test_valid_tree_round_trips); + RUN(test_missing_property_is_reported); + RUN(test_struct_offset_past_the_blob_is_rejected); + RUN(test_strings_offset_past_the_blob_is_rejected); + RUN(test_struct_size_running_past_the_blob_is_rejected); + RUN(test_totalsize_below_the_header_is_rejected); + RUN(test_property_length_past_the_struct_block_is_rejected); + RUN(test_property_nameoff_past_the_strings_block_is_rejected); + RUN(test_unterminated_node_name_is_rejected); + RUN(test_null_arguments_are_rejected); + printf("\n%d/10 tests passed\n", tests_passed); + return 0; +} From 46498666e262f118e1448c0e9b1b700be9d566be Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 14:54:57 +0530 Subject: [PATCH 3/9] fix(fdt): bound the parser against the caller's length, not the blob's claim Follow-up to the review on #84. Three findings from it, all in files this branch already owns. Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative to hdr->totalsize, which is read out of the same untrusted blob. Checking the block offsets against it proves the header is internally consistent and says nothing about how many bytes are really mapped: a 40-byte buffer declaring totalsize = 0x100000 with agreeing offsets passed every check, and eos_fdt_get_prop() then walked a megabyte past the allocation. The original reproducer (an inflated *offset* against an honest totalsize) was rejected; its mirror image was not. Adds the length-carrying entry points eos_fdt_validate_sized() and eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and check totalsize against that before anything else. The existing two-argument forms stay as wrappers that trust the blob's own totalsize, documented in the header as a warrant the caller has to make good; eos_fdt_load() now passes max_size, which it had all along. Finding 2 (Medium) -- a property larger than the caller's buffer was copied short and reported as success, so a clipped value was indistinguishable from a complete one. In a boot path this reads bootargs. Now returns -7 and sets *buf_len to the full length so the caller can size a retry. Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing harnesses. .ai/security.md names device tree among the parsers that get fuzz coverage rather than unit tests alone. It drives the sized entry points and passes the real size, which is what makes the harness meaningful -- the unsized forms would let a fuzzer authorise its own out-of-bounds read. Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check in eos_fdt_load(); validate() already rejects that blob, and the comment above it described a bound it was not applying. Header now documents the full -1..-8 return code table. Verified: cmake -DEBLDR_BUILD_TESTS=ON, ctest 21/21 PASS same under -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS test_fdt_loader 14 tests PASS (was 10) new tests against the pre-fix parser both FAIL as they should - inflated totalsize accepted at the sized entry point - truncating call returned 0 instead of -7 grep for eos_fdt_get_prop / eos_fdt_validate no callers outside the parser and its tests, so the -7 change breaks nothing today Refs #84 --- core/fdt_loader.c | 60 ++++++++++++++++++++++------- include/eos_fdt_loader.h | 60 +++++++++++++++++++++++++++++ tests/fuzz/CMakeLists.txt | 10 ++++- tests/fuzz/fuzz_fdt.c | 60 +++++++++++++++++++++++++++++ tests/unit/test_fdt_loader.c | 75 +++++++++++++++++++++++++++++++++++- 5 files changed, 250 insertions(+), 15 deletions(-) create mode 100644 tests/fuzz/fuzz_fdt.c diff --git a/core/fdt_loader.c b/core/fdt_loader.c index bbd1220..5271516 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -35,13 +35,21 @@ static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize) return off <= totalsize - len; } -int eos_fdt_validate(const void *fdt_blob) +int eos_fdt_validate_sized(const void *fdt_blob, uint32_t avail) { if (!fdt_blob) return -1; + if (avail < sizeof(fdt_header_t)) return -6; const fdt_header_t *hdr = (const fdt_header_t *)fdt_blob; if (fdt32_to_cpu(hdr->magic) != FDT_MAGIC) return -2; if (fdt32_to_cpu(hdr->version) < 16) return -3; + /* Before anything else: the blob does not get to say how big it is. Every + * bound below is relative to totalsize, so without this the checks only + * prove the header is self-consistent — a 40-byte buffer claiming a 1 MiB + * totalsize with agreeing block offsets passes all of them, and the walk + * then runs a megabyte past the allocation. */ + if (fdt32_to_cpu(hdr->totalsize) > avail) return -6; + /* magic and version alone say nothing about where the blob claims its * blocks are. The struct and string offsets are read straight out of * flash and then used as pointers, so a blob that clears the two checks @@ -59,6 +67,15 @@ int eos_fdt_validate(const void *fdt_blob) return 0; } +int eos_fdt_validate(const void *fdt_blob) +{ + if (!fdt_blob) return -1; + /* No length to check against, so take the blob's own claim. Documented in + * the header as a warrant the caller has to make good. */ + const fdt_header_t *hdr = (const fdt_header_t *)fdt_blob; + return eos_fdt_validate_sized(fdt_blob, fdt32_to_cpu(hdr->totalsize)); +} + int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) { if (!dest || max_size < sizeof(fdt_header_t)) return -1; @@ -67,31 +84,30 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) const void *src = (const void *)(uintptr_t)flash_addr; memcpy(dest, src, sizeof(fdt_header_t)); - int rc = eos_fdt_validate(dest); + /* Only the header has been copied so far, but max_size is what the caller + * really owns, so that is the bound the blob has to satisfy. */ + int rc = eos_fdt_validate_sized(dest, max_size); if (rc != 0) return rc; const fdt_header_t *hdr = (const fdt_header_t *)dest; uint32_t total = fdt32_to_cpu(hdr->totalsize); if (total > max_size) return -4; - /* validate() has already rejected a totalsize below the header, but the - * header copied above is all that was read: re-check against what the - * caller actually offered before the full copy. */ - if (total < sizeof(fdt_header_t)) return -4; /* Copy full DTB */ memcpy(dest, src, total); return 0; } -int eos_fdt_get_prop(const void *fdt, const char *node_path, - const char *prop_name, void *buf, uint32_t *buf_len) +int eos_fdt_get_prop_sized(const void *fdt, uint32_t fdt_len, + const char *node_path, const char *prop_name, + void *buf, uint32_t *buf_len) { if (!fdt || !node_path || !prop_name || !buf || !buf_len) return -1; /* The blob is whatever was in flash. Every offset below comes out of its * header, so none of them can be trusted until validate() has bounded - * them against totalsize. */ - int vrc = eos_fdt_validate(fdt); + * them against both totalsize and the length the caller actually owns. */ + int vrc = eos_fdt_validate_sized(fdt, fdt_len); if (vrc != 0) return vrc; const fdt_header_t *hdr = (const fdt_header_t *)fdt; @@ -169,9 +185,17 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path, if (len > struct_size - offset) return -6; if (in_target && strcmp(pname, prop_name) == 0) { - uint32_t copy_len = len < *buf_len ? len : *buf_len; - memcpy(buf, dt_struct + offset, copy_len); - *buf_len = copy_len; + /* Truncating and returning 0 told the caller it had the whole + * value. This path reads bootargs: a clipped string that + * reports success drops whatever sat at its end, and nothing + * distinguishes that from a short property. Report the full + * length so the caller can size a retry. */ + if (len > *buf_len) { + *buf_len = len; + return -7; + } + memcpy(buf, dt_struct + offset, len); + *buf_len = len; return 0; } /* len is bounded above, so the pad cannot wrap. */ @@ -188,6 +212,16 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path, return -5; } +int eos_fdt_get_prop(const void *fdt, const char *node_path, + const char *prop_name, void *buf, uint32_t *buf_len) +{ + if (!fdt) return -1; + /* Trusts the blob's own totalsize; see the warrant in the header. */ + const fdt_header_t *hdr = (const fdt_header_t *)fdt; + return eos_fdt_get_prop_sized(fdt, fdt32_to_cpu(hdr->totalsize), + node_path, prop_name, buf, buf_len); +} + void eos_fdt_pass_to_kernel(uint32_t dtb_addr) { /* The DTB address is passed to the kernel via: diff --git a/include/eos_fdt_loader.h b/include/eos_fdt_loader.h index 30eec7b..0056a89 100644 --- a/include/eos_fdt_loader.h +++ b/include/eos_fdt_loader.h @@ -34,10 +34,70 @@ typedef struct { uint32_t size_dt_struct; } fdt_header_t; +/** + * Return codes. Every entry point below returns 0 on success and one of these + * on failure; they are distinct so a caller can tell a programming error from + * a malformed blob from a buffer that was too small. + * + * -1 a required argument was NULL + * -2 the blob does not carry the FDT magic + * -3 the blob declares an unsupported version (< 16) + * -4 the blob does not fit the destination buffer + * -5 no such node or property + * -6 the blob is malformed: an offset or length escapes it + * -7 the property is larger than the caller's buffer (see below) + * -8 the node path is deeper than FDT_MAX_PATH_DEPTH components + */ + +/** + * Validate a blob whose length is known. + * + * @param avail bytes actually readable at @p fdt_blob. + * + * Prefer this to eos_fdt_validate(). Every bound inside the header — the + * struct and string block offsets and sizes — is expressed relative to the + * header's own totalsize field, which is attacker-controlled. Checking those + * against each other proves only that the blob is internally consistent; it + * says nothing about how many bytes are really mapped. Passing the real + * length is what turns the consistency check into a bounds check. + */ +int eos_fdt_validate_sized(const void *fdt_blob, uint32_t avail); + +/** + * Validate a blob, trusting its own totalsize. + * + * The caller warrants that at least totalsize bytes are readable at + * @p fdt_blob. This cannot be checked from here, which is why + * eos_fdt_validate_sized() exists; use this only where the blob's extent has + * already been established by other means. + */ int eos_fdt_validate(const void *fdt_blob); + int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size); + +/** + * Read a property, on a blob whose length is known. + * + * @param fdt_len bytes actually readable at @p fdt. + * @param buf_len in: capacity of @p buf. out: bytes written on success, or + * the property's full length when -7 is returned, so the + * caller can size a retry. + * + * Returns -7 rather than truncating. A boot path reads bootargs through here, + * and a silently clipped value that reports success loses whatever sat at the + * end of the string. + */ +int eos_fdt_get_prop_sized(const void *fdt, uint32_t fdt_len, + const char *node_path, const char *prop_name, + void *buf, uint32_t *buf_len); + +/** + * Read a property, trusting the blob's own totalsize. Same warrant as + * eos_fdt_validate(). + */ int eos_fdt_get_prop(const void *fdt, const char *node_path, const char *prop_name, void *buf, uint32_t *buf_len); + void eos_fdt_pass_to_kernel(uint32_t dtb_addr); #ifdef __cplusplus diff --git a/tests/fuzz/CMakeLists.txt b/tests/fuzz/CMakeLists.txt index 85d6b71..db22d0b 100644 --- a/tests/fuzz/CMakeLists.txt +++ b/tests/fuzz/CMakeLists.txt @@ -48,6 +48,14 @@ if(EBLDR_BUILD_FUZZ) LINK_FLAGS "${FUZZ_FLAGS}" ) + # --- fuzz_fdt --- + add_executable(fuzz_fdt fuzz_fdt.c) + target_link_libraries(fuzz_fdt PRIVATE eboot_core) + set_target_properties(fuzz_fdt PROPERTIES + COMPILE_FLAGS "${FUZZ_FLAGS}" + LINK_FLAGS "${FUZZ_FLAGS}" + ) + # --- fuzz_bootctl --- add_executable(fuzz_bootctl fuzz_bootctl.c) target_link_libraries(fuzz_bootctl PRIVATE eboot_core) @@ -56,5 +64,5 @@ if(EBLDR_BUILD_FUZZ) LINK_FLAGS "${FUZZ_FLAGS}" ) - message(STATUS " Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update, fuzz_crypto, fuzz_bootctl") + message(STATUS " Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update, fuzz_crypto, fuzz_bootctl, fuzz_fdt") endif() diff --git a/tests/fuzz/fuzz_fdt.c b/tests/fuzz/fuzz_fdt.c new file mode 100644 index 0000000..efe0076 --- /dev/null +++ b/tests/fuzz/fuzz_fdt.c @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project + +/** + * @file fuzz_fdt.c + * @brief libFuzzer harness for the flattened device tree parser + * + * The DTB is off-device input: it comes out of flash or from a prior boot + * stage, and every offset and length in its header is attacker-controlled. + * .ai/security.md names device tree among the parsers that "get fuzz + * coverage, not just unit tests"; core/fdt_loader.c had neither until + * recently, and the ten hand-written blobs in tests/unit/test_fdt_loader.c + * cover the shapes that were reasoned about rather than the ones nobody + * thought of. + * + * The size-carrying entry points are the ones driven here. Passing `size` + * is what makes the harness meaningful: the unsized forms take the blob's + * own totalsize as the bound, so a fuzzer that inflates that field would be + * telling the parser it may read past the buffer libFuzzer allocated, and + * every report would be the harness's fault rather than the parser's. + */ + +#include "eos_fdt_loader.h" + +#include +#include + +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + if (size > UINT32_MAX) { + return 0; + } + + uint32_t len = (uint32_t)size; + + if (eos_fdt_validate_sized(data, len) != 0) { + return 0; + } + + /* Only reached for a blob whose header survived validation, which is + * where the interesting walking bugs live. Both a path that exists in + * most trees and one that does not, so the FDT_END and not-found exits + * are exercised as well as the match. */ + unsigned char buf[256]; + uint32_t buf_len = sizeof buf; + (void)eos_fdt_get_prop_sized(data, len, "/chosen", "bootargs", + buf, &buf_len); + + buf_len = sizeof buf; + (void)eos_fdt_get_prop_sized(data, len, "/", "compatible", + buf, &buf_len); + + /* A one-byte buffer drives the -7 truncation path, which is the branch + * that reports a length back to the caller. */ + unsigned char tiny[1]; + buf_len = sizeof tiny; + (void)eos_fdt_get_prop_sized(data, len, "/", "bootargs", tiny, &buf_len); + + return 0; +} diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c index 10df768..763cc09 100644 --- a/tests/unit/test_fdt_loader.c +++ b/tests/unit/test_fdt_loader.c @@ -140,6 +140,19 @@ static int get_prop_exact(const blob_t *b, const char *node, const char *prop, return rc; } +/* Same, but through the length-carrying entry point, passing the real + * allocation size rather than letting the blob declare it. */ +static int get_prop_sized_exact(const blob_t *b, const char *node, + const char *prop, void *out, uint32_t *out_len) +{ + unsigned char *heap = malloc(b->len); + ASSERT(heap != NULL); + memcpy(heap, b->bytes, b->len); + int rc = eos_fdt_get_prop_sized(heap, b->len, node, prop, out, out_len); + free(heap); + return rc; +} + /* ---- Tests ------------------------------------------------------------ */ static void test_valid_tree_round_trips(void) @@ -268,6 +281,62 @@ static void test_null_arguments_are_rejected(void) ASSERT(eos_fdt_get_prop(b.bytes, "/", "bootargs", out, NULL) != 0); } +/* The mirror image of the offset tests above. Those inflate an offset and + * leave totalsize honest; this leaves every offset internally consistent and + * inflates totalsize itself. Nothing in the header contradicts anything else, + * so a check that bounds the blob against its own totalsize has nothing to + * catch -- the only thing that knows better is the caller's allocation. */ +static void test_an_inflated_totalsize_is_rejected_when_the_length_is_known(void) +{ + blob_t b; build_valid(&b, "ro"); + uint32_t honest = b.len; + set_field(&b, FIELD(totalsize), 0x100000); + b.len = honest; /* the allocation stays what it really was */ + + char out[16]; uint32_t len = sizeof out; + + /* Told the truth about the buffer, the parser refuses it. */ + ASSERT(eos_fdt_validate_sized(b.bytes, honest) != 0); + ASSERT(get_prop_sized_exact(&b, "/", "bootargs", out, &len) != 0); +} + +/* The unsized entry points cannot catch the blob above, and this pins that so + * the difference is a documented contract rather than an accident. If this + * ever starts returning non-zero, eos_fdt_validate() has learned the length + * from somewhere and the warrant in the header should be removed. */ +static void test_the_unsized_entry_point_trusts_the_blobs_own_totalsize(void) +{ + blob_t b; build_valid(&b, "ro"); + set_field(&b, FIELD(totalsize), b.len + 0x10000); + ASSERT(eos_fdt_validate(b.bytes) == 0); +} + +/* A property that does not fit used to be copied short and reported as a + * success, so a clipped bootargs was indistinguishable from a complete one. */ +static void test_a_property_too_large_for_the_buffer_is_not_truncated(void) +{ + blob_t b; build_valid(&b, "root=/dev/mmcblk0p2 rw quiet"); + + char out[8]; + uint32_t len = sizeof out; + memset(out, 0xAA, sizeof out); + + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) == -7); + /* and it reports what the caller would need to allocate */ + ASSERT(len == sizeof "root=/dev/mmcblk0p2 rw quiet"); + /* nothing was written into the short buffer */ + ASSERT(out[0] == (char)0xAA); +} + +static void test_a_property_that_exactly_fits_still_succeeds(void) +{ + blob_t b; build_valid(&b, "ro"); + char out[3]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) == 0); + ASSERT(len == 3); + ASSERT(strcmp(out, "ro") == 0); +} + int main(void) { printf("=== eBootloader FDT Loader Tests ===\n"); @@ -281,6 +350,10 @@ int main(void) RUN(test_property_nameoff_past_the_strings_block_is_rejected); RUN(test_unterminated_node_name_is_rejected); RUN(test_null_arguments_are_rejected); - printf("\n%d/10 tests passed\n", tests_passed); + RUN(test_an_inflated_totalsize_is_rejected_when_the_length_is_known); + RUN(test_the_unsized_entry_point_trusts_the_blobs_own_totalsize); + RUN(test_a_property_too_large_for_the_buffer_is_not_truncated); + RUN(test_a_property_that_exactly_fits_still_succeeds); + printf("\n%d tests passed\n", tests_passed); return 0; } From 9ff5a432f5c6adf17e15176e8c75e4f8f1c2f87a Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:31:51 +0530 Subject: [PATCH 4/9] fix(build): drop the duplicate core/fdt_loader.c registration master gained core/fdt_loader.c in eboot_core while this branch was open, so after the rebase it appeared twice and tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once failed: AssertionError: ['core/fdt_loader.c'] is not false : duplicate eboot_core sources: ['core/fdt_loader.c'] Kept master's entry, dropped the one this branch added. That guard is the same shape as the one this stack adds for toolchain specs -- a build-file check that catches the class rather than the instance -- and it did its job. Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped. Refs #84 --- CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index cfe5d46..f8fe6eb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -90,7 +90,6 @@ add_library(eboot_core STATIC core/fw_transport_uart.c core/rtos_boot.c core/rtos_params.c - core/fdt_loader.c core/boot_menu.c core/device_table.c core/runtime_services.c From cf4bf4f552345c4e553a953b10570ed4e5e37114 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 22:41:43 +0530 Subject: [PATCH 5/9] fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses Answers the second review on #84. Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an out-of-bounds read by design. Both dereferenced hdr->totalsize before any length was known and handed that attacker-controlled value to the _sized form as its bound, so calling either on a short buffer read past it before a single check had run. The header called that a caller warrant; a warrant is not a check, and it is the exact bug this PR exists to fix. Neither had an in-tree caller. Removed rather than documented. There is now one form of each entry point and it always takes the length: int eos_fdt_validate(const void *fdt_blob, uint32_t avail); int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...); An exported unsafe twin in a TCB header is a future boot-path caller reintroducing the bug with no compiler complaint, which is worth more than the convenience of a one-argument call. eos_fdt_load() already passed max_size. The test that pinned the wrapper's behaviour is gone with the wrapper, and get_prop_sized_exact() collapsed into get_prop_exact() since they became the same function. Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ defaults OFF and no job set it, so the harness added here joined five others that no CI job compiles. A harness that is never built cannot fail to build, which is how fuzz_devicetree came to declare a function that did not exist and sit there unnoticed (eos#50). Adds a `fuzz-build` job: configure with clang, build every harness, and run each for five seconds over its own generated inputs. That is not a campaign -- it is enough to catch a harness that no longer compiles or crashes at once, which is the failure this repo has actually had. Note it needs EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt, so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all -- the job would have passed having compiled nothing. Found that locally before writing the job, not after. NOT RUN, and this is the honest limit: this host has no libFuzzer runtime (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link step cannot be reproduced here for any harness, old or new. What I verified is that CMake configures with both flags and reports all six targets -- "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update, fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this job is what makes them visible. Coordination note: #90 adds a `CI Gate` whose needs list is [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second must add fuzz-build to that list -- and #90's own test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is not, which is the guard working rather than a trap. Finding 1 (High) is a PR-body correction, made there: the diff carries #94's master repair because this branch is stacked on it, and the body described only the FDT parser. Verified: ctest 22/22 PASS pytest tests/ 38 passed test_fdt_loader 13 tests PASS grep for a length-free entry point none remains in the header Refs #84 --- .github/workflows/ci.yml | 37 +++++++++++++++++++++++++ core/fdt_loader.c | 29 ++++---------------- include/eos_fdt_loader.h | 45 +++++++++++------------------- tests/fuzz/fuzz_fdt.c | 8 +++--- tests/unit/test_fdt_loader.c | 53 ++++++++++-------------------------- 5 files changed, 77 insertions(+), 95 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cfbad9..0019343 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,6 +124,43 @@ jobs: retention-days: 90 # ── Static Analysis ─────────────────────────────────────────────────────── + # The fuzz harnesses are behind EBLDR_BUILD_FUZZ, which defaults OFF, and + # no job set it -- so tests/fuzz/ was never compiled anywhere. A harness + # that is not built cannot fail to build, which is how fuzz_devicetree came + # to declare a function that did not exist and sit there unnoticed. + # + # This does not fuzz. It builds the targets with clang and runs each for a + # few seconds over its own generated inputs, which is enough to catch a + # harness that no longer compiles or crashes immediately. A real campaign + # belongs in a scheduled workflow, not on every pull request. + fuzz-build: + name: Fuzz Harness Build + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@v4 + - name: Build fuzz targets + run: | + set -euo pipefail + # EBLDR_BUILD_TESTS too: tests/fuzz/ is added from + # tests/CMakeLists.txt, so EBLDR_BUILD_FUZZ alone builds nothing and + # the job would pass having compiled no harness at all. + cmake -B build/fuzz -DEBLDR_BUILD_TESTS=ON -DEBLDR_BUILD_FUZZ=ON \ + -DCMAKE_C_COMPILER=clang -DCMAKE_BUILD_TYPE=Debug + cmake --build build/fuzz -j"$(nproc)" + - name: Smoke-run each harness + run: | + set -euo pipefail + shopt -s nullglob + harnesses=(build/fuzz/tests/fuzz/fuzz_*) + if [ ${#harnesses[@]} -eq 0 ]; then + echo "::error::EBLDR_BUILD_FUZZ=ON produced no harnesses" + exit 1 + fi + for f in "${harnesses[@]}"; do + echo "=== $(basename "$f") ===" + "$f" -max_total_time=5 -print_final_stats=1 + done + static-analysis: name: Static Analysis (cppcheck + clang-tidy) runs-on: ubuntu-22.04 diff --git a/core/fdt_loader.c b/core/fdt_loader.c index 5271516..235589e 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -35,7 +35,7 @@ static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize) return off <= totalsize - len; } -int eos_fdt_validate_sized(const void *fdt_blob, uint32_t avail) +int eos_fdt_validate(const void *fdt_blob, uint32_t avail) { if (!fdt_blob) return -1; if (avail < sizeof(fdt_header_t)) return -6; @@ -67,14 +67,6 @@ int eos_fdt_validate_sized(const void *fdt_blob, uint32_t avail) return 0; } -int eos_fdt_validate(const void *fdt_blob) -{ - if (!fdt_blob) return -1; - /* No length to check against, so take the blob's own claim. Documented in - * the header as a warrant the caller has to make good. */ - const fdt_header_t *hdr = (const fdt_header_t *)fdt_blob; - return eos_fdt_validate_sized(fdt_blob, fdt32_to_cpu(hdr->totalsize)); -} int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) { @@ -86,7 +78,7 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) /* Only the header has been copied so far, but max_size is what the caller * really owns, so that is the bound the blob has to satisfy. */ - int rc = eos_fdt_validate_sized(dest, max_size); + int rc = eos_fdt_validate(dest, max_size); if (rc != 0) return rc; const fdt_header_t *hdr = (const fdt_header_t *)dest; @@ -98,16 +90,16 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) return 0; } -int eos_fdt_get_prop_sized(const void *fdt, uint32_t fdt_len, - const char *node_path, const char *prop_name, - void *buf, uint32_t *buf_len) +int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, + const char *node_path, const char *prop_name, + void *buf, uint32_t *buf_len) { if (!fdt || !node_path || !prop_name || !buf || !buf_len) return -1; /* The blob is whatever was in flash. Every offset below comes out of its * header, so none of them can be trusted until validate() has bounded * them against both totalsize and the length the caller actually owns. */ - int vrc = eos_fdt_validate_sized(fdt, fdt_len); + int vrc = eos_fdt_validate(fdt, fdt_len); if (vrc != 0) return vrc; const fdt_header_t *hdr = (const fdt_header_t *)fdt; @@ -212,15 +204,6 @@ int eos_fdt_get_prop_sized(const void *fdt, uint32_t fdt_len, return -5; } -int eos_fdt_get_prop(const void *fdt, const char *node_path, - const char *prop_name, void *buf, uint32_t *buf_len) -{ - if (!fdt) return -1; - /* Trusts the blob's own totalsize; see the warrant in the header. */ - const fdt_header_t *hdr = (const fdt_header_t *)fdt; - return eos_fdt_get_prop_sized(fdt, fdt32_to_cpu(hdr->totalsize), - node_path, prop_name, buf, buf_len); -} void eos_fdt_pass_to_kernel(uint32_t dtb_addr) { diff --git a/include/eos_fdt_loader.h b/include/eos_fdt_loader.h index 0056a89..f11cd39 100644 --- a/include/eos_fdt_loader.h +++ b/include/eos_fdt_loader.h @@ -50,33 +50,27 @@ typedef struct { */ /** - * Validate a blob whose length is known. + * Validate a blob. * * @param avail bytes actually readable at @p fdt_blob. * - * Prefer this to eos_fdt_validate(). Every bound inside the header — the - * struct and string block offsets and sizes — is expressed relative to the - * header's own totalsize field, which is attacker-controlled. Checking those - * against each other proves only that the blob is internally consistent; it - * says nothing about how many bytes are really mapped. Passing the real - * length is what turns the consistency check into a bounds check. + * There is deliberately no length-free form. Every bound inside the header -- + * the struct and string block offsets and sizes -- is expressed relative to + * the header's own totalsize field, which is attacker-controlled; checking + * those against each other proves the blob is internally consistent and says + * nothing about how many bytes are really mapped. An earlier revision kept a + * one-argument wrapper that read totalsize and passed it as the bound, which + * is an out-of-bounds read on a short buffer before any check has run. A + * documented caller warrant is not a check, and an exported unsafe twin in a + * TCB header is a future boot-path caller reintroducing the bug with no + * compiler complaint. */ -int eos_fdt_validate_sized(const void *fdt_blob, uint32_t avail); - -/** - * Validate a blob, trusting its own totalsize. - * - * The caller warrants that at least totalsize bytes are readable at - * @p fdt_blob. This cannot be checked from here, which is why - * eos_fdt_validate_sized() exists; use this only where the blob's extent has - * already been established by other means. - */ -int eos_fdt_validate(const void *fdt_blob); +int eos_fdt_validate(const void *fdt_blob, uint32_t avail); int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size); /** - * Read a property, on a blob whose length is known. + * Read a property. * * @param fdt_len bytes actually readable at @p fdt. * @param buf_len in: capacity of @p buf. out: bytes written on success, or @@ -87,16 +81,9 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size); * and a silently clipped value that reports success loses whatever sat at the * end of the string. */ -int eos_fdt_get_prop_sized(const void *fdt, uint32_t fdt_len, - const char *node_path, const char *prop_name, - void *buf, uint32_t *buf_len); - -/** - * Read a property, trusting the blob's own totalsize. Same warrant as - * eos_fdt_validate(). - */ -int eos_fdt_get_prop(const void *fdt, const char *node_path, - const char *prop_name, void *buf, uint32_t *buf_len); +int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, + const char *node_path, const char *prop_name, + void *buf, uint32_t *buf_len); void eos_fdt_pass_to_kernel(uint32_t dtb_addr); diff --git a/tests/fuzz/fuzz_fdt.c b/tests/fuzz/fuzz_fdt.c index efe0076..335fb59 100644 --- a/tests/fuzz/fuzz_fdt.c +++ b/tests/fuzz/fuzz_fdt.c @@ -33,7 +33,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) uint32_t len = (uint32_t)size; - if (eos_fdt_validate_sized(data, len) != 0) { + if (eos_fdt_validate(data, len) != 0) { return 0; } @@ -43,18 +43,18 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) * are exercised as well as the match. */ unsigned char buf[256]; uint32_t buf_len = sizeof buf; - (void)eos_fdt_get_prop_sized(data, len, "/chosen", "bootargs", + (void)eos_fdt_get_prop(data, len, "/chosen", "bootargs", buf, &buf_len); buf_len = sizeof buf; - (void)eos_fdt_get_prop_sized(data, len, "/", "compatible", + (void)eos_fdt_get_prop(data, len, "/", "compatible", buf, &buf_len); /* A one-byte buffer drives the -7 truncation path, which is the branch * that reports a length back to the caller. */ unsigned char tiny[1]; buf_len = sizeof tiny; - (void)eos_fdt_get_prop_sized(data, len, "/", "bootargs", tiny, &buf_len); + (void)eos_fdt_get_prop(data, len, "/", "bootargs", tiny, &buf_len); return 0; } diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c index 763cc09..cc8a1cd 100644 --- a/tests/unit/test_fdt_loader.c +++ b/tests/unit/test_fdt_loader.c @@ -135,20 +135,7 @@ static int get_prop_exact(const blob_t *b, const char *node, const char *prop, unsigned char *heap = malloc(b->len); ASSERT(heap != NULL); memcpy(heap, b->bytes, b->len); - int rc = eos_fdt_get_prop(heap, node, prop, out, out_len); - free(heap); - return rc; -} - -/* Same, but through the length-carrying entry point, passing the real - * allocation size rather than letting the blob declare it. */ -static int get_prop_sized_exact(const blob_t *b, const char *node, - const char *prop, void *out, uint32_t *out_len) -{ - unsigned char *heap = malloc(b->len); - ASSERT(heap != NULL); - memcpy(heap, b->bytes, b->len); - int rc = eos_fdt_get_prop_sized(heap, b->len, node, prop, out, out_len); + int rc = eos_fdt_get_prop(heap, b->len, node, prop, out, out_len); free(heap); return rc; } @@ -158,7 +145,7 @@ static int get_prop_sized_exact(const blob_t *b, const char *node, static void test_valid_tree_round_trips(void) { blob_t b; build_valid(&b, "ro"); - ASSERT(eos_fdt_validate(b.bytes) == 0); + ASSERT(eos_fdt_validate(b.bytes, b.len) == 0); char out[16]; uint32_t len = sizeof out; ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) == 0); @@ -178,7 +165,7 @@ static void test_struct_offset_past_the_blob_is_rejected(void) blob_t b; build_valid(&b, "ro"); set_field(&b, FIELD(off_dt_struct), 0x100000); - ASSERT(eos_fdt_validate(b.bytes) != 0); + ASSERT(eos_fdt_validate(b.bytes, b.len) != 0); char out[16]; uint32_t len = sizeof out; ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); } @@ -188,7 +175,7 @@ static void test_strings_offset_past_the_blob_is_rejected(void) blob_t b; build_valid(&b, "ro"); set_field(&b, FIELD(off_dt_strings), 0x100000); - ASSERT(eos_fdt_validate(b.bytes) != 0); + ASSERT(eos_fdt_validate(b.bytes, b.len) != 0); char out[16]; uint32_t len = sizeof out; ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); } @@ -198,7 +185,7 @@ static void test_struct_size_running_past_the_blob_is_rejected(void) blob_t b; build_valid(&b, "ro"); set_field(&b, FIELD(size_dt_struct), 0xFFFFFF00U); - ASSERT(eos_fdt_validate(b.bytes) != 0); + ASSERT(eos_fdt_validate(b.bytes, b.len) != 0); char out[16]; uint32_t len = sizeof out; ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); } @@ -207,7 +194,7 @@ static void test_totalsize_below_the_header_is_rejected(void) { blob_t b; build_valid(&b, "ro"); set_field(&b, FIELD(totalsize), 8); - ASSERT(eos_fdt_validate(b.bytes) != 0); + ASSERT(eos_fdt_validate(b.bytes, b.len) != 0); } static void test_property_length_past_the_struct_block_is_rejected(void) @@ -273,12 +260,12 @@ static void test_null_arguments_are_rejected(void) { blob_t b; build_valid(&b, "ro"); char out[16]; uint32_t len = sizeof out; - ASSERT(eos_fdt_validate(NULL) != 0); - ASSERT(eos_fdt_get_prop(NULL, "/", "bootargs", out, &len) != 0); - ASSERT(eos_fdt_get_prop(b.bytes, NULL, "bootargs", out, &len) != 0); - ASSERT(eos_fdt_get_prop(b.bytes, "/", NULL, out, &len) != 0); - ASSERT(eos_fdt_get_prop(b.bytes, "/", "bootargs", NULL, &len) != 0); - ASSERT(eos_fdt_get_prop(b.bytes, "/", "bootargs", out, NULL) != 0); + ASSERT(eos_fdt_validate(NULL, 0) != 0); + ASSERT(eos_fdt_get_prop(NULL, b.len, "/", "bootargs", out, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, b.len, NULL, "bootargs", out, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, b.len, "/", NULL, out, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, b.len, "/", "bootargs", NULL, &len) != 0); + ASSERT(eos_fdt_get_prop(b.bytes, b.len, "/", "bootargs", out, NULL) != 0); } /* The mirror image of the offset tests above. Those inflate an offset and @@ -296,19 +283,8 @@ static void test_an_inflated_totalsize_is_rejected_when_the_length_is_known(void char out[16]; uint32_t len = sizeof out; /* Told the truth about the buffer, the parser refuses it. */ - ASSERT(eos_fdt_validate_sized(b.bytes, honest) != 0); - ASSERT(get_prop_sized_exact(&b, "/", "bootargs", out, &len) != 0); -} - -/* The unsized entry points cannot catch the blob above, and this pins that so - * the difference is a documented contract rather than an accident. If this - * ever starts returning non-zero, eos_fdt_validate() has learned the length - * from somewhere and the warrant in the header should be removed. */ -static void test_the_unsized_entry_point_trusts_the_blobs_own_totalsize(void) -{ - blob_t b; build_valid(&b, "ro"); - set_field(&b, FIELD(totalsize), b.len + 0x10000); - ASSERT(eos_fdt_validate(b.bytes) == 0); + ASSERT(eos_fdt_validate(b.bytes, honest) != 0); + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) != 0); } /* A property that does not fit used to be copied short and reported as a @@ -351,7 +327,6 @@ int main(void) RUN(test_unterminated_node_name_is_rejected); RUN(test_null_arguments_are_rejected); RUN(test_an_inflated_totalsize_is_rejected_when_the_length_is_known); - RUN(test_the_unsized_entry_point_trusts_the_blobs_own_totalsize); RUN(test_a_property_too_large_for_the_buffer_is_not_truncated); RUN(test_a_property_that_exactly_fits_still_succeeds); printf("\n%d tests passed\n", tests_passed); From bc6f2231ce26545fb8a7a78617c67a1333a4c07b Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 23:07:15 +0530 Subject: [PATCH 6/9] ci: move the fuzz-harness build to its own PR The fuzz-build job added in the previous commit does its job -- it is red, on harnesses this PR did not write: undefined reference to `eos_fw_update_init' undefined reference to `eos_fw_update_process_chunk' undefined reference to `eos_recovery_parse_packet' None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and fuzz_recovery_protocol.c declare an API that was never written, exactly as eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever because nothing ever linked them. That is a real finding and it deserves a fix, not a red tick on an unrelated PR. The job and the repairs move to their own change; this PR keeps tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for. Refs #84 --- .github/workflows/ci.yml | 37 ------------------------------------- 1 file changed, 37 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0019343..1cfbad9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,43 +124,6 @@ jobs: retention-days: 90 # ── Static Analysis ─────────────────────────────────────────────────────── - # The fuzz harnesses are behind EBLDR_BUILD_FUZZ, which defaults OFF, and - # no job set it -- so tests/fuzz/ was never compiled anywhere. A harness - # that is not built cannot fail to build, which is how fuzz_devicetree came - # to declare a function that did not exist and sit there unnoticed. - # - # This does not fuzz. It builds the targets with clang and runs each for a - # few seconds over its own generated inputs, which is enough to catch a - # harness that no longer compiles or crashes immediately. A real campaign - # belongs in a scheduled workflow, not on every pull request. - fuzz-build: - name: Fuzz Harness Build - runs-on: ubuntu-22.04 - steps: - - uses: actions/checkout@v4 - - name: Build fuzz targets - run: | - set -euo pipefail - # EBLDR_BUILD_TESTS too: tests/fuzz/ is added from - # tests/CMakeLists.txt, so EBLDR_BUILD_FUZZ alone builds nothing and - # the job would pass having compiled no harness at all. - cmake -B build/fuzz -DEBLDR_BUILD_TESTS=ON -DEBLDR_BUILD_FUZZ=ON \ - -DCMAKE_C_COMPILER=clang -DCMAKE_BUILD_TYPE=Debug - cmake --build build/fuzz -j"$(nproc)" - - name: Smoke-run each harness - run: | - set -euo pipefail - shopt -s nullglob - harnesses=(build/fuzz/tests/fuzz/fuzz_*) - if [ ${#harnesses[@]} -eq 0 ]; then - echo "::error::EBLDR_BUILD_FUZZ=ON produced no harnesses" - exit 1 - fi - for f in "${harnesses[@]}"; do - echo "=== $(basename "$f") ===" - "$f" -max_total_time=5 -print_final_stats=1 - done - static-analysis: name: Static Analysis (cppcheck + clang-tidy) runs-on: ubuntu-22.04 From 436f2ca99d8a9aeb86402f7fe3a4b52347f7fe9f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Fri, 4 Sep 2026 11:47:08 +0530 Subject: [PATCH 7/9] fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage Answers the third review on #84. Finding 3 (Low, and the one that matters on hardware) -- the header was read through a cast fdt_header_t* and direct member loads, while every struct-block read goes through the fdt_read_u32() memcpy helper precisely because the blob may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an odd offset: nothing promises 4-byte alignment, and on a strict-alignment cross target a direct member load is the same fault class this parser exists to avoid. The unit suite could never catch it -- blob_t.bytes happens to be aligned. All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains `undefined` so the fuzz job checks alignment too. Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the walk skipped 4 bytes and treated whatever followed as the next token, so a struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a TCB parser that walks garbage to completion is accepting input it does not understand. FDT_NOP -- the one legal unknown, padding the spec allows between tokens -- is now named in the header and passes; anything else returns -6. Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101 (nothing added here; the harness is inert until that job lands and then compiled by it -- said on the thread, not just here), and the header no longer documents return code -8, which nothing at this head returns. #85 adds the -8 return and re-documents it together with the FDT_MAX_PATH_DEPTH move. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 16 tests PASS (was 13) discrimination, each fix in isolation: default: break restored -> test_a_garbage_tag_is_refused_not_skipped FAILS (expects -6, gets "not found") direct member load restored -> under UBSan the new unaligned-blob test reports "load of misaligned address 0x...671 for type 'const uint32_t'" at the exact line -- and cannot fire on the fixed code, which runs the same test clean NOP counter-check: interleaved FDT_NOP tokens still resolve, so the stricter default does not reject real trees. Refs #84 --- core/fdt_loader.c | 59 ++++++++++++++++------- include/eos_fdt_loader.h | 2 +- tests/fuzz/CMakeLists.txt | 2 +- tests/unit/test_fdt_loader.c | 90 ++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 19 deletions(-) diff --git a/core/fdt_loader.c b/core/fdt_loader.c index 235589e..0178bd5 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -8,6 +8,7 @@ #include "eos_fdt_loader.h" #include "eos_hal.h" +#include #include /* Big-endian to host conversion */ @@ -26,6 +27,17 @@ static uint32_t fdt_read_u32(const uint8_t *ptr) return fdt32_to_cpu(val); } +/* Header fields, read the same way the struct block is read. The blob may + * be unaligned -- fuzz input, a buffer inside a larger message, a copy at an + * odd offset -- and dereferencing a cast fdt_header_t* is a misaligned load + * on strict-alignment targets, the same fault class this parser exists to + * avoid. One rule for the whole blob: every multi-byte read goes through + * memcpy. */ +static uint32_t fdt_hdr_u32(const void *blob, size_t field_off) +{ + return fdt_read_u32((const uint8_t *)blob + field_off); +} + /* Does [off, off+len) sit inside a blob of totalsize bytes, without the * addition wrapping? Every offset in the header is attacker-controlled. */ static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize) @@ -39,16 +51,18 @@ int eos_fdt_validate(const void *fdt_blob, uint32_t avail) { if (!fdt_blob) return -1; if (avail < sizeof(fdt_header_t)) return -6; - const fdt_header_t *hdr = (const fdt_header_t *)fdt_blob; - if (fdt32_to_cpu(hdr->magic) != FDT_MAGIC) return -2; - if (fdt32_to_cpu(hdr->version) < 16) return -3; + if (fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, magic)) != FDT_MAGIC) + return -2; + if (fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, version)) < 16) + return -3; /* Before anything else: the blob does not get to say how big it is. Every * bound below is relative to totalsize, so without this the checks only * prove the header is self-consistent — a 40-byte buffer claiming a 1 MiB * totalsize with agreeing block offsets passes all of them, and the walk * then runs a megabyte past the allocation. */ - if (fdt32_to_cpu(hdr->totalsize) > avail) return -6; + if (fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, totalsize)) > avail) + return -6; /* magic and version alone say nothing about where the blob claims its * blocks are. The struct and string offsets are read straight out of @@ -56,13 +70,15 @@ int eos_fdt_validate(const void *fdt_blob, uint32_t avail) * above could still point them anywhere; eos_fdt_get_prop walked off the * end of a 40-byte allocation and took a bus fault. Reject the blob here * instead, because this is the gate every path goes through. */ - uint32_t totalsize = fdt32_to_cpu(hdr->totalsize); + uint32_t totalsize = fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, totalsize)); if (totalsize < sizeof(fdt_header_t)) return -6; - if (!fdt_block_in_bounds(fdt32_to_cpu(hdr->off_dt_struct), - fdt32_to_cpu(hdr->size_dt_struct), totalsize)) + if (!fdt_block_in_bounds(fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, off_dt_struct)), + fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, size_dt_struct)), + totalsize)) return -6; - if (!fdt_block_in_bounds(fdt32_to_cpu(hdr->off_dt_strings), - fdt32_to_cpu(hdr->size_dt_strings), totalsize)) + if (!fdt_block_in_bounds(fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, off_dt_strings)), + fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, size_dt_strings)), + totalsize)) return -6; return 0; } @@ -81,8 +97,7 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size) int rc = eos_fdt_validate(dest, max_size); if (rc != 0) return rc; - const fdt_header_t *hdr = (const fdt_header_t *)dest; - uint32_t total = fdt32_to_cpu(hdr->totalsize); + uint32_t total = fdt_hdr_u32(dest, offsetof(fdt_header_t, totalsize)); if (total > max_size) return -4; /* Copy full DTB */ @@ -102,11 +117,12 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, int vrc = eos_fdt_validate(fdt, fdt_len); if (vrc != 0) return vrc; - const fdt_header_t *hdr = (const fdt_header_t *)fdt; - const uint8_t *dt_struct = (const uint8_t *)fdt + fdt32_to_cpu(hdr->off_dt_struct); - const char *dt_strings = (const char *)fdt + fdt32_to_cpu(hdr->off_dt_strings); - uint32_t struct_size = fdt32_to_cpu(hdr->size_dt_struct); - uint32_t strings_size = fdt32_to_cpu(hdr->size_dt_strings); + const uint8_t *dt_struct = (const uint8_t *)fdt + + fdt_hdr_u32(fdt, offsetof(fdt_header_t, off_dt_struct)); + const char *dt_strings = (const char *)fdt + + fdt_hdr_u32(fdt, offsetof(fdt_header_t, off_dt_strings)); + uint32_t struct_size = fdt_hdr_u32(fdt, offsetof(fdt_header_t, size_dt_struct)); + uint32_t strings_size = fdt_hdr_u32(fdt, offsetof(fdt_header_t, size_dt_strings)); /* Simple linear search through struct block */ uint32_t offset = 0; @@ -197,8 +213,17 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, } case FDT_END: return -5; /* Property not found */ - default: + case FDT_NOP: + /* Legal padding; the spec allows it anywhere between tokens. */ break; + default: + /* Refuse, do not resynchronise. Skipping an unknown tag and + * treating whatever sits 4 bytes on as the next token meant a + * struct block of arbitrary bytes parsed to completion. Every + * read stayed bounded, so this was not memory-unsafe -- but a + * parser in the TCB that walks garbage to a clean "not found" + * is quietly accepting input it does not understand. */ + return -6; } } return -5; diff --git a/include/eos_fdt_loader.h b/include/eos_fdt_loader.h index f11cd39..ccc1e2d 100644 --- a/include/eos_fdt_loader.h +++ b/include/eos_fdt_loader.h @@ -19,6 +19,7 @@ extern "C" { #define FDT_BEGIN_NODE 0x00000001U #define FDT_END_NODE 0x00000002U #define FDT_PROP 0x00000003U +#define FDT_NOP 0x00000004U #define FDT_END 0x00000009U typedef struct { @@ -46,7 +47,6 @@ typedef struct { * -5 no such node or property * -6 the blob is malformed: an offset or length escapes it * -7 the property is larger than the caller's buffer (see below) - * -8 the node path is deeper than FDT_MAX_PATH_DEPTH components */ /** diff --git a/tests/fuzz/CMakeLists.txt b/tests/fuzz/CMakeLists.txt index db22d0b..633cf84 100644 --- a/tests/fuzz/CMakeLists.txt +++ b/tests/fuzz/CMakeLists.txt @@ -14,7 +14,7 @@ if(EBLDR_BUILD_FUZZ) return() endif() - set(FUZZ_FLAGS "-fsanitize=fuzzer,address -fno-omit-frame-pointer") + set(FUZZ_FLAGS "-fsanitize=fuzzer,address,undefined -fno-omit-frame-pointer") # --- fuzz_image_verify --- add_executable(fuzz_image_verify fuzz_image_verify.c) diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c index cc8a1cd..7b0095f 100644 --- a/tests/unit/test_fdt_loader.c +++ b/tests/unit/test_fdt_loader.c @@ -313,6 +313,93 @@ static void test_a_property_that_exactly_fits_still_succeeds(void) ASSERT(strcmp(out, "ro") == 0); } +/* An unrecognised tag used to be skipped -- the walk resynchronised 4 bytes + * on and a struct block of arbitrary bytes parsed to a clean "not found". + * Bounded, but a TCB parser that walks garbage to completion is accepting + * input it does not understand. Now only FDT_NOP passes. */ +static void test_a_garbage_tag_is_refused_not_skipped(void) +{ + blob_t b; + memset(&b, 0, sizeof(b)); + b.len = sizeof(fdt_header_t); + uint32_t off_struct = b.len; + append_be32(&b, FDT_BEGIN_NODE); append_padded(&b, ""); + append_be32(&b, 0xDEADBEEFU); /* not a token */ + append_be32(&b, FDT_END_NODE); + append_be32(&b, FDT_END); + uint32_t size_struct = b.len - off_struct; + uint32_t off_strings = b.len; + append_padded(&b, "bootargs"); + uint32_t size_strings = b.len - off_strings; + fdt_header_t *h = (fdt_header_t *)b.bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b.len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); + + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) == -6); +} + +/* And the one legal unknown, FDT_NOP, is padding the spec allows anywhere + * between tokens -- refusing it would reject real device trees. */ +static void test_nop_padding_does_not_break_resolution(void) +{ + blob_t b; + memset(&b, 0, sizeof(b)); + b.len = sizeof(fdt_header_t); + uint32_t off_struct = b.len; + append_be32(&b, FDT_NOP); + append_be32(&b, FDT_BEGIN_NODE); append_padded(&b, ""); + append_be32(&b, FDT_NOP); + append_be32(&b, FDT_PROP); append_be32(&b, 3); append_be32(&b, 0); + append_padded(&b, "ro"); + append_be32(&b, FDT_NOP); + append_be32(&b, FDT_END_NODE); + append_be32(&b, FDT_END); + uint32_t size_struct = b.len - off_struct; + uint32_t off_strings = b.len; + append_padded(&b, "bootargs"); + uint32_t size_strings = b.len - off_strings; + fdt_header_t *h = (fdt_header_t *)b.bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b.len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); + + char out[16]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/", "bootargs", out, &len) == 0); + ASSERT(strcmp(out, "ro") == 0); +} + +/* The blob arrives wherever it arrives -- fuzz input, a buffer inside a + * larger message -- and nothing promises 4-byte alignment. Header fields go + * through the same memcpy rule as the struct block now; this pins it by + * parsing from an odd offset, which UBSan's alignment check turns into a + * hard failure if a direct member load ever comes back. */ +static void test_an_unaligned_blob_parses(void) +{ + blob_t b; build_valid(&b, "ro"); + + unsigned char *heap = malloc(b.len + 1); + ASSERT(heap != NULL); + memcpy(heap + 1, b.bytes, b.len); + + char out[16]; uint32_t len = sizeof out; + ASSERT(eos_fdt_validate(heap + 1, b.len) == 0); + ASSERT(eos_fdt_get_prop(heap + 1, b.len, "/", "bootargs", out, &len) == 0); + ASSERT(strcmp(out, "ro") == 0); + free(heap); +} + int main(void) { printf("=== eBootloader FDT Loader Tests ===\n"); @@ -329,6 +416,9 @@ int main(void) RUN(test_an_inflated_totalsize_is_rejected_when_the_length_is_known); RUN(test_a_property_too_large_for_the_buffer_is_not_truncated); RUN(test_a_property_that_exactly_fits_still_succeeds); + RUN(test_a_garbage_tag_is_refused_not_skipped); + RUN(test_nop_padding_does_not_break_resolution); + RUN(test_an_unaligned_blob_parses); printf("\n%d tests passed\n", tests_passed); return 0; } From 745527648a8cef9bae60cb928d0f1e0ca7b461d3 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 1 Sep 2026 19:27:47 +0530 Subject: [PATCH 8/9] fix(fdt): resolve a node by its path, not by its depth and last name Replaces slash-counting and last-component matching with a component walk gated on ancestor matches, so /soc/uart and /decoy/uart are distinguishable. Also carries the fixes from the review on this PR: Finding 1 (High) -- the property arm tested in_target alone, and in_target is cleared only by the target's own FDT_END_NODE, so it stayed true for the entire subtree and a property found on a *child* was returned as the target's own. On this PR's own decoy tree, eos_fdt_get_prop(blob, "/soc", "reg", ...) returned 0 with "soc-uart" when /soc has no reg property at all and the correct answer is -5. Real device trees emit properties before subnodes, so a property that is present on the target is still found first -- the bug bites when the target lacks it, turning "not found" into a silently wrong value from a nested node. For an attacker-supplied blob the ordering is not enforced at all. Fixed by also requiring depth == target_depth, which is exact: depth is target_depth for the target's own properties and target_depth + 1 or deeper inside any child. Finding 2 (Low) -- test_the_parent_of_the_node_has_to_match_too asserted a second positive resolution rather than a rejection, so the suite read as though it had a negative case for wrong parents when the negative coverage actually lives in test_a_node_below_the_root_resolves. Renamed to test_each_uart_returns_its_own_value, and the load-bearing property (decoy is emitted before soc, so a last-component matcher would return "decoy-uart") is now stated on the test that depends on it. Finding 3 (Low) -- a path deeper than FDT_MAX_PATH_DEPTH returned -1, the same code as a NULL argument, so a caller could not tell a programming error from malformed input. Now -8, documented in the header's code table. Verified: ctest 21/21 PASS same under -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS test_fdt_loader 20 tests PASS (was 14) test_a_property_on_a_child_is_not_returned_as_the_parents, run against this same parser with only the depth guard reverted: [FAIL] get_prop_exact(&b, "/soc", "reg", out, &len) != 0 so it discriminates rather than restating the fix. Refs #85 --- core/fdt_loader.c | 67 ++++++++++++++---- tests/unit/test_fdt_loader.c | 128 +++++++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 14 deletions(-) diff --git a/core/fdt_loader.c b/core/fdt_loader.c index 0178bd5..14e1d8e 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -11,6 +11,9 @@ #include #include +/* Deepest node path eos_fdt_get_prop() will resolve. */ +#define FDT_MAX_PATH_DEPTH 16 + /* Big-endian to host conversion */ static uint32_t fdt32_to_cpu(uint32_t be) { @@ -130,12 +133,32 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, int target_depth = -1; bool in_target = false; - /* Count slashes to determine target depth */ - int path_depth = 0; - for (const char *p = node_path; *p; p++) { - if (*p == '/') path_depth++; + /* Split the path into components. The old code counted slashes and + * compared only the last component, which was wrong twice over: the + * root node already occupies depth 1, so "/chosen" looked for depth 1 + * -- the root itself -- and never matched, leaving "/" the only path + * that resolved; and matching the last component alone meant + * "/soc/uart" would have accepted any node named "uart" at that depth, + * whatever its parent. */ + const char *comp[FDT_MAX_PATH_DEPTH]; + uint32_t comp_len[FDT_MAX_PATH_DEPTH]; + int ncomp = 0; + for (const char *p = node_path; *p; ) { + while (*p == '/') p++; + if (!*p) break; + const char *start = p; + while (*p && *p != '/') p++; + /* -8, not -1: -1 means a caller passed NULL, which is a programming + * error, while a path deeper than we track is input. A caller that + * cannot tell them apart cannot handle either correctly. */ + if (ncomp >= FDT_MAX_PATH_DEPTH) return -8; + comp[ncomp] = start; + comp_len[ncomp] = (uint32_t)(p - start); + ncomp++; } - if (path_depth == 0) path_depth = 1; + /* The root is depth 1, so a path of n components ends at depth n + 1. */ + const int target_path_depth = ncomp + 1; + int matched = 0; /* offset < struct_size only guarantees one byte; every read below wants * four or more, so each is checked for the width it actually takes. */ @@ -155,20 +178,26 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, if (offset > struct_size) return -6; depth++; - /* Check if this node matches the target path */ - if (depth == path_depth) { - /* Simple check: compare last component */ - const char *last_slash = strrchr(node_path, '/'); - const char *target_name = last_slash ? last_slash + 1 : node_path; - if (strcmp(name, target_name) == 0 || target_name[0] == '\0') { - in_target = true; - target_depth = depth; + /* Advance along the requested path only while every ancestor + * has matched, so a node is found at its own path and not + * merely by its own name. */ + if (depth >= 2 && matched == depth - 2 && depth - 2 < ncomp) { + uint32_t want = comp_len[depth - 2]; + if ((uint32_t)strlen(name) == want && + strncmp(name, comp[depth - 2], want) == 0) { + matched = depth - 1; } } + if (!in_target && depth == target_path_depth && matched == ncomp) { + in_target = true; + target_depth = depth; + } break; } case FDT_END_NODE: if (in_target && depth == target_depth) in_target = false; + /* Leaving a node un-matches it for the branch we return to. */ + if (matched >= depth - 1 && depth >= 2) matched = depth - 2; /* An unbalanced blob would drive depth negative and let a later * BEGIN_NODE match path_depth at the wrong nesting level. */ if (depth == 0) return -6; @@ -192,7 +221,17 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, * followed the blob into buf. */ if (len > struct_size - offset) return -6; - if (in_target && strcmp(pname, prop_name) == 0) { + /* depth, not just in_target: in_target is cleared by the + * target's own END_NODE, so it stays true for the whole subtree + * and a property on a *child* was returned as the target's own. + * Real trees put properties before subnodes, so a property that + * is present on the target is still found first -- the bug bit + * when the target lacked it and "not found" became a silently + * wrong value from a nested node. depth is exactly target_depth + * for the target's own properties and target_depth + 1 or more + * inside any child. */ + if (in_target && depth == target_depth && + strcmp(pname, prop_name) == 0) { /* Truncating and returning 0 told the caller it had the whole * value. This path reads bootargs: a clipped string that * reports success drops whatever sat at its end, and nothing diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c index 7b0095f..c0e14c4 100644 --- a/tests/unit/test_fdt_loader.c +++ b/tests/unit/test_fdt_loader.c @@ -119,6 +119,54 @@ static void build_valid(blob_t *b, const char *value) put_be32((unsigned char *)&h->size_dt_strings, size_strings); } +/* / { soc { uart { reg = ; }; }; decoy { }; } + * + * "uart" sits at depth 3 under "soc", and a second node named "uart" is + * placed under "decoy" so that matching by last component alone would find + * the wrong one. */ +static void build_nested(blob_t *b, const char *value, const char *decoy) +{ + memset(b, 0, sizeof(*b)); + b->len = sizeof(fdt_header_t); + uint32_t off_struct = b->len; + uint32_t vlen = (uint32_t)strlen(value) + 1; + uint32_t dlen = (uint32_t)strlen(decoy) + 1; + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, ""); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "decoy"); + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart"); + append_be32(b, FDT_PROP); append_be32(b, dlen); append_be32(b, 0); + append_padded(b, decoy); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "soc"); + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart"); + append_be32(b, FDT_PROP); append_be32(b, vlen); append_be32(b, 0); + append_padded(b, value); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END); + uint32_t size_struct = b->len - off_struct; + + uint32_t off_strings = b->len; + append_padded(b, "reg"); + uint32_t size_strings = b->len - off_strings; + + fdt_header_t *h = (fdt_header_t *)b->bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b->len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); +} + static void set_field(blob_t *b, size_t field_offset, uint32_t v) { put_be32(b->bytes + field_offset, v); @@ -398,6 +446,80 @@ static void test_an_unaligned_blob_parses(void) ASSERT(eos_fdt_get_prop(heap + 1, b.len, "/", "bootargs", out, &len) == 0); ASSERT(strcmp(out, "ro") == 0); free(heap); +static void test_a_node_below_the_root_resolves(void) +{ + /* "/chosen" used to look for depth 1 -- the root -- so no path below + * the root ever resolved and only "/" worked. + * + * This assertion is also what proves the parent is matched, and that is + * load-bearing rather than incidental: build_nested emits "decoy" before + * "soc", so a matcher comparing only the last component would meet + * /decoy/uart first and return "decoy-uart". Getting "soc-uart" here + * means the ancestors were checked. */ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0); + ASSERT(strcmp(out, "soc-uart") == 0); +} + +static void test_each_uart_returns_its_own_value(void) +{ + /* Matching the last component alone would return decoy/uart's value + * for /soc/uart, whichever the walk reached first. */ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/decoy/uart", "reg", out, &len) == 0); + ASSERT(strcmp(out, "decoy-uart") == 0); +} + +static void test_a_path_that_is_not_in_the_tree_is_not_found(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/spi", "reg", out, &len) != 0); + len = sizeof out; + ASSERT(get_prop_exact(&b, "/nosuch/uart", "reg", out, &len) != 0); +} + +static void test_a_deeper_path_than_the_tree_is_not_found(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart/child", "reg", out, &len) != 0); +} + +/* The property arm used to test in_target alone, and in_target is only + * cleared by the target's own END_NODE -- so it stayed true through the whole + * subtree and a child's property came back as the parent's. /soc has no "reg" + * of its own; the correct answer is "not found", and what came back was + * /soc/uart's value. That is the same class of mistake this PR is named for: + * reading the wrong node's registers. */ +static void test_a_property_on_a_child_is_not_returned_as_the_parents(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc", "reg", out, &len) != 0); + + /* and the child itself still resolves, so the guard is not just + * refusing everything below the root */ + len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0); + ASSERT(strcmp(out, "soc-uart") == 0); +} + +/* A path deeper than FDT_MAX_PATH_DEPTH is input, not a programming error, + * and used to share -1 with "a caller passed NULL". */ +static void test_an_overlong_path_is_distinguishable_from_a_null_argument(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + + char deep[128] = ""; + for (int i = 0; i < 20; i++) strcat(deep, "/a"); + + ASSERT(get_prop_exact(&b, deep, "reg", out, &len) == -8); + len = sizeof out; + ASSERT(eos_fdt_get_prop(NULL, "/soc", "reg", out, &len) == -1); } int main(void) @@ -419,6 +541,12 @@ int main(void) RUN(test_a_garbage_tag_is_refused_not_skipped); RUN(test_nop_padding_does_not_break_resolution); RUN(test_an_unaligned_blob_parses); + RUN(test_a_node_below_the_root_resolves); + RUN(test_each_uart_returns_its_own_value); + RUN(test_a_path_that_is_not_in_the_tree_is_not_found); + RUN(test_a_deeper_path_than_the_tree_is_not_found); + RUN(test_a_property_on_a_child_is_not_returned_as_the_parents); + RUN(test_an_overlong_path_is_distinguishable_from_a_null_argument); printf("\n%d tests passed\n", tests_passed); return 0; } From df01368aada19101f0347ce11f60941e0be61173 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 22:56:59 +0530 Subject: [PATCH 9/9] fix(fdt): match node names past the unit address, where real device trees put it Replaces slash-counting and last-component matching with a component walk gated on ancestor matches, and matches a component against the node name up to its unit address -- `/soc/uart` resolves `uart@40011000`, an explicit `@40011000` still selects exactly that node, and an address not in the tree does not fall back to a loose match. Carries the earlier review fixes (depth == target_depth on the property arm; FDT_MAX_PATH_DEPTH moved to the public header; -8 for an over-deep path, distinct from -1) and, from the overnight round: - the stray `/* Deepest node path ... */` comment left in the .c after the define moved to the header is gone (Low) - return code -8 is documented in the header's code table again, on this branch, because this is the branch where anything returns it -- #84 dropped the line for exactly that reason Rebased onto #84's alignment/NOP round; the test-file merge keeps both sides' suites (garbage-tag, NOP-padding and unaligned-blob from #84; the node-path and unit-address suites from here). Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 24 tests PASS Refs #85 --- core/fdt_loader.c | 36 +++++++++++++--- include/eos_fdt_loader.h | 6 +++ tests/unit/test_fdt_loader.c | 82 +++++++++++++++++++++++++++++++++++- 3 files changed, 118 insertions(+), 6 deletions(-) diff --git a/core/fdt_loader.c b/core/fdt_loader.c index 14e1d8e..de90ac8 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -11,9 +11,6 @@ #include #include -/* Deepest node path eos_fdt_get_prop() will resolve. */ -#define FDT_MAX_PATH_DEPTH 16 - /* Big-endian to host conversion */ static uint32_t fdt32_to_cpu(uint32_t be) { @@ -50,6 +47,36 @@ static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize) return off <= totalsize - len; } +/* Does a device tree node name match one path component? + * + * Real node names carry a unit address -- `uart@40011000`, `serial@10000000` + * -- while a path is usually written without one. Exact string equality meant + * /soc/uart resolved only against trees whose peripherals happen to be named + * without an address, which is to say against hand-built test blobs and not + * against any actual DTB. (/ and /chosen were unaffected: by convention they + * carry no unit address, which is why the bootargs path this parser exists to + * serve kept working and the gap went unnoticed.) + * + * A component containing no '@' matches the name up to its '@', so both + * /soc/uart and /soc/uart@40011000 resolve. A component that does contain one + * is compared in full, so an explicit address still selects exactly the node + * asked for -- which matters when a tree has several of the same peripheral. + */ +static bool fdt_name_matches(const char *name, const char *comp, + uint32_t comp_len) +{ + uint32_t full = (uint32_t)strlen(name); + + if (memchr(comp, '@', comp_len) != NULL) { + return full == comp_len && strncmp(name, comp, comp_len) == 0; + } + + const char *at = memchr(name, '@', full); + uint32_t bare = at ? (uint32_t)(at - name) : full; + + return bare == comp_len && strncmp(name, comp, comp_len) == 0; +} + int eos_fdt_validate(const void *fdt_blob, uint32_t avail) { if (!fdt_blob) return -1; @@ -183,8 +210,7 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, * merely by its own name. */ if (depth >= 2 && matched == depth - 2 && depth - 2 < ncomp) { uint32_t want = comp_len[depth - 2]; - if ((uint32_t)strlen(name) == want && - strncmp(name, comp[depth - 2], want) == 0) { + if (fdt_name_matches(name, comp[depth - 2], want)) { matched = depth - 1; } } diff --git a/include/eos_fdt_loader.h b/include/eos_fdt_loader.h index ccc1e2d..d68bfa5 100644 --- a/include/eos_fdt_loader.h +++ b/include/eos_fdt_loader.h @@ -15,6 +15,11 @@ extern "C" { #endif +/* Longest node path this parser resolves, in components. A caller cannot + * act on return code -8 without knowing the limit, so it is declared here + * rather than privately in fdt_loader.c. */ +#define FDT_MAX_PATH_DEPTH 16 + #define FDT_MAGIC 0xD00DFEEDU #define FDT_BEGIN_NODE 0x00000001U #define FDT_END_NODE 0x00000002U @@ -47,6 +52,7 @@ typedef struct { * -5 no such node or property * -6 the blob is malformed: an offset or length escapes it * -7 the property is larger than the caller's buffer (see below) + * -8 the node path is deeper than FDT_MAX_PATH_DEPTH components */ /** diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c index c0e14c4..3a68ae5 100644 --- a/tests/unit/test_fdt_loader.c +++ b/tests/unit/test_fdt_loader.c @@ -167,6 +167,50 @@ static void build_nested(blob_t *b, const char *value, const char *decoy) put_be32((unsigned char *)&h->size_dt_strings, size_strings); } +/* Like build_nested, but with the unit addresses a real device tree carries. + * Exact-equality matching resolved nothing here, which is the case the PR's + * own headline example describes and could not do. */ +static void build_addressed(blob_t *b, const char *v0, const char *v1) +{ + memset(b, 0, sizeof(*b)); + b->len = sizeof(fdt_header_t); + uint32_t off_struct = b->len; + uint32_t l0 = (uint32_t)strlen(v0) + 1; + uint32_t l1 = (uint32_t)strlen(v1) + 1; + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, ""); + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "soc"); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart@40011000"); + append_be32(b, FDT_PROP); append_be32(b, l0); append_be32(b, 0); + append_padded(b, v0); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart@40004400"); + append_be32(b, FDT_PROP); append_be32(b, l1); append_be32(b, 0); + append_padded(b, v1); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END); + uint32_t size_struct = b->len - off_struct; + + uint32_t off_strings = b->len; + append_padded(b, "reg"); + uint32_t size_strings = b->len - off_strings; + + fdt_header_t *h = (fdt_header_t *)b->bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b->len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); +} + static void set_field(blob_t *b, size_t field_offset, uint32_t v) { put_be32(b->bytes + field_offset, v); @@ -446,6 +490,8 @@ static void test_an_unaligned_blob_parses(void) ASSERT(eos_fdt_get_prop(heap + 1, b.len, "/", "bootargs", out, &len) == 0); ASSERT(strcmp(out, "ro") == 0); free(heap); +} + static void test_a_node_below_the_root_resolves(void) { /* "/chosen" used to look for depth 1 -- the root -- so no path below @@ -519,7 +565,39 @@ static void test_an_overlong_path_is_distinguishable_from_a_null_argument(void) ASSERT(get_prop_exact(&b, deep, "reg", out, &len) == -8); len = sizeof out; - ASSERT(eos_fdt_get_prop(NULL, "/soc", "reg", out, &len) == -1); + ASSERT(eos_fdt_get_prop(NULL, b.len, "/soc", "reg", out, &len) == -1); +} + +/* The headline case: a path written without a unit address, against a tree + * that has one. Exact string equality never resolved this, so /soc/uart did + * not work on any real DTB -- only on trees built without addresses, which is + * what every other fixture in this file constructs. */ +static void test_a_path_without_a_unit_address_resolves_a_node_with_one(void) +{ + blob_t b; build_addressed(&b, "uart0", "uart1"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0); + /* The first match wins, as it does for any other duplicate name. */ + ASSERT(strcmp(out, "uart0") == 0); +} + +/* And an explicit address still selects exactly the node asked for, which is + * what makes the loose match safe on a tree with several of a peripheral. */ +static void test_an_explicit_unit_address_selects_that_node(void) +{ + blob_t b; build_addressed(&b, "uart0", "uart1"); + char out[24]; uint32_t len = sizeof out; + + ASSERT(get_prop_exact(&b, "/soc/uart@40004400", "reg", out, &len) == 0); + ASSERT(strcmp(out, "uart1") == 0); + + len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart@40011000", "reg", out, &len) == 0); + ASSERT(strcmp(out, "uart0") == 0); + + /* An address that is not in the tree must not fall back to a loose match. */ + len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart@deadbeef", "reg", out, &len) != 0); } int main(void) @@ -547,6 +625,8 @@ int main(void) RUN(test_a_deeper_path_than_the_tree_is_not_found); RUN(test_a_property_on_a_child_is_not_returned_as_the_parents); RUN(test_an_overlong_path_is_distinguishable_from_a_null_argument); + RUN(test_a_path_without_a_unit_address_resolves_a_node_with_one); + RUN(test_an_explicit_unit_address_selects_that_node); printf("\n%d tests passed\n", tests_passed); return 0; }