fix(fdt): bound a device tree parser that was never compiled by Kartikey1306 · Pull Request #84 · embeddedos-org/eBoot

Kartikey1306 · 2026-09-01T10:32:15Z

The file is not in the build

core/fdt_loader.c appears in no source list, so it has never been compiled. include/eos_fdt_loader.h offers its four functions to callers and eos_fdt_pass_to_kernel names rtos_boot.c as the consumer — but nothing checks what is in it: not the compiler, not ctest, not the sanitizer job.

$ for f in core/*.c; do grep -rqF "$(basename $f)" --include=CMakeLists.txt . || echo "ORPHAN $f"; done
ORPHAN core/fdt_loader.c

What is in it is a parser for a blob that comes out of flash, which uses every header field as an offset or a length without checking any of them.

Reachable today with a malformed blob

eos_fdt_validate() checks magic and version and nothing else, so it accepts a blob whose off_dt_struct points anywhere:

hdr.totalsize     = 40;        /* the header alone */
hdr.off_dt_struct = 0x100000;  /* 1 MiB past a 40-byte allocation */

eos_fdt_validate(blob);   /* returns 0 — accepted */
eos_fdt_get_prop(...);

==21905==ERROR: AddressSanitizer: BUS on unknown address
    #1 fdt_read_u32 fdt_loader.c:25
    #2 eos_fdt_get_prop fdt_loader.c:82

Five more hold once you are inside the loop:


tag read	takes 4 bytes where `offset < struct_size` guarantees 1
node name	`strlen()` reads until it finds a zero — past the block for a name that runs to its end
`nameoff`	indexes the strings block unchecked, so `strcmp()` reads an arbitrary address
property `len`	clamped to the caller's buffer before the memcpy — that bounds the write, not the read, so an oversized `len` copies whatever follows the blob out to the caller
`FDT_END_NODE`	decrements `depth` with no floor

The fix

validate() is the gate every path goes through, so the block offsets are bounded against totalsize there, and get_prop() calls it before trusting the header. Inside the loop, each read is checked for the width it actually takes, the name and property-name scans are bounded with memchr inside their own blocks, and the padded advances are re-checked for overrun.

Wiring it in

core/fdt_loader.c joins eboot_core; tests/unit/test_fdt_loader.c joins ctest — ten cases: one well-formed tree that must still parse, and nine malformed ones.

Run against the unfixed parser the suite fails on the third case (validate() accepts the out-of-bounds struct offset). With the bounds in place:

20/20 Test #20: test_fdt_loader .................. Passed
100% tests passed out of 20        # 19 before

and 20/20 again under -DEBLDR_SANITIZE=ON.

Left alone deliberately

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. That is a behaviour change rather than a safety one, so it is not in this PR; the tests query "/" and say so in a comment rather than encoding the broken form.

Touches the same CMakeLists.txt region as #82, which adds core/secure_boot.c. Trivial to resolve in whichever lands second.

🤖 Generated with Claude Code

codecov-commenter · 2026-09-01T10:33:31Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Kartikey1306 · 2026-09-01T15:01:23Z

Hi @srpatcha @hshanmug12, just following up on PR #84 whenever you get a chance to review it. All CI checks are passing, and the PR is currently just waiting on an approving review. Would appreciate your feedback when you have some time. Thanks!

srpatcha

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

head: 49e1cc1 author: Kartikey1306 ci: pass

Verdict: Adds bounds checks to core/fdt_loader.c, puts the file into eboot_core, and adds 10 unit cases. The direction is right and the loop-internal checks are correct, but the central gate is still bounded against a length the blob itself supplies, so the primary entry point is not actually bounded — and the PR body overstates today's reachability.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	`core/fdt_loader.c:30-52` (`eos_fdt_validate`)	Every new bound is relative to `hdr->totalsize`, which is read out of the same untrusted blob. `eos_fdt_validate()` takes only a pointer, so it has no way to learn how many bytes were actually mapped. The PR's own reproducer (`totalsize=40`, `off_dt_struct=0x100000`) is now rejected, but the inverse is not: a 40-byte buffer whose header claims `totalsize=0x100000` with internally consistent block offsets passes `validate()`, and `eos_fdt_get_prop()` then walks up to 1 MiB past the allocation. `eos_fdt_load()` is safe because it clamps `total > max_size`, but `get_prop()` is a public entry point that can be handed a DTB pointer from ROM or a prior stage, and the new test suite itself calls it directly on a bare pointer. The hardening does not close the class of bug it is written against.	Give the size-bearing entry points a real length. Add `int eos_fdt_validate_sized(const void *blob, uint32_t avail)` that checks `totalsize <= avail` before anything else, make `eos_fdt_validate()` a wrapper that documents in `include/eos_fdt_loader.h` that the caller warrants at least `totalsize` bytes, and add a `uint32_t fdt_len` parameter to `eos_fdt_get_prop()`. Add the mirror-image test: a blob with an inflated `totalsize` in an exactly-sized heap allocation, which is the case `get_prop_exact()` is already set up to catch.
2	Medium	`core/fdt_loader.c:135-139`	`copy_len = len < buf_len ? len : buf_len` then `return 0`. A property larger than the caller's buffer is silently truncated and reported as success; the caller cannot distinguish a complete value from a clipped one. In a boot path this reads `bootargs`, so a long argument string silently loses its tail — including whatever policy flag happened to sit at the end. Pre-existing, but this is the exact block the PR rewrites and it is the failure path the rest of the PR is about.	Return a distinct code (e.g. `-7`) when `len > buf_len`, and set `buf_len = len` so the caller can size a retry. Add a test asserting the truncating call does not return 0.
3	Medium	`tests/unit/test_fdt_loader.c` (whole file), `tests/fuzz/`	`.ai/security.md` names device tree explicitly: "Every externally reachable parser is attack surface: manifests, TLVs, OTA payloads, IPC frames, network packets, device tree ... These paths get fuzz coverage, not just unit tests." The repo already has the harness pattern — `tests/fuzz/fuzz_image_verify.c`, `fuzz_bootctl.c`, `fuzz_crypto.c`, `fuzz_fw_update.c`, `fuzz_recovery_protocol.c` behind `EBLDR_BUILD_FUZZ`. This PR adds ten hand-written blobs and no fuzz target, so the parser gets less coverage than every other untrusted input in the tree.	Add `tests/fuzz/fuzz_fdt.c` alongside the existing five: take `data`/`size`, call `eos_fdt_validate()` then `eos_fdt_get_prop(data, "/", "bootargs", buf, &len)`, and register it in `tests/fuzz/CMakeLists.txt`. With finding 1 fixed it can pass `size` as the real length, which is what makes the harness meaningful.
4	Low	PR body, "Reachable today with a malformed blob"	Not reachable today, in either sense. Verified on `master` (`13a7a02`): `grep -rn "fdt_loader" --include=CMakeLists.txt --include=.cmake --include=Makefile --include=.mk .` returns nothing, and `grep -rn "eos_fdt_" --include=.c --include=.h .` returns no hit outside `core/fdt_loader.c` and `include/eos_fdt_loader.h`. The file is neither compiled nor called, so the quoted `AddressSanitizer: BUS` trace cannot have come from any shipped build — it came from a harness written for this PR. The body also says "`eos_fdt_pass_to_kernel` names `rtos_boot.c` as the consumer"; that is a comment inside the function body (`core/fdt_loader.c:134-138`), not a call. The defect is real and worth fixing; the exposure claim is not supported.	Reword to "unreachable today because nothing links or calls the file; reachable as soon as a consumer lands, which is why it is being bounded before that happens." State that the trace is from the new test harness.
5	Low	`CMakeLists.txt:93`	`core/fdt_loader.c` joins `eboot_core`, which links it into the bootloader image, while grep shows zero consumers. Master design §5.1: "eBoot keeps the trusted computing base minimal and auditable." This adds ~140 lines of untrusted-input parser plus its flash cost to the TCB to serve a test. Compiling and testing the orphan is clearly better than leaving it uncompiled, so this is a judgment call, not a blocker — but the version that satisfies both is available.	Build it as an `OBJECT` library (or link the test against `core/fdt_loader.c` directly) so CI, cppcheck/clang-tidy and the sanitizer job all cover it without putting it in the boot image, then move it into `eboot_core` in the PR that adds the first real caller.
6	Low	`core/fdt_loader.c:58-61`	`if (total < sizeof(fdt_header_t)) return -4;` is unreachable: the `eos_fdt_validate(dest)` call at line 46 already returns `-6` for that blob, so control never arrives here with `total < 40`. The comment ("the header copied above is all that was read: re-check against what the caller actually offered") misstates what the check does — it re-checks the same field against the same constant, not against `max_size`. Harmless, but misleading comments in TCB code are a cost, since §5.1 asks for the TCB to be auditable.	Drop the check, or keep it and correct the comment to "defence in depth: validate() already rejects this, restated so a future reordering cannot drop the bound."

