fix(fdt): resolve a node by its path, not by its depth and last name - #85
fix(fdt): resolve a node by its path, not by its depth and last name#85Kartikey1306 wants to merge 9 commits into
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#85 "fix(fdt): resolve a node by its path, not by its depth and last name"
head: a4c0601 author: Kartikey1306 ci: pass
Verdict: Replaces slash-counting and last-component matching with a component walk gated on ancestor matches. The matcher itself is correct — I traced it against the PR's own decoy tree and against sibling, wrong-parent, too-deep and repeated-name cases. But in_target is still only cleared at target_depth, so a property of any descendant of the resolved node is returned as the resolved node's own. That is the same class of bug the PR body argues against ("that is how you read the wrong peripheral's registers"), and the PR's own test tree exhibits it.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | core/fdt_loader.c FDT_PROP arm, if (in_target && strcmp(pname, prop_name) == 0) |
in_target is set when the target node opens and cleared only by FDT_END_NODE at depth == target_depth. It therefore stays true for the whole subtree, and the property arm never checks the current depth. So a property that exists on a child is returned as the parent's. On the PR's own build_nested tree — / { decoy { uart { reg="decoy-uart" } } soc { uart { reg="soc-uart" } } } — eos_fdt_get_prop(blob, "/soc", "reg", …) returns 0 with "soc-uart", even though /soc has no reg property and the correct answer is -5. Traced by hand: soc opens at depth 2, matched reaches ncomp, in_target=true, target_depth=2; uart opens at depth 3 and the !in_target guard skips the update; the reg property at depth 3 then matches. Real DTBs put 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 attacker-supplied blobs the ordering is not enforced at all. |
One line: if (in_target && depth == target_depth && strcmp(pname, prop_name) == 0). depth is exactly target_depth for the target's own properties and target_depth + 1 or deeper inside children, so this is precise and needs no new state. Add the case the tree already supports: ASSERT(get_prop_exact(&b, "/soc", "reg", out, &len) != 0); — it fails today. |
| 2 | Low | tests/unit/test_fdt_loader.c:561-569 test_the_parent_of_the_node_has_to_match_too |
The name claims the test proves a wrong parent is rejected; the body asserts /decoy/uart resolves to "decoy-uart", which is a second positive resolution, not a rejection. The property that actually proves parent matching is in the preceding test: decoy is emitted before soc in build_nested, so a last-component matcher would have returned "decoy-uart" for /soc/uart, and test_a_node_below_the_root_resolves asserts "soc-uart". Useful test, misleading name — and it makes the suite read as if it has a negative case for wrong parents when the negative coverage lives elsewhere. |
Rename to test_each_uart_returns_its_own_value, and add a comment on test_a_node_below_the_root_resolves noting that decoy precedes soc in the blob, which is what makes its assertion load-bearing. |
| 3 | Low | core/fdt_loader.c, if (ncomp >= FDT_MAX_PATH_DEPTH) return -1; |
A path deeper than 16 components returns -1, the same code the function returns for a NULL argument. The file now returns -1 through -6 and include/eos_fdt_loader.h:37-41 documents none of them, so a caller cannot tell a programming error from a too-deep path from a malformed blob — and the two paths deserve different handling (one is a bug, one is input). |
Give it a distinct code (-8), and add the code table to the header as a doc comment above the declarations. |
Carried forward from #84 — this PR is stacked on it and its diff contains those hunks, so they are still open here. Not re-argued; see .ai/autoreview/reports/eBoot-84-49e1cc1a.md:
- High —
eos_fdt_validate()bounds every block against the blob's owntotalsizeand takes no buffer length, so an inflatedtotalsizein a smaller allocation still passes andget_prop()reads past it. - Medium — a property larger than
*buf_lenis silently truncated and reported as success. - Medium — no
tests/fuzz/fuzz_fdt.c, while five other untrusted parsers in the repo have one. - Low —
core/fdt_loader.centerseboot_corewith zero consumers (§5.1 minimal TCB). - Low — dead duplicate
total < sizeof(fdt_header_t)check ineos_fdt_load()with a comment that misdescribes it.
Architecture conformance
Conforms. §21 Tier 1 — Foundation; device-tree path resolution is boot-time work in the repo that owns it. core/ is right per .ai/architect.md ("core/ shared boot logic") — the change is target-independent string and tree walking with no board or SoC specifics, so hal/ and boards/ are correctly untouched. No new include, link line or target_link_libraries entry points up a tier; the only new dependency is strncmp/strlen from <string.h>, already included. §8 Stage 0 / Stage 1 boundaries are not crossed — this is neither manifest nor image verification. The CMakeLists.txt hunk is #84's, unchanged.
.ai/architect.md asks that restructuring and behaviour change not share a commit. This PR changes behaviour only — the matcher is replaced, nothing is moved — so that rule is satisfied. Stacking the semantic fix separately from #84's bounds fix is the right split and the body says which commit to read.
Proposed changes
if (in_target && depth == target_depth && strcmp(pname, prop_name) == 0)(finding 1), plusASSERT(get_prop_exact(&b, "/soc", "reg", out, &len) != 0);as a fifth path case. This is the only change that alters behaviour and it should land with this PR, not after it — the PR's stated purpose is that a node resolves to its own data.- Rename the test in finding 2 and add the ordering comment; no code change.
- Distinct return code for the depth cap and a documented code table in the header (finding 3).
- Then the #84 items, in that PR or a follow-up — the
totalsizebound in particular, since it is what makes afuzz_fdt.charness meaningful.
Items 2-4 are independent of item 1 and of each other.
Not checked
- No test result was reproduced. The body quotes
14/14in-suite, a specific pre-fix failure attests/unit/test_fdt_loader.c:326, and21/21 for the repo under -DEBLDR_SANITIZE=ON. I ran nothing: reproducing requires checking out the PR head in/home/srpatcha/eos/eBoot, which the run brief forbids. Finding 1 is from reading the code and hand-tracingbuild_nested, not from execution — I did not observe the wrong return value, and that is the one thing that would settle it. - The repo-total counts in #84 and #85 disagree and I could not resolve which is right. #84 says
20/20 ... # 19 before; #85 says21/21. Both add the same singleadd_test(NAME test_fdt_loader …)and nothing else to ctest, so both should report the same repo total. One of the two numbers was not produced by the run it is attached to. Which one is unknown from here. - Sanitizer coverage unverified. No job in
checks.txtis named for-DEBLDR_SANITIZE=ON, so that claim rests on the author's local run. 24 checks pass, includingHost Build & Tests,Build & Test (Linux x86_64),Static Analysis (cppcheck + clang-tidy),CodeQL, 11 EoSim targets and 2 cross-compiles.Create GitHub Releaseandassignreportskipping, expected on a PR. No required check is failing. - Stack-depth cost not measured.
const char *comp[16]plusuint32_t comp_len[16]adds roughly 192 bytes of frame on a 64-bit host, less on 32-bit targets. Whether that fits the Stage 0/Stage 1 stack budget on the §22 Tier-1 boards was not determined; there is no stack-usage job inchecks.txtto consult. - Duplicate-path blobs. The
!in_targetguard means the first occurrence of a path wins if a malformed blob contains it twice. Deliberate-looking and defensible; not raised, not tested either. - Merge state.
mergeStateStatus: BLOCKED,mergeable: MERGEABLEat this head. I did not attempt the merge with #84 or #82.
Automated architecture review of a4c0601c6b8d — 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.
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 embeddedos-org#85
a4c0601 to
7121207
Compare
…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
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 embeddedos-org#85
7121207 to
067101b
Compare
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
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 embeddedos-org#85
067101b to
c06fd2a
Compare
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#85 "fix(fdt): resolve a node by its path, not by its depth and last name"
head: c06fd2a author: Kartikey1306 ci: pass
Verdict: The path resolver is correct. I traced the matcher by hand and then confirmed it against the branch's own decoy tree, and I confirmed the regression claim by running this PR's suite against #84's parser — it fails at exactly the assertion the body names. Two things worth changing: /soc/uart still will not resolve on a real device tree because node names carry unit addresses, and the test counts in the body do not match what the branch produces.
Correction to my review of #84
First, a correction I owe this stack, because I posted it on #84 and cannot amend that comment through this pipeline.
In my #84 review I wrote that #84, #85, #89 and #94 "each independently repair the same broken master" and would "conflict inside core/ed25519_verify.c". That is wrong. The stack is linear and shared:
$ git log --oneline origin/master..refs/pr/85
c06fd2a fix(fdt): resolve a node by its path, not by its depth and last name
9ff5a43 fix(build): drop the duplicate core/fdt_loader.c registration
4649866 fix(fdt): bound the parser against the caller's length, not the blob's claim
a1103a3 fix(fdt): bound a device tree parser that was never compiled
f704d87 fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken
f704d87 is the head of #94. #84 and #89 are both based on it, and #85 is based on #84. There is one repair, not four, and these will merge cleanly in order. The ed25519_verify.c and eos_image.h changes I flagged in #84 are inherited, not duplicated, and my "High" rating there rested on a conflict that does not exist.
What survives from that finding is much smaller and applies to #84, not here: #84's body does not say it is stacked on #94, which is why the inherited commit reads as unexplained scope. This PR does say so in its first line, which is the right thing to do. Apologies for the noise on #84.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | core/fdt_loader.c:176 |
Node names are matched by exact string equality, but real device tree node names carry unit addresses — uart@40011000, soc, serial@10000000. /soc/uart therefore still does not resolve on any actual DTB; it resolves only against trees whose peripheral nodes are named without an address, which is what tests/unit/test_fdt_loader.c:138,145 construct. The PR's own headline example is the case that still fails. /chosen and / are unaffected (no unit address by convention), so the bootargs path this parser exists to serve does keep working. |
Match a component against the node name up to @: if the component contains no @, compare against name truncated at its first @. Add one test case with uart@40011000 resolved by /soc/uart, and one confirming /soc/uart@40011000 still matches exactly. |
| 2 | Low | pr body vs branch |
The body reports "14/14, and 21/21 for the repo under -DEBLDR_SANITIZE=ON". The branch produces 20 cases in test_fdt_loader and 22 tests in the repo. The numbers are stale rather than invented — the suite and the repo both grew after they were written — but a reviewer checking the claim against the branch finds neither number. |
Re-run and paste current output, or drop the counts. |
| 3 | Low | core/fdt_loader.c:14 vs include/eos_fdt_loader.h:49 |
The public header documents return code -8 as "the node path is deeper than FDT_MAX_PATH_DEPTH components", but FDT_MAX_PATH_DEPTH is a #define private to the .c. A caller reading the header cannot find out what the limit is. |
Move the #define into include/eos_fdt_loader.h. |
| 4 | Low | core/fdt_loader.c:176 |
strlen(name) is recomputed here although name_len was already derived from the bounded memchr twelve lines above. Harmless — name is NUL-terminated inside the block by then — but it re-walks the string and reads as if the length were unknown. |
if (name_len - 1 == want && ...). |
Still open from #84, restated only as pointers (they are inherited by this branch, and a reviewer landing here may not have read that thread): the exported eos_fdt_validate() / eos_fdt_get_prop() retain a by-design out-of-bounds read and have no in-tree callers; tests/fuzz/fuzz_fdt.c is in a directory no CI job builds (EBLDR_BUILD_FUZZ is OFF at CMakeLists.txt:29 and appears in no workflow); default: break; in the tag switch treats arbitrary garbage as a NOP. Detail is in the #84 comment rather than repeated here.
What I verified
The matcher logic, traced against the branch's own tree and then run. matched tracks how many leading components have matched; it advances only when matched == depth - 2, so an ancestor that failed to match blocks every descendant, and FDT_END_NODE rolls it back to depth - 2 on the way out. The decoy case is the one that proves it — /decoy/uart and /soc/uart both exist in the fixture with different values, and each path returns its own:
[PASS] test_a_node_below_the_root_resolves
[PASS] test_each_uart_returns_its_own_value
[PASS] test_a_path_that_is_not_in_the_tree_is_not_found
[PASS] test_a_deeper_path_than_the_tree_is_not_found
[PASS] test_a_property_on_a_child_is_not_returned_as_the_parents
[PASS] test_an_overlong_path_is_distinguishable_from_a_null_argument
20 tests passed
Whole repo at this head: 100% tests passed, 0 tests failed out of 22, build clean, 0 errors.
The regression claim holds. This PR's suite compiled against #84's parser:
$ ./build/tests/eboot_test_fdt_loader ; echo exit=$?
[FAIL] tests/unit/test_fdt_loader.c:400: get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0
exit=1
Same assertion the body names; the line number moved from 326 to 400 as the file grew.
The depth == target_depth guard added to FDT_PROP is the quieter half of this change and the one I would not have caught from the description alone. in_target is only cleared by the target's own FDT_END_NODE, so before this it stayed true for the entire subtree and a property found on a grandchild was returned as the target's. test_a_property_on_a_child_is_not_returned_as_the_parents pins it.
Architecture conformance
Master design §8 and §5.1: conforms. No dependency added in any direction; the change is confined to core/fdt_loader.c plus its test. Tier 1 – Foundation (§21), correct repo. Layout matches the eBoot shape in .ai/architect.md.
.ai/security.md "Input validation" requires defined behaviour for malformed input on every externally reachable parser and names device tree among them. The -8 return for an over-deep path is the right shape — it separates "input I will not handle" from "caller passed NULL", and the body says so explicitly. Finding 1 is a functional gap, not a safety one: an unmatched path returns -5, which is safe, just useless.
The design-level gap I raised on #84 is unchanged by this PR and is the more important one: §8.1 requires signed manifests and images but is silent on the device tree, and nothing in eos_fdt_load() verifies the blob before eos_fdt_pass_to_kernel() hands it to the kernel. Making path resolution correct means bootargs is now read from wherever it actually is — which raises the value of an unverified DTB rather than lowering it. Proposal is in .ai/autoreview/proposals/2026-09.md.
Proposed changes
- Unit-address matching (finding 1). In the component comparison, let a component without
@match a node name up to its first@:Two test cases as described above.const char *at = memchr(name, '@', name_len - 1); uint32_t base = at ? (uint32_t)((const char *)at - name) : name_len - 1; bool has_at = memchr(comp[depth-2], '@', want) != NULL; uint32_t n = has_at ? name_len - 1 : base; if (n == want && strncmp(name, comp[depth-2], want) == 0) matched = depth - 1;
- Move
FDT_MAX_PATH_DEPTHto the public header (finding 3). - Replace
strlen(name)withname_len - 1(finding 4). - Refresh the counts in the body (finding 2).
All four are local and covered by the suite already on this branch.
Not checked
- Host x86-64 only. The eight cross-compile jobs and eleven EoSim jobs in
checks.txtare green; I did not reproduce any of them. - No fuzzing was run.
fuzz_fdt.ccompiles underclang -fsanitize=fuzzer,addressbut does not link on this host —libclang_rt.fuzzer.ais absent. So the walk was exercised only by the twenty hand-written blobs, and no CI job would have done more. This PR makes/chosenresolve for the first time, which means the harness's/chosenquery only starts reaching the matching path now; it has never been run against it. - Sanitizers at this head. I ran ASan/UBSan on #84 (22/22) but not again on #85; the body claims it and the incremental change is pure control flow, so I have no reason to doubt it — I just did not repeat it.
- Merge order. The stack is linear and I confirmed the commit graph, but I did not attempt the merges themselves.
- Malformed-blob depth accounting.
matchedanddepthare consistent for well-formed trees, which I traced. For a blob with unbalancedBEGIN_NODE/END_NODEtags I reasoned about the rollback but did not construct a case; the bounds checks make it memory-safe either way, so the exposure is a wrong lookup result, not a fault.
Automated architecture review of c06fd2a9f228 — 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
c06fd2a to
70d80fc
Compare
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 embeddedos-org#85
…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
Rebuilt from the review on cf4bf4f. The FDT work is unchanged; everything else in this branch is a response to a finding. Finding 3 -- scope. core/ed25519_verify.c, include/eos_image.h and tests/unit/test_ed25519.c are gone from this branch. They were a master repair, and embeddedos-org#94 owns it. The branch is now stacked on embeddedos-org#94 rather than racing it; git dropped the duplicate as "patch contents already upstream", which is the confirmation that all three PRs were carrying the same fix. Finding 1 -- the Fuzz Harness Build job this PR adds was red, and it was red for a reason worth stating plainly: three of the five harnesses call functions that have never existed anywhere in the tree. 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 Deleted, with their add_executable blocks. Each declared its target with a local extern instead of including a header, so nothing ever checked that the function existed; and the job that would have caught it did not exist until this PR added it. Deleting a harness that has never compiled removes no coverage -- but it does remove the appearance of coverage, which is finding 2 and is the part that matters. eBoot has no fuzz coverage of firmware-update stream parsing or the recovery protocol, and now says so. 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), so it passed a pointer as a flash address and a size_t as an output-struct pointer. C does not check a declaration against a definition in another translation unit, so it built, ran, and fuzzed nothing. 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. Finding 4 -- the "-8 the node path is deeper than FDT_MAX_PATH_DEPTH" line is gone from include/eos_fdt_loader.h. No path returned it and the constant is defined nowhere. Findings 5 and 6 -- the two fuzz_fdt.c comments now say what is true. The `size` justification no longer argues against unsized entry points this PR removed, and "/chosen" is described as the not-found probe it is until embeddedos-org#85 lands, rather than as coverage of the match path. Finding 7 is moot: the ed25519 comment rewrite that dropped the eBoot#57 attribution is no longer part of this branch. Verified: ctest 22/22, and 22/22 again under -DEBLDR_SANITIZE=ON. All three surviving harnesses link and run against a stub driver (no libFuzzer on this machine, so the smoke-run step is checked in CI, not here).
Corrected counts, and the unit-address caseFinding 2 — the body said "14/14, and 21/21 for the repo under Finding 1 is the one that mattered, and you are right that it made the headline example untrue. Node names were matched by exact string equality, but a real device tree names peripherals
That last one is the counter-check: a loose match that ignored an explicit address would be worse than the bug, on a board with several of the same peripheral. Finding 3 — Finding 4 — Rebased onto #84, which collapsed the parser to one length-carrying entry point per operation — so the last unsized call in this files NULL-argument test went with it. |
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 embeddedos-org#85
|
Restacked onto #84's rebuilt head ( #85 went red on No content change here; the two FDT commits replay unchanged. Verified after the restack: |
…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
70d80fc to
daea335
Compare
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
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 embeddedos-org#85
…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
daea335 to
7a934b4
Compare
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#85 "fix(fdt): resolve a node by its path, not by its depth and last name"
head: 7a934b4 author: Kartikey1306 ci: pass
Verdict: The path resolution is correct and it fixes a real wrong-node defect, not just a not-found one. I traced the ancestor tracking by hand and reproduced the numbers: at this head cmake --build is clean, ctest --no-tests=error is 22/22, again 22/22 under -DEBLDR_SANITIZE=ON (ASan+UBSan), and eboot_test_fdt_loader reports 21 tests passed — so the counts given in the 17:34Z comment hold. What does not hold is the 17:36Z comment about the fuzz tree.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/fuzz/CMakeLists.txt:20,28,36,44,52,60 |
The 17:36Z comment says "tests/fuzz/ now holds only the three harnesses that link (fuzz_image_verify, fuzz_crypto, fuzz_fdt)". At this head all six .c files are present and all six add_executable blocks are live. Three of them cannot link: nm -g --defined-only libeboot_core.a reports eos_fw_update_init, eos_fw_update_process_chunk, eos_recovery_parse_packet and eos_bootctl_parse all absent. The same comment says #85 "went red on Fuzz Harness Build" — no workflow in this repo mentions fuzz at any head in this stack (I grepped all 16 files under .github/workflows via gh api at 7a934b49), and checks.txt lists 24 checks with no fuzz check among them, so nothing can have gone red on it. |
State the fuzz tree as it is, or actually drop the three dead harnesses here. Either way the fuzz-harness cleanup belongs to #99/#100/#101, which are open against exactly this. Do not carry a fourth copy of it. |
| 2 | Low | core/fdt_loader.c:13 |
/* Deepest node path eos_fdt_get_prop() will resolve. */ is left behind after the #define moved to include/eos_fdt_loader.h:21. It now sits immediately above fdt32_to_cpu and reads as that function's comment. |
Delete line 13. |
| 3 | Low | PR body | Two stale statements. "Stacked on #84, which puts core/fdt_loader.c into the build in the first place" — CMakeLists.txt:111 on origin/master (22d8f8b, via #57) already does, and #57 added tests/unit/test_cmake_core_sources.py to keep it there. "14/14, and 21/21 … under -DEBLDR_SANITIZE=ON" — corrected on the thread to 21 and 22/22, but the body a reader lands on still carries the old numbers. |
Edit the body. A number corrected in a comment is still wrong where people read it. |
Inherited from #84 and unchanged here, so raised there rather than counted twice: the FDT header is read through an unaligned fdt_header_t * cast while the struct block goes through the memcpy helper, and default: break; resynchronises the walk on an unknown tag. This PR does resolve one thing #84 leaves dangling — -8 and FDT_MAX_PATH_DEPTH are now real (include/eos_fdt_loader.h:21, core/fdt_loader.c:167) rather than documented-only.
Architecture conformance
Conforms. Tier 1 Foundation (§21). core/fdt_loader.c still includes only eos_fdt_loader.h, eos_hal.h and <string.h> — no upward dependency (§5.1). Exporting FDT_MAX_PATH_DEPTH from the public header is the right direction under .ai/architect.md ("a caller cannot act on a return code without knowing the limit"), and it does not widen the header for one caller — it publishes the bound the contract already referred to.
On the substance, I checked the resolver against the cases the tests do not construct:
matchedunwinds correctly onFDT_END_NODE(matched >= depth - 1 && depth >= 2→matched = depth - 2), including the case where a mid-path component fails and a later sibling at the same depth succeeds.depth - 2 < ncompstops ancestor matching past the end of the requested path, so a deeper subtree cannot advancematched.fdt_name_matchesis asymmetric in the right direction: a component with@requiresfull == comp_len, so/soc/uart@deadbeefcannot fall back to a loose match onuart@40011000. A component without@compares against the name truncated at@.strlen(name)is safe here only because thememchrinFDT_BEGIN_NODEhas already proven a NUL inside the block — worth keeping those two lines adjacent if this is ever refactored.FDT_PROPnow requiresdepth == target_depth, which is the part that turns a subtree-wide match into the target's own properties. Under the old code a target lacking the property returned a child's value instead of not-found; on a boot path readingbootargsthat is a silently wrong value, so this is the more important half of the PR and the body undersells it.ncomp >= FDT_MAX_PATH_DEPTHis tested beforecomp[ncomp]is written, so the 17-component path returns-8rather than overrunning the arrays.
Proposed changes
- Delete the dangling comment at
core/fdt_loader.c:13. - Correct the 17:36Z claim about
tests/fuzz/, and edit the body per finding 3. - Leave the fuzz-harness deletions to #99/#100/#101.
Not checked
- Real device tree input: NOT RUN. Every fixture is hand-built in
tests/unit/test_fdt_loader.c. The unit-address handling is the change most likely to surprise, and nothing here parses adtc-produced blob. One.dtbchecked in as a fixture would close that; I did not verify the behaviour against one. - Fuzz link and run: NOT RUN.
fuzz_fdt.ccompiles at this head; this host has no libFuzzer/ASan runtime (libclang_rt.fuzzer.aabsent), so no harness links here. - Cross builds (
arm-none-eabi, STM32F4) NOT RUN locally;checks.txtreports them green. pytest tests/NOT RUN by me; the 17:34Z comment reports 38 passed and I did not reproduce it.- The
#94/#84portions of this diff (core/ed25519_verify.c,include/eos_image.h,tests/unit/test_ed25519.c, the bounds work ineos_fdt_validate) are reviewed under those PRs. - The local clone does not have
7a934b49; everything above was read from agh api tarballsnapshot at that exact sha, not from the working tree (which sits onfix/ed25519-low-order-keys).
Automated architecture review of 7a934b494d6f — 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
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 embeddedos-org#85
…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
7a934b4 to
df01368
Compare
eos_fdt_get_prop()cannot find any node below the rootThe target depth came from counting slashes:
So
"/chosen"looks for depth 1. The root node occupies depth 1 andchosensits at depth 2, so the comparison runs against the root's own empty name and never matches."/"is the only path that resolves — which is why the suite added with the bounds fix queries"/"and says so in a comment rather than encoding the broken form.And it would have found the wrong node if it could
Matching compared the last component alone:
Once the depth was right,
/soc/uartwould accept a node nameduartunder any parent — whichever the linear walk reached first. On a device tree that is how you read the wrong peripheral's registers.Fix
Split the path into components and advance through them only while every ancestor has matched, so a node is found at its own path. A path of n components ends at depth n + 1, the root being depth 1.
FDT_MAX_PATH_DEPTHbounds the split, and leaving a node un-matches it for the branch the walk returns to.Tests
Four cases, against a tree carrying
/soc/uartand a decoy/decoy/uartwith a different value, so last-component matching cannot pass by luck:uartreturns its own value/soc/spi,/nosuch/uart/soc/uart/childRun against the unfixed parser, the suite stops exactly where it should:
With the fix: 14/14, and 21/21 for the repo under
-DEBLDR_SANITIZE=ON.🤖 Generated with Claude Code