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

core/ed25519_verify.c

@@ -289,6 +289,16 @@ static int point_is_identity(gf p[4])
     return diff == 0;
 }
 
+static void scalarbase(gf r[4], const uint8_t *s)
+{
+    gf q[4];
+    fe_copy16(q[0], BX);
+    fe_copy16(q[1], BY);
+    fe_copy16(q[2], gf1);
+    fe_mul(q[3], BX, BY);
+    scalarmult(r, q, s);
+}
+
 /* Reject a public key outside the prime-order subgroup.
  *
  * Decoding a point is not enough. Ed25519 has eight points of low order, and
@@ -304,51 +314,9 @@ static int point_is_identity(gf p[4])
  * so there is no separate constant to transcribe wrongly: a mistyped L would
  * reject valid keys, and only in the field.
  *
- * A arrives negated from unpackneg(). [L](-A) = -[L]A and the identity is its
- * own negation, so neither condition is affected by the sign.
- *
- * Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached
- * this before I did and states both conditions in one expression.
+ * The key arrives negated from unpackneg(). [L](-A) = -[L]A and the identity
+ * is its own negation, so neither condition is affected by the sign.
  */
-static int key_has_prime_order(gf A[4])
-{
-    uint8_t order_l[32];
-    gf q[4], multiple[4];
-    int i;
-
-    for (i = 0; i < 32; i++)
-        order_l[i] = (uint8_t)ORDER_L[i];
-    for (i = 0; i < 4; i++)
-        fe_copy16(q[i], A[i]);
-
-    scalarmult(multiple, q, order_l);
-    return point_is_identity(multiple) && !point_is_identity(A);
-}
-
-static void scalarbase(gf r[4], const uint8_t *s)
-{
-    gf q[4];
-    fe_copy16(q[0], BX);
-    fe_copy16(q[1], BY);
-    fe_copy16(q[2], gf1);
-    fe_mul(q[3], BX, BY);
-    scalarmult(r, q, s);
-}
-
-static int point_is_identity(gf p[4])
-{
-    uint8_t encoded[32];
-    point_pack(encoded, p);
-
-    uint8_t diff = (uint8_t)(encoded[0] ^ 1U);
-    for (int i = 1; i < 32; i++)
-        diff |= encoded[i];
-    return diff == 0;
-}
-
-/* Public keys must be non-identity points in Ed25519's prime-order subgroup.
- * Merely decoding a point is insufficient: an identity or torsion key can
- * make the verification equation true without knowledge of a private key. */
 static int public_key_is_valid_subgroup(gf public_key[4])
 {
     uint8_t order_l[32];

core/fdt_loader.c

@@ -8,6 +8,7 @@
 
 #include "eos_fdt_loader.h"
 #include "eos_hal.h"
+#include <stddef.h>
 #include <string.h>
 
 /* Big-endian to host conversion */
@@ -26,15 +27,63 @@ static uint32_t fdt_read_u32(const uint8_t *ptr)
     return fdt32_to_cpu(val);
 }
 