Architecture conformance

Conforms. §21 Tier 1 — Foundation (eos, eBoot, ebuild, EoSim); device-tree parsing is boot-time work and belongs in eBoot, not above it. core/ is the right subdirectory per .ai/architect.md ("core/ shared boot logic"); this is target-independent parsing with no board specifics, so boards/ and hal/ are correctly untouched. No new include, link line or target_link_libraries entry points up a tier: the only new dependency is <string.h> (memchr), and the file's includes remain eos_fdt_loader.h, eos_hal.h, <string.h>. §8 places manifest and image verification in Stage 1 and this is neither, so no Stage 0/Stage 1 boundary is crossed. §5.1's "minimal and auditable TCB" is the one clause in tension, per finding 5.

§8.1 lists the required boot concepts and says nothing about validating externally supplied blobs before parsing them, even though that is the whole subject of this PR. That gap is a design-document gap, not a PR defect — proposal appended to .ai/autoreview/proposals/2026-09.md.

Proposed changes

Smallest sequence that keeps every target building:

Length-carrying API (finding 1), the only change that alters the header:

/* include/eos_fdt_loader.h */
int eos_fdt_validate_sized(const void *fdt_blob, uint32_t avail);
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);

eos_fdt_validate() and eos_fdt_get_prop() stay as wrappers, documented as trusting the blob's own totalsize, so nothing existing breaks. In eos_fdt_validate_sized(), if (fdt32_to_cpu(hdr->totalsize) > avail) return -6; goes before the block checks. eos_fdt_load() calls the sized form with max_size.

Truncation code (finding 2), 3 lines plus one test.
tests/fuzz/fuzz_fdt.c + registration (finding 3).
Move core/fdt_loader.c from eboot_core to an object library used by tests only (finding 5); revisit when a caller lands.
Drop the dead check at core/fdt_loader.c:58-61 (finding 6).
Correct the reachability paragraph in the PR body (finding 4).

Findings 2, 3, 5 and 6 are independent of each other and of 1. Deferring the node-path defect to #85 is the right call and is disclosed both in the body and in the test comment at tests/unit/test_fdt_loader.c:250-254 — no objection.

Not checked

The test results were not reproduced. The PR quotes 20/20 tests passed and a failure on the third case against the unfixed parser. I did not run it: doing so requires checking out the PR head in /home/srpatcha/eos/eBoot, which the run brief forbids. The 24 CI checks in checks.txt are all pass (Host Build & Tests, Build & Test (Linux x86_64), Static Analysis (cppcheck + clang-tidy), CodeQL, 11 EoSim targets, 2 cross-compiles, 3 Cross-Platform), which is independent evidence the suite builds and passes at this head — it is not evidence for the "fails on the third case before the fix" half of the claim, which no CI job can show. Create GitHub Release reports skipping, which is expected on a PR.
Sanitizer claim not reproduced. "20/20 again under -DEBLDR_SANITIZE=ON" — I found no job named for that flag in checks.txt, so this is asserted, not shown by CI. Whether the sanitizer job actually covers the new file was not determined.
Overflow at extreme sizes not traced to a conclusion. offset += (name_len + 3) & ~3U at core/fdt_loader.c:103 wraps if name_len >= 0xFFFFFFFD, which needs a ~4 GiB size_dt_struct. Not reachable on any target in §22's tiers, and the preceding memchr over that span would fault first. Recorded, not raised.
off_mem_rsvmap is never bounds-checked, in this PR or before it. Currently unused by any code path in the file, so not raised — but it becomes a finding the moment the reserve map is read.
Merge conflict with #82 as the body notes (CMakeLists.txt region adding core/secure_boot.c). mergeStateStatus is BLOCKED / mergeable: MERGEABLE at this head; I did not attempt the merge.

Automated architecture review of 49e1cc1a019c — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

…s claim Follow-up to the review on embeddedos-org#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 embeddedos-org#84

…rged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — embeddedos-org#93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. embeddedos-org#87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) embeddedos-org#93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — embeddedos-org#86 and embeddedos-org#57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only embeddedos-org#57's public_key_is_valid_subgroup() is wired to the call site, so embeddedos-org#86's key_has_prime_order() was dead. Kept the live function, folded embeddedos-org#86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on embeddedos-org#86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight.

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) <noreply@anthropic.com>

…s claim Follow-up to the review on embeddedos-org#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 embeddedos-org#84

Answers the review on embeddedos-org#89. Finding 1 (High) -- the digest was printf'd and never compared to anything, so the drift guard the PR is named for did not exist. Worse, it could not: EOS_ED25519_CONTRACT_DIGEST came from the header this repo's own generator had just written, a hash taken over its own output. Editing the generator on one side produced a self-consistent corpus with a new digest, a green test, and a divergence visible only to a human reading two CI logs in two repositories. Three places asserted the guarantee -- the PR body, this file's docblock, and the generator's emitted header comment -- and none implemented it. The unused <string.h> include was the strcmp that never got written. Now pinned as a literal in committed source, with the vector count and the accept count beside it, because those are generated too: a corpus that shrank from 76 to 40, or lost two of its three RFC 8032 positives, previously passed. positives == 0 only fired when the last positive went. Finding 2 (Medium) -- tests/vectors/ed25519_contract_vectors.h is now committed rather than generated into the build tree. What the TCB's signature verifier was tested against should be visible from a tag. The reason it was generated -- two repos holding the same file and keeping it in step by hand -- is what the pinned digest now handles. That also drops find_package(Python3 ... REQUIRED), which had made a Python interpreter a hard configure-time dependency of a pure-C test suite. Finding 3 (Medium) -- embeddedos-org#86 has merged, so this branch is rebased onto master and reduced to its own three files. It previously carried all of embeddedos-org#84 and embeddedos-org#85 (device tree parsing, +475 lines, unrelated to Ed25519) plus embeddedos-org#86's verifier change, none of which was declared in the body. Verified: ctest 22/22 PASS test_ed25519_contract 76 vectors, 3 accept, 73 refuse the guard now discriminates: adding one vector to the generator gives [FAIL] corpus digest changed expected 1059febe...282a2 got e08b0632...28905 and exit 1. Before this commit the same edit printed a different digest and exited 0. cmake configure with no Python on PATH succeeds (no find_package) Refs embeddedos-org#89

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 embeddedos-org#84

srpatcha

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

head: 9ff5a43 author: Kartikey1306 ci: pass

Verdict: The FDT bounds work is correct, well-targeted and now actually built — I reproduced the build, the 22/22 ctest run and the same result under -DEBLDR_SANITIZE=ON. The problem is scope: this PR also carries an undisclosed repair of the broken origin/master in core/ed25519_verify.c and include/eos_image.h, which is the entire stated purpose of #94.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	`core/ed25519_verify.c` (−44/+12), `include/eos_image.h` (+10/−6), `tests/unit/test_ed25519.c` (+94/−32)	The PR body describes only the FDT parser. It does not mention that the diff deletes `key_has_prime_order()` and a duplicate `point_is_identity()` from the Ed25519 verifier, rewrites the low-order-key test vectors, and re-points the `eos_image_header_t` static asserts from `reserved[]` onto `tlv_len`/`tlv_hash`. That is a change to secure-boot signature verification arriving under a title that says "fdt".	State it in the body, or drop it and rebase once #94 lands. See the note below on why this matters more than usual here.
2	Medium	`core/fdt_loader.c:93`, `core/fdt_loader.c:213`	`eos_fdt_validate()` and `eos_fdt_get_prop()` keep an out-of-bounds read by design: both dereference `hdr->totalsize` before any length is known, then hand that attacker-controlled value to the `_sized` form as the bound. The header documents this as a caller warrant, but a warrant is not a check, and this is the exact bug the PR is fixing. Neither has an in-tree caller (`grep -rn eos_fdt_get_prop --include=*.c` → only the definition).	Delete both, or make them `static`/internal. Leaving an exported unsafe twin in a TCB header means a future boot-path caller reintroduces the bug with no compiler complaint.
3	Medium	`tests/fuzz/fuzz_fdt.c`, `tests/fuzz/CMakeLists.txt:51`	The new fuzz harness is added to a directory no CI job ever builds. `EBLDR_BUILD_FUZZ` is `OFF` by default (`CMakeLists.txt:29`) and appears in no file under `.github/workflows/` — verified by grep, and `checks.txt` has no fuzz job among its 24 entries. `fuzz_fdt` joins five existing harnesses that are likewise never compiled. This is the same defect the PR opens by describing: a source file in no build. `.ai/security.md` requires device tree to get "fuzz coverage, not just unit tests"; this does not deliver that yet.	Add a workflow job running `cmake -DEBLDR_BUILD_FUZZ=ON -DCMAKE_C_COMPILER=clang` plus a bounded `-max_total_time` run of each target. Pre-existing for the other five, so a separate PR is reasonable — but the claim in the file header should not read as though coverage now exists.
4	Low	`core/fdt_loader.c:205`	`default: break;` treats any unrecognised 32-bit tag as a 4-byte NOP. `FDT_NOP` legitimately behaves that way, but so now does arbitrary garbage: a blob of random bytes is walked to the end and returns `-5` (not found) rather than `-6` (malformed). Memory-safe, but not fail-closed, and the DTB is unverified flash content.	Accept `FDT_NOP` explicitly and return `-6` from `default:`.
5	Low	`core/fdt_loader.c:132`, `core/fdt_loader.c:168`	`while (offset + 4 <= struct_size)` and `if (offset + 8 > struct_size)` wrap when `struct_size` approaches `UINT32_MAX`. Reachable only through `_sized` with an `avail` near 4 GiB, so not a real eBoot configuration; the effect would be a non-terminating walk, not an OOB read.	Compare as `offset > struct_size - 4` after establishing `struct_size >= 4`, consistent with the `len > struct_size - offset` idiom already used at line 181.

On finding 1. Four open PRs — #84, #85, #89 and #94 — each independently repair the same broken master. Whichever merges after the first will conflict inside core/ed25519_verify.c. That file is broken today precisely because #57, #86 and #92 landed overlapping subgroup checks and the resolution left two definitions of point_is_identity() behind. Repeating that resolution three more times, in TCB crypto, under three unrelated PR titles, is how the current breakage happened. It is worth naming rather than absorbing.

Verification I ran

origin/master at 22d8f8b does not compile:

$ cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug -DEBLDR_BUILD_TESTS=ON && cmake --build build
include/eos_image.h:135:23: error: 'eos_image_header_t' has no member named 'reserved'
include/eos_image.h:142:55: error: 'eos_image_header_t' has no member named 'reserved'
gmake: *** [Makefile:101: all] Error 2

$ gcc -fsyntax-only -Iinclude core/ed25519_verify.c
core/ed25519_verify.c:338:12: error: redefinition of 'point_is_identity'
core/ed25519_verify.c:281:12: note: previous definition ...

This PR's head builds clean and passes:

$ cmake --build build -j4          # 0 errors
$ ctest --no-tests=error
100% tests passed, 0 tests failed out of 22      # incl. test_fdt_loader

$ cmake -DEBLDR_SANITIZE=ON ... && ctest --no-tests=error
100% tests passed, 0 tests failed out of 22

So the body's "20/20" is stale (22 tests now, after later merges) but the substance holds.

One thing the diff fixes that the body does not claim: tests/unit/test_ed25519.c previously hardcoded tests_run = 11 at the end of main(), so the pass/fail comparison was against a constant rather than a count, and run_test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery() was defined but never called. Both are corrected here. That is a weakened check being restored, and it deserves to be in the body rather than found by reading the diff.

Architecture conformance

Master design §8 (eBoot as trusted startup platform) and §5.1 ("eBoot keeps the trusted computing base minimal and auditable"): conforms. No new dependency in any direction — core/fdt_loader.c includes only eos_fdt_loader.h and libc, and nothing in the diff reaches up a tier. File placement matches the .ai/architect.md eBoot layout (core/, include/, tests/unit/, tests/fuzz/). Tier 1 – Foundation per §21, correct repo.

.ai/security.md "Input validation" names device tree explicitly as an externally reachable parser that must have every buffer write bounded and defined behaviour for malformed, truncated and oversized input. Before this PR that parser was not compiled at all. This moves eBoot toward that requirement; findings 2–4 are what stands between this and meeting it.

One gap the design itself does not cover. §8.1 lists signed manifests and signed images as required boot concepts and says nothing about the device tree. eos_fdt_load() copies a DTB out of flash and eos_fdt_pass_to_kernel() hands its address to the kernel with no signature or hash check anywhere in the path. Bounding the parser makes a malformed DTB safe to parse; it does not make a substituted DTB safe to act on, and bootargs is a kernel command line. That is a hole in the master design rather than in this PR — proposal appended to .ai/autoreview/proposals/2026-09.md.

Proposed changes

Smallest sequence that keeps everything building:

Amend the PR body to state the ed25519_verify.c / eos_image.h / test_ed25519.c changes and why they are here (master does not compile without them). No code change.
Delete eos_fdt_validate() and eos_fdt_get_prop() from include/eos_fdt_loader.h and core/fdt_loader.c; they have no callers. If they must stay for an out-of-tree consumer, mark them deprecated in the header rather than only describing the hazard in prose.
In eos_fdt_get_prop_sized(), add case FDT_NOP: break; and make default: return -6. tests/unit/test_fdt_loader.c gains one case: a blob whose struct block is random bytes must return -6, not -5.
Separately (not this PR): a CI job that builds and briefly runs the six fuzz targets.

Steps 2 and 3 are each a few lines and covered by the suite already in this PR.

Not checked

Merge order against #85, #89, #94. I read all four heads but did not attempt the merges, so I cannot tell you which conflicts are textual and which are semantic.
The fuzz harness was never executed. fuzz_fdt.c compiles under clang -fsanitize=fuzzer,address, but linking failed on this host — libclang_rt.fuzzer.a is not installed. So I confirmed it builds; I did not confirm it finds nothing, and no CI job would have either.
Cross-compiled targets. I built host x86-64 only. The eight cross jobs and eleven EoSim jobs in checks.txt are reported green; I took that at face value rather than reproducing it.
The -7 truncation contract. Correct for current in-tree code because eos_fdt_get_prop() has no callers. If an out-of-tree consumer exists that checks only rc == 0 and then trusts *buf_len, this change hands it a length larger than its buffer. I could not rule that out from this repository.
Whether the low-order test vectors are the eight canonical points. The comment says each order was computed rather than copied. I did not recompute them; the test passes, which shows they are all rejected, not that the array is the set it claims to be.

Automated architecture review of 9ff5a432f5c6 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

…harnesses Answers the second review on embeddedos-org#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: embeddedos-org#90 adds a `CI Gate` whose needs list is [test, build-arm, static-analysis]. Whichever of embeddedos-org#84 and embeddedos-org#90 lands second must add fuzz-build to that list -- and embeddedos-org#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 embeddedos-org#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 embeddedos-org#84

srpatcha

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

head: cf4bf4f author: Kartikey1306 ci: fail

