From 673bf507d8a3dde2531a00df54e8720181140da7 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Wed, 28 Oct 2026 14:30:30 +0000 Subject: [PATCH 1/4] stm32u5: return errors from flash write and erase hal_flash_write() never read the FLASH_SR error flags and returned 0 whatever happened. With TrustZone it also returned 0 for an address out of range, and without TrustZone it did not check the address at all. hal_flash_erase() failed only for a zero length or an address below the flash, and returned 0 for pages past the top of the flash. Check OPERR, PROGERR, WRPERR, PGAERR, SIZERR and PGSERR after each quad-word program and each page erase. On an error, clear PG or PER and the flags, and return -1. The status is read once before EOP is cleared, because clearing EOP writes the error flags back as well, which clears them. Both functions now return -1 for a negative length or a span that is not all inside the flash, before touching it. The STM32U5 write test now extracts hal_flash_erase() as well. Its flash is two 8 KB pages, and the mock hal_flash_wait_complete() erases the page selected in FLASH_CR on STRT. It can also fail one program or erase with chosen FLASH_SR flags, leaving the flash unchanged. The host register file cannot clear write-1-to-clear flags, so the mock sets FLASH_NS_SR to the result of each operation. New tests check that each error flag stops a write on the unit that set it and an erase on the page that set it, with PG or PER cleared, that other flags do not fail a write, and that ranges outside the flash are refused before anything changes. The error and range tests fail on the HAL without the checks. --- hal/stm32u5.c | 33 ++++- hal/stm32u5.h | 4 + tools/unit-tests/Makefile | 13 +- tools/unit-tests/unit-stm32u5-write.c | 203 +++++++++++++++++++++++++- 4 files changed, 237 insertions(+), 16 deletions(-) diff --git a/hal/stm32u5.c b/hal/stm32u5.c index 1c467a9530..1f39e02ebe 100644 --- a/hal/stm32u5.c +++ b/hal/stm32u5.c @@ -80,6 +80,7 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) uint32_t qword[4]; uint8_t *qword_bytes = (uint8_t *)qword; volatile uint32_t *sr, *cr; + uint32_t sr_val; hal_flash_clear_errors(0); #if (TZ_SECURE()) @@ -87,6 +88,10 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) #endif dst = (uint32_t*)address; + if ((len < 0) || (address < FLASHMEM_ADDRESS_SPACE) || + (address > FLASH_TOP) || ((uint32_t)len > FLASH_TOP - address + 1)) + return -1; /* Address out of range */ + #if (TZ_SECURE()) if ((((FLASH_OPTR & FLASH_OPTR_DBANK) == 0) && (address <= FLASH_TOP)) || (address < FLASH_BANK2_BASE)) { cr = &FLASH_CR; @@ -99,7 +104,7 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) sr = &FLASH_NS_SR; } else { - return 0; /* Address out of range */ + return -1; /* Address out of range */ } #else cr = &FLASH_NS_CR; @@ -140,9 +145,16 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) ISB(); } hal_flash_wait_complete(0); - if ((*sr & FLASH_SR_EOP) != 0) + /* Read the status once: clearing EOP writes back the error flags + * too, which clears them. */ + sr_val = *sr; + if ((sr_val & FLASH_SR_EOP) != 0) *sr |= FLASH_SR_EOP; *cr &= ~FLASH_CR_PG; + if ((sr_val & FLASH_SR_ERRORS) != 0) { + hal_flash_clear_errors(0); + return -1; + } i = i_aligned + 16; } @@ -218,16 +230,18 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) uint32_t end_address; uint32_t p; volatile uint32_t *cr = &FLASH_NS_CR; + volatile uint32_t *sr = &FLASH_NS_SR; hal_flash_clear_errors(0); - if (len == 0) + if (len <= 0) return -1; #if (TZ_SECURE()) address = flash_addr_unalias(address); #endif - if (address < ARCH_FLASH_OFFSET) - return -1; + if ((address < ARCH_FLASH_OFFSET) || (address > FLASH_TOP) || + ((uint32_t)len > FLASH_TOP - address + 1)) + return -1; /* Address out of range */ end_address = address + len; for (p = address; p < end_address; p += FLASH_PAGE_SIZE) { @@ -235,10 +249,12 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) uint32_t base; uint32_t bker = 0; cr = &FLASH_NS_CR; + sr = &FLASH_NS_SR; if ((((FLASH_OPTR & FLASH_OPTR_DBANK) == 0) && (p <= FLASH_TOP)) || (p < FLASH_BANK2_BASE)) { #if (TZ_SECURE()) cr = &FLASH_CR; + sr = &FLASH_SR; #endif base = FLASHMEM_ADDRESS_SPACE; } @@ -248,7 +264,7 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) } else { *cr &= ~FLASH_CR_PER ; - return 0; /* Address out of range */ + return -1; /* Address out of range */ } /* BKER refers to the physical bank, whatever the SWAP_BANK setting * (RM0456 7.5.8): invert it when the banks are swapped, so that the @@ -263,6 +279,11 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) DMB(); *cr |= FLASH_CR_STRT; hal_flash_wait_complete(0); + if ((*sr & FLASH_SR_ERRORS) != 0) { + *cr &= ~FLASH_CR_PER; + hal_flash_clear_errors(0); + return -1; + } } /* If the erase operation is completed, disable the associated bits */ *cr &= ~FLASH_CR_PER ; diff --git a/hal/stm32u5.h b/hal/stm32u5.h index 122efaac06..b7a256965b 100644 --- a/hal/stm32u5.h +++ b/hal/stm32u5.h @@ -207,6 +207,10 @@ #define FLASH_SR_OPTWERR (1 << 13) #define FLASH_SR_BSY (1 << 16) #define FLASH_SR_WDW (1 << 17) +/* Flags that report a failed program or erase operation */ +#define FLASH_SR_ERRORS (FLASH_SR_OPERR | FLASH_SR_PROGERR | \ + FLASH_SR_WRPERR | FLASH_SR_PGAERR | \ + FLASH_SR_SIZERR | FLASH_SR_PGSERR) #define FLASH_CR_PG (1 << 0) #define FLASH_CR_PER (1 << 1) diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 015740530c..1af5a4ea63 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -2210,15 +2210,18 @@ unit-stm32l5-write: unit-stm32l5-write.c stm32l5_write_extract.h gcc -o $@ unit-stm32l5-write.c $(CFLAGS) $(LDFLAGS) # unit-stm32u5-write is the 16-byte-unit twin of the STM32L5 test: -# the real hal_flash_write() and clear_errors from hal/stm32u5.c -# (hal_flash_wait_complete is mocked in the test for the -# program-window check), FLASH_NS_SR/CR on a host register file, -# destination flash at a 32-bit host address, canary after the source. +# the real hal_flash_write(), hal_flash_erase() and clear_errors from +# hal/stm32u5.c (hal_flash_wait_complete is mocked in the test for the +# program-window check, page erase and error flags), FLASH_NS_SR/CR on +# a host register file, destination flash at a 32-bit host address, +# canary after the source. stm32u5_write_extract.h: ../../hal/stm32u5.c # hal_flash_wait_complete is mocked in the test (program-window - # check), so only the real clear_errors + write are extracted. + # check), so only the real clear_errors, write and erase are + # extracted. sed -n '/^void RAMFUNCTION hal_flash_clear_errors/,/^}/p' $< > $@ sed -n '/^int RAMFUNCTION hal_flash_write/,/^}/p' $< >> $@ + sed -n '/^int RAMFUNCTION hal_flash_erase/,/^}/p' $< >> $@ unit-stm32u5-write: unit-stm32u5-write.c stm32u5_write_extract.h gcc -o $@ unit-stm32u5-write.c $(CFLAGS) $(LDFLAGS) diff --git a/tools/unit-tests/unit-stm32u5-write.c b/tools/unit-tests/unit-stm32u5-write.c index 39b01f5df4..d7b57147c8 100644 --- a/tools/unit-tests/unit-stm32u5-write.c +++ b/tools/unit-tests/unit-stm32u5-write.c @@ -27,6 +27,13 @@ * A unit that is erased and stays erased is not programmed. Readback * cannot tell that from programming 0xFF, so the mock also counts the * program operations (waits with FLASH_CR_PG set) for those tests. + * + * hal_flash_write() and hal_flash_erase() used to return 0 whatever + * happened: they never read the FLASH_SR error flags, and an address out + * of range returned 0 too. The mock wait can now fail one program or + * page erase with chosen error flags, and both functions must stop and + * return non-zero, as they must for a range outside the flash. + * * Copyright (C) 2026 wolfSSL Inc. * * This file is part of wolfBoot. @@ -55,13 +62,15 @@ * (non-secure path: FLASH_NS_CR / FLASH_NS_SR). */ #define RAMFUNCTION #define ISB() do { } while (0) +#define DMB() do { } while (0) #define TZ_SECURE() (0) /* Host FLASH register file (offsets as in hal/stm32u5.h). */ -static uint32_t g_flash_regs[0x40 / sizeof(uint32_t)]; +static uint32_t g_flash_regs[0x44 / sizeof(uint32_t)]; #define FLASH_NS_BASE ((uintptr_t)g_flash_regs) #define FLASH_NS_SR (*(volatile uint32_t *)(FLASH_NS_BASE + 0x20)) #define FLASH_NS_CR (*(volatile uint32_t *)(FLASH_NS_BASE + 0x28)) +#define FLASH_OPTR (*(volatile uint32_t *)(FLASH_NS_BASE + 0x40)) #define FLASH_SR_EOP (1 << 0) #define FLASH_SR_OPERR (1 << 1) #define FLASH_SR_PROGERR (1 << 3) @@ -72,14 +81,30 @@ static uint32_t g_flash_regs[0x40 / sizeof(uint32_t)]; #define FLASH_SR_OPTWERR (1 << 13) #define FLASH_SR_BSY (1 << 16) #define FLASH_SR_WDW (1 << 17) +#define FLASH_SR_ERRORS (FLASH_SR_OPERR | FLASH_SR_PROGERR | \ + FLASH_SR_WRPERR | FLASH_SR_PGAERR | \ + FLASH_SR_SIZERR | FLASH_SR_PGSERR) #define FLASH_CR_PG (1 << 0) +#define FLASH_CR_PER (1 << 1) +#define FLASH_CR_PNB_SHIFT 3 +#define FLASH_CR_PNB_MASK 0xFF +#define FLASH_CR_BKER (1 << 11) +#define FLASH_CR_STRT (1 << 16) +#define FLASH_OPTR_DBANK (1 << 21) +#define FLASH_OPTR_SWAP_BANK (1 << 20) /* Destination flash: pre-filled with stale data (rewrite scenario). * hal_flash_write() takes the address as uint32_t (32-bit MCU), so on * the 64-bit host the flash must live at an address that fits in 32 - * bits: map it at a fixed low location. */ -#define FLASH_MEM_SZ 256 + * bits: map it at a fixed low location. It is the whole flash of the + * HAL's range checks: two 8 KB pages, single bank. */ +#define FLASH_PAGE_SIZE 0x2000 +#define FLASH_MEM_SZ (2 * FLASH_PAGE_SIZE) #define FLASH_MEM_ADDR 0x11000000UL +#define FLASHMEM_ADDRESS_SPACE FLASH_MEM_ADDR +#define ARCH_FLASH_OFFSET FLASH_MEM_ADDR +#define FLASH_TOP (FLASH_MEM_ADDR + FLASH_MEM_SZ - 1) +#define FLASH_BANK2_BASE (FLASH_MEM_ADDR + FLASH_MEM_SZ) static uint8_t *g_flash_mem; /* Snapshot of the flash at the last program-window boundary; the @@ -88,6 +113,14 @@ static uint8_t g_flash_prev[FLASH_MEM_SZ]; /* Program operations: waits for completion with FLASH_CR_PG set. */ static int g_programs; +/* Page erases: waits for completion with FLASH_CR_PER and STRT set. */ +static int g_erases; + +/* Error injection: the program or erase numbered g_fail_op (from 1) does + * not change the flash and sets g_fail_flags in FLASH_NS_SR. */ +static int g_ops; +static int g_fail_op; +static uint32_t g_fail_flags; /* Source buffer followed by a canary: pre-fix, a short write reads * bytes past len and lands them in the destination flash. The canary @@ -104,12 +137,29 @@ static uint8_t g_data[DATA_SZ + CANARY_SZ]; * other way: the bytes changed since the previous window must fit * within one aligned 16-byte program unit. The pre-fix HAL issued its * four word stores relative to the caller address, so an unaligned - * start split them across two units and this assertion goes red. */ + * start split them across two units and this assertion goes red. + * + * It also models the page erase started by FLASH_CR_STRT, and the error + * injection. The host register file cannot clear the write-1-to-clear + * error flags, so FLASH_NS_SR is set to the result of this operation + * only: what the HAL reads back right after the wait. */ static void hal_flash_wait_complete(uint8_t bank) { int i; int first = -1; int last = -1; + int program = (FLASH_NS_CR & FLASH_CR_PG) != 0; + int erase = (FLASH_NS_CR & (FLASH_CR_PER | FLASH_CR_STRT)) == + (FLASH_CR_PER | FLASH_CR_STRT); + int fail = 0; + + (void)bank; + FLASH_NS_SR = 0; + if ((program || erase) && (++g_ops == g_fail_op)) { + FLASH_NS_SR = g_fail_flags; + /* Only an error flag stops the operation. */ + fail = (g_fail_flags & FLASH_SR_ERRORS) != 0; + } for (i = 0; i < FLASH_MEM_SZ; i++) { if (g_flash_mem[i] != g_flash_prev[i]) { @@ -120,8 +170,21 @@ static void hal_flash_wait_complete(uint8_t bank) } if (first >= 0) ck_assert_int_le(last, (first & ~0x0F) + 15); + if (program && fail) + memcpy(g_flash_mem, g_flash_prev, FLASH_MEM_SZ); + if (erase) { + uint32_t page = (FLASH_NS_CR >> FLASH_CR_PNB_SHIFT) & + FLASH_CR_PNB_MASK; + + ck_assert_uint_lt(page, FLASH_MEM_SZ / FLASH_PAGE_SIZE); + if (!fail) + memset(g_flash_mem + page * FLASH_PAGE_SIZE, 0xFF, + FLASH_PAGE_SIZE); + FLASH_NS_CR &= ~FLASH_CR_STRT; + g_erases++; + } memcpy(g_flash_prev, g_flash_mem, FLASH_MEM_SZ); - if ((FLASH_NS_CR & FLASH_CR_PG) != 0) + if (program) g_programs++; } @@ -134,6 +197,10 @@ static void setup(void) memset(g_flash_regs, 0, sizeof(g_flash_regs)); g_programs = 0; + g_erases = 0; + g_ops = 0; + g_fail_op = 0; + g_fail_flags = 0; for (i = 0; i < FLASH_MEM_SZ; i++) g_flash_mem[i] = 0x12; /* stale */ memcpy(g_flash_prev, g_flash_mem, FLASH_MEM_SZ); @@ -402,6 +469,126 @@ START_TEST(test_ff_in_partly_programmed_unit) } END_TEST +static const uint32_t g_error_flags[] = { + FLASH_SR_OPERR, FLASH_SR_PROGERR, FLASH_SR_WRPERR, + FLASH_SR_PGAERR, FLASH_SR_SIZERR, FLASH_SR_PGSERR +}; +#define ERROR_FLAGS_CNT (int)(sizeof(g_error_flags) / sizeof(g_error_flags[0])) + +/* Each error flag fails the write on the unit that sets it: the unit + * before is programmed, that unit and the ones after are not, and PG is + * cleared. */ +START_TEST(test_write_sr_error_stops) +{ + int f; + int i; + + for (f = 0; f < ERROR_FLAGS_CNT; f++) { + setup(); + g_fail_op = 2; + g_fail_flags = g_error_flags[f]; + ck_assert_int_ne(hal_flash_write((uint32_t)(uintptr_t)g_flash_mem, + g_data, 64), 0); + + ck_assert_int_eq(g_programs, 2); + ck_assert_uint_eq(FLASH_NS_CR & FLASH_CR_PG, 0); + ck_assert_int_eq(memcmp(g_flash_mem, g_data, 16), 0); + for (i = 16; i < FLASH_MEM_SZ; i++) + ck_assert_uint_eq(g_flash_mem[i], 0x12); + } +} +END_TEST + +/* Status flags that are not errors do not fail the write. */ +START_TEST(test_write_other_flags_ok) +{ + g_fail_op = 1; + g_fail_flags = FLASH_SR_EOP | FLASH_SR_OPTWERR; + ck_assert_int_eq(hal_flash_write((uint32_t)(uintptr_t)g_flash_mem, + g_data, 32), 0); + + ck_assert_int_eq(g_programs, 2); + ck_assert_int_eq(memcmp(g_flash_mem, g_data, 32), 0); +} +END_TEST + +/* A range that is not all inside the flash is refused before anything + * is programmed. A range that ends at the top of the flash is written. */ +START_TEST(test_write_out_of_range) +{ + uint32_t base = (uint32_t)(uintptr_t)g_flash_mem; + int i; + + ck_assert_int_ne(hal_flash_write(base - 16, g_data, 16), 0); + ck_assert_int_ne(hal_flash_write(base + FLASH_MEM_SZ - 16, g_data, 32), + 0); + ck_assert_int_ne(hal_flash_write(base + FLASH_MEM_SZ, g_data, 1), 0); + ck_assert_int_ne(hal_flash_write(base, g_data, -1), 0); + ck_assert_int_eq(g_programs, 0); + for (i = 0; i < FLASH_MEM_SZ; i++) + ck_assert_uint_eq(g_flash_mem[i], 0x12); + + ck_assert_int_eq(hal_flash_write(base + FLASH_MEM_SZ - 16, g_data, 16), + 0); + ck_assert_int_eq(memcmp(g_flash_mem + FLASH_MEM_SZ - 16, g_data, 16), 0); +} +END_TEST + +/* An erase over the whole flash erases each page and clears PER. */ +START_TEST(test_erase_pages) +{ + ck_assert_int_eq(hal_flash_erase((uint32_t)(uintptr_t)g_flash_mem, + FLASH_MEM_SZ), 0); + + ck_assert_int_eq(g_erases, 2); + ck_assert_int_eq(flash_is_erased(0, FLASH_MEM_SZ), 1); + ck_assert_uint_eq(FLASH_NS_CR & FLASH_CR_PER, 0); +} +END_TEST + +/* Each error flag fails the erase on the page that sets it, the next + * page is not erased, and PER is cleared. */ +START_TEST(test_erase_sr_error_stops) +{ + int f; + int i; + + for (f = 0; f < ERROR_FLAGS_CNT; f++) { + setup(); + g_fail_op = 1; + g_fail_flags = g_error_flags[f]; + ck_assert_int_ne(hal_flash_erase((uint32_t)(uintptr_t)g_flash_mem, + FLASH_MEM_SZ), 0); + + ck_assert_int_eq(g_erases, 1); + ck_assert_uint_eq(FLASH_NS_CR & FLASH_CR_PER, 0); + for (i = 0; i < FLASH_MEM_SZ; i++) + ck_assert_uint_eq(g_flash_mem[i], 0x12); + } +} +END_TEST + +/* An erase range that is not all inside the flash, or is empty, is + * refused before any page is erased. */ +START_TEST(test_erase_out_of_range) +{ + uint32_t base = (uint32_t)(uintptr_t)g_flash_mem; + int i; + + ck_assert_int_ne(hal_flash_erase(base - FLASH_PAGE_SIZE, + FLASH_PAGE_SIZE), 0); + ck_assert_int_ne(hal_flash_erase(base + FLASH_PAGE_SIZE, + 2 * FLASH_PAGE_SIZE), 0); + ck_assert_int_ne(hal_flash_erase(base + FLASH_MEM_SZ, FLASH_PAGE_SIZE), + 0); + ck_assert_int_ne(hal_flash_erase(base, 0), 0); + ck_assert_int_ne(hal_flash_erase(base, -1), 0); + ck_assert_int_eq(g_erases, 0); + for (i = 0; i < FLASH_MEM_SZ; i++) + ck_assert_uint_eq(g_flash_mem[i], 0x12); +} +END_TEST + Suite *stm32u5_write_suite(void) { Suite *s = suite_create("stm32u5-write"); @@ -421,6 +608,12 @@ Suite *stm32u5_write_suite(void) tcase_add_test(tc, test_skip_unaligned_data_programmed); tcase_add_test(tc, test_ff_over_nonerased_programmed); tcase_add_test(tc, test_ff_in_partly_programmed_unit); + tcase_add_test(tc, test_write_sr_error_stops); + tcase_add_test(tc, test_write_other_flags_ok); + tcase_add_test(tc, test_write_out_of_range); + tcase_add_test(tc, test_erase_pages); + tcase_add_test(tc, test_erase_sr_error_stops); + tcase_add_test(tc, test_erase_out_of_range); suite_add_tcase(s, tc); return s; From 3d64b8ae018a7dcf70556a229a8bfaef076f5713 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Wed, 28 Oct 2026 14:30:30 +0000 Subject: [PATCH 2/4] pkcs11_store: report failed flash commits to wolfPKCS11 The keyvault ignored the return values of hal_flash_erase() and hal_flash_write(), and wolfPKCS11_Store_Close() returns void, so a record that did not reach flash looked committed. wolfPKCS11 then had no reason to write it again. Count erase and program failures in cache_commit_entry(), restore_backup() and check_vault(). A store window records the count at wolfPKCS11_Store_Open(), and the new wolfPKCS11_Store_CloseCommit() returns BUFFER_E when it changed during the window. wolfPKCS11_Store_Remove() does the same. The sector cache is shared, so every window open during a failure reports it; a window that lost nothing only makes wolfPKCS11 write all records again. The commit sequence itself is unchanged. user_settings.h defines WOLFPKCS11_CUSTOM_STORE_CLOSE_COMMIT, so a wolfPKCS11 that supports it closes written records with wolfPKCS11_Store_CloseCommit() and marks the token for a full store when it fails. The pinned wolfPKCS11 ignores the define and keeps calling wolfPKCS11_Store_Close(). docs/HAL.md now says that hal_flash_erase() returns non-zero on failure, and why the keyvault needs both functions to check the flash error flags. --- docs/HAL.md | 7 +++- include/user_settings.h | 2 + src/pkcs11_store.c | 87 +++++++++++++++++++++++++++++++++-------- 3 files changed, 78 insertions(+), 18 deletions(-) diff --git a/docs/HAL.md b/docs/HAL.md index 054ff61220..ea6851dc34 100644 --- a/docs/HAL.md +++ b/docs/HAL.md @@ -64,7 +64,12 @@ Erase operations must be performed via the specific IAP interface of the target `address` marks the start of the area that the bootloader wants to erase, and `len` specifies the size of the area to be erased. `address` is guaranteed to be aligned to `WOLFBOOT_SECTOR_SIZE`, and `len` is guaranteed to be a multiple of `WOLFBOOT_SECTOR_SIZE`. This function must take into account -the geometry of the flash sectors, and erase all the sectors in between. +the geometry of the flash sectors, and erase all the sectors in between. `hal_flash_erase` should +return 0 upon success, or a negative value in case of failure. + +The secure PKCS#11 keyvault (`WOLFCRYPT_TZ_PKCS11=1`) reports a failed erase or write to wolfPKCS11, +which then writes all of its records again. Both functions should therefore check the flash +controller's error flags after each operation, and fail for an address outside the flash. `void hal_prepare_boot(void)` diff --git a/include/user_settings.h b/include/user_settings.h index 01c57b1e4c..fecb96eb94 100644 --- a/include/user_settings.h +++ b/include/user_settings.h @@ -604,6 +604,8 @@ extern int tolower(int c); # define HAVE_PWDBASED # define HAVE_PBKDF2 # define WOLFPKCS11_CUSTOM_STORE + /* src/pkcs11_store.c reports failed flash commits */ +# define WOLFPKCS11_CUSTOM_STORE_CLOSE_COMMIT # define WOLFBOOT_SECURE_PKCS11 # ifndef WOLFPKCS11_USER_SETTINGS # define WOLFPKCS11_USER_SETTINGS diff --git a/src/pkcs11_store.c b/src/pkcs11_store.c index a1ee90fc25..7a02c8aaf4 100644 --- a/src/pkcs11_store.c +++ b/src/pkcs11_store.c @@ -34,6 +34,7 @@ #include #include #include /* wc_ForceZero */ +#include /* BUFFER_E */ #ifndef KEYVAULT_OBJ_SIZE #define KEYVAULT_OBJ_SIZE 0x1000 /* 4KB per object */ @@ -54,6 +55,8 @@ #define SESSION_COUNT_E -8 #define LOGGED_IN_E -9 #define OBJ_COUNT_E -10 +/* Data did not commit to flash, as reported by wolfPKCS11's own stores */ +#define STORE_FLASH_E BUFFER_E #ifndef UNIT_TEST @@ -113,6 +116,7 @@ struct store_handle { void *buffer; struct obj_hdr *hdr; uint32_t in_buffer_offset; + uint32_t flash_failures; /* store_flash_failures at Store_Open */ }; #define STORE_FLAGS_OPEN (1 << 0) @@ -150,6 +154,15 @@ static uint8_t cache_sector_mem static struct cache_entry store_cache[WOLFBOOT_PKCS11_STORE_CACHE_SECTORS]; static uint32_t cache_lru_tick; +/* + * Count of flash erase and program failures. A store window (Store_Open to + * Store_Close) and a Store_Remove fail when the count changed while they ran. + * The cache is shared, so a window may commit, and fail on, another window's + * sectors: every window open at the time reports the failure. Reporting a + * window that lost nothing only makes wolfPKCS11 write all records again. + */ +static uint32_t store_flash_failures; + static uint8_t *cache_get_sector(uint32_t offset); static void cache_flush_all(void); static uint8_t *sector_ptr(uint32_t offset); @@ -260,20 +273,29 @@ static void store_flash_lock(void) hal_cache_invalidate(); } +/* Count a failed flash erase or program (ret != 0). */ +static void store_flash_result(int ret) +{ + if (ret != 0) { + store_flash_failures++; + } +} + static void cache_commit_entry(struct cache_entry *entry) { hal_flash_unlock(); /* Write backup sector first */ - hal_flash_erase((uintptr_t)BACKUP_SECTOR_ADDRESS, WOLFBOOT_SECTOR_SIZE); - hal_flash_write((uintptr_t)BACKUP_SECTOR_ADDRESS, entry->sector, - WOLFBOOT_SECTOR_SIZE); + store_flash_result(hal_flash_erase((uintptr_t)BACKUP_SECTOR_ADDRESS, + WOLFBOOT_SECTOR_SIZE)); + store_flash_result(hal_flash_write((uintptr_t)BACKUP_SECTOR_ADDRESS, + entry->sector, WOLFBOOT_SECTOR_SIZE)); /* Erase + write actual destination sector */ - hal_flash_erase((uintptr_t)vault_base + entry->offset, - WOLFBOOT_SECTOR_SIZE); - hal_flash_write((uintptr_t)vault_base + entry->offset, entry->sector, - WOLFBOOT_SECTOR_SIZE); + store_flash_result(hal_flash_erase((uintptr_t)vault_base + entry->offset, + WOLFBOOT_SECTOR_SIZE)); + store_flash_result(hal_flash_write((uintptr_t)vault_base + entry->offset, + entry->sector, WOLFBOOT_SECTOR_SIZE)); store_flash_lock(); #ifdef PKCS11_STORE_STATS @@ -420,9 +442,10 @@ static void restore_backup(uint32_t offset) { hal_flash_unlock(); /* Erase + copy from backup */ - hal_flash_erase((uintptr_t)vault_base + offset, WOLFBOOT_SECTOR_SIZE); - hal_flash_write((uintptr_t)vault_base + offset, BACKUP_SECTOR_ADDRESS, - WOLFBOOT_SECTOR_SIZE); + store_flash_result(hal_flash_erase((uintptr_t)vault_base + offset, + WOLFBOOT_SECTOR_SIZE)); + store_flash_result(hal_flash_write((uintptr_t)vault_base + offset, + BACKUP_SECTOR_ADDRESS, WOLFBOOT_SECTOR_SIZE)); store_flash_lock(); #ifdef PKCS11_STORE_STATS stats_erases++; @@ -461,7 +484,9 @@ static void check_vault(void) memset(s0 + sizeof(uint32_t), 0x00, BITMAP_SIZE); cache_flush_all(); hal_flash_unlock(); - hal_flash_erase((uintptr_t)vault_base + WOLFBOOT_SECTOR_SIZE * 2, total_vault_size); + store_flash_result(hal_flash_erase( + (uintptr_t)vault_base + WOLFBOOT_SECTOR_SIZE * 2, + total_vault_size)); store_flash_lock(); #ifdef PKCS11_STORE_STATS stats_erases += total_vault_size / WOLFBOOT_SECTOR_SIZE; @@ -686,6 +711,8 @@ int wolfPKCS11_Store_Open(int type, CK_ULONG id1, CK_ULONG id2, int read, struct store_handle *handle; uint8_t *buf; int is_new = 0; + /* Flash failures from here on, Open included, belong to the window. */ + uint32_t flash_failures = store_flash_failures; /* Check if there is one handle available to open the slot */ handle = find_free_handle(); @@ -760,17 +787,39 @@ int wolfPKCS11_Store_Open(int type, CK_ULONG id1, CK_ULONG id2, int read, /* Set start of the buffer after the tok/obj id fields */ handle->in_buffer_offset = (2 * sizeof(uint32_t)); + handle->flash_failures = flash_failures; *store = handle; return 0; } -void wolfPKCS11_Store_Close(void* store) +/* + * Close a window and report whether its data committed. wolfPKCS11 calls this + * when built with WOLFPKCS11_CUSTOM_STORE_CLOSE_COMMIT (set in user_settings.h), + * and wolfPKCS11_Store_Close() otherwise. + * + * The sectors are committed even when commit is 0 (the record's write failed): + * the cache is shared, so one window's changes cannot be dropped on their own. + * wolfPKCS11 writes the record again on its next store. + */ +int wolfPKCS11_Store_CloseCommit(void* store, int commit) { struct store_handle *handle = store; + int ret = 0; + + (void)commit; /* Commit all pending sectors: the header sector last, so the header * is the atomic commit point of the window. */ cache_flush_all(); + if (store_flash_failures != handle->flash_failures) { + ret = STORE_FLASH_E; + } memset(handle, 0, sizeof(*handle)); + return ret; +} + +void wolfPKCS11_Store_Close(void* store) +{ + (void)wolfPKCS11_Store_CloseCommit(store, 1); } /* Live object size from the (possibly cached) header sector: the same @@ -891,16 +940,20 @@ int wolfPKCS11_Store_Write(void* store, unsigned char* buffer, int len) int wolfPKCS11_Store_Remove(int type, CK_ULONG id1, CK_ULONG id2) { uint8_t* buf; + uint32_t flash_failures = store_flash_failures; check_vault(); buf = find_object_buffer((int32_t)type, (uint32_t)id1, (uint32_t)id2); + if (buf != NULL) { + /* Erase the payload before invalidating the metadata, so key + * material does not remain recoverable in flash after removal. */ + erase_object_payload(buf); + delete_object((int32_t)type, (uint32_t)id1, (uint32_t)id2); + } + if (store_flash_failures != flash_failures) + return STORE_FLASH_E; if (buf == NULL) return NOT_AVAILABLE_E; - - /* Erase the payload before invalidating the metadata, so key - * material does not remain recoverable in flash after removal. */ - erase_object_payload(buf); - delete_object((int32_t)type, (uint32_t)id1, (uint32_t)id2); return 0; } From ce852614ce6a054f4c1e8be1a9faf559f95c0d95 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Wed, 28 Oct 2026 14:30:30 +0000 Subject: [PATCH 3/4] unit-tests: cover flash failures in the PKCS#11 store test The mock flash gains hal_flash_erase_fail, a one-shot erase failure like hal_flash_write_fail. New tests check that a failed write or erase while a window commits fails its close and not the next window's, that a failure is reported by every window open at the time but not by one opened later, that wolfPKCS11_Store_Remove() reports a failure and returns NOT_AVAILABLE_E only for a missing object, and that failures while Store_Open formats a blank vault or restores the header from the backup sector belong to that window. They fail on the store without failure tracking. --- tools/unit-tests/unit-mock-flash.c | 6 ++ tools/unit-tests/unit-pkcs11_store.c | 149 +++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/tools/unit-tests/unit-mock-flash.c b/tools/unit-tests/unit-mock-flash.c index 0af2e81eea..e265a27686 100644 --- a/tools/unit-tests/unit-mock-flash.c +++ b/tools/unit-tests/unit-mock-flash.c @@ -35,6 +35,8 @@ static int erased_nvm_bank0 = 0; static int erased_nvm_bank1 = 0; static int erased_vault = 0; static int hal_flash_write_fail = 0; +/* When set, the next hal_flash_erase() fails and the hook clears itself. */ +static int hal_flash_erase_fail = 0; const char *argv0; #ifdef NVM_FLASH_ECC @@ -254,6 +256,10 @@ int hal_flash_write(haladdr_t address, const uint8_t *data, int len) int hal_flash_erase(haladdr_t address, int len) { ck_assert_msg(!locked, "Attempting to erase a locked FLASH"); + if (hal_flash_erase_fail) { + hal_flash_erase_fail = 0; + return -1; + } if ((address >= WOLFBOOT_PARTITION_BOOT_ADDRESS) && (address < WOLFBOOT_PARTITION_BOOT_ADDRESS + WOLFBOOT_PARTITION_SIZE)) { #ifdef NVM_FLASH_ECC diff --git a/tools/unit-tests/unit-pkcs11_store.c b/tools/unit-tests/unit-pkcs11_store.c index e03da28511..5411e728dc 100644 --- a/tools/unit-tests/unit-pkcs11_store.c +++ b/tools/unit-tests/unit-pkcs11_store.c @@ -1039,6 +1039,148 @@ START_TEST (test_power_fail_during_rewrite_never_mixes_generations) { } END_TEST +/* Open a write window on the object and write the string into it. */ +static void *open_window_and_write(int type, CK_ULONG tok, CK_ULONG obj, + const char *payload) +{ + void *store = NULL; + int len = (int)strlen(payload) + 1; + int ret; + + ret = wolfPKCS11_Store_Open(type, tok, obj, 0, &store); + ck_assert_int_eq(ret, 0); + ret = wolfPKCS11_Store_Write(store, (unsigned char *)payload, len); + ck_assert_int_eq(ret, len); + return store; +} + +/* A flash erase or program failure while a window commits fails that + * window's close, so wolfPKCS11 knows the record may not have committed. The + * failure is not sticky: the next window that commits cleanly succeeds. */ +START_TEST(test_close_reports_flash_failure) +{ + const int type = DYNAMIC_TYPE_RSA; + const CK_ULONG id_tok = 80; + void *store; + char rd[64]; + int ret; + + ret = mmap_file(vault_path, vault_base, keyvault_size, NULL); + ck_assert_int_eq(ret, 0); + memset(vault_base, 0xEE, keyvault_size); + + store = open_window_and_write(type, id_tok, 1, "first"); + hal_flash_write_fail = 1; + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), STORE_FLASH_E); + ck_assert_int_eq(hal_flash_write_fail, 0); + + store = open_window_and_write(type, id_tok, 1, "second"); + hal_flash_erase_fail = 1; + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), STORE_FLASH_E); + ck_assert_int_eq(hal_flash_erase_fail, 0); + + store = open_window_and_write(type, id_tok, 1, "third"); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), 0); + + ret = wolfPKCS11_Store_Open(type, id_tok, 1, 1, &store); + ck_assert_int_eq(ret, 0); + ret = wolfPKCS11_Store_Read(store, (unsigned char *)rd, (int)sizeof(rd)); + ck_assert_int_eq(ret, (int)strlen("third") + 1); + ck_assert_str_eq(rd, "third"); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), 0); +} +END_TEST + +/* The sector cache is shared, so closing one window also commits the sectors + * of the others. A failure then fails every window open at the time, and no + * window opened after it. */ +START_TEST(test_flash_failure_reported_to_open_windows) +{ + const int type = DYNAMIC_TYPE_RSA; + const CK_ULONG id_tok = 90; + void *store_a; + void *store_b; + void *store_c; + int ret; + + ret = mmap_file(vault_path, vault_base, keyvault_size, NULL); + ck_assert_int_eq(ret, 0); + memset(vault_base, 0xEE, keyvault_size); + + store_a = open_window_and_write(type, id_tok, 1, "window a"); + store_b = open_window_and_write(type, id_tok, 2, "window b"); + hal_flash_write_fail = 1; + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store_b, 1), STORE_FLASH_E); + store_c = open_window_and_write(type, id_tok, 3, "window c"); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store_a, 1), STORE_FLASH_E); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store_c, 1), 0); +} +END_TEST + +/* Store_Remove reports a flash failure while it erases the payload and + * drops the node, and NOT_AVAILABLE_E only for a missing object. */ +START_TEST(test_remove_reports_flash_failure) +{ + const int type = DYNAMIC_TYPE_RSA; + const CK_ULONG id_tok = 100; + void *store; + int ret; + + ret = mmap_file(vault_path, vault_base, keyvault_size, NULL); + ck_assert_int_eq(ret, 0); + memset(vault_base, 0xEE, keyvault_size); + + store = open_window_and_write(type, id_tok, 1, "remove me"); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), 0); + store = open_window_and_write(type, id_tok, 2, "remove me too"); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), 0); + + hal_flash_write_fail = 1; + ck_assert_int_eq(wolfPKCS11_Store_Remove(type, id_tok, 1), STORE_FLASH_E); + ck_assert_int_eq(hal_flash_write_fail, 0); + hal_flash_erase_fail = 1; + ck_assert_int_eq(wolfPKCS11_Store_Remove(type, id_tok, 2), STORE_FLASH_E); + ck_assert_int_eq(hal_flash_erase_fail, 0); + ck_assert_int_eq(wolfPKCS11_Store_Remove(type, id_tok, 3), + NOT_AVAILABLE_E); +} +END_TEST + +/* Failures while Store_Open checks the vault, formatting a blank one or + * restoring the header from the backup sector, belong to the window opened. */ +START_TEST(test_vault_check_failure_reported) +{ + const int type = DYNAMIC_TYPE_RSA; + const CK_ULONG id_tok = 110; + void *store; + int ret; + + ret = mmap_file(vault_path, vault_base, keyvault_size, NULL); + ck_assert_int_eq(ret, 0); + memset(vault_base, 0xEE, keyvault_size); + + /* Blank vault: the format fails to erase the backup sector. */ + hal_flash_erase_fail = 1; + store = open_window_and_write(type, id_tok, 1, "formatted"); + ck_assert_int_eq(hal_flash_erase_fail, 0); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), STORE_FLASH_E); + + /* A close commits the header sector last, so the backup sector holds the + * header. With the header's magic gone, the next Open restores it. */ + store = open_window_and_write(type, id_tok, 1, "formatted"); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), 0); + *(uint32_t *)vault_base = 0; +#ifdef MOCK_STALE_CACHE + *(uint32_t *)vault_flash_at((uintptr_t)vault_base) = 0; +#endif + hal_flash_erase_fail = 1; + ret = wolfPKCS11_Store_Open(type, id_tok, 1, 1, &store); + ck_assert_int_eq(ret, 0); + ck_assert_int_eq(hal_flash_erase_fail, 0); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(store, 1), STORE_FLASH_E); +} +END_TEST + Suite *wolfboot_suite(void) { @@ -1057,6 +1199,7 @@ Suite *wolfboot_suite(void) TCase* tcase_interleaved = tcase_create("interleaved_windows"); TCase* tcase_concurrent_read = tcase_create("concurrent_reader"); TCase* tcase_power_fail = tcase_create("power_fail_rewrite"); + TCase* tcase_flash_fail = tcase_create("flash_failure_reported"); tcase_add_test(tcase_store_and_load_objs, test_store_and_load_objs); tcase_add_test(tcase_cross_sector_write, test_cross_sector_write_preserves_length); tcase_add_test(tcase_close, test_close_clears_handle_state); @@ -1074,6 +1217,11 @@ Suite *wolfboot_suite(void) tcase_add_test(tcase_concurrent_read, test_concurrent_reader_sees_pending_writes); tcase_add_test(tcase_power_fail, test_power_fail_during_rewrite_never_mixes_generations); + tcase_add_test(tcase_flash_fail, test_close_reports_flash_failure); + tcase_add_test(tcase_flash_fail, + test_flash_failure_reported_to_open_windows); + tcase_add_test(tcase_flash_fail, test_remove_reports_flash_failure); + tcase_add_test(tcase_flash_fail, test_vault_check_failure_reported); suite_add_tcase(s, tcase_store_and_load_objs); suite_add_tcase(s, tcase_cross_sector_write); suite_add_tcase(s, tcase_close); @@ -1086,6 +1234,7 @@ Suite *wolfboot_suite(void) suite_add_tcase(s, tcase_interleaved); suite_add_tcase(s, tcase_concurrent_read); suite_add_tcase(s, tcase_power_fail); + suite_add_tcase(s, tcase_flash_fail); return s; } From df87734ea57bf9c61d63afeecbb5f158f5bc8bd5 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 29 Oct 2026 14:55:43 +0000 Subject: [PATCH 4/4] pkcs11_store: ignore a close without an open wolfPKCS11 2.2.0 and earlier close a data record even when opening it failed, and pass NULL. wolfPKCS11_Store_CloseCommit(), which wolfPKCS11_Store_Close() calls, wrote through the handle and faulted. It now returns 0 for NULL. unit-pkcs11_store covers both closes. --- src/pkcs11_store.c | 5 +++++ tools/unit-tests/unit-pkcs11_store.c | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/pkcs11_store.c b/src/pkcs11_store.c index 7a02c8aaf4..a7e4e07516 100644 --- a/src/pkcs11_store.c +++ b/src/pkcs11_store.c @@ -807,6 +807,11 @@ int wolfPKCS11_Store_CloseCommit(void* store, int commit) int ret = 0; (void)commit; + /* Nothing was opened. wolfPKCS11 2.2.0 and earlier close a data record + * even when opening it failed. */ + if (handle == NULL) { + return 0; + } /* Commit all pending sectors: the header sector last, so the header * is the atomic commit point of the window. */ cache_flush_all(); diff --git a/tools/unit-tests/unit-pkcs11_store.c b/tools/unit-tests/unit-pkcs11_store.c index 5411e728dc..375ff22a3f 100644 --- a/tools/unit-tests/unit-pkcs11_store.c +++ b/tools/unit-tests/unit-pkcs11_store.c @@ -364,6 +364,16 @@ START_TEST(test_close_clears_handle_state) } END_TEST +START_TEST(test_close_null_store) +{ + /* wolfPKCS11 2.2.0 and earlier close a data record even when opening it + * failed, so a close without an open does nothing. */ + wolfPKCS11_Store_Close(NULL); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(NULL, 0), 0); + ck_assert_int_eq(wolfPKCS11_Store_CloseCommit(NULL, 1), 0); +} +END_TEST + START_TEST(test_delete_object_ignores_metadata_prefix) { const int32_t type = DYNAMIC_TYPE_RSA; @@ -1203,6 +1213,7 @@ Suite *wolfboot_suite(void) tcase_add_test(tcase_store_and_load_objs, test_store_and_load_objs); tcase_add_test(tcase_cross_sector_write, test_cross_sector_write_preserves_length); tcase_add_test(tcase_close, test_close_clears_handle_state); + tcase_add_test(tcase_close, test_close_null_store); tcase_add_test(tcase_delete_object, test_delete_object_ignores_metadata_prefix); tcase_add_test(tcase_delete_corrupted, test_delete_object_corrupted_pos_no_oob); tcase_add_test(tcase_delete_corrupted,