-int eos_fdt_validate(const void *fdt_blob)
+/* Header fields, read the same way the struct block is read. The blob may
+ * be unaligned -- fuzz input, a buffer inside a larger message, a copy at an
+ * odd offset -- and dereferencing a cast fdt_header_t* is a misaligned load
+ * on strict-alignment targets, the same fault class this parser exists to
+ * avoid. One rule for the whole blob: every multi-byte read goes through
+ * memcpy. */
+static uint32_t fdt_hdr_u32(const void *blob, size_t field_off)
+{
+    return fdt_read_u32((const uint8_t *)blob + field_off);
+}
+
+/* Does [off, off+len) sit inside a blob of totalsize bytes, without the
+ * addition wrapping? Every offset in the header is attacker-controlled. */
+static bool fdt_block_in_bounds(uint32_t off, uint32_t len, uint32_t totalsize)
+{
+    if (off < sizeof(fdt_header_t)) return false;
+    if (len > totalsize) return false;
+    return off <= totalsize - len;
+}
+
+int eos_fdt_validate(const void *fdt_blob, uint32_t avail)
 {
     if (!fdt_blob) return -1;
-    const fdt_header_t *hdr = (const fdt_header_t *)fdt_blob;
-    if (fdt32_to_cpu(hdr->magic) != FDT_MAGIC) return -2;
-    if (fdt32_to_cpu(hdr->version) < 16) return -3;
+    if (avail < sizeof(fdt_header_t)) return -6;
+    if (fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, magic)) != FDT_MAGIC)
+        return -2;
+    if (fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, version)) < 16)
+        return -3;
+
+    /* Before anything else: the blob does not get to say how big it is. Every
+     * bound below is relative to totalsize, so without this the checks only
+     * prove the header is self-consistent — a 40-byte buffer claiming a 1 MiB
+     * totalsize with agreeing block offsets passes all of them, and the walk
+     * then runs a megabyte past the allocation. */
+    if (fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, totalsize)) > avail)
+        return -6;
+
+    /* magic and version alone say nothing about where the blob claims its
+     * blocks are. The struct and string offsets are read straight out of
+     * flash and then used as pointers, so a blob that clears the two checks
+     * above could still point them anywhere; eos_fdt_get_prop walked off the
+     * end of a 40-byte allocation and took a bus fault. Reject the blob here
+     * instead, because this is the gate every path goes through. */
+    uint32_t totalsize = fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, totalsize));
+    if (totalsize < sizeof(fdt_header_t)) return -6;
+    if (!fdt_block_in_bounds(fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, off_dt_struct)),
+                             fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, size_dt_struct)),
+                             totalsize))
+        return -6;
+    if (!fdt_block_in_bounds(fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, off_dt_strings)),
+                             fdt_hdr_u32(fdt_blob, offsetof(fdt_header_t, size_dt_strings)),
+                             totalsize))
+        return -6;
     return 0;
 }
 
+
 int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size)
 {
     if (!dest || max_size < sizeof(fdt_header_t)) return -1;
@@ -43,27 +92,37 @@ int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size)
     const void *src = (const void *)(uintptr_t)flash_addr;
     memcpy(dest, src, sizeof(fdt_header_t));
 
-    int rc = eos_fdt_validate(dest);
+    /* Only the header has been copied so far, but max_size is what the caller
+     * really owns, so that is the bound the blob has to satisfy. */
+    int rc = eos_fdt_validate(dest, max_size);
     if (rc != 0) return rc;
 
-    const fdt_header_t *hdr = (const fdt_header_t *)dest;
-    uint32_t total = fdt32_to_cpu(hdr->totalsize);
+    uint32_t total = fdt_hdr_u32(dest, offsetof(fdt_header_t, totalsize));
     if (total > max_size) return -4;
 
     /* Copy full DTB */
     memcpy(dest, src, total);
     return 0;
 }
 
-int eos_fdt_get_prop(const void *fdt, const char *node_path,
-                      const char *prop_name, void *buf, uint32_t *buf_len)
+int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len,
+                     const char *node_path, const char *prop_name,
+                     void *buf, uint32_t *buf_len)
 {
     if (!fdt || !node_path || !prop_name || !buf || !buf_len) return -1;
 
-    const fdt_header_t *hdr = (const fdt_header_t *)fdt;
-    const uint8_t *dt_struct = (const uint8_t *)fdt + fdt32_to_cpu(hdr->off_dt_struct);
-    const char *dt_strings = (const char *)fdt + fdt32_to_cpu(hdr->off_dt_strings);
-    uint32_t struct_size = fdt32_to_cpu(hdr->size_dt_struct);
+    /* The blob is whatever was in flash. Every offset below comes out of its
+     * header, so none of them can be trusted until validate() has bounded
+     * them against both totalsize and the length the caller actually owns. */
+    int vrc = eos_fdt_validate(fdt, fdt_len);
+    if (vrc != 0) return vrc;
+
+    const uint8_t *dt_struct = (const uint8_t *)fdt +
+        fdt_hdr_u32(fdt, offsetof(fdt_header_t, off_dt_struct));
+    const char *dt_strings = (const char *)fdt +
+        fdt_hdr_u32(fdt, offsetof(fdt_header_t, off_dt_strings));
+    uint32_t struct_size = fdt_hdr_u32(fdt, offsetof(fdt_header_t, size_dt_struct));
+    uint32_t strings_size = fdt_hdr_u32(fdt, offsetof(fdt_header_t, size_dt_strings));
 
     /* Simple linear search through struct block */
     uint32_t offset = 0;
@@ -78,15 +137,22 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path,
     }
     if (path_depth == 0) path_depth = 1;
 