Verdict: The FDT bounds work is correct and lands in the right place — every read in eos_fdt_get_prop is now checked for the width it actually takes, and eos_fdt_validate gained the caller-supplied length that makes the header's self-declared totalsize non-authoritative. Two things block it: the CI job this PR itself adds is red, and the PR silently carries a master repair to core/ed25519_verify.c and include/eos_image.h that is the entire scope of open PRs #94 and #97.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	`.github/workflows/ci.yml:18`	The `Fuzz Harness Build` job added here fails on this head. `tests/fuzz/fuzz_fw_update.c:18-19` forward-declares `eos_fw_update_init` and `eos_fw_update_process_chunk`, and `tests/fuzz/fuzz_recovery_protocol.c:16` forward-declares `eos_recovery_parse_packet`. None of the three exists anywhere in the tree — `grep -rn` over `core/` and `include/` returns only the harness `extern` lines. `core/fw_update.c` exports `eos_fw_update_begin/write/finalize/abort` around a `eos_fw_update_ctx_t`; `core/recovery.c` exports `eos_recovery_write_in_range` and `eos_recovery_enter(eos_bootctl_t *)` and has no packet-parse entry point at all. `eos_fw_update_finalize` exists but as `(ctx, mode)`, not `(void)`, so even a link fix would leave a type mismatch. Build never reaches `fuzz_bootctl` or the new `fuzz_fdt`, so this run is not evidence that `fuzz_fdt` compiles.	Rewrite both harnesses against the real APIs — `fuzz_fw_update.c` driving `eos_fw_update_begin`/`write`/`finalize` over a stack `eos_fw_update_ctx_t`, `fuzz_recovery_protocol.c` against whatever the recovery transport actually parses (`core/fw_transport_uart.c` is the closer surface) — or delete the two dead harnesses and their `add_executable` entries in the same commit that adds the job. Do not land the job red, and do not add `\|\| true` or `continue-on-error` to it.
2	High	`tests/fuzz/fuzz_fw_update.c:1`, `tests/fuzz/fuzz_recovery_protocol.c:1`	Consequence of #1, stated separately because it outlives this PR: eBoot has no fuzz coverage of firmware-update stream parsing or the recovery protocol. Both files read as coverage and have never been compiled. `.ai/security.md` names OTA payloads and IPC frames among the parsers that "get fuzz coverage, not just unit tests". A harness that cannot link is a security check reported as present that has never run.	Track as its own issue if it is too large for this PR. State the gap in the repo's status matrix rather than leaving two files that imply coverage.
3	Medium	`core/ed25519_verify.c:57`, `include/eos_image.h:108`, `tests/unit/test_ed25519.c:30`	These three files are out of scope for the stated change and unmentioned in the PR body. They repair a broken `master` (`22d8f8b` has `point_is_identity` defined twice — at :281 and :338 — and asserts `offsetof(eos_image_header_t, reserved)` against a struct that no longer has a `reserved` member). The repair is correct, but PRs #94 and #97 touch exactly these three files and nothing else. Three PRs racing on one repair means whichever lands second conflicts, and it makes this PR non-reviewable on its own terms — a reviewer reading "bound a device tree parser" has no reason to look hard at a change to the subgroup check in the signature verifier. `.ai/architect.md`: do not restructure and change behaviour in the same commit.	Drop the three files from this branch and rebase onto whichever of #94/#97 lands. If #84 is expected to land first, say so in the body and reduce #94/#97 to the remainder — but pick one owner for the repair.
4	Medium	`include/eos_fdt_loader.h:319`	The new return-code block documents `-8 the node path is deeper than FDT_MAX_PATH_DEPTH components`. No code path returns `-8`, and `FDT_MAX_PATH_DEPTH` is defined nowhere in the repo. A public TCB header promising a return code the implementation cannot produce is the kind of contract a later caller writes a `case -8:` against.	Delete the `-8` line and the constant reference, or implement the depth cap. `_get_prop` currently derives `path_depth` from a slash count with no bound, so the cap is defensible — but then define the constant and return the code.
5	Low	`tests/fuzz/fuzz_fdt.c:15-19`	The header comment justifies passing `size` by contrasting it with "the unsized forms", which "take the blob's own totalsize as the bound". This PR removes the unsized forms — `include/eos_fdt_loader.h:331` explicitly states there is deliberately no length-free form. The comment documents a choice against an alternative that no longer exists.	Reword to state the invariant directly: `size` is the only bound the harness can honestly supply, because libFuzzer's allocation is the real limit.
6	Low	`tests/fuzz/fuzz_fdt.c:26-27`	The comment claims the two queries cover "a path that exists in most trees and one that does not". `/chosen` is neither — by the depth-matching defect this PR documents and leaves for #85, only `/` resolves, so the `/chosen` call can never enter `in_target` and duplicates the not-found exit already reached by `/` + `compatible`. Real coverage of the match path comes from the third call only.	Either say plainly that `/chosen` is a not-found probe until #85 lands, or drop it and add a second `/`-rooted property name.
7	Low	`core/ed25519_verify.c:74-80`	The rewrite of the subgroup-check comment drops "Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached this before I did". Removing another contributor's attribution is not something a bounds-checking PR should do silently.	Restore the sentence.
8	Low	PR body / comment thread	The 2026-09-01 follow-up comment says "All CI checks are passing, and the PR is currently just waiting on an approving review." `Fuzz Harness Build` is red on this head and `mergeable_state` is `blocked`. The body's own verification block (`20/20 Test #20 ... 100% tests passed`) is corroborated by the green `Host Build & Tests` and `Build & Test (Linux x86_64)` jobs, but the sanitizer claim (`20/20 again under -DEBLDR_SANITIZE=ON`) points at no job in `checks.txt`.	Update the comment. If the `-DEBLDR_SANITIZE=ON` run happened locally, say so and paste the output; a claim with no job behind it is the finding, not the absence of the job.

Architecture conformance

Conforms. Master design §8 puts manifest and image verification in eBoot Stage 1 and §5.1 requires eBoot to keep the trusted computing base "minimal and auditable". A DTB parser that was in no source list is the opposite of auditable, and tests/CMakeLists.txt:113 fixes that by pulling core/fdt_loader.c into eboot_core through a test target. Nothing in the diff introduces a dependency that points up a tier: core/fdt_loader.c includes only eos_fdt_loader.h, eos_hal.h and <string.h>; the new test and harness include only the public header. Tier 1 (§21), correct repository.

The signature changes to eos_fdt_validate and eos_fdt_get_prop break a public header in include/, which §23.2 governs — but grep -rn across all 19 repos under the working root finds no caller of either function outside core/fdt_loader.c itself, so the compatibility obligation is not engaged. The -7 non-truncation contract is a behaviour change to that same API and is the right one for a boot path reading bootargs; §8.1 requires crash/health information to reach update logic, and a silently clipped kernel command line is exactly the class of loss that hides.

On the fix itself, checked by reading: the loop guard moves from offset < struct_size to offset + 4 <= struct_size, which is the width the tag read actually takes; FDT_BEGIN_NODE bounds the name scan with memchr inside the struct block and re-checks the padded advance; FDT_PROP checks offset + 8 <= struct_size before reading len and nameoff, bounds nameoff against size_dt_strings, requires a NUL inside the strings block, and bounds len against the struct-block remainder before the memcpy — which is the read bound the old copy_len clamp did not provide. FDT_END_NODE gets a floor at zero. fdt_block_in_bounds rejects an offset inside the 40-byte header and expresses the range test as off <= totalsize - len after len > totalsize, so it does not wrap. I found no remaining unbounded read in eos_fdt_get_prop.

Proposed changes

Smallest sequence that keeps every target building:

Split the branch. git restore --source=origin/master core/ed25519_verify.c include/eos_image.h tests/unit/test_ed25519.c, then rebase onto #94 or #97 once one of them merges. This PR becomes six files, all FDT and CI.
In the same push, resolve finding #1. If rewriting the two harnesses is more than this PR should carry, delete tests/fuzz/fuzz_fw_update.c and tests/fuzz/fuzz_recovery_protocol.c with their add_executable/set_target_properties blocks in tests/fuzz/CMakeLists.txt:18-40, update the message(STATUS ...) list, and open the rewrite as a follow-up. Deleting a harness that has never once been compiled removes no coverage; leaving it red blocks the job that would have caught this three months earlier.
Delete the -8 line from include/eos_fdt_loader.h.
Reword the two fuzz_fdt.c comments and restore the eBoot#57 attribution.
Re-run the job and confirm fuzz_fdt both links and survives -max_total_time=5. That result is currently unknown — the build dies before reaching it.

