diff --git a/core/fdt_loader.c b/core/fdt_loader.c index 0178bd5..de90ac8 100644 --- a/core/fdt_loader.c +++ b/core/fdt_loader.c @@ -47,6 +47,36 @@ static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize) return off <= totalsize - len; } +/* Does a device tree node name match one path component? + * + * Real node names carry a unit address -- `uart@40011000`, `serial@10000000` + * -- while a path is usually written without one. Exact string equality meant + * /soc/uart resolved only against trees whose peripherals happen to be named + * without an address, which is to say against hand-built test blobs and not + * against any actual DTB. (/ and /chosen were unaffected: by convention they + * carry no unit address, which is why the bootargs path this parser exists to + * serve kept working and the gap went unnoticed.) + * + * A component containing no '@' matches the name up to its '@', so both + * /soc/uart and /soc/uart@40011000 resolve. A component that does contain one + * is compared in full, so an explicit address still selects exactly the node + * asked for -- which matters when a tree has several of the same peripheral. + */ +static bool fdt_name_matches(const char *name, const char *comp, + uint32_t comp_len) +{ + uint32_t full = (uint32_t)strlen(name); + + if (memchr(comp, '@', comp_len) != NULL) { + return full == comp_len && strncmp(name, comp, comp_len) == 0; + } + + const char *at = memchr(name, '@', full); + uint32_t bare = at ? (uint32_t)(at - name) : full; + + return bare == comp_len && strncmp(name, comp, comp_len) == 0; +} + int eos_fdt_validate(const void *fdt_blob, uint32_t avail) { if (!fdt_blob) return -1; @@ -130,12 +160,32 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, int target_depth = -1; bool in_target = false; - /* Count slashes to determine target depth */ - int path_depth = 0; - for (const char *p = node_path; *p; p++) { - if (*p == '/') path_depth++; + /* Split the path into components. The old code counted slashes and + * compared only the last component, which was wrong twice over: the + * root node already occupies depth 1, so "/chosen" looked for depth 1 + * -- the root itself -- and never matched, leaving "/" the only path + * that resolved; and matching the last component alone meant + * "/soc/uart" would have accepted any node named "uart" at that depth, + * whatever its parent. */ + const char *comp[FDT_MAX_PATH_DEPTH]; + uint32_t comp_len[FDT_MAX_PATH_DEPTH]; + int ncomp = 0; + for (const char *p = node_path; *p; ) { + while (*p == '/') p++; + if (!*p) break; + const char *start = p; + while (*p && *p != '/') p++; + /* -8, not -1: -1 means a caller passed NULL, which is a programming + * error, while a path deeper than we track is input. A caller that + * cannot tell them apart cannot handle either correctly. */ + if (ncomp >= FDT_MAX_PATH_DEPTH) return -8; + comp[ncomp] = start; + comp_len[ncomp] = (uint32_t)(p - start); + ncomp++; } - if (path_depth == 0) path_depth = 1; + /* The root is depth 1, so a path of n components ends at depth n + 1. */ + const int target_path_depth = ncomp + 1; + int matched = 0; /* offset < struct_size only guarantees one byte; every read below wants * four or more, so each is checked for the width it actually takes. */ @@ -155,20 +205,25 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, if (offset > struct_size) return -6; depth++; - /* Check if this node matches the target path */ - if (depth == path_depth) { - /* Simple check: compare last component */ - const char *last_slash = strrchr(node_path, '/'); - const char *target_name = last_slash ? last_slash + 1 : node_path; - if (strcmp(name, target_name) == 0 || target_name[0] == '\0') { - in_target = true; - target_depth = depth; + /* Advance along the requested path only while every ancestor + * has matched, so a node is found at its own path and not + * merely by its own name. */ + if (depth >= 2 && matched == depth - 2 && depth - 2 < ncomp) { + uint32_t want = comp_len[depth - 2]; + if (fdt_name_matches(name, comp[depth - 2], want)) { + matched = depth - 1; } } + if (!in_target && depth == target_path_depth && matched == ncomp) { + in_target = true; + target_depth = depth; + } break; } case FDT_END_NODE: if (in_target && depth == target_depth) in_target = false; + /* Leaving a node un-matches it for the branch we return to. */ + if (matched >= depth - 1 && depth >= 2) matched = depth - 2; /* An unbalanced blob would drive depth negative and let a later * BEGIN_NODE match path_depth at the wrong nesting level. */ if (depth == 0) return -6; @@ -192,7 +247,17 @@ int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, * followed the blob into buf. */ if (len > struct_size - offset) return -6; - if (in_target && strcmp(pname, prop_name) == 0) { + /* depth, not just in_target: in_target is cleared by the + * target's own END_NODE, so it stays true for the whole subtree + * and a property on a *child* was returned as the target's own. + * Real trees put properties before subnodes, so a property that + * is present on the target is still found first -- the bug bit + * when the target lacked it and "not found" became a silently + * wrong value from a nested node. depth is exactly target_depth + * for the target's own properties and target_depth + 1 or more + * inside any child. */ + if (in_target && depth == target_depth && + strcmp(pname, prop_name) == 0) { /* Truncating and returning 0 told the caller it had the whole * value. This path reads bootargs: a clipped string that * reports success drops whatever sat at its end, and nothing diff --git a/include/eos_fdt_loader.h b/include/eos_fdt_loader.h index ccc1e2d..d68bfa5 100644 --- a/include/eos_fdt_loader.h +++ b/include/eos_fdt_loader.h @@ -15,6 +15,11 @@ extern "C" { #endif +/* Longest node path this parser resolves, in components. A caller cannot + * act on return code -8 without knowing the limit, so it is declared here + * rather than privately in fdt_loader.c. */ +#define FDT_MAX_PATH_DEPTH 16 + #define FDT_MAGIC 0xD00DFEEDU #define FDT_BEGIN_NODE 0x00000001U #define FDT_END_NODE 0x00000002U @@ -47,6 +52,7 @@ typedef struct { * -5 no such node or property * -6 the blob is malformed: an offset or length escapes it * -7 the property is larger than the caller's buffer (see below) + * -8 the node path is deeper than FDT_MAX_PATH_DEPTH components */ /** diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index aa4511d..61a1073 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -198,6 +198,12 @@ add_executable(eboot_test_fw_decrypt unit/test_fw_decrypt.c) target_link_libraries(eboot_test_fw_decrypt PRIVATE eboot_core) add_test(NAME test_fw_decrypt COMMAND eboot_test_fw_decrypt) +# --- test_fdt_loader: device tree parsing against malformed blobs --- +# core/fdt_loader.c was in no source list, so it had never been compiled. +add_executable(eboot_test_fdt_loader unit/test_fdt_loader.c) +target_link_libraries(eboot_test_fdt_loader PRIVATE eboot_core) +add_test(NAME test_fdt_loader COMMAND eboot_test_fdt_loader) + # --- Valgrind test targets --- # # The list is derived from the suites registered above, not written out again. diff --git a/tests/unit/test_fdt_loader.c b/tests/unit/test_fdt_loader.c index 7b0095f..3a68ae5 100644 --- a/tests/unit/test_fdt_loader.c +++ b/tests/unit/test_fdt_loader.c @@ -119,6 +119,98 @@ static void build_valid(blob_t *b, const char *value) put_be32((unsigned char *)&h->size_dt_strings, size_strings); } +/* / { soc { uart { reg = ; }; }; decoy { }; } + * + * "uart" sits at depth 3 under "soc", and a second node named "uart" is + * placed under "decoy" so that matching by last component alone would find + * the wrong one. */ +static void build_nested(blob_t *b, const char *value, const char *decoy) +{ + memset(b, 0, sizeof(*b)); + b->len = sizeof(fdt_header_t); + uint32_t off_struct = b->len; + uint32_t vlen = (uint32_t)strlen(value) + 1; + uint32_t dlen = (uint32_t)strlen(decoy) + 1; + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, ""); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "decoy"); + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart"); + append_be32(b, FDT_PROP); append_be32(b, dlen); append_be32(b, 0); + append_padded(b, decoy); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "soc"); + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart"); + append_be32(b, FDT_PROP); append_be32(b, vlen); append_be32(b, 0); + append_padded(b, value); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END); + uint32_t size_struct = b->len - off_struct; + + uint32_t off_strings = b->len; + append_padded(b, "reg"); + uint32_t size_strings = b->len - off_strings; + + fdt_header_t *h = (fdt_header_t *)b->bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b->len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); +} + +/* Like build_nested, but with the unit addresses a real device tree carries. + * Exact-equality matching resolved nothing here, which is the case the PR's + * own headline example describes and could not do. */ +static void build_addressed(blob_t *b, const char *v0, const char *v1) +{ + memset(b, 0, sizeof(*b)); + b->len = sizeof(fdt_header_t); + uint32_t off_struct = b->len; + uint32_t l0 = (uint32_t)strlen(v0) + 1; + uint32_t l1 = (uint32_t)strlen(v1) + 1; + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, ""); + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "soc"); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart@40011000"); + append_be32(b, FDT_PROP); append_be32(b, l0); append_be32(b, 0); + append_padded(b, v0); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_BEGIN_NODE); append_padded(b, "uart@40004400"); + append_be32(b, FDT_PROP); append_be32(b, l1); append_be32(b, 0); + append_padded(b, v1); + append_be32(b, FDT_END_NODE); + + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END_NODE); + append_be32(b, FDT_END); + uint32_t size_struct = b->len - off_struct; + + uint32_t off_strings = b->len; + append_padded(b, "reg"); + uint32_t size_strings = b->len - off_strings; + + fdt_header_t *h = (fdt_header_t *)b->bytes; + put_be32((unsigned char *)&h->magic, FDT_MAGIC); + put_be32((unsigned char *)&h->version, 17); + put_be32((unsigned char *)&h->last_comp_version, 16); + put_be32((unsigned char *)&h->totalsize, b->len); + put_be32((unsigned char *)&h->off_dt_struct, off_struct); + put_be32((unsigned char *)&h->size_dt_struct, size_struct); + put_be32((unsigned char *)&h->off_dt_strings, off_strings); + put_be32((unsigned char *)&h->size_dt_strings, size_strings); +} + static void set_field(blob_t *b, size_t field_offset, uint32_t v) { put_be32(b->bytes + field_offset, v); @@ -400,6 +492,114 @@ static void test_an_unaligned_blob_parses(void) free(heap); } +static void test_a_node_below_the_root_resolves(void) +{ + /* "/chosen" used to look for depth 1 -- the root -- so no path below + * the root ever resolved and only "/" worked. + * + * This assertion is also what proves the parent is matched, and that is + * load-bearing rather than incidental: build_nested emits "decoy" before + * "soc", so a matcher comparing only the last component would meet + * /decoy/uart first and return "decoy-uart". Getting "soc-uart" here + * means the ancestors were checked. */ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0); + ASSERT(strcmp(out, "soc-uart") == 0); +} + +static void test_each_uart_returns_its_own_value(void) +{ + /* Matching the last component alone would return decoy/uart's value + * for /soc/uart, whichever the walk reached first. */ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/decoy/uart", "reg", out, &len) == 0); + ASSERT(strcmp(out, "decoy-uart") == 0); +} + +static void test_a_path_that_is_not_in_the_tree_is_not_found(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/spi", "reg", out, &len) != 0); + len = sizeof out; + ASSERT(get_prop_exact(&b, "/nosuch/uart", "reg", out, &len) != 0); +} + +static void test_a_deeper_path_than_the_tree_is_not_found(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart/child", "reg", out, &len) != 0); +} + +/* The property arm used to test in_target alone, and in_target is only + * cleared by the target's own END_NODE -- so it stayed true through the whole + * subtree and a child's property came back as the parent's. /soc has no "reg" + * of its own; the correct answer is "not found", and what came back was + * /soc/uart's value. That is the same class of mistake this PR is named for: + * reading the wrong node's registers. */ +static void test_a_property_on_a_child_is_not_returned_as_the_parents(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc", "reg", out, &len) != 0); + + /* and the child itself still resolves, so the guard is not just + * refusing everything below the root */ + len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0); + ASSERT(strcmp(out, "soc-uart") == 0); +} + +/* A path deeper than FDT_MAX_PATH_DEPTH is input, not a programming error, + * and used to share -1 with "a caller passed NULL". */ +static void test_an_overlong_path_is_distinguishable_from_a_null_argument(void) +{ + blob_t b; build_nested(&b, "soc-uart", "decoy-uart"); + char out[24]; uint32_t len = sizeof out; + + char deep[128] = ""; + for (int i = 0; i < 20; i++) strcat(deep, "/a"); + + ASSERT(get_prop_exact(&b, deep, "reg", out, &len) == -8); + len = sizeof out; + ASSERT(eos_fdt_get_prop(NULL, b.len, "/soc", "reg", out, &len) == -1); +} + +/* The headline case: a path written without a unit address, against a tree + * that has one. Exact string equality never resolved this, so /soc/uart did + * not work on any real DTB -- only on trees built without addresses, which is + * what every other fixture in this file constructs. */ +static void test_a_path_without_a_unit_address_resolves_a_node_with_one(void) +{ + blob_t b; build_addressed(&b, "uart0", "uart1"); + char out[24]; uint32_t len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart", "reg", out, &len) == 0); + /* The first match wins, as it does for any other duplicate name. */ + ASSERT(strcmp(out, "uart0") == 0); +} + +/* And an explicit address still selects exactly the node asked for, which is + * what makes the loose match safe on a tree with several of a peripheral. */ +static void test_an_explicit_unit_address_selects_that_node(void) +{ + blob_t b; build_addressed(&b, "uart0", "uart1"); + char out[24]; uint32_t len = sizeof out; + + ASSERT(get_prop_exact(&b, "/soc/uart@40004400", "reg", out, &len) == 0); + ASSERT(strcmp(out, "uart1") == 0); + + len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart@40011000", "reg", out, &len) == 0); + ASSERT(strcmp(out, "uart0") == 0); + + /* An address that is not in the tree must not fall back to a loose match. */ + len = sizeof out; + ASSERT(get_prop_exact(&b, "/soc/uart@deadbeef", "reg", out, &len) != 0); +} + int main(void) { printf("=== eBootloader FDT Loader Tests ===\n"); @@ -419,6 +619,14 @@ int main(void) RUN(test_a_garbage_tag_is_refused_not_skipped); RUN(test_nop_padding_does_not_break_resolution); RUN(test_an_unaligned_blob_parses); + RUN(test_a_node_below_the_root_resolves); + RUN(test_each_uart_returns_its_own_value); + RUN(test_a_path_that_is_not_in_the_tree_is_not_found); + RUN(test_a_deeper_path_than_the_tree_is_not_found); + RUN(test_a_property_on_a_child_is_not_returned_as_the_parents); + RUN(test_an_overlong_path_is_distinguishable_from_a_null_argument); + RUN(test_a_path_without_a_unit_address_resolves_a_node_with_one); + RUN(test_an_explicit_unit_address_selects_that_node); printf("\n%d tests passed\n", tests_passed); return 0; }