From 42e6dba558a27e3162b5b0bf5a4dd24075923b68 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Thu, 10 Sep 2026 09:48:59 +0200 Subject: [PATCH] pkcs11 store: invalidate the flash read cache after every commit The store commits sectors with hal_flash_erase()/hal_flash_write() and then reads them back through the memory map - sector_ptr(), cache_get_sector()'s refill, and the raw magic reads in check_vault(). On a part that caches flash reads (STM32 ICACHE) those reads can return pre-erase bytes. check_vault() is the worst case: a stale magic there does not merely read wrong, it triggers restore_backup() or a full vault re-initialisation, losing the token. --- hal/stm32c5.c | 10 +++-- hal/stm32u3.c | 8 +++- hal/stm32u5.c | 8 +++- include/hal.h | 14 +++++++ src/pkcs11_store.c | 21 ++++++++-- tools/unit-tests/Makefile | 8 ++++ tools/unit-tests/unit-mock-flash.c | 60 ++++++++++++++++++++++++++++ tools/unit-tests/unit-pkcs11_store.c | 2 + 8 files changed, 121 insertions(+), 10 deletions(-) diff --git a/hal/stm32c5.c b/hal/stm32c5.c index 252d94b5..b5571c8f 100644 --- a/hal/stm32c5.c +++ b/hal/stm32c5.c @@ -123,7 +123,6 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) if ((sr & FLASH_SR_ERR_MASK) != 0) { flash_clear_errors(); FLASH_CR &= ~FLASH_CR_PG; - hal_cache_invalidate(); return -1; } @@ -131,7 +130,6 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) i += write_len; DSB(); } - hal_cache_invalidate(); return 0; } @@ -153,6 +151,12 @@ void RAMFUNCTION hal_flash_lock(void) flash_wait_complete(); if ((FLASH_CR & FLASH_CR_LOCK) == 0) FLASH_CR |= FLASH_CR_LOCK; + /* Drop the flash read cache at the end of the batch rather than in + * hal_flash_write()/hal_flash_erase(): every write/erase sequence + * ends with a lock, so one invalidate per batch replaces one per + * operation (and per error return), and every consumer is covered, + * not just the ones that remember to ask. */ + hal_cache_invalidate(); } void RAMFUNCTION hal_flash_opt_unlock(void) @@ -231,12 +235,10 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) if ((sr & FLASH_SR_ERR_MASK) != 0) { flash_clear_errors(); FLASH_CR &= ~FLASH_CR_PER; - hal_cache_invalidate(); return -1; } } FLASH_CR &= ~FLASH_CR_PER; - hal_cache_invalidate(); return 0; } diff --git a/hal/stm32u3.c b/hal/stm32u3.c index 762eaf5b..7fb9e3fe 100644 --- a/hal/stm32u3.c +++ b/hal/stm32u3.c @@ -105,7 +105,6 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len) FLASH_NS_CR &= ~FLASH_CR_PG; i += 8; } - hal_cache_invalidate(); return 0; } @@ -128,6 +127,12 @@ void RAMFUNCTION hal_flash_lock(void) flash_wait_complete(); if ((FLASH_NS_CR & FLASH_CR_LOCK) == 0) FLASH_NS_CR |= FLASH_CR_LOCK; + /* Drop the flash read cache at the end of the batch rather than in + * hal_flash_write()/hal_flash_erase(): every write/erase sequence + * ends with a lock, so one invalidate per batch replaces one per + * operation (and per error return), and every consumer is covered, + * not just the ones that remember to ask. */ + hal_cache_invalidate(); } void RAMFUNCTION hal_flash_opt_unlock(void) @@ -194,7 +199,6 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len) flash_wait_complete(); } FLASH_NS_CR &= ~FLASH_CR_PER; - hal_cache_invalidate(); return 0; } diff --git a/hal/stm32u5.c b/hal/stm32u5.c index ac5725d4..c67e85ba 100644 --- a/hal/stm32u5.c +++ b/hal/stm32u5.c @@ -158,6 +158,12 @@ void RAMFUNCTION hal_flash_lock(void) #endif if ((FLASH_NS_CR & FLASH_CR_LOCK) == 0) FLASH_NS_CR |= FLASH_CR_LOCK; + /* Drop the flash read cache at the end of the batch rather than in + * hal_flash_write()/hal_flash_erase(): every write/erase sequence + * ends with a lock, so one invalidate per batch replaces one per + * operation (and per error return), and every consumer is covered, + * not just the ones that remember to ask. */ + hal_cache_invalidate(); } void RAMFUNCTION hal_flash_opt_unlock(void) @@ -619,7 +625,7 @@ void hal_cache_disable(void) ICACHE_CR &= ~ICACHE_CR_CEN; } -void hal_cache_invalidate(void) +void RAMFUNCTION hal_cache_invalidate(void) { /* only try and invalidate cache if enabled */ if ((ICACHE_CR & ICACHE_CR_CEN) == 0) diff --git a/include/hal.h b/include/hal.h index d2f2a0c8..f042023c 100644 --- a/include/hal.h +++ b/include/hal.h @@ -92,6 +92,20 @@ uint64_t hal_get_timer_us(void); #endif void hal_flash_unlock(void); void hal_flash_lock(void); +/* + * Drop any CPU-side cache of flash contents. + * + * On parts where flash reads are cached (e.g. the STM32 ICACHE), the CPU can + * still see pre-erase bytes after hal_flash_write()/hal_flash_erase() have + * completed. Any code that writes flash and then reads it back through the + * memory map must call this in between. + * + * src/libwolfboot.c provides a weak no-op, so targets without such a cache + * need not implement it; a HAL that has one overrides it and must also call + * it from its own hal_flash_lock(), which is where every write/erase batch + * ends. + */ +void hal_cache_invalidate(void); /* * Lock the flash region [address, address + len) against writes. * Return 0 on success, or a negative value on failure. diff --git a/src/pkcs11_store.c b/src/pkcs11_store.c index d84d16b0..5ad3dde1 100644 --- a/src/pkcs11_store.c +++ b/src/pkcs11_store.c @@ -245,6 +245,21 @@ static struct cache_entry *cache_find(uint32_t offset) return NULL; } +/* + * End a flash-mutating batch. Every sequence in this file is bracketed by + * hal_flash_unlock() ... store_flash_lock(), and the store reads committed + * sectors back through the memory map (sector_ptr(), cache_get_sector()'s + * refill, and the raw magic reads in check_vault()). On a part that caches + * flash reads those reads can return pre-erase bytes, so the cache is + * dropped here rather than relying on the HAL to do it internally: the + * store's correctness must not depend on which HAL it is linked against. + */ +static void store_flash_lock(void) +{ + hal_flash_lock(); + hal_cache_invalidate(); +} + static void cache_commit_entry(struct cache_entry *entry) { hal_flash_unlock(); @@ -260,7 +275,7 @@ static void cache_commit_entry(struct cache_entry *entry) hal_flash_write((uintptr_t)vault_base + entry->offset, entry->sector, WOLFBOOT_SECTOR_SIZE); - hal_flash_lock(); + store_flash_lock(); #ifdef PKCS11_STORE_STATS stats_commits++; stats_erases += 2; @@ -408,7 +423,7 @@ static void restore_backup(uint32_t offset) 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); - hal_flash_lock(); + store_flash_lock(); #ifdef PKCS11_STORE_STATS stats_erases++; stats_programs++; @@ -447,7 +462,7 @@ static void check_vault(void) cache_flush_all(); hal_flash_unlock(); hal_flash_erase((uintptr_t)vault_base + WOLFBOOT_SECTOR_SIZE * 2, total_vault_size); - hal_flash_lock(); + store_flash_lock(); #ifdef PKCS11_STORE_STATS stats_erases += total_vault_size / WOLFBOOT_SECTOR_SIZE; #endif diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 8b844f46..daddf0ee 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -124,6 +124,7 @@ TESTS+=unit-rp2350-flash-write TESTS+=unit-fwtpm-rsp-overrun TESTS+=unit-fwtpm-cmd-toctou TESTS+=unit-fdt-memrsv-wrap +TESTS+=unit-pkcs11_store-stalecache TESTS+=unit-aurix-erased-fill TESTS+=unit-aurix-erased-fill-invert TESTS+=unit-t2080-fman-loader @@ -257,6 +258,10 @@ unit-enc-nvm-flagshome:CFLAGS+=-DNVM_FLASH_WRITEONCE -DMOCK_PARTITIONS \ unit-enc-nvm-flagshome:WOLFCRYPT_SRC+=$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/chacha.c unit-delta:CFLAGS+=-DNVM_FLASH_WRITEONCE -DMOCK_PARTITIONS -DDELTA_UPDATES -DDELTA_BLOCK_SIZE=512 unit-pkcs11_store:CFLAGS+=-I$(WOLFBOOT_LIB_WOLFPKCS11) -DMOCK_PARTITIONS -DMOCK_KEYVAULT -DSECURE_PKCS11 -DWOLFPKCS11_USER_SETTINGS +# Same suite against a flash-read cache that only refreshes on +# hal_cache_invalidate(): fails if the store reads back a sector it just +# committed without dropping the cache first. +unit-pkcs11_store-stalecache:CFLAGS+=-I$(WOLFBOOT_LIB_WOLFPKCS11) -DMOCK_PARTITIONS -DMOCK_KEYVAULT -DSECURE_PKCS11 -DWOLFPKCS11_USER_SETTINGS -DMOCK_STALE_CACHE unit-psa_store:CFLAGS+=-I$(WOLFBOOT_LIB_WOLFPSA) -DMOCK_PARTITIONS -DMOCK_KEYVAULT -DWOLFCRYPT_TZ_PSA unit-update-flash:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \ -DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT -DPART_SWAP_EXT \ @@ -939,6 +944,9 @@ unit-update-disk-fit: ../../include/target.h unit-update-disk-fit.c unit-pkcs11_store: ../../include/target.h unit-pkcs11_store.c gcc -o $@ $(WOLFCRYPT_SRC) unit-pkcs11_store.c $(CFLAGS) $(WOLFCRYPT_CFLAGS) $(LDFLAGS) +unit-pkcs11_store-stalecache: ../../include/target.h unit-pkcs11_store.c + gcc -o $@ $(WOLFCRYPT_SRC) unit-pkcs11_store.c $(CFLAGS) $(WOLFCRYPT_CFLAGS) $(LDFLAGS) + unit-psa_store: ../../include/target.h unit-psa_store.c gcc -o $@ $(WOLFCRYPT_SRC) unit-psa_store.c $(CFLAGS) $(WOLFCRYPT_CFLAGS) $(LDFLAGS) diff --git a/tools/unit-tests/unit-mock-flash.c b/tools/unit-tests/unit-mock-flash.c index c8bc786d..7a0074f1 100644 --- a/tools/unit-tests/unit-mock-flash.c +++ b/tools/unit-tests/unit-mock-flash.c @@ -53,6 +53,40 @@ static int vault_powerfail_at = -1; static int vault_flash_ops; static jmp_buf vault_powerfail_jmp; +/* Stale-cache model (MOCK_STALE_CACHE). + * + * Models a part that caches flash reads, such as the STM32 ICACHE: flash + * operations land in a shadow buffer (the real flash contents) while + * vault_base keeps whatever the CPU last saw, and only + * hal_cache_invalidate() refreshes it. Code that writes a sector and reads + * it back without invalidating therefore observes pre-erase bytes, exactly + * as it would on silicon. Off by default, so the ordinary suite is + * unaffected. + */ +#ifdef MOCK_STALE_CACHE +static uint8_t *vault_shadow; +static int vault_shadow_valid; + +static void vault_cache_prime(void) +{ + if (!vault_shadow_valid) { + if (vault_shadow == NULL) { + vault_shadow = malloc(keyvault_size); + ck_assert_ptr_nonnull(vault_shadow); + } + memcpy(vault_shadow, vault_base, keyvault_size); + vault_shadow_valid = 1; + } +} + +/* Flash side of a vault write/erase: the CPU view is left untouched. */ +static uint8_t *vault_flash_at(uintptr_t address) +{ + vault_cache_prime(); + return vault_shadow + (address - (uintptr_t)vault_base); +} +#endif + static void vault_flash_op(void) { vault_flash_ops++; @@ -99,6 +133,9 @@ int hal_flash_write(haladdr_t address, const uint8_t *data, int len) #ifdef MOCK_KEYVAULT if ((address >= (const uintptr_t)vault_base) && (address < (const uintptr_t)vault_base + keyvault_size)) { vault_flash_op(); +#ifdef MOCK_STALE_CACHE + a = vault_flash_at(address); +#endif for (i = 0; i < len; i++) { a[i] = data[i]; } @@ -145,8 +182,12 @@ int hal_flash_erase(haladdr_t address, int len) vault_flash_op(); printf("Erasing vault from %p : %p bytes\n", address, len); erased_vault++; +#ifdef MOCK_STALE_CACHE + memset(vault_flash_at(address), 0xFF, len); +#else memset((void *)(uintptr_t)address, 0xFF, len); #endif +#endif #ifdef WOLFBOOT_DIAGNOSTICS_ADDRESS } else if ((address >= (haladdr_t)WOLFBOOT_DIAGNOSTICS_ADDRESS) && (address < (haladdr_t)WOLFBOOT_DIAGNOSTICS_ADDRESS + @@ -170,6 +211,20 @@ void hal_flash_lock(void) locked++; } +#ifdef MOCK_KEYVAULT +/* src/libwolfboot.c carries the weak default, but the keyvault suites do not + * include it (suites that do already have the symbol, hence the guard). + * Under MOCK_STALE_CACHE this is what makes flash visible to the CPU again. */ +void hal_cache_invalidate(void) +{ +#ifdef MOCK_STALE_CACHE + if (vault_shadow_valid) { + memcpy(vault_base, vault_shadow, keyvault_size); + } +#endif +} +#endif /* MOCK_KEYVAULT */ + void hal_prepare_boot(void) { } @@ -313,6 +368,11 @@ static int mmap_file(const char *path, uint8_t *address, uint32_t len, if (ret_address) *ret_address = mmaped_addr; +#if defined(MOCK_KEYVAULT) && defined(MOCK_STALE_CACHE) + /* New backing store: the shadow is re-primed from it on first use. */ + vault_shadow_valid = 0; +#endif + close(fd); return 0; } diff --git a/tools/unit-tests/unit-pkcs11_store.c b/tools/unit-tests/unit-pkcs11_store.c index cf07f691..b3b5de6c 100644 --- a/tools/unit-tests/unit-pkcs11_store.c +++ b/tools/unit-tests/unit-pkcs11_store.c @@ -749,6 +749,8 @@ static void vault_power_cycle(void) memset(openstores_handles, 0, sizeof(openstores_handles)); cache_lru_tick = 0; locked = 1; + /* A reboot also drops any CPU-side cache of flash. */ + hal_cache_invalidate(); } static int vault_obj_write(int type, CK_ULONG tok, CK_ULONG obj,