From 8ec8443345f497232edd67e0de0c034e0df71fb6 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 19 Aug 2026 19:48:53 -0700 Subject: [PATCH] Fixes from skoll review findings --- .gitignore | 1 + config/examples/cm4_sdcard.config | 4 + hal/cm4.c | 6 + src/sdhci.c | 17 +- tools/unit-tests/Makefile | 7 +- tools/unit-tests/unit-cm4-rauc-slot.c | 339 ++++++++++++++++++++++++++ 6 files changed, 367 insertions(+), 7 deletions(-) create mode 100644 tools/unit-tests/unit-cm4-rauc-slot.c diff --git a/.gitignore b/.gitignore index 909dede2..9e5ece75 100644 --- a/.gitignore +++ b/.gitignore @@ -238,6 +238,7 @@ tools/unit-tests/unit-va416x0-fram tools/unit-tests/unit-wolfhsm_flash_hal tools/unit-tests/unit-ubootenv tools/unit-tests/unit-cm4-sdhci +tools/unit-tests/unit-cm4-rauc-slot tools/unit-tests/__pycache__/* diff --git a/config/examples/cm4_sdcard.config b/config/examples/cm4_sdcard.config index 46382767..a74f4f1c 100644 --- a/config/examples/cm4_sdcard.config +++ b/config/examples/cm4_sdcard.config @@ -32,6 +32,10 @@ CFLAGS_EXTRA+=-DSDHCI_FORCE_CARD_DETECT # DMA cache-maintenance hooks, while the disk configs run with the D-cache on # (CM4_USE_MMU), so SDMA could hash/verify stale DDR. Matches cm4_emmc.config. CFLAGS_EXTRA+=-DSDHCI_SDMA_DISABLED +# Opt into the Arasan EMMC2 between-block BRR clear for multi-block PIO reads. +# The CM4 microSD path runs the same EMMC2 block as the eMMC path (which gets +# this via DISK_EMMC); other DISK_SDCARD targets do not need it (src/sdhci.c). +CFLAGS_EXTRA+=-DSDHCI_PIO_BRR_CLEAR # Opt into the bounded multi-block write settle-wait: the BCM2711 EMMC2 does not # raise TC until CMD12, so cap the pre-CMD12 wait (src/sdhci.c) instead of # spinning forever; completion is then re-confirmed via CMD13. diff --git a/hal/cm4.c b/hal/cm4.c index f586dccd..24143019 100644 --- a/hal/cm4.c +++ b/hal/cm4.c @@ -673,6 +673,8 @@ void cm4_fault_handler(unsigned long esr, unsigned long elr, unsigned long far) #endif /* DEBUG && DEBUG_UART */ #if defined(HAVE_FIPS) +#include /* wc_ForceZero */ + /* Upper bound on the busy-wait for the RNG200 FIFO to fill. This is a coarse, * A72-clock-dependent spin count (not a wall-clock timeout); it only guards * against a wedged RNG so the seed read cannot hang forever. Tune if needed. */ @@ -737,6 +739,10 @@ int wolfBoot_fips_seed(unsigned char* output, unsigned int sz) n = (sz - pos) < 4 ? (sz - pos) : 4; for (i = 0; i < n; i++) output[pos++] = (unsigned char)(word >> (i * 8)); + /* Raw TRNG output seeds the FIPS Hash-DRBG: do not leave the last word + * on the stack for a later frame to observe. wc_ForceZero (not memset) + * so the compiler cannot elide the dead store. */ + wc_ForceZero(&word, sizeof(word)); } return 0; } diff --git a/src/sdhci.c b/src/sdhci.c index 009e6191..1092a61a 100644 --- a/src/sdhci.c +++ b/src/sdhci.c @@ -1551,12 +1551,17 @@ static int sdhci_transfer(int dir, uint32_t cmd_index, uint32_t block_addr, } sz -= xfer_sz; - #if defined(DISK_EMMC) || defined(DISK_SDCARD) - /* Between-block workaround for the Arasan EMMC2 block (used for - * both eMMC and the CM4 microSD path, which runs the same - * controller in multi-block PIO). For multi-block READ: clear - * BRR by writing 1 to it (W1C), then the outer loop waits for - * BRR to be set again when the next block's data is available. + #if defined(DISK_EMMC) || defined(SDHCI_PIO_BRR_CLEAR) + /* Between-block workaround for the Arasan EMMC2 block. This is + * a controller quirk, NOT a generic SD behavior, so it is scoped + * rather than keyed off DISK_SDCARD: that macro is also set by + * the polarfire/zynqmp/versal/zynq7000/tegra234 SD targets, + * whose multi-block PIO reads are already validated without it. + * Platforms needing the quirk opt in with SDHCI_PIO_BRR_CLEAR + * (the CM4 microSD path, which drives the same EMMC2 block). + * For multi-block READ: clear BRR by writing 1 to it (W1C), + * then the outer loop waits for BRR to be set again when the + * next block's data is available. * For WRITE: BWR auto-clears when buffer full, don't touch. */ if (sz > 0 && dir == SDHCI_DIR_READ) { SDHCI_REG_SET(SDHCI_SRS12, SDHCI_SRS12_BRR); diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index af36e8fe..066ee793 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -66,7 +66,8 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 unit-image-nopart unit-image-sha384 unit-image-sha3-384 unit-image-dts \ unit-image-dts-sha384 unit-image-dts-sha3-384 unit-store-sbrk \ unit-tpm-blob unit-policy-create unit-policy-sign unit-rot-auth unit-sdhci-response-bits \ - unit-sdhci-disk-unaligned unit-sdhci-dma-error unit-cm4-sdhci unit-sign-encrypted-output \ + unit-sdhci-disk-unaligned unit-sdhci-dma-error unit-cm4-sdhci unit-cm4-rauc-slot \ + unit-sign-encrypted-output \ unit-sign-hybrid-keyload \ unit-sign-header-failure \ unit-keygen-xmss-params @@ -554,6 +555,10 @@ unit-cm4-sdhci: ../../include/target.h unit-cm4-sdhci.c gcc -o $@ $^ $(CFLAGS) -I../.. -ffunction-sections -fdata-sections $(LDFLAGS) \ -Wl,--gc-sections +unit-cm4-rauc-slot: ../../include/target.h unit-cm4-rauc-slot.c ../../src/ubootenv.c + gcc -o $@ unit-cm4-rauc-slot.c ../../src/ubootenv.c $(CFLAGS) -I../.. \ + -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections + unit-ubootenv: unit-ubootenv.c ../../src/ubootenv.c gcc -o $@ $< $(CFLAGS) -I../.. $(LDFLAGS) diff --git a/tools/unit-tests/unit-cm4-rauc-slot.c b/tools/unit-tests/unit-cm4-rauc-slot.c new file mode 100644 index 00000000..65878404 --- /dev/null +++ b/tools/unit-tests/unit-cm4-rauc-slot.c @@ -0,0 +1,339 @@ +/* unit-cm4-rauc-slot.c + * + * Unit tests for the CM4 RAUC A/B slot arbiter hal_boot_slot_select() in + * hal/cm4.c. The state machine itself (uboot_env_select_slot) is covered by + * unit-ubootenv.c; this exercises the HAL wrapper that binds it to disk I/O, + * and in particular the writeback gating that anti-rollback failover depends + * on: a decrement must be persisted for a good env, and must NOT be persisted + * when the env could not be read or failed CRC (writing the wiped buffer back + * would destroy the last-good on-disk RAUC state). + * + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#include +#include +#include +#include +#include + +/* Mock of the EMMC2 register window (hal/cm4.c casts this base); the RAUC path + * never touches it, but the SDHCI glue in the same file needs a valid target. */ +static uint8_t cm4_mock_emmc[0x100]; +#define BCM2711_EMMC2_BASE ((uintptr_t)cm4_mock_emmc) + +/* Compile the CM4 HAL for the host with the RAUC A/B arbiter enabled. + * DISK_SDCARD satisfies the file's outer disk-block guard, and MMU is the + * enclosing guard for the boot-slot/DTB block that holds the arbiter. + * (CM4_USE_MMU stays off on the host: it also requires __aarch64__.) */ +#define ARCH_AARCH64 +#define DISK_SDCARD 1 +#define MMU 1 +#define CM4_RAUC_AB 1 + +#include "../../hal/cm4.c" + +/* ---- mock disk backing the env partition -------------------------------- */ + +static uint8_t mock_part[UBOOT_ENV_SIZE]; +/* Bytes the HAL is allowed to read; < UBOOT_ENV_SIZE simulates a short read. */ +static int mock_read_len; +/* Bytes a write is allowed to store; < UBOOT_ENV_SIZE simulates a failure. */ +static int mock_write_len; +static int mock_write_calls; + +int disk_part_read(int drv, int part, uint64_t off, uint64_t sz, uint8_t *buf) +{ + (void)drv; (void)part; (void)off; + if (mock_read_len < 0) + return -1; + if ((uint64_t)mock_read_len < sz) + sz = (uint64_t)mock_read_len; + memcpy(buf, mock_part + off, (size_t)sz); + return (int)sz; +} + +int disk_part_write(int drv, int part, uint64_t off, uint64_t sz, + const uint8_t *buf) +{ + (void)drv; (void)part; (void)off; + mock_write_calls++; + if (mock_write_len < 0) + return -1; + if ((uint64_t)mock_write_len < sz) + sz = (uint64_t)mock_write_len; + memcpy(mock_part + off, buf, (size_t)sz); + return (int)sz; +} + +/* ---- helpers ------------------------------------------------------------- */ + +/* Build a CRC-valid env in mock_part with BOOT_ORDER plus the given counters. + * A negative count omits that slot's BOOT__LEFT key entirely. */ +static void build_env(const char *order, int a_left, int b_left) +{ + char val[UBOOT_ENV_VAL_MAX]; + + memset(mock_part, 0, sizeof(mock_part)); + uboot_env_reseal(mock_part, UBOOT_ENV_SIZE); + ck_assert_int_eq(uboot_env_set(mock_part, UBOOT_ENV_SIZE, "BOOT_ORDER", + order), 0); + if (a_left >= 0) { + snprintf(val, sizeof(val), "%d", a_left); + ck_assert_int_eq(uboot_env_set(mock_part, UBOOT_ENV_SIZE, + "BOOT_A_LEFT", val), 0); + } + if (b_left >= 0) { + snprintf(val, sizeof(val), "%d", b_left); + ck_assert_int_eq(uboot_env_set(mock_part, UBOOT_ENV_SIZE, + "BOOT_B_LEFT", val), 0); + } + uboot_env_reseal(mock_part, UBOOT_ENV_SIZE); + ck_assert_int_eq(uboot_env_verify(mock_part, UBOOT_ENV_SIZE), 1); +} + +static void reset_mocks(void) +{ + mock_read_len = UBOOT_ENV_SIZE; + mock_write_len = UBOOT_ENV_SIZE; + mock_write_calls = 0; + cm4_rauc_selected = 0; + cm4_rauc_root = NULL; + memset(cm4_rauc_slot_name, 0, sizeof(cm4_rauc_slot_name)); + memset(cm4_uboot_env, 0, sizeof(cm4_uboot_env)); +} + +/* Read a counter straight out of the mock partition (post-writeback state). */ +static long part_counter(const char *key) +{ + char val[UBOOT_ENV_VAL_MAX]; + if (uboot_env_get(mock_part, UBOOT_ENV_SIZE, key, val, sizeof(val)) < 0) + return -1; + return atol(val); +} + +/* ---- tests --------------------------------------------------------------- */ + +/* (a) Good env, valid CRC: slot A selected, counter decremented AND persisted. */ +START_TEST(test_good_env_decrements_and_persists) +{ + reset_mocks(); + build_env("A B", 3, 3); + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_selected, 1); + ck_assert_str_eq(cm4_rauc_slot_name, "A"); + ck_assert_str_eq(cm4_rauc_root, CM4_ROOT_A); + ck_assert_int_eq(mock_write_calls, 1); + /* The decrement must be on disk, not just in RAM. */ + ck_assert_int_eq((int)part_counter("BOOT_A_LEFT"), 2); + ck_assert_int_eq((int)part_counter("BOOT_B_LEFT"), 3); +} +END_TEST + +/* Slot B is chosen when BOOT_ORDER asks for it, and maps to CM4_ROOT_B. */ +START_TEST(test_order_selects_slot_b) +{ + reset_mocks(); + build_env("B A", 3, 3); + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_selected, 1); + ck_assert_str_eq(cm4_rauc_slot_name, "B"); + ck_assert_str_eq(cm4_rauc_root, CM4_ROOT_B); + ck_assert_int_eq((int)part_counter("BOOT_B_LEFT"), 2); +} +END_TEST + +/* An exhausted slot is skipped in favour of one that still has tries. */ +START_TEST(test_exhausted_slot_is_skipped) +{ + reset_mocks(); + build_env("A B", 0, 3); + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_selected, 1); + ck_assert_str_eq(cm4_rauc_slot_name, "B"); + ck_assert_str_eq(cm4_rauc_root, CM4_ROOT_B); +} +END_TEST + +/* (e) All slots exhausted: counters are re-armed and a slot is selected on the + * second pass, so the device still boots rather than bricking. */ +START_TEST(test_all_exhausted_rearms_and_selects) +{ + reset_mocks(); + build_env("A B", 0, 0); + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_selected, 1); + ck_assert_str_eq(cm4_rauc_slot_name, "A"); + /* Re-armed then decremented for this boot. */ + ck_assert_int_eq((int)part_counter("BOOT_A_LEFT"), + UBOOT_ENV_DEFAULT_TRIES - 1); +} +END_TEST + +/* (b) Short read: the env is not trusted, so no slot is selected and - most + * importantly - nothing is written back over the on-disk RAUC state. */ +START_TEST(test_short_read_no_writeback) +{ + reset_mocks(); + build_env("A B", 3, 3); + mock_read_len = UBOOT_ENV_SIZE - 1; /* partition smaller than the env */ + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(mock_write_calls, 0); + /* On-disk counters untouched. */ + ck_assert_int_eq((int)part_counter("BOOT_A_LEFT"), 3); +} +END_TEST + +/* A hard read error behaves the same way: no writeback. */ +START_TEST(test_read_error_no_writeback) +{ + reset_mocks(); + build_env("A B", 3, 3); + mock_read_len = -1; + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(mock_write_calls, 0); + ck_assert_int_eq((int)part_counter("BOOT_A_LEFT"), 3); +} +END_TEST + +/* (c) Full read but corrupt CRC: the state machine reinitializes in RAM. The + * HAL must NOT persist that wiped buffer - doing so would erase the last-good + * RAUC state and permanently defeat failover. */ +START_TEST(test_bad_crc_does_not_persist_reset) +{ + reset_mocks(); + build_env("B A", 3, 3); + mock_part[8] ^= 0xFF; /* corrupt a payload byte, leaving the CRC stale */ + ck_assert_int_eq(uboot_env_verify(mock_part, UBOOT_ENV_SIZE), 0); + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(mock_write_calls, 0); +} +END_TEST + +/* (d) Writeback fails: the counter is not durably decremented, so the HAL must + * fall back to the static cmdline rather than boot a slot whose try count did + * not stick (which would retry a hung slot forever). */ +START_TEST(test_write_failure_falls_back_to_static_cmdline) +{ + reset_mocks(); + build_env("A B", 3, 3); + mock_write_len = -1; + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(mock_write_calls, 1); + ck_assert_int_eq(cm4_rauc_selected, 0); /* -> static LINUX_BOOTARGS */ +} +END_TEST + +/* A short (partial) write is a failure too - not a success. */ +START_TEST(test_partial_write_falls_back) +{ + reset_mocks(); + build_env("A B", 3, 3); + mock_write_len = UBOOT_ENV_SIZE / 2; + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_selected, 0); +} +END_TEST + +/* An unrecognised bootname must fail loudly (static cmdline), not be coerced + * into slot A/B by a prefix match. */ +START_TEST(test_unknown_slot_name_falls_back) +{ + reset_mocks(); + build_env("system0 system1", -1, -1); + + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_selected, 0); +} +END_TEST + +/* A selected slot leaves cm4_rauc_build_bootargs() able to render the line; + * with no slot it must refuse so the caller uses the static bootargs. */ +START_TEST(test_bootargs_follow_selection) +{ + char buf[256]; + + reset_mocks(); + build_env("A B", 3, 3); + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_build_bootargs(buf, sizeof(buf)), 0); + ck_assert(strstr(buf, "root=" CM4_ROOT_A) != NULL); + ck_assert(strstr(buf, "rauc.slot=A") != NULL); + + /* No selection -> refuse. */ + cm4_rauc_selected = 0; + ck_assert_int_eq(cm4_rauc_build_bootargs(buf, sizeof(buf)), -1); +} +END_TEST + +/* Truncation must be reported, never silently emit a half command line. */ +START_TEST(test_bootargs_truncation_refused) +{ + char small[8]; + + reset_mocks(); + build_env("A B", 3, 3); + ck_assert_int_eq(hal_boot_slot_select(), 0); + ck_assert_int_eq(cm4_rauc_build_bootargs(small, sizeof(small)), -1); +} +END_TEST + +Suite *cm4_rauc_suite(void) +{ + Suite *s; + TCase *tc; + + s = suite_create("cm4-rauc-slot"); + tc = tcase_create("hal_boot_slot_select"); + tcase_add_test(tc, test_good_env_decrements_and_persists); + tcase_add_test(tc, test_order_selects_slot_b); + tcase_add_test(tc, test_exhausted_slot_is_skipped); + tcase_add_test(tc, test_all_exhausted_rearms_and_selects); + tcase_add_test(tc, test_short_read_no_writeback); + tcase_add_test(tc, test_read_error_no_writeback); + tcase_add_test(tc, test_bad_crc_does_not_persist_reset); + tcase_add_test(tc, test_write_failure_falls_back_to_static_cmdline); + tcase_add_test(tc, test_partial_write_falls_back); + tcase_add_test(tc, test_unknown_slot_name_falls_back); + tcase_add_test(tc, test_bootargs_follow_selection); + tcase_add_test(tc, test_bootargs_truncation_refused); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + int fails; + SRunner *sr; + + sr = srunner_create(cm4_rauc_suite()); + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return (fails == 0) ? 0 : 1; +}