-    while (offset < struct_size) {
+    /* offset < struct_size only guarantees one byte; every read below wants
+     * four or more, so each is checked for the width it actually takes. */
+    while (offset + 4 <= struct_size) {
         uint32_t tag = fdt_read_u32(dt_struct + offset);
         offset += 4;
 
         switch (tag) {
         case FDT_BEGIN_NODE: {
+            /* strlen() here read until it happened to find a zero, which
+             * for a name running to the end of the block is past it. */
             const char *name = (const char *)(dt_struct + offset);
-            uint32_t name_len = (uint32_t)strlen(name) + 1;
+            const void *nul = memchr(name, '\0', struct_size - offset);
+            if (!nul) return -6;
+            uint32_t name_len = (uint32_t)((const char *)nul - name) + 1;
             offset += (name_len + 3) & ~3U;
+            if (offset > struct_size) return -6;
             depth++;
 
             /* Check if this node matches the target path */
@@ -103,32 +169,67 @@ int eos_fdt_get_prop(const void *fdt, const char *node_path,
         }
         case FDT_END_NODE:
             if (in_target && depth == target_depth) in_target = false;
+            /* 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;
             depth--;
             break;
         case FDT_PROP: {
+            if (offset + 8 > struct_size) return -6;
             uint32_t len = fdt_read_u32(dt_struct + offset);
             uint32_t nameoff = fdt_read_u32(dt_struct + offset + 4);
             offset += 8;
+
+            /* nameoff indexes the strings block; unchecked it named any
+             * address, and strcmp then read from it. */
+            if (nameoff >= strings_size) return -6;
             const char *pname = dt_strings + nameoff;
+            if (!memchr(pname, '\0', strings_size - nameoff)) return -6;
+
+            /* The value has to be inside the struct block before it is read:
+             * copy_len was clamped to the caller's buffer, which bounded the
+             * write but not the read, so an oversized len leaked whatever
+             * followed the blob into buf. */
+            if (len > struct_size - offset) return -6;
 
             if (in_target && strcmp(pname, prop_name) == 0) {
-                uint32_t copy_len = len < *buf_len ? len : *buf_len;
-                memcpy(buf, dt_struct + offset, copy_len);
-                *buf_len = copy_len;
+                /* 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
+                 * distinguishes that from a short property. Report the full
+                 * length so the caller can size a retry. */
+                if (len > *buf_len) {
+                    *buf_len = len;
+                    return -7;
+                }
+                memcpy(buf, dt_struct + offset, len);
+                *buf_len = len;
                 return 0;
             }
+            /* len is bounded above, so the pad cannot wrap. */
             offset += (len + 3) & ~3U;
+            if (offset > struct_size) return -6;
             break;
         }
         case FDT_END:
             return -5; /* Property not found */
-        default:
+        case FDT_NOP:
+            /* Legal padding; the spec allows it anywhere between tokens. */
             break;
+        default:
+            /* Refuse, do not resynchronise. Skipping an unknown tag and
+             * treating whatever sits 4 bytes on as the next token meant a
+             * struct block of arbitrary bytes parsed to completion. Every
+             * read stayed bounded, so this was not memory-unsafe -- but a
+             * parser in the TCB that walks garbage to a clean "not found"
+             * is quietly accepting input it does not understand. */
+            return -6;
         }
     }
     return -5;
 }
 
