fdt: rewrite device tree parser with capacity bound and full validation

Author: dgarske

.github/workflows/test-parse-tools.yml

@@ -46,3 +46,12 @@ jobs:
       - name: Run fdt-parser test (nxp_t1024.dtb)
         run: |
           ./tools/fdt-parser/fdt-parser ./tools/fdt-parser/nxp_t1024.dtb -t
+
+      - name: Run fdt-parser malformed-input corpus under ASan/UBSan
+        run: |
+          gcc -o /tmp/fdt-parser-asan -Wall -Werror -g -O1 \
+            -fsanitize=address,undefined -fno-omit-frame-pointer \
+            -I include -DWOLFBOOT_FDT -DWOLFBOOT_FDT_CORPUS -DPRINTF_ENABLED \
+            tools/fdt-parser/fdt-parser.c src/fdt.c
+          ASAN_OPTIONS=detect_leaks=0 \
+            /tmp/fdt-parser-asan ./tools/fdt-parser/nxp_t1024.dtb -f

docs/Targets.md

@@ -5007,7 +5007,7 @@ Image kernel-1: 0x200000 (24617472 bytes)
 Loading Image fdt-1: 0x1177A3DC -> 0x1000 (39384 bytes)
 Image fdt-1: 0x1000 (39384 bytes)
 Loading DTS: 0x1000 -> 0x1000 (39384 bytes)
-FDT: Version 17, Size 39384
+FDT: Size 39384
 FDT: Setting bootargs: earlycon root=/dev/mmcblk0p2 rootwait
 FDT: Set chosen (28076), bootargs=earlycon root=/dev/mmcblk0p2 rootwait
 Booting at 0x200000

hal/cm4.c

@@ -152,9 +152,9 @@ void* hal_get_dts_update_address(void)
  * clock, serial no.) to the kernel it loads - which is wolfBoot. */
 extern void* cm4_fw_dtb;
 
-/* Upper bound for the firmware DTB copy. fdt_check_header() does not validate
- * totalsize, so cap it: a valid DTB is well under the arm64 boot-protocol 2MB
- * limit, and this keeps a corrupt header from overrunning the DTS landing zone.
+/* Size of the DTS landing zone, and so the capacity handed to fdt_open()
+ * for the firmware DTB: a valid DTB is well under the arm64 boot-protocol
+ * 2MB limit, and this keeps a corrupt header from overrunning the zone.
  * Override with -DCM4_FDT_MAX_SIZE=<bytes>. */
 #ifndef CM4_FDT_MAX_SIZE
 #define CM4_FDT_MAX_SIZE 0x200000
@@ -358,24 +358,25 @@ void* hal_get_boot_dts(void)
      * validated NULL-DTB handoff is unchanged. */
     return NULL;
 #else
+    fdt_ctx ctx;
     void *fdt = cm4_fw_dtb;
     uint32_t sz;
     int off;
 
-    if (fdt == NULL || fdt_check_header(fdt) != 0) {
-        wolfBoot_printf("cm4: no valid firmware DTB (%p)\n", fdt);
+    if (fdt == NULL) {
+        wolfBoot_printf("cm4: no firmware DTB\n");
         return NULL;
     }
-    sz = (uint32_t)fdt_totalsize(fdt);
-    /* fdt_check_header() does not validate totalsize; bound it so a corrupt DTB
-     * header cannot make the copy/fixup clobber memory past the DTS landing
-     * zone (leaving room for the fixup headroom too). */
-    if (sz < sizeof(struct fdt_header) ||
-            sz > CM4_FDT_MAX_SIZE - WOLFBOOT_FDT_FIXUP_HEADROOM) {
-        wolfBoot_printf("cm4: firmware DTB size %u out of range\n",
-            (unsigned)sz);
+    /* Bound the parse by the largest DTB this path can ever accept: the
+     * DTS landing zone less the fixup headroom. A corrupt header can then
+     * neither be walked out of bounds here nor make the copy below clobber
+     * memory past that zone. */
+    if (fdt_open(&ctx, fdt,
+            CM4_FDT_MAX_SIZE - WOLFBOOT_FDT_FIXUP_HEADROOM) != 0) {
+        wolfBoot_printf("cm4: no valid firmware DTB (%p)\n", fdt);
         return NULL;
     }
+    sz = fdt_size(&ctx);
     /* SECURITY: the firmware DTB lives on the unsigned FAT partition (unless
      * the RPi EEPROM secure-boot is enabled), so it is NOT covered by wolfBoot's
      * signature. Only /chosen/bootargs is overwritten below; /memory,
@@ -405,12 +406,16 @@ void* hal_get_boot_dts(void)
     fdt = (void*)WOLFBOOT_LOAD_DTS_ADDRESS;
 
     /* Zero the appended headroom so the grown blob holds no uninitialized
-     * bytes, then record the new total size. */
+     * bytes, then re-open on the landing zone and record the new size. */
     memset((uint8_t*)fdt + sz, 0, WOLFBOOT_FDT_FIXUP_HEADROOM);
-    fdt_set_totalsize(fdt, sz + WOLFBOOT_FDT_FIXUP_HEADROOM);
-    off = fdt_find_node_offset(fdt, -1, "chosen");
+    if (fdt_open(&ctx, fdt, CM4_FDT_MAX_SIZE) != 0 ||
+            fdt_grow(&ctx, WOLFBOOT_FDT_FIXUP_HEADROOM) != 0) {
+        wolfBoot_printf("cm4: relocated DTB rejected; refusing firmware DTB\n");
+        return NULL;
+    }
+    off = fdt_subnode_offset(&ctx, 0, "chosen");
     if (off == -FDT_ERR_NOTFOUND) {
-        off = fdt_add_subnode(fdt, 0, "chosen");
+        off = fdt_add_subnode(&ctx, 0, "chosen");
     }
     if (off < 0) {
         /* Fail closed: without /chosen we cannot inject our known-good bootargs,
@@ -426,13 +431,13 @@ void* hal_get_boot_dts(void)
         const char *args = LINUX_BOOTARGS;
         if (cm4_rauc_build_bootargs(cm4_bootargs, sizeof(cm4_bootargs)) == 0)
             args = cm4_bootargs;
-        if (fdt_fixup_str(fdt, off, "chosen", "bootargs", args) != 0) {
+        if (fdt_fixup_str(&ctx, off, "chosen", "bootargs", args) != 0) {
             wolfBoot_printf("cm4: DTB bootargs fixup failed; refusing firmware DTB\n");
             return NULL;
         }
     }
 #else
-    if (fdt_fixup_str(fdt, off, "chosen", "bootargs", LINUX_BOOTARGS) != 0) {
+    if (fdt_fixup_str(&ctx, off, "chosen", "bootargs", LINUX_BOOTARGS) != 0) {
         wolfBoot_printf("cm4: DTB bootargs fixup failed; refusing firmware DTB\n");
         return NULL;
     }

hal/mpfs250.c

@@ -601,10 +601,10 @@ int mpfs_read_serial_number(uint8_t *serial)
 #define MICROCHIP_OUI_1 0x04
 #define MICROCHIP_OUI_2 0xA3
 
-static int mpfs_dts_fixup_inplace(void* dts_addr)
+static int mpfs_dts_fixup_inplace(void* dts_addr, uint32_t capacity)
 {
+    fdt_ctx ctx;
     int off, ret;
-    struct fdt_header *fdt = (struct fdt_header *)dts_addr;
     uint8_t device_serial_number[DEVICE_SERIAL_NUMBER_SIZE];
     uint8_t mac_addr[6];
 #if defined(MPFS_DDR_INIT) && defined(WOLFBOOT_MMODE_SMODE_BOOT)
@@ -620,31 +620,33 @@ static int mpfs_dts_fixup_inplace(void* dts_addr)
     unsigned int i;
 #endif
 
-    /* Verify FDT header */
-    ret = fdt_check_header(dts_addr);
+    /* Validate the blob against the window it actually occupies. */
+    ret = fdt_open(&ctx, dts_addr, capacity);
     if (ret != 0) {
         wolfBoot_printf("FDT: Invalid header! %d\n", ret);
         return ret;
     }
 
-    wolfBoot_printf("FDT: Version %d, Size %d\n",
-        fdt_version(fdt), fdt_totalsize(fdt));
+    wolfBoot_printf("FDT: Size %d\n", (int)fdt_size(&ctx));
 
-    /* Expand total size to allow adding/modifying properties.
+    /* Reserve free space to allow adding/modifying properties.
      * Sizing comes from WOLFBOOT_FDT_FIXUP_HEADROOM in include/fdt.h. */
-    fdt_set_totalsize(fdt,
-        fdt_totalsize(fdt) + WOLFBOOT_FDT_FIXUP_HEADROOM);
+    ret = fdt_grow(&ctx, WOLFBOOT_FDT_FIXUP_HEADROOM);
+    if (ret != 0) {
+        wolfBoot_printf("FDT: No headroom for fixups (%d)\n", ret);
+        return ret;
+    }
 
     /* Find /chosen node */
-    off = fdt_find_node_offset(fdt, -1, "chosen");
+    off = fdt_subnode_offset(&ctx, 0, "chosen");
     if (off < 0) {
         /* Create /chosen node if it doesn't exist */
-        off = fdt_add_subnode(fdt, 0, "chosen");
+        off = fdt_add_subnode(&ctx, 0, "chosen");
     }
 
     if (off >= 0) {
         /* Set bootargs property */
-        fdt_fixup_str(fdt, off, "chosen", "bootargs", LINUX_BOOTARGS);
+        fdt_fixup_str(&ctx, off, "chosen", "bootargs", LINUX_BOOTARGS);
     }
 
 #if defined(MPFS_DDR_INIT) && defined(WOLFBOOT_MMODE_SMODE_BOOT)
@@ -665,9 +667,9 @@ static int mpfs_dts_fixup_inplace(void* dts_addr)
      * parked harts on the kernel's request (SMP).  cpu@0 (E51) is already
      * disabled in the Yocto DTB; cpu@1 stays enabled so Linux boots on it. */
     for (i = 0; i < sizeof(cpu_off) / sizeof(cpu_off[0]); i++) {
-        off = fdt_find_node_offset(fdt, -1, cpu_off[i]);
+        off = fdt_find_node_offset(&ctx, -1, cpu_off[i]);
         if (off >= 0) {
-            ret = fdt_fixup_str(fdt, off, cpu_off[i], "status",
+            ret = fdt_fixup_str(&ctx, off, cpu_off[i], "status",
                                 "disabled");
             if (ret != 0) {
                 wolfBoot_printf("FDT: Failed to disable %s (%d)\n",
@@ -713,9 +715,9 @@ static int mpfs_dts_fixup_inplace(void* dts_addr)
         mac_addr[3], mac_addr[4], mac_addr[5]);
 
     /* Set local-mac-address for ethernet@20110000 (mac0) */
-    off = fdt_find_node_offset(fdt, -1, "ethernet@20110000");
+    off = fdt_find_node_offset(&ctx, -1, "ethernet@20110000");
     if (off >= 0) {
-        ret = fdt_setprop(fdt, off, "local-mac-address", mac_addr, 6);
+        ret = fdt_setprop(&ctx, off, "local-mac-address", mac_addr, 6);
         if (ret != 0) {
             wolfBoot_printf("FDT: Failed to set mac0 address (%d)\n", ret);
         }
@@ -732,9 +734,9 @@ static int mpfs_dts_fixup_inplace(void* dts_addr)
         mac_addr[0], mac_addr[1], mac_addr[2],
         mac_addr[3], mac_addr[4], mac_addr[5]);
 
-    off = fdt_find_node_offset(fdt, -1, "ethernet@20112000");
+    off = fdt_find_node_offset(&ctx, -1, "ethernet@20112000");
     if (off >= 0) {
-        ret = fdt_setprop(fdt, off, "local-mac-address", mac_addr, 6);
+        ret = fdt_setprop(&ctx, off, "local-mac-address", mac_addr, 6);
         if (ret != 0) {
             wolfBoot_printf("FDT: Failed to set mac1 address (%d)\n", ret);
         }
@@ -824,9 +826,10 @@ int wolfBoot_fit_memcpy(void *dst, const void *src, uint32_t len)
  * (WOLFBOOT_LOAD_DTS_ADDRESS) but CPU writes to DDR do not land here, so copy
  * it (non-cached read) into an L2 scratch buffer, run the FDT fixups there
  * (CPU L2 writes work), then PDMA the result back to DDR. */
-int hal_dts_fixup(void* dts_addr)
+int hal_dts_fixup(void* dts_addr, uint32_t capacity)
 {
     static uint8_t l2_dtb[64 * 1024] __attribute__((aligned(8)));
+    fdt_ctx ctx;
     const uint8_t *ddr_nc;
     uint32_t sz;
     int ret;
@@ -835,29 +838,29 @@ int hal_dts_fixup(void* dts_addr)
         return -1;
     }
     ddr_nc = (const uint8_t *)((uintptr_t)dts_addr | 0x40000000UL);
-    if (fdt_check_header((void *)ddr_nc) != 0) {
+    /* The source is bounded by whichever is smaller: the caller's DDR
+     * window, or what the L2 scratch buffer can hold once the fixup
+     * headroom is set aside.  fdt_open() enforces it, so the memcpy below
+     * cannot overrun l2_dtb however corrupt the header is. */
+    sz = (uint32_t)(sizeof(l2_dtb) - WOLFBOOT_FDT_FIXUP_HEADROOM);
+    if (capacity < sz) {
+        sz = capacity;
+    }
+    if (fdt_open(&ctx, (void *)ddr_nc, sz) != 0) {
         wolfBoot_printf("FDT: invalid header at %p\n", dts_addr);
         return -1;
     }
-    sz = (uint32_t)fdt_totalsize((void *)ddr_nc);
-    /* Overflow-safe bound: compare without adding.  A near-UINT32_MAX
-     * totalsize (fdt_check_header does not bound it) would make
-     * sz + WOLFBOOT_FDT_FIXUP_HEADROOM wrap to a small value that passes the
-     * check, after which memcpy(l2_dtb, ., sz) overruns the 64 KB buffer.
-     * sizeof(l2_dtb) (64 KB) is always greater than the headroom. */
-    if (sz > sizeof(l2_dtb) - WOLFBOOT_FDT_FIXUP_HEADROOM) {
-        wolfBoot_printf("FDT: dtb too large for L2 fixup (%u > %u)\n",
-            (unsigned)sz,
-            (unsigned)(sizeof(l2_dtb) - WOLFBOOT_FDT_FIXUP_HEADROOM));
-        return -1;
-    }
+    sz = fdt_size(&ctx);
     /* DDR (non-cached) -> L2 */
     memcpy(l2_dtb, ddr_nc, sz);
-    /* fixup in the CPU-writable L2 buffer */
-    ret = mpfs_dts_fixup_inplace(l2_dtb);
+    /* fixup in the CPU-writable L2 buffer, which may use the whole of it */
+    ret = mpfs_dts_fixup_inplace(l2_dtb, (uint32_t)sizeof(l2_dtb));
     /* L2 -> DDR via PDMA (expanded totalsize) */
-    if (wolfBoot_fit_memcpy(dts_addr, l2_dtb,
-            (uint32_t)fdt_totalsize(l2_dtb)) != 0) {
+    if (fdt_open(&ctx, l2_dtb, (uint32_t)sizeof(l2_dtb)) != 0) {
+        wolfBoot_printf("FDT: fixed-up dtb rejected\n");
+        return -1;
+    }
+    if (wolfBoot_fit_memcpy(dts_addr, l2_dtb, fdt_size(&ctx)) != 0) {
         wolfBoot_printf("FDT: dtb copy-back to DDR failed\n");
         return -1;
     }
@@ -868,12 +871,12 @@ int hal_dts_fixup(void* dts_addr)
  * run the fixups directly in place (the original behavior, kept so
  * FDT-enabled non-DDR builds do not silently fall back to the weak
  * no-op hal_dts_fixup). */
-int hal_dts_fixup(void* dts_addr)
+int hal_dts_fixup(void* dts_addr, uint32_t capacity)
 {
     if (dts_addr == NULL) {
         return -1;
     }
-    return mpfs_dts_fixup_inplace(dts_addr);
+    return mpfs_dts_fixup_inplace(dts_addr, capacity);
 }
 #endif /* WOLFBOOT_RISCV_MMODE && MPFS_DDR_INIT */
 

hal/nxp_t10xx.c

@@ -3394,29 +3394,33 @@ void* hal_get_dts_address(void)
     return (void*)WOLFBOOT_DTS_BOOT_ADDRESS;
 }
 
-int hal_dts_fixup(void* dts_addr)
+int hal_dts_fixup(void* dts_addr, uint32_t capacity)
 {
 #ifndef BUILD_LOADER_STAGE1
-    struct fdt_header *fdt = (struct fdt_header *)dts_addr;
+    fdt_ctx ctx;
+    fdt_ctx* fdt = &ctx;
     int off, i;
     uint32_t cell;
     uint32_t *reg;
     const char* prev_compat;
 
-    /* verify the FTD is valid */
-    off = fdt_check_header(dts_addr);
+    /* Validate the blob against the window it actually occupies. */
+    off = fdt_open(&ctx, dts_addr, capacity);
     if (off != 0) {
         wolfBoot_printf("FDT: Invalid header! %d\n", off);
         return off;
     }
 
     /* display FTD information */
-    wolfBoot_printf("FDT: Version %d, Size %d\n",
-        fdt_version(fdt), fdt_totalsize(fdt));
+    wolfBoot_printf("FDT: Size %d\n", (int)fdt_size(fdt));
 
-    /* expand total size */
-    fdt->totalsize += 2048; /* expand by 2KB */
-    wolfBoot_printf("FDT: Expanded (2KB) to %d bytes\n", fdt->totalsize);
+    /* Reserve headroom for the fixups below. */
+    off = fdt_grow(fdt, 2048U);
+    if (off != 0) {
+        wolfBoot_printf("FDT: No headroom for fixups (%d)\n", off);
+        return off;
+    }
+    wolfBoot_printf("FDT: Expanded (2KB) to %d bytes\n", (int)fdt_size(fdt));
 
     /* fixup the memory region - single bank */
     off = fdt_find_devtype(fdt, -1, "memory");
@@ -3513,6 +3517,11 @@ int hal_dts_fixup(void* dts_addr)
 
         wolfBoot_printf("FDT: Set %s@%d (%d), %s=%d,%d\n",
             "qman-portal", i, off, "fsl,liodn", liodns[0], liodns[1]);
+        /* Property cells are big-endian on the wire. Identity on the
+         * PowerPC targets that run this, but without it the host tests
+         * write host-order bytes that hardware never produces. */
+        liodns[0] = cpu_to_fdt32(liodns[0]);
+        liodns[1] = cpu_to_fdt32(liodns[1]);
         fdt_setprop(fdt, off, "fsl,liodn", liodns, sizeof(liodns));
 
         /* Add fman@0 node and fsl,liodon = FMAN_DMA_LIODN + index */
@@ -3521,6 +3530,7 @@ int hal_dts_fixup(void* dts_addr)
             liodns[0] = FMAN_DMA_LIODN + i + 1;
             wolfBoot_printf("FDT: Set %s@%d/%s (%d), %s=%d\n",
                 "qman-portal", i, "fman@0", childoff, "fsl,liodn", liodns[0]);
+            liodns[0] = cpu_to_fdt32(liodns[0]);
             fdt_setprop(fdt, childoff, "fsl,liodn", liodns, sizeof(liodns[0]));
             off = childoff;
         }
@@ -3585,8 +3595,14 @@ int hal_dts_fixup(void* dts_addr)
                         0x10, 0x00,       0x00, 0x00,       0x00, 0x80000000
         };
         uint32_t bus_range[2], base;
-        bus_range[0] = 0;
-        bus_range[1] = i-1;
+        unsigned int c;
+
+        /* Cells are big-endian on the wire; a no-op on PowerPC. */
+        for (c = 0; c < sizeof(dma_ranges)/sizeof(dma_ranges[0]); c++) {
+            dma_ranges[c] = cpu_to_fdt32(dma_ranges[c]);
+        }
+        bus_range[0] = cpu_to_fdt32(0);
+        bus_range[1] = cpu_to_fdt32((uint32_t)(i-1));
 
         /* find offset for pci controlller base register */
         off = fdt_node_offset_by_compatible(fdt, -1, "fsl,qoriq-pcie");
@@ -3632,6 +3648,7 @@ int hal_dts_fixup(void* dts_addr)
 
 #endif /* !BUILD_LOADER_STAGE1 */
     (void)dts_addr;
+    (void)capacity;
     return 0;
 }
 #endif /* MMU */