Skip to content

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

Open
Kartikey1306 wants to merge 1 commit into
embeddedos-org:masterfrom
Kartikey1306:fix/fdt-loader-bounds
Open

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

Conversation

@Kartikey1306

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' 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!

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>
@Kartikey1306

Copy link
Copy Markdown
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants