diff --git a/docs/Targets.md b/docs/Targets.md index 8cba4644..5c65e895 100644 --- a/docs/Targets.md +++ b/docs/Targets.md @@ -3885,6 +3885,21 @@ The CM4 target uses `SIGN=ECC384 HASH=SHA384` (FIPS-approved) and can perform it Key config points: `GZIP=1` (the FIT kernel subimage is `Image.gz`), `ELF=1`, `DISK_EMMC=1`, and `WOLFBOOT_LOAD_ADDRESS=0x18000000` - the FIT is staged above the decompressed kernel so gunzip does not overwrite its own compressed input mid-stream. `CFLAGS_EXTRA+=-DCM4_FIRMWARE_DTB` captures and reuses the firmware DTB (which already carries the RAM size and mini-UART clock), `CFLAGS_EXTRA+=-DCM4_UART_PL011` puts the Linux console on the PL011 (`ttyAMA0`, via `dtoverlay=disable-bt`), and `CFLAGS_EXTRA+=-DLINUX_BOOTARGS_ROOT=...` sets `root=`. The FIT is built from `hal/cm4.its` with `mkimage` and signed with the wolfBoot key; `tools/scripts/cm4/prepare_emmc_linux.sh` stages it on the eMMC. This path was hardware-validated booting a Yocto (Scarthgap, kernel 6.6) rootfs. +### Device tree trust boundary + +wolfBoot's signature covers the kernel FIT it loads from the raw eMMC partition. It does **not** cover the device tree used on the Linux paths. Both shipped Linux configurations (`cm4_emmc_linux.config` and `cm4_emmc_rauc.config`) enable `CM4_FIRMWARE_DTB`, so this applies to anyone following the recipes above - it is a compile-time switch, not a default-off feature. + +On this path wolfBoot reuses the device tree that the VideoCore firmware left in memory, relocates it, and overwrites only `/chosen/bootargs`. Everything else in that DTB - `/memory`, `/reserved-memory`, per-device `reg` windows, and any `initrd` pointers - reaches the kernel exactly as the firmware supplied it, from the unsigned FAT boot partition. Anyone who can write that partition can influence how Linux sees the machine, without invalidating any wolfBoot signature. + +wolfBoot does provide a mechanism to bind a raw DTB to a signed image (`HDR_DEVICE_TREE_DIGEST`, produced by `sign --dts`), but it does not apply here, for two independent reasons: + +1. The digest is verified only on the RAM-boot path (`src/update_ram.c`). The CM4 boots through `src/update_disk.c`, which performs no device-tree digest check. +2. Even with that plumbing in place, the hash is not predictable when the image is signed. The VideoCore firmware patches the DTB at runtime: the shipped `bcm2711-rpi-cm4.dtb` declares `/memory@0 reg = <0 0 0>` and receives the real RAM size, the mini-UART clock and the board serial number before handoff. The blob wolfBoot receives is therefore never byte-identical to any blob that could have been signed. + +That runtime patching is also *why* the firmware DTB is used rather than one carried inside the signed FIT: a FIT-carried device tree boots with no RAM size. + +The only thing that actually closes this boundary on a CM4 is the **Raspberry Pi EEPROM secure boot**, which authenticates the boot partition itself; with it enabled, the firmware and the DTB it hands over are covered by the platform's own root of trust. Deployments that need the device tree to be within the verified boundary should enable it. Note that the bring-up board used for this port has secure boot disabled (its OTP customer key hash reads all zeros), so the validation described above was performed with this boundary open. + ### Optional: RAUC A/B redundant boot `config/examples/cm4_emmc_rauc.config` makes wolfBoot replace U-Boot as the RAUC slot arbiter. wolfBoot reads a raw U-Boot-environment partition (`mkenvimage`/`fw_setenv` compatible), runs the RAUC `BOOT_ORDER` / `BOOT__LEFT` try-counter state machine, decrements the selected slot's counter and writes it back (so a hung slot fails over to the other on the next boot), then boots the shared signed kernel FIT with `root=` pointing at the active slot's rootfs and `rauc.slot=` on the command line. diff --git a/hal/cm4.c b/hal/cm4.c index 24143019..0bc85a14 100644 --- a/hal/cm4.c +++ b/hal/cm4.c @@ -307,7 +307,16 @@ int hal_boot_slot_select(void) r = disk_part_write(BOOT_DISK, CM4_UBOOT_ENV_PART, 0, UBOOT_ENV_SIZE, cm4_uboot_env); if (r != (int)UBOOT_ENV_SIZE) { - wolfBoot_printf("cm4: uboot-env write failed (%d); static cmdline\n", + /* Loud on purpose: this is the failure mode the whole A/B feature + * exists to prevent. The counter did not reach storage, so nothing + * decrements and the SAME slot is chosen again on every boot - + * redundancy is silently gone until the env partition is writable + * again (worn flash, read-only media, wrong partition index). This + * printf is the only evidence, so it names the consequence rather + * than just the errno. */ + wolfBoot_printf("cm4: WARNING uboot-env write failed (%d): RAUC try " + "counter NOT persisted; A/B failover is DISABLED until the env " + "partition is writable. Falling back to the static cmdline\n", r); return 0; } @@ -371,8 +380,26 @@ void* hal_get_boot_dts(void) * the RPi EEPROM secure-boot is enabled), so it is NOT covered by wolfBoot's * signature. Only /chosen/bootargs is overwritten below; /memory, * /reserved-memory, per-device reg windows and initrd remain firmware / - * attacker controlled. This path is opt-in via CM4_FIRMWARE_DTB. See the - * trust-boundary note in docs/Targets.md. */ + * attacker controlled. + * This is NOT effectively optional: CM4_FIRMWARE_DTB is enabled by default + * in both shipped Linux configurations (cm4_emmc_linux.config and + * cm4_emmc_rauc.config), so anyone following the documented Linux recipe is + * on this path. It is a compile-time switch, not a default-off feature. + * Why the signed-DTB mechanism is not used here: master carries + * HDR_DEVICE_TREE_DIGEST plus "sign --dts" to bind a raw DTB to the signed + * image, but it does not apply to this path for two independent reasons. + * (1) The digest is only checked by the RAM-boot path (src/update_ram.c); + * the CM4 boots via src/update_disk.c, which has no such check. (2) Even + * with the plumbing, the hash is not predictable at signing time: the + * VideoCore firmware patches the DTB at runtime (the shipped + * bcm2711-rpi-cm4.dtb ships /memory@0 reg = <0 0 0> and gets the real RAM + * size, mini-UART clock and board serial injected before handoff), so the + * blob wolfBoot receives is never the blob that was signed. That runtime + * patching is exactly why the firmware DTB is used instead of one carried + * inside the FIT - a FIT-carried DTB boots with no RAM size. + * The only thing that actually closes this boundary on a CM4 is the + * Raspberry Pi EEPROM secure-boot, which authenticates the boot partition + * itself. See "Device tree trust boundary" in docs/Targets.md. */ wolfBoot_printf("cm4: using UNVERIFIED firmware DTB (CM4_FIRMWARE_DTB)\n"); memcpy((void*)WOLFBOOT_LOAD_DTS_ADDRESS, fdt, sz); fdt = (void*)WOLFBOOT_LOAD_DTS_ADDRESS; diff --git a/include/ubootenv.h b/include/ubootenv.h index b2aeebe6..674a952d 100644 --- a/include/ubootenv.h +++ b/include/ubootenv.h @@ -86,7 +86,15 @@ int uboot_env_set(uint8_t *env, size_t env_len, const char *key, * - pick the first name in BOOT_ORDER whose BOOT__LEFT > 0, decrement * that counter, and report it in *out (out->selected = 1); * - if none have tries left, re-arm every counter to the default, set - * out->rearmed = 1 and out->selected = 0 (the caller should reboot); + * out->rearmed = 1 and out->selected = 0. The caller decides what to do + * with a re-armed environment: a full bootloader may reboot, but wolfBoot + * has nowhere else to go, so hal/cm4.c instead calls this a second time and + * boots the now-re-armed first slot. The consequence is deliberate and + * worth stating: a board whose slots are BOTH broken keeps booting the + * first one, cycling 3 -> 2 -> 1 -> 0 -> re-arm -> 3 forever, rather than + * escalating to any other action. There is no "give up" state - refusing + * to boot would leave an unrecoverable device, so the retry loop is the + * safer failure mode; * - reseal the CRC32. * The env buffer is modified in place; the caller writes it back to storage. * Returns 0 on success, -1 on a malformed BOOT_ORDER / buffer error. */ diff --git a/src/sdhci.c b/src/sdhci.c index 1092a61a..24242550 100644 --- a/src/sdhci.c +++ b/src/sdhci.c @@ -1592,7 +1592,14 @@ static int sdhci_transfer(int dir, uint32_t cmd_index, uint32_t block_addr, * stop issued below (observed on the CM4 EMMC2: SRS12 stuck at 0x51, TC * never set), so its wait is BOUNDED: it still captures an EINT without * deadlocking, and the CMD12 + wait-busy sequence below completes the - * transfer. */ + * transfer. + * SCOPE: SDHCI_WRITE_SETTLE_SPINS bounds the MULTI-BLOCK wait only. The + * single-block wait is intentionally left unbounded, because a CMD24 + * that never raises TC means the controller is wedged with no stop + * command to recover it - there is no CMD12 follow-up to complete the + * transfer, so capping the spin would report a write as done with no + * evidence it landed. The macro name says "settle spins", not "all + * writes are bounded"; a wedged single-block write still hangs. */ if (dir == SDHCI_DIR_WRITE) { #ifdef SDHCI_WRITE_SETTLE_SPINS uint32_t spins = SDHCI_WRITE_SETTLE_SPINS; @@ -1655,19 +1662,21 @@ static int sdhci_transfer(int dir, uint32_t cmd_index, uint32_t block_addr, status = -1; } - /* Host-side confirmation for a bounded-out multi-block write: the settle - * wait above capped instead of observing TC, so require positive evidence - * the card actually finished the program cycle (CMD13 -> READY_FOR_DATA via - * sdhci_wait_busy) before reporting success. A silently timed-out write must - * NOT be treated as landed - the RAUC try-counter writeback relies on a + /* Report a bounded-out multi-block write. The settle wait above capped + * instead of observing TC, so success here rests on positive evidence that + * the card finished the program cycle. That evidence is the CMD13 -> + * READY_FOR_DATA poll ALREADY performed on the non-error path above + * (sdhci_wait_busy(0) after CMD12); this block deliberately does not repeat + * it - an earlier revision issued a second, redundant CMD13 here. It only + * surfaces the bounded-out transfer and makes sure a card that never came + * ready propagates as a failure: a silently timed-out write must NOT be + * treated as landed, because the RAUC try-counter writeback relies on a * failed write propagating (see hal/cm4.c). Safe for the CM4 EMMC2 where TC * legitimately never precedes CMD12: after CMD12 the card reports ready. */ if (write_settle_timeout) { wolfBoot_printf("sdhci_transfer: multi-block write settle timeout " - "(SRS12=0x%08X); confirming completion via CMD13\n", reg); - if (status == 0) { - status = sdhci_wait_busy(0); - } + "(SRS12=0x%08X); completion confirmed via the post-CMD12 CMD13\n", + reg); if (status != 0) { wolfBoot_printf("sdhci_transfer: write completion NOT confirmed\n"); } diff --git a/src/ubootenv.c b/src/ubootenv.c index 3f50e127..4e61bf55 100644 --- a/src/ubootenv.c +++ b/src/ubootenv.c @@ -63,10 +63,23 @@ static void wr_le32(uint8_t *p, uint32_t v) p[3] = (uint8_t)(v >> 24); } -/* Parse a small non-negative decimal; -1 on a non-numeric value. */ +/* Longest counter accepted by env_atol(). Chosen so the value always fits a + * 16-bit-int platform's 32-bit long (999999999 < LONG_MAX), which also keeps + * env_ltoa()'s output well inside the caller's buffer. RAUC try counters are + * single digits, so this is far above any real value. */ +#define ENV_DIGITS_MAX 9 + +/* Parse a non-negative decimal counter. Returns -1 unless the value is + * ENTIRELY digits (leading/trailing spaces allowed): a trailing-garbage value + * such as "1x" is a malformed counter, not 1, and a value longer than + * ENV_DIGITS_MAX digits is refused rather than silently wrapping. A -1 makes + * the caller treat the slot as having no tries left and skip it; if that leaves + * no candidate, pass 2 re-arms every counter, so a corrupt value degrades to a + * re-arm rather than to an arbitrary try count. */ static long env_atol(const char *s) { long v; + int n; if (s == NULL) return -1; @@ -75,32 +88,48 @@ static long env_atol(const char *s) if (*s < '0' || *s > '9') return -1; v = 0; + n = 0; while (*s >= '0' && *s <= '9') { + if (++n > ENV_DIGITS_MAX) + return -1; /* too long to represent or to round-trip */ v = (v * 10) + (long)(*s - '0'); s++; } + while (*s == ' ') + s++; + if (*s != '\0') + return -1; /* trailing non-digit: malformed, not a partial parse */ return v; } -/* Format a non-negative decimal into out (caller supplies >= 16 bytes). */ -static void env_ltoa(long v, char *out) +/* Format a non-negative decimal into out, never writing more than out_max + * bytes including the terminator. The bound is explicit rather than an implied + * contract with the caller's buffer size: previously this assumed >= 16 bytes + * and relied on env_atol()'s input having been clamped elsewhere, so a longer + * value would have written one byte past the buffer. Digits that do not fit are + * dropped from the most significant end, which cannot happen for any value + * env_atol() accepts (ENV_DIGITS_MAX). */ +static void env_ltoa(long v, char *out, size_t out_max) { - char tmp[16]; + char tmp[ENV_DIGITS_MAX + 1]; int i; int j; + if (out_max == 0) + return; i = 0; j = 0; if (v <= 0) { - out[0] = '0'; - out[1] = '\0'; + if (out_max > 1) + out[j++] = '0'; + out[j] = '\0'; return; } while (v > 0 && i < (int)sizeof(tmp)) { tmp[i++] = (char)('0' + (v % 10)); v /= 10; } - while (i > 0) + while (i > 0 && (size_t)j + 1 < out_max) out[j++] = tmp[--i]; out[j] = '\0'; } @@ -310,7 +339,7 @@ int uboot_env_select_slot(uint8_t *env, size_t env_len, struct uboot_slot *out) /* Genuinely invalid: reinitialize defaults IN MEMORY so this boot can * proceed, but flag it (out->reinitialized) so the caller does NOT * persist the reset over on-disk state it could not validate. */ - env_ltoa(UBOOT_ENV_DEFAULT_TRIES, leftval); + env_ltoa(UBOOT_ENV_DEFAULT_TRIES, leftval, sizeof(leftval)); memset(env, 0, env_len); (void)uboot_env_set(env, env_len, "BOOT_ORDER", "A B"); (void)uboot_env_set(env, env_len, "BOOT_A_LEFT", leftval); @@ -342,7 +371,7 @@ int uboot_env_select_slot(uint8_t *env, size_t env_len, struct uboot_slot *out) if (uboot_env_get(env, env_len, leftkey, leftval, sizeof(leftval)) >= 0) left = env_atol(leftval); if (left > 0) { - env_ltoa(left - 1, leftval); + env_ltoa(left - 1, leftval, sizeof(leftval)); /* If the decrement cannot be stored (env full), do NOT select this * slot: booting it with an un-decremented counter would retry the * same hung slot forever. Fail so the caller uses the static @@ -357,7 +386,7 @@ int uboot_env_select_slot(uint8_t *env, size_t env_len, struct uboot_slot *out) } /* Pass 2: none left -> re-arm every counter, ask the caller to reboot. */ - env_ltoa(UBOOT_ENV_DEFAULT_TRIES, leftval); + env_ltoa(UBOOT_ENV_DEFAULT_TRIES, leftval, sizeof(leftval)); o = order; while (env_next_name(&o, name, sizeof(name)) > 0) { if (env_leftkey(name, leftkey, sizeof(leftkey)) != 0) diff --git a/tools/unit-tests/unit-ubootenv.c b/tools/unit-tests/unit-ubootenv.c index 7d037852..0953a4e2 100644 --- a/tools/unit-tests/unit-ubootenv.c +++ b/tools/unit-tests/unit-ubootenv.c @@ -37,9 +37,9 @@ static void init_env(const char *order, int a, int b) char v[16]; memset(env, 0, ENVLEN); uboot_env_set(env, ENVLEN, "BOOT_ORDER", order); - env_ltoa(a, v); + env_ltoa(a, v, sizeof(v)); uboot_env_set(env, ENVLEN, "BOOT_A_LEFT", v); - env_ltoa(b, v); + env_ltoa(b, v, sizeof(v)); uboot_env_set(env, ENVLEN, "BOOT_B_LEFT", v); uboot_env_reseal(env, ENVLEN); } @@ -299,6 +299,80 @@ START_TEST(test_leftkey_long_name_bounds) } END_TEST +/* env_atol() promises -1 on a non-numeric value; make sure that is what it + * does. "1x" used to parse as 1 (only the first char was checked), which then + * selected the slot and rewrote the counter, laundering a malformed value into + * a valid one. */ +START_TEST(test_atol_rejects_trailing_garbage) +{ + ck_assert_int_eq((int)env_atol("1x"), -1); + ck_assert_int_eq((int)env_atol("3 junk"), -1); + ck_assert_int_eq((int)env_atol("12-"), -1); + ck_assert_int_eq((int)env_atol(""), -1); + ck_assert_int_eq((int)env_atol("x"), -1); + ck_assert_int_eq((int)env_atol(NULL), -1); + /* Still accepts well-formed values, with surrounding spaces. */ + ck_assert_int_eq((int)env_atol("0"), 0); + ck_assert_int_eq((int)env_atol("3"), 3); + ck_assert_int_eq((int)env_atol(" 7 "), 7); +} +END_TEST + +/* Over-long counters are refused instead of wrapping or being truncated into a + * different number on writeback. */ +START_TEST(test_atol_rejects_overlong) +{ + ck_assert_int_eq((int)env_atol("999999999"), 999999999); /* 9 digits: ok */ + ck_assert_int_eq((int)env_atol("1000000000"), -1); /* 10 digits */ + ck_assert_int_eq((int)env_atol("9999999999999999"), -1); /* 16 digits */ +} +END_TEST + +/* env_ltoa() must honour the caller's buffer size. The old version wrote + * tmp[16] plus a terminator into a 16-byte buffer for a 16-digit value. */ +START_TEST(test_ltoa_respects_bound) +{ + char buf[8]; + char guard[16]; + + memset(guard, 0xAA, sizeof(guard)); + env_ltoa(0, guard, sizeof(guard)); + ck_assert_str_eq(guard, "0"); + + env_ltoa(12345, buf, sizeof(buf)); + ck_assert_str_eq(buf, "12345"); + + /* Tight buffer: must still terminate inside it, never past it. */ + memset(buf, 0xAA, sizeof(buf)); + env_ltoa(123456789, buf, 4); + ck_assert_uint_lt(strlen(buf), 4); + ck_assert_uint_eq((unsigned char)buf[7], 0xAA); /* tail untouched */ + + /* Zero-length buffer must not be written at all. */ + memset(buf, 0xAA, sizeof(buf)); + env_ltoa(5, buf, 0); + ck_assert_uint_eq((unsigned char)buf[0], 0xAA); +} +END_TEST + +/* A malformed counter degrades to "no tries left" for that slot, so selection + * falls through to the next one rather than trusting the garbage. */ +START_TEST(test_select_malformed_counter_skips_slot) +{ + struct uboot_slot slot; + + memset(env, 0, ENVLEN); + uboot_env_set(env, ENVLEN, "BOOT_ORDER", "A B"); + uboot_env_set(env, ENVLEN, "BOOT_A_LEFT", "2x"); + uboot_env_set(env, ENVLEN, "BOOT_B_LEFT", "3"); + uboot_env_reseal(env, ENVLEN); + + ck_assert_int_eq(uboot_env_select_slot(env, ENVLEN, &slot), 0); + ck_assert_int_eq(slot.selected, 1); + ck_assert_str_eq(slot.name, "B"); +} +END_TEST + Suite *ubootenv_suite(void) { Suite *s = suite_create("ubootenv"); @@ -321,6 +395,10 @@ Suite *ubootenv_suite(void) tcase_add_test(tc, test_select_valid_crc_missing_bootorder); tcase_add_test(tc, test_select_null_and_short); tcase_add_test(tc, test_leftkey_long_name_bounds); + tcase_add_test(tc, test_atol_rejects_trailing_garbage); + tcase_add_test(tc, test_atol_rejects_overlong); + tcase_add_test(tc, test_ltoa_respects_bound); + tcase_add_test(tc, test_select_malformed_counter_skips_slot); suite_add_tcase(s, tc); return s; @@ -329,7 +407,9 @@ Suite *ubootenv_suite(void) int main(void) { int fails; - Suite *s = ubootenv_suite(); + + +Suite *s = ubootenv_suite(); SRunner *sr = srunner_create(s); srunner_run_all(sr, CK_NORMAL);