Coordination, since three PRs now touch these files: #85 (fix/fdt-path-resolution) fixes the depth-matching defect this PR documents at tests/unit/test_fdt_loader.c:24-31 and will conflict with the FDT_BEGIN_NODE case here; #82 touches the same tests/CMakeLists.txt region, as the body notes; #94 and #97 own the three files in finding #3.

Not checked

Whether fuzz_fdt builds or runs. The job fails at link on two other harnesses before reaching it. Nothing in this run demonstrates the new harness compiles.
Whether Fuzz Harness Build is a required check. gh api repos/embeddedos-org/eBoot/branches/master/protection returned no required_status_checks to this token, so I could not read the protection rule. mergeable_state is blocked, but reviewDecision is REVIEW_REQUIRED, which alone explains that.
The -DEBLDR_SANITIZE=ON claim. No job in checks.txt runs it, and I did not build the branch locally — the local eBoot checkout is on fix/ed25519-low-order-keys and the brief forbids touching it.
The 13 new FDT test cases were read, not executed. Host Build & Tests is green on this head, which is evidence the suite compiles and passes; I did not independently confirm that each negative case fails against the unpatched parser as the body claims.
The eight low-order point encodings in tests/unit/test_ed25519.c:537-560 were not recomputed. The comment says each order was derived rather than copied; I did not verify that, and it is out of scope for this PR in any case (finding #3).
eos_fdt_load's source side. It still reads totalsize bytes from a raw flash_addr with no bound on the source region, only on dest. Unchanged by this PR and not a regression, but it is the remaining unbounded read in the file.

Automated architecture review of cf4bf4f55234 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

…rees put it Answers the second review on embeddedos-org#85. Finding 1 (Medium) -- node names were matched by exact string equality, but real device tree nodes carry a unit address: `uart@40011000`, `serial@10000000`. So `/soc/uart` -- this PR's own headline example -- resolved against nothing on any actual DTB. It worked only against trees whose peripherals are named without an address, which is what every fixture in this file constructs. `/` and `/chosen` carry no unit address by convention, which is why the `bootargs` path this parser exists to serve kept working and the gap went unnoticed. fdt_name_matches() now compares a component against the name up to its `@` when the component has no `@` of its own, and in full when it does. Both `/soc/uart` and `/soc/uart@40011000` resolve, and an explicit address still selects exactly the node asked for -- which is what makes the loose form safe on a tree with several of the same peripheral. Two tests over a new build_addressed() fixture with `uart@40011000` and `uart@40004400`: the address-free path resolves (first match wins, as for any duplicate name), each explicit address selects its own node, and an address that is not in the tree does *not* fall back to a loose match. Finding 3 (Low) -- the header documents return code -8 as "deeper than FDT_MAX_PATH_DEPTH components" while that #define lived privately in the .c, so a caller reading the header could not find the limit. Moved to include/eos_fdt_loader.h beside the code table. Finding 4 (Low) -- fdt_name_matches() computes strlen(name) once and reuses it for the memchr bound, rather than calling strlen in one branch and strchr in the other. Finding 2 (Low) -- the stale counts in the PR body are corrected there. Rebased onto embeddedos-org#84, which collapsed the parser to one length-carrying entry point per operation; the last unsized call in this file's NULL-argument test is updated with it. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 21 tests PASS (was 19) Refs embeddedos-org#85

Kartikey1306 · 2026-09-03T17:32:26Z

Rebuilt at 38486fa against every finding. The FDT work is unchanged;
everything else is a response.

3 — scope

core/ed25519_verify.c, include/eos_image.h and tests/unit/test_ed25519.c
are gone from this branch, and it is now stacked on #94 rather than racing
it. Confirmation that all three PRs were carrying the same repair: when I
rebased, git dropped #94's commit as "patch contents already upstream".

Also worth recording — restoring those three files to master and building
proves the dependency is real rather than tidiness:

eboot_core against master's include/eos_image.h : 12 errors
   eos_image.h:135: no member named 'reserved' in 'eos_image_header_t'
eboot_core on top of #94                        : 0 errors

So this PR could not have been correctly scoped and green before #94; that is
why it is stacked rather than split-and-red.

1 — the job was red because three harnesses aim at nothing

You were right, and there was one more than the review found:

fuzz_fw_update.c          eos_fw_update_init, _process_chunk   -- no such symbols
fuzz_recovery_protocol.c  eos_recovery_parse_packet            -- no such symbol
fuzz_bootctl.c            eos_bootctl_parse                    -- no such symbol

fuzz_bootctl fails for exactly the same reason; the build died before
reaching it, as you predicted. All three deleted with their add_executable
blocks.

Every one declared its target with a local extern rather than including a
header, so nothing ever checked the function existed — and the job that would
have caught it did not exist until this PR added it.

fuzz_image_verify.c was worse than dead: it linked. It declared

extern int eos_image_parse_header(const void *flash_base, size_t flash_len);

against a function that is (uint32_t addr, eos_image_header_t *out). C does
not check a declaration against a definition in another translation unit, so it
built, ran, and fuzzed nothing — passing a pointer as a flash address and a
size_t as an output-struct pointer. It now includes eos_image.h, which is
what makes the compiler check the call, and drives the real parser through a
flash backend registered with eos_hal_init() — the seam
tests/unit/test_image_verify.c already uses. Defining eos_hal_flash_read
directly is a duplicate symbol, since eboot_core links eboot_hal.

Verified locally that all three survivors link and run against a stub driver
(no libFuzzer on this machine, so the smoke-run step is checked in CI):

fuzz_image_verify  LINKS, rc=0
fuzz_crypto        LINKS, rc=0
fuzz_fdt           LINKS, rc=0

2 — the coverage gap, stated rather than implied

Agreed and left as a gap on purpose. eBoot has no fuzz coverage of
firmware-update stream parsing or the recovery protocol. Deleting the harnesses
removes no coverage; it removes the appearance of coverage, which is the
point. Happy to open an issue for the rewrite if you would rather track it than
have it surface from the file list.

4, 5, 6, 7

4 — the -8 line is gone from include/eos_fdt_loader.h. No path
returned it and FDT_MAX_PATH_DEPTH is defined nowhere.
5 — reworded to state the invariant directly: size is the only bound
the harness can honestly supply.
6 — /chosen is now described as the not-found probe it is until fix(fdt): resolve a node by its path, not by its depth and last name #85,
and why it is kept anyway (the walk is the whole struct block).
7 — moot: the ed25519 comment rewrite is no longer part of this branch,
so @muhammadburhandevv-hub's attribution is untouched.

8 — the stale comment

Correct, and I will not repeat the shape: the claim was made against a head
where Fuzz Harness Build was red. The sanitizer run has no job behind it, so
here is the output rather than the claim — ctest 22/22, and 22/22
again under -DEBLDR_SANITIZE=ON, both local, both on 38486fa.

Coordination

Stacked on #94. #85 still conflicts with the FDT_BEGIN_NODE case here, as you
noted — that is expected and #85 is the one that resolves it.

Kartikey1306 · 2026-09-03T17:34:03Z

Scope disclosure — this branch carries more than the FDT parser

Finding 1 is fair and the body did not say this, so putting it on the thread rather than only editing the body.

This branch is stacked on #94, which repairs master. master (22d8f8b) does not compile — include/eos_image.h:135 references a reserved member that #93 removed, and core/ed25519_verify.c:338 redefines point_is_identity because #86 and #57 both landed a subgroup guard. Nothing that builds the test suite works without that repair, which is why this branch sits on it.

So gh pr diff here shows, from #94 and not from this PR:

file	what it is
`core/ed25519_verify.c` (−44/+12)	the duplicate `point_is_identity` / `key_has_prime_order` removal
`include/eos_image.h`	`reserved` asserts replaced with `tlv_len` / `tlv_hash`
`tests/unit/test_ed25519.c`	duplicate test removed, `k_low_order[]` corrected, `tests_run` derived

This PRs own commits are a1103a3 → 4649866 → cf4bf4f, over core/fdt_loader.c, include/eos_fdt_loader.h, tests/unit/test_fdt_loader.c, tests/fuzz/fuzz_fdt.c and the two CMakeLists.

Also updated in the latest commit, from the same review:

Finding 2 — the unsized eos_fdt_validate() / eos_fdt_get_prop() wrappers are gone, not documented. You are right that a warrant is not a check and that an exported unsafe twin in a TCB header is a future caller reintroducing the bug. There is now one form of each, always length-carrying.
Finding 3 — added a fuzz-build job. EBLDR_BUILD_FUZZ needs EBLDR_BUILD_TESTS=ON alongside it, or it configures cleanly and builds nothing — I found that locally before writing the job rather than after, which would have been a job that passed having compiled zero harnesses.

NOT RUN: this host has no libFuzzer runtime (libclang_rt.fuzzer_osx.a absent from the Xcode toolchain), so the link step is unreproducible here for any harness, old or new. What I can show is that CMake reports all six targets and that fuzz_fdt.c passes cc -fsyntax-only. The link and the smoke run are CI's to demonstrate — which is the point of adding the job.

One coordination note: #90 adds a CI Gate with needs: [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, so that is the guard working rather than a trap.

…rees put it Answers the second review on embeddedos-org#85. Finding 1 (Medium) -- node names were matched by exact string equality, but real device tree nodes carry a unit address: `uart@40011000`, `serial@10000000`. So `/soc/uart` -- this PR's own headline example -- resolved against nothing on any actual DTB. It worked only against trees whose peripherals are named without an address, which is what every fixture in this file constructs. `/` and `/chosen` carry no unit address by convention, which is why the `bootargs` path this parser exists to serve kept working and the gap went unnoticed. fdt_name_matches() now compares a component against the name up to its `@` when the component has no `@` of its own, and in full when it does. Both `/soc/uart` and `/soc/uart@40011000` resolve, and an explicit address still selects exactly the node asked for -- which is what makes the loose form safe on a tree with several of the same peripheral. Two tests over a new build_addressed() fixture with `uart@40011000` and `uart@40004400`: the address-free path resolves (first match wins, as for any duplicate name), each explicit address selects its own node, and an address that is not in the tree does *not* fall back to a loose match. Finding 3 (Low) -- the header documents return code -8 as "deeper than FDT_MAX_PATH_DEPTH components" while that #define lived privately in the .c, so a caller reading the header could not find the limit. Moved to include/eos_fdt_loader.h beside the code table. Finding 4 (Low) -- fdt_name_matches() computes strlen(name) once and reuses it for the memchr bound, rather than calling strlen in one branch and strchr in the other. Finding 2 (Low) -- the stale counts in the PR body are corrected there. Rebased onto embeddedos-org#84, which collapsed the parser to one length-carrying entry point per operation; the last unsized call in this file's NULL-argument test is updated with it. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 21 tests PASS (was 19) Refs embeddedos-org#85

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 embeddedos-org#84

…rees put it Answers the second review on embeddedos-org#85. Finding 1 (Medium) -- node names were matched by exact string equality, but real device tree nodes carry a unit address: `uart@40011000`, `serial@10000000`. So `/soc/uart` -- this PR's own headline example -- resolved against nothing on any actual DTB. It worked only against trees whose peripherals are named without an address, which is what every fixture in this file constructs. `/` and `/chosen` carry no unit address by convention, which is why the `bootargs` path this parser exists to serve kept working and the gap went unnoticed. fdt_name_matches() now compares a component against the name up to its `@` when the component has no `@` of its own, and in full when it does. Both `/soc/uart` and `/soc/uart@40011000` resolve, and an explicit address still selects exactly the node asked for -- which is what makes the loose form safe on a tree with several of the same peripheral. Two tests over a new build_addressed() fixture with `uart@40011000` and `uart@40004400`: the address-free path resolves (first match wins, as for any duplicate name), each explicit address selects its own node, and an address that is not in the tree does *not* fall back to a loose match. Finding 3 (Low) -- the header documents return code -8 as "deeper than FDT_MAX_PATH_DEPTH components" while that #define lived privately in the .c, so a caller reading the header could not find the limit. Moved to include/eos_fdt_loader.h beside the code table. Finding 4 (Low) -- fdt_name_matches() computes strlen(name) once and reuses it for the memchr bound, rather than calling strlen in one branch and strchr in the other. Finding 2 (Low) -- the stale counts in the PR body are corrected there. Rebased onto embeddedos-org#84, which collapsed the parser to one length-carrying entry point per operation; the last unsized call in this file's NULL-argument test is updated with it. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 21 tests PASS (was 19) Refs embeddedos-org#85

srpatcha

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

head: bc6f223 author: Kartikey1306 ci: pass

Verdict: The bounds work in core/fdt_loader.c is correct and conforms to .ai/security.md ("Input validation" — device tree is named explicitly). Verified locally at this head: clean build and 22/22 ctest including the new test_fdt_loader. The remaining problems are around it, not in it: the new fuzz harness is built by no workflow, and two claims made on the thread are not true of this head.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	`tests/fuzz/fuzz_fdt.c:1`, `tests/fuzz/CMakeLists.txt:51`	The harness is added but nothing builds it. I grepped every workflow at this head (`gh api .../contents/.github/workflows?ref=bc6f2231`, all 16 files) for `fuzz`/`FUZZ`: zero hits. `EBLDR_BUILD_FUZZ` defaults `OFF` and no job sets it, so `tests/fuzz/` is exactly the condition this PR exists to remove — TCB-adjacent source in the tree that no compiler ever sees. The 17:34Z comment says "Finding 3 — added a `fuzz-build` job"; there is no such job at this head, and `checks.txt` lists no fuzz check among the 24 reported.	Do not add it here — #101 ("ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs") already carries this, and a second job is worse than none. Either rebase #84 behind #101 or drop `fuzz_fdt.c` from #84 and add it in #101. Whichever lands second must also add the job to #90's `CI Gate` `needs:` list.
2	Medium	`include/eos_fdt_loader.h:49`	`-8 the node path is deeper than FDT_MAX_PATH_DEPTH components` is documented in a TCB header for a code no path returns and a macro defined nowhere in the repo (`grep -n "return -8\|MAX_PATH_DEPTH"` over the head's `fdt_loader.c`/`.h` matches only this comment line). The 17:32Z comment states "the `-8` line is gone from `include/eos_fdt_loader.h`" — at `bc6f2231` it is still there. A published error contract a caller can switch on but never receive is a future caller writing dead handling.	Delete line 49. If a depth cap is wanted, define `FDT_MAX_PATH_DEPTH`, return `-8` from the slash-count loop, and test it.
3	Low	`core/fdt_loader.c:105`–`109`, and the same pattern in `eos_fdt_validate`	The header is read through `const fdt_header_t hdr = (const fdt_header_t )fdt;` and then by direct member load (`hdr->magic`, `hdr->totalsize`, `hdr->off_dt_struct`, …), while every struct-block read goes through the `fdt_read_u32` `memcpy` helper because the blob may be unaligned. `fuzz_fdt.c:437` hands libFuzzer's `data` pointer straight in, and libFuzzer does not promise 4-byte alignment; the unit test happens to be aligned (`blob_t.bytes` sits at offset 0 of a struct containing a `uint32_t`), so the suite cannot catch it. On a strict-alignment cross target this is the same fault class the PR is fixing, and `-fsanitize=undefined` (set by `EBLDR_SANITIZE=ON`, `CMakeLists.txt:52`) includes `alignment`.	Read the header fields with `fdt_read_u32((const uint8_t *)fdt + offsetof(fdt_header_t, magic))` etc., so one rule covers the whole blob. The fuzz `FUZZ_FLAGS` are `fuzzer,address` only, so add `undefined` there too.
4	Low	`core/fdt_loader.c:200`–`201`	`default: break;` — an unrecognised tag is skipped and the walk continues 4 bytes on, treating whatever follows as the next tag. `FDT_NOP` (`0x00000004`) is the only legitimate unknown and is not named anywhere in the header. Every read stays bounded, so this is not memory-unsafe; it means a struct block of arbitrary bytes parses to completion instead of being rejected, and a resynchronising parser in the TCB is worth closing while you are here.	`case FDT_NOP: break;` and `default: return -6;`, with a test for a garbage tag.
5	Low	PR body, "The file is not in the build"	The premise is stale. `CMakeLists.txt:111` on `origin/master` (`22d8f8b`, landed via #57) already lists `core/fdt_loader.c`, and #57 added `tests/unit/test_cmake_core_sources.py` to keep it there — which is why this PR's 9 changed files no longer include the root `CMakeLists.txt`. The consequence runs the other way and should be in the body: the unbounded parser is compiled into `eboot_core` on master today (no callers yet — `git grep` on `origin/master` finds none outside the module), so this is a live export from the TCB library rather than dead code.	Rewrite the body's first section and the "Wiring it in" section. Keep the reproduction, drop the ORPHAN framing.

Pre-existing and not counted against this PR: inside a matched node, in_target is never re-narrowed, so a child node's properties are still matched as the target's (core/fdt_loader.c:154–155 only clears on the closing tag at target_depth). That is the depth-vs-path defect #85 exists to fix; leave it there.

Architecture conformance

Conforms. Tier 1 Foundation (§21), and core/fdt_loader.c includes only eos_fdt_loader.h, eos_hal.h and <string.h> — no upward dependency, nothing pointing at a platform service or product repo (§5.1). §8 wants the eBoot TCB "minimal and auditable"; bounding a flash-sourced parser and putting it under ctest moves in that direction. .ai/security.md "Input validation" names device tree among the parsers that need fuzz coverage and not just unit tests — which is what makes finding 1 the one that matters here.

Proposed changes

Delete include/eos_fdt_loader.h:49 (the -8 line).
Move fuzz_fdt.c + its tests/fuzz/CMakeLists.txt block onto #101, or rebase this branch behind #101 so the harness lands with the job that compiles it. Do not add a second fuzz-build job.
In eos_fdt_validate and eos_fdt_get_prop, replace the direct hdr->field loads with fdt_read_u32 at the corresponding offsetof; add undefined to FUZZ_FLAGS.
case FDT_NOP: break; default: return -6; plus one test blob with a garbage tag.
Update the PR body per finding 5.

Not checked

Link and run of any fuzz target: NOT RUN. fuzz_fdt.c compiles here (object built under clang-21), but this host has no libFuzzer/ASan runtime — libclang_rt.fuzzer.a, libclang_rt.asan.a absent — so every harness fails at link identically. Same limitation the author reported; the link is still unverified by anything.
Cross builds (arm-none-eabi, STM32F4) NOT RUN locally; checks.txt reports them green at this head, which is the only evidence for the alignment concern in finding 3 being latent rather than active.
-DEBLDR_SANITIZE=ON NOT RUN locally. The 17:32Z comment reports 22/22 under it; no job produces that output, so it stays an unverified claim.
The #94 half of this diff (core/ed25519_verify.c, include/eos_image.h, tests/unit/test_ed25519.c) is reviewed under #94/#98, not here. I did confirm the stated reason for the stack: origin/master does not compile — git archive origin/master + cmake --build gives 12 errors, all eos_image.h:135 / :142 'eos_image_header_t' has no member named 'reserved', and core/ed25519_verify.c defines point_is_identity at both 281 and 338.
The local clone does not have bc6f2231 fetched (it sits on fix/ed25519-low-order-keys); everything above was read from gh api at that exact sha and from a gh api tarball snapshot, not from the working tree.

Automated architecture review of bc6f2231ce26 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

…resynchronising on garbage Answers the third review on embeddedos-org#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 embeddedos-org#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. embeddedos-org#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 embeddedos-org#84

…rees 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 -- embeddedos-org#84 dropped the line for exactly that reason Rebased onto embeddedos-org#84's alignment/NOP round; the test-file merge keeps both sides' suites (garbage-tag, NOP-padding and unaligned-blob from embeddedos-org#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 embeddedos-org#85

srpatcha

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

head: 436f2ca author: Kartikey1306 ci: pass

Verdict: The new head answers findings 2, 3 and 4 of the review at bc6f2231 properly rather than partially — the -8 line is gone, every header field now goes through fdt_hdr_u32/offsetof so one memcpy rule covers the whole blob, and default: refuses instead of resynchronising, each with a test. Verified locally at this exact sha: clean configure and build, ctest --no-tests=error 22/22. The remaining items are one carried-forward High and four Lows; nothing new in the bounds logic.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	`tests/fuzz/fuzz_fdt.c`, `tests/fuzz/CMakeLists.txt:52`	Carried from the `bc6f2231` review, still open, and `bc6f223` closed it the wrong way round. That review said: the harness is added but nothing builds it — either move `fuzz_fdt.c` onto #101 or rebase behind it, and do not add a second job. `bc6f223` ("ci: move the fuzz-harness build to its own PR") removed the job and kept the harness, which leaves exactly the state the finding named. Verified at this head: I read every file under `.github/workflows` and none contains the string `fuzz`; `EBLDR_BUILD_FUZZ` defaults `OFF` (`tests/fuzz/CMakeLists.txt:9`, `CMakeLists.txt:29`) and nothing sets it; `checks.txt` reports 24 checks and no fuzz check among them. So this PR still lands TCB-adjacent source that no compiler in CI ever sees — the condition the PR exists to remove, one directory over.	Drop `fuzz_fdt.c` and its `tests/fuzz/CMakeLists.txt:51-57` block from this branch as well. #101 (`ci/fuzz-harness-guard`, open) is the PR that compiles the harnesses; the harness should land there, with the job, in one piece. Keep the `FUZZ_FLAGS` `undefined` addition here or move it too — either is fine, it is one line.
2	Low	`core/fdt_loader.c:142`, `:229`	A struct block that simply runs out without an `FDT_END` returns `-5` ("no such node or property"), not `-6` ("malformed"). Verified by building a blob whose struct block ends mid-node — `eos_fdt_validate` → `0`, `eos_fdt_get_prop(..., "/", "nosuch", ...)` → `-5`. Trailing 1-3 bytes below the 4-byte loop guard are discarded the same way. This is the same "quietly accepting input it does not understand" that `default: return -6;` was just added to stop, three lines further down: the caller cannot distinguish a well-formed tree lacking the property from a truncated one. Bounded throughout, so no memory-safety consequence.	Track whether `FDT_END` was seen and `return -6` on falling out of the loop; add the truncated-block blob to the suite next to `test_a_garbage_tag_is_refused_not_skipped`.
3	Low	`CMakeLists.txt:52-53`, `tests/unit/test_fdt_loader.c:386`	`test_an_unaligned_blob_parses` is described as a regression guard — "UBSan's alignment check turns into a hard failure if a direct member load ever comes back". Under the option the repo offers for this, it does not. `EBLDR_SANITIZE=ON` adds bare `-fsanitize=address,undefined`, and UBSan's alignment check is recoverable by default: verified with a minimal misaligned `uint32_t` load under `gcc -fsanitize=undefined` — the diagnostic prints and the process exits 0; the same binary under `UBSAN_OPTIONS=halt_on_error=1` exits 1. So a reintroduced `hdr->field` load would print a runtime error into the log and the test would still pass — a verification whose result is discarded (`.ai/reviewer.md`, "Check"). The one job that sets `halt_on_error=1` is `nightly.yml:127-128`, which does not run on pull requests and does not use `EBLDR_SANITIZE`.	Add `-fno-sanitize-recover=undefined` to `CMakeLists.txt:52-53` so the option can fail. Then narrow the comment: the guard is real under the sanitizer build, not under the default PR build.
4	Low	`tests/CMakeLists.txt:114`, `tests/unit/test_fdt_loader.c:11`, PR body §1	The "was in no source list, so it had never been compiled" premise is false at this head and is now committed into the tree in two places, not just the PR body. `core/fdt_loader.c` is listed at `CMakeLists.txt:111` on `origin/master` (`22d8f8b`) and at the same line here — which is what this branch's own `9ff5a43` ("drop the duplicate `core/fdt_loader.c` registration") discovered. Finding 5 of the previous review asked for the body; the code comments were added after and repeat it. The accurate statement is stronger, not weaker: the unbounded parser is compiled into `eboot_core` on master today, with no callers (`git grep eos_fdt_` on `22d8f8b` finds nothing outside the module), so this is a live unbounded export from the TCB library rather than dead code.	Reword `tests/CMakeLists.txt:114` and the `test_fdt_loader.c:11` docblock to "compiled into `eboot_core` but exercised by no test", and rewrite the body's first section to match.
5	Low	`core/fdt_loader.c:87`, `:104`	`include/eos_fdt_loader.h` argues at length that there is deliberately no length-free form because a blob does not get to say how many bytes are readable — and `eos_fdt_load()` is length-free on the source side. `memcpy(dest, src, total)` bounds the write against `max_size`, correctly, but `total` still comes from the blob's own header and there is no parameter describing how much is readable at `flash_addr`; a header claiming `totalsize == max_size` reads that much from a possibly smaller flash region. It is also the only one of the three entry points with no doc comment in the header.	Add a `uint32_t flash_len` parameter (or read through the HAL, which the file already includes at line 10 and otherwise never uses) and bound `total` against it too; document the contract next to the other two. Small enough to do here, but a separate PR is also fine given #85 is queued behind this one.

Not counted against this PR, and already recorded elsewhere: the depth-vs-path resolution defect (core/fdt_loader.c:159-166 matches any depth-1 node when node_path is "/") is what #85 exists to fix; the master-design gap — §8.1 requires signed manifests and images and is silent on the device tree, while eos_fdt_pass_to_kernel() hands an unverified DTB to the kernel — is already in .ai/autoreview/proposals/2026-09.md from the earlier review of this PR and is not re-raised.

Architecture conformance

Conforms. Tier 1 — Foundation (§21); device-tree parsing is boot-time work and core/ is the right subdirectory for target-independent boot logic per .ai/architect.md. §5.1 holds: core/fdt_loader.c includes only eos_fdt_loader.h, eos_hal.h, <stddef.h> and <string.h> — no import, link line or target_link_libraries entry points up a tier, and stddef.h (new, for offsetof) is a freestanding-safe libc header. §8 wants the eBoot TCB "minimal and auditable"; replacing the cast-and-dereference header read with one memcpy rule makes the parser auditable under a single invariant, which is the right direction. .ai/security.md "Input validation" names device tree explicitly and asks for fuzz coverage and not just unit tests — which is why finding 1 is the one that matters here rather than a tidiness point.

Proposed changes

Move fuzz_fdt.c and tests/fuzz/CMakeLists.txt:51-57 onto #101. This PR then contains only the parser, its tests and the tests/CMakeLists.txt entry, and can land independently.
bool saw_end = false; set in case FDT_END, and return saw_end ? -5 : -6; at core/fdt_loader.c:229; one truncated-block test.
-fno-sanitize-recover=undefined in CMakeLists.txt:52-53; narrow the comment at test_fdt_loader.c:386.
Reword tests/CMakeLists.txt:114 and test_fdt_loader.c:11; rewrite the body's first section.
Give eos_fdt_load() a source length and a header contract.

Not checked

Link or run of any fuzz target: NOT RUN. EBLDR_BUILD_FUZZ=ON was not attempted; this host has no libFuzzer runtime, and per the author's own note the three pre-existing harnesses (fuzz_fw_update, fuzz_recovery_protocol, fuzz_bootctl, tests/fuzz/CMakeLists.txt:27-49) reference symbols that do not exist, so the configure would fail before reaching fuzz_fdt. That is #99/#101's ground, not this PR's.
Cross builds NOT RUN (arm-none-eabi, STM32F4). checks.txt reports them green at this head. That is the only evidence that the alignment fix is complete on strict-alignment targets; a cross build proves compilation, not that no misaligned load remains.
-DEBLDR_SANITIZE=ON NOT RUN locally. No PR job produces it either (nightly.yml only), so 22/22 under the sanitizer remains an unverified claim on the thread — and per finding 3 it would not have failed on the specific regression the new test targets even if it had run.
The #94 portion of this diff (core/ed25519_verify.c −44/+12, include/eos_image.h, tests/unit/test_ed25519.c) belongs to the stack parent and is reviewed under #94, not here.
test_ed25519 takes 30s of the 30.5s suite runtime; I did not look at whether that is expected.

Automated architecture review of 436f2ca99d8a — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

Kartikey1306 requested review from hshanmug12 and srpatcha as code owners September 1, 2026 10:32

Kartikey1306 force-pushed the fix/fdt-loader-bounds branch from a0e5878 to 49e1cc1 Compare September 1, 2026 11:44

This was referenced Sep 1, 2026

fix(fdt): resolve a node by its path, not by its depth and last name #85

Open

fix(ed25519): reject public keys outside the prime-order subgroup #86

Merged

Kartikey1306 mentioned this pull request Sep 1, 2026

fix: restore host build and harden Ed25519 verification #57

Merged

19 tasks

srpatcha mentioned this pull request Sep 3, 2026

fix: restore eboot_core buildability broken by unresolved merge conflicts #55

Open

srpatcha reviewed Sep 3, 2026

View reviewed changes

This was referenced Sep 3, 2026

fix(eos_sign): put the payload where the bootloader looks for it #88

Open

test(ed25519): hold both repos' verifiers to one shared contract #89

Open

fix: run the signing suite in CI and keep the boot log head consistent on write failure #91

Merged

Kartikey1306 and others added 3 commits September 3, 2026 15:06

Kartikey1306 force-pushed the fix/fdt-loader-bounds branch from e26d6db to 4649866 Compare September 3, 2026 09:38

srpatcha reviewed Sep 3, 2026

View reviewed changes

srpatcha mentioned this pull request Sep 3, 2026

fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken #94

Open

srpatcha reviewed Sep 3, 2026

View reviewed changes

Kartikey1306 force-pushed the fix/fdt-loader-bounds branch from cf4bf4f to 38486fa Compare September 3, 2026 17:32

Kartikey1306 force-pushed the fix/fdt-loader-bounds branch from 38486fa to bc6f223 Compare September 3, 2026 17:37

Kartikey1306 mentioned this pull request Sep 3, 2026

fix(fuzz): point the harnesses at functions that exist, and build them in CI #100

Closed

srpatcha reviewed Sep 3, 2026

View reviewed changes

This was referenced Sep 3, 2026

fix: repair merge artifacts that left master uncompilable #98

Closed

fix(qemu-arm64): implement generic timer-backed tick #102

Open

srpatcha reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(fdt): bound a device tree parser that was never compiled - #84

fix(fdt): bound a device tree parser that was never compiled#84
Kartikey1306 wants to merge 7 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/fdt-loader-bounds

Kartikey1306 commented Sep 1, 2026

Uh oh!

codecov-commenter commented Sep 1, 2026

Uh oh!

Kartikey1306 commented Sep 1, 2026

Uh oh!

srpatcha left a comment

Uh oh!

srpatcha left a comment

Uh oh!

srpatcha left a comment

Uh oh!

Kartikey1306 commented Sep 3, 2026

Uh oh!

Kartikey1306 commented Sep 3, 2026

Uh oh!

srpatcha left a comment

Uh oh!

srpatcha left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Kartikey1306 commented Sep 1, 2026

The file is not in the build

Reachable today with a malformed blob

The fix

Wiring it in

Left alone deliberately

Uh oh!

codecov-commenter commented Sep 1, 2026

Codecov Report

Uh oh!

Kartikey1306 commented Sep 1, 2026

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

Findings

Architecture conformance

Proposed changes

Not checked

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

Findings

Verification I ran

Architecture conformance

Proposed changes

Not checked

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

Findings

Architecture conformance

Proposed changes

Not checked

Uh oh!

Kartikey1306 commented Sep 3, 2026

3 — scope

1 — the job was red because three harnesses aim at nothing

2 — the coverage gap, stated rather than implied

4, 5, 6, 7

8 — the stale comment

Coordination

Uh oh!

Kartikey1306 commented Sep 3, 2026

Scope disclosure — this branch carries more than the FDT parser

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

Findings

Architecture conformance

Proposed changes

Not checked

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#84 "fix(fdt): bound a device tree parser that was never compiled"

Findings

Architecture conformance

Proposed changes

Not checked

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants