From 9b1a48554ab43cb093809536881c3d2b6884fff1 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 21 Aug 2026 09:08:26 +0200 Subject: [PATCH] share the DTS size bounds in fdt.h and state the window assumption The WOLFBOOT_DTS_MAX_SIZE/WOLFBOOT_DTS_MIN_SIZE pair was defined separately in update_disk.c (F-7066) and update_ram.c (pre-existing), so the two copies could drift. Move it to include/fdt.h, the FDT dialect header both translation units already pull in via image.h; the hal override (nxp_ppc.h, included before fdt.h in boot_ppc.c) keeps its precedence. Also replace the 'bounded by the staging region' comment, which claimed more than the code guarantees: the copy is clamped to WOLFBOOT_DTS_MAX_SIZE, so the staging window at WOLFBOOT_LOAD_DTS_ADDRESS must be at least that large (or the bound overridden for the target), and the header comment now says so. Skoll review finding 3, 2026-08-21 wolfboot review. --- include/fdt.h | 12 ++++++++++++ src/update_disk.c | 17 ++++------------- src/update_ram.c | 17 ++++------------- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/include/fdt.h b/include/fdt.h index 6d982737..2f60d396 100644 --- a/include/fdt.h +++ b/include/fdt.h @@ -73,6 +73,18 @@ struct fdt_property { #define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1)) #define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE)) +/* Bounds for the attacker-influenced fdt_totalsize before relocating or + * forwarding a DTB. MIN is the FDT v17 header size (also enforced by the + * signer): fdt_check_header validates magic/version but not totalsize, so a + * crafted header with a tiny totalsize must be rejected rather than + * loaded/forwarded as a partial tree. The MAX default assumes a staging + * window at WOLFBOOT_LOAD_DTS_ADDRESS of at least 1 MiB; targets with a + * smaller window must override WOLFBOOT_DTS_MAX_SIZE (see hal/nxp_ppc.h). */ +#ifndef WOLFBOOT_DTS_MAX_SIZE +#define WOLFBOOT_DTS_MAX_SIZE (1024U * 1024U) +#endif +#define WOLFBOOT_DTS_MIN_SIZE (40U) + #define FDT_FIRST_SUPPORTED_VERSION 0x10 #define FDT_LAST_SUPPORTED_VERSION 0x11 diff --git a/src/update_disk.c b/src/update_disk.c index e371925a..4ee5bad6 100644 --- a/src/update_disk.c +++ b/src/update_disk.c @@ -249,17 +249,6 @@ static void disk_decrypted_header_clear(uint8_t *hdr) extern int wolfBoot_get_dts_size(void *dts_addr); -#if defined(MMU) || defined(WOLFBOOT_FDT) -/* Bounds for the attacker-influenced fdt_totalsize before relocating a DTB. - * MIN is the FDT v17 header size (also enforced by the signer): fdt_check_header - * validates magic/version but not totalsize, so a crafted header with a tiny - * totalsize must be rejected rather than loaded/forwarded as a partial tree. */ -#ifndef WOLFBOOT_DTS_MAX_SIZE -#define WOLFBOOT_DTS_MAX_SIZE (1024U * 1024U) -#endif -#define WOLFBOOT_DTS_MIN_SIZE (40U) -#endif - #if defined(WOLFBOOT_NO_LOAD_ADDRESS) || !defined(WOLFBOOT_LOAD_ADDRESS) /* from the linker, where wolfBoot ends */ extern uint8_t _end_wb[]; @@ -650,8 +639,10 @@ void RAMFUNCTION wolfBoot_start(void) parsed >= (int)WOLFBOOT_DTS_MIN_SIZE && (uint32_t)parsed <= WOLFBOOT_DTS_MAX_SIZE) { /* Relocate to the load DTS address. The copy length is - * the parsed DTB size (bounded by the staging region), - * not the FIT-declared property length. */ + * the parsed DTB size, clamped to WOLFBOOT_DTS_MAX_SIZE, + * not the FIT-declared property length. The staging window + * at WOLFBOOT_LOAD_DTS_ADDRESS must be at least that large + * (or the bound must be overridden for the target). */ dts_addr = (uint8_t*)WOLFBOOT_LOAD_DTS_ADDRESS; dts_size = (uint32_t)parsed; wolfBoot_printf("Loading DTS: %p -> %p (%d bytes)\n", diff --git a/src/update_ram.c b/src/update_ram.c index b632b16d..8b98cbd4 100644 --- a/src/update_ram.c +++ b/src/update_ram.c @@ -51,17 +51,6 @@ extern void hal_flash_dualbank_swap(void); extern uint32_t kernel_load_addr; extern uint32_t dts_load_addr; -#if defined(MMU) || defined(WOLFBOOT_FDT) -/* Bounds for the attacker-influenced fdt_totalsize before relocating a DTB. - * MIN is the FDT v17 header size (also enforced by the signer): fdt_check_header - * validates magic/version but not totalsize, so a crafted header with a tiny - * totalsize must be rejected rather than loaded/forwarded as a partial tree. */ -#ifndef WOLFBOOT_DTS_MAX_SIZE -#define WOLFBOOT_DTS_MAX_SIZE (1024U * 1024U) -#endif -#define WOLFBOOT_DTS_MIN_SIZE (40U) -#endif - #if defined(__WOLFBOOT) && defined(WOLFBOOT_LOAD_ADDRESS) extern uint8_t _end[]; /* linker symbol: end of wolfBoot BSS */ #endif @@ -662,8 +651,10 @@ backup_on_failure: parsed >= (int)WOLFBOOT_DTS_MIN_SIZE && (uint32_t)parsed <= WOLFBOOT_DTS_MAX_SIZE) { /* Relocate to the load DTS address. The copy length is - * the parsed DTB size (bounded by the staging region), - * not the FIT-declared property length. */ + * the parsed DTB size, clamped to WOLFBOOT_DTS_MAX_SIZE, + * not the FIT-declared property length. The staging window + * at WOLFBOOT_LOAD_DTS_ADDRESS must be at least that large + * (or the bound must be overridden for the target). */ dts_addr = (uint8_t*)WOLFBOOT_LOAD_DTS_ADDRESS; dts_size = (uint32_t)parsed; wolfBoot_printf("Loading DTS: %p -> %p (%d bytes)\n",