Report failed flash commits from the PKCS#11 keyvault, check STM32U5 flash errors

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)`
 

hal/stm32u5.c

@@ -80,13 +80,18 @@ 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())
     address = flash_addr_unalias(address);
 #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,27 +230,31 @@ 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) {
         uint32_t reg;
         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 ;

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)

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

src/pkcs11_store.c

@@ -34,6 +34,7 @@
 #include <wolfssl/wolfcrypt/settings.h>
 #include <wolfssl/wolfcrypt/types.h>
 #include <wolfssl/wolfcrypt/memory.h> /* wc_ForceZero */
+#include <wolfssl/wolfcrypt/error-crypt.h> /* 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;
 }
 

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)

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