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

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

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
  */
 
 /**

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.

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 = <value>; }; }; 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;
 }