+
 void eos_fdt_pass_to_kernel(uint32_t dtb_addr)
 {
     /* The DTB address is passed to the kernel via:

include/eos_fdt_loader.h

@@ -19,6 +19,7 @@ extern "C" {
 #define FDT_BEGIN_NODE      0x00000001U
 #define FDT_END_NODE        0x00000002U
 #define FDT_PROP            0x00000003U
+#define FDT_NOP             0x00000004U
 #define FDT_END             0x00000009U
 
 typedef struct {
@@ -34,10 +35,56 @@ typedef struct {
     uint32_t size_dt_struct;
 } fdt_header_t;
 
-int eos_fdt_validate(const void *fdt_blob);
+/**
+ * Return codes. Every entry point below returns 0 on success and one of these
+ * on failure; they are distinct so a caller can tell a programming error from
+ * a malformed blob from a buffer that was too small.
+ *
+ *   -1  a required argument was NULL
+ *   -2  the blob does not carry the FDT magic
+ *   -3  the blob declares an unsupported version (< 16)
+ *   -4  the blob does not fit the destination buffer
+ *   -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)
+ */
+
+/**
+ * Validate a blob.
+ *
+ * @param avail  bytes actually readable at @p fdt_blob.
+ *
+ * There is deliberately no length-free form. Every bound inside the header --
+ * the struct and string block offsets and sizes -- is expressed relative to
+ * the header's own totalsize field, which is attacker-controlled; checking
+ * those against each other proves the blob is internally consistent and says
+ * nothing about how many bytes are really mapped. An earlier revision kept a
+ * one-argument wrapper that read totalsize and passed it as the bound, which
+ * is an out-of-bounds read on a short buffer before any check has run. A
+ * documented caller warrant is not a check, and an exported unsafe twin in a
+ * TCB header is a future boot-path caller reintroducing the bug with no
+ * compiler complaint.
+ */
+int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
+
 int eos_fdt_load(uint32_t flash_addr, void *dest, uint32_t max_size);
-int eos_fdt_get_prop(const void *fdt, const char *node_path,
-                      const char *prop_name, void *buf, uint32_t *buf_len);
+
+/**
+ * Read a property.
+ *
+ * @param fdt_len  bytes actually readable at @p fdt.
+ * @param buf_len  in: capacity of @p buf. out: bytes written on success, or
+ *                 the property's full length when -7 is returned, so the
+ *                 caller can size a retry.
+ *
+ * Returns -7 rather than truncating. A boot path reads bootargs through here,
+ * and a silently clipped value that reports success loses whatever sat at the
+ * end of the string.
+ */
+int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len,
+                     const char *node_path, const char *prop_name,
+                     void *buf, uint32_t *buf_len);
+
 void eos_fdt_pass_to_kernel(uint32_t dtb_addr);
 
 #ifdef __cplusplus

include/eos_image.h

@@ -108,7 +108,8 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) +
 
 /* Every remaining field, pinned.
  *
- * Four of the fourteen fields were asserted. Transposing two adjacent
+ * Three of the thirteen field offsets were asserted (the fourth pre-existing
+ * assert is sizeof, which is not a field). Transposing two adjacent
  * same-width fields moves neither sizeof nor any of those four offsets, so it
  * compiled clean: with load_addr and entry_addr swapped, all four existing
  * asserts still passed and the bootloader would load an image at its entry
@@ -132,15 +133,18 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, flags) == 24,
                       "flags must stay at offset 24");
 EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, sig_len) == 61,
                       "sig_len must stay at offset 61");
-EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, reserved) == 62,
-                      "reserved[] must stay at offset 62");
+/* tlv_len and tlv_hash are asserted above, where #93 introduced them; the
+ * 30 bytes they occupy are the ones this block used to pin as reserved[]. */
 
 /* Field widths. An offset assert cannot see a field growing into padding that
- * happens to keep every later offset -- reserved[] absorbs exactly that. */
+ * happens to keep every later offset -- the 30 bytes at 62 absorb exactly
+ * that, which is why both halves of that span carry a width assert. */
 EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->hash) == 32,
                       "hash[] is 32 bytes on the wire");
-EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->reserved) == 30,
-                      "reserved[] is 30 bytes on the wire");
+EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_len) == 2,
+                      "tlv_len is 2 bytes on the wire");
+EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_hash) == 28,
+                      "tlv_hash is 28 bytes on the wire");
 EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->signature) == 64,
                       "signature[] is 64 bytes on the wire");