WOLFCRYPT_TZ_WOLFHSM: Skoll review fixes + flash-adapter unit test

aidangarske · aidangarske · commit e312cbb286a5 · 2026-05-01T16:15:50.000-07:00
- callable: runtime flash config init (drop non-portable static cast),
    panic on init failures, volatile *rspSz read, clear borrowed NS
    pointers post-dispatch.
  - flash_hal: propagate hal_flash_* errors, hoist unlock/lock outside
    loop, per-iteration cached_sector wipe, validate config before
    copying into ctx, rename sector_base to sector_offset.
  - test-app: aes_inited guard, KeyEvict after KeyCommit, ForceZero
    consistency, drop redundant keyId reassignment.
  - test-app/Makefile: WOLFBOOT_LIB_WOLFHSM default for standalone
    test-app builds.
  - tools/unit-tests/unit-wolfhsm_flash_hal.c: 10-test host unit test
    for the flash adapter, modeled on unit-psa_store.

  CMSE pointer-range checks intentionally not applied: m33mu lacks
  TT/TTAT, and PKCS11/PSA/fwTPM siblings all skip CMSE
diff --git a/src/wolfhsm_callable.c b/src/wolfhsm_callable.c
@@ -10,6 +10,7 @@
 #include <stdint.h>
 #include <string.h>
 
+#include "loader.h"
 #include "store_sbrk.h"
 #include "wolfboot/wcs_wolfhsm.h"
 #include "wolfboot/wolfhsm_flash_hal.h"
@@ -49,11 +50,9 @@ extern uint32_t _flash_keyvault;
 extern uint32_t _flash_keyvault_size;
 
 static whFlashH5Ctx              g_flash_ctx;
-static const whFlashH5Ctx        g_flash_cfg = {
-    .base           = (uint32_t)&_flash_keyvault,
-    .size           = (uint32_t)&_flash_keyvault_size,
-    .partition_size = WCS_WOLFHSM_PARTITION_SIZE,
-};
+/* Fields filled at runtime in wcs_wolfhsm_init: pointer-to-integer casts of
+ * linker symbols are not strictly conforming static initializers. */
+static whFlashH5Ctx              g_flash_cfg;
 
 static whNvmFlashContext         g_nvm_flash_ctx;
 static whNvmFlashConfig          g_nvm_flash_cfg = {
@@ -94,26 +93,29 @@ void wcs_wolfhsm_init(void)
 {
     int rc;
 
+    g_flash_cfg.base           = (uint32_t)&_flash_keyvault;
+    g_flash_cfg.size           = (uint32_t)&_flash_keyvault_size;
+    g_flash_cfg.partition_size = WCS_WOLFHSM_PARTITION_SIZE;
+
     rc = wc_InitRng(g_crypto_ctx.rng);
     if (rc != 0) {
-        return;
+        wolfBoot_panic();
     }
     rc = wh_Nvm_Init(&g_nvm_ctx, &g_nvm_cfg);
     if (rc != WH_ERROR_OK) {
-        return;
+        wolfBoot_panic();
     }
     rc = wh_Server_Init(&g_server, &g_server_cfg);
     if (rc != WH_ERROR_OK) {
-        return;
+        wolfBoot_panic();
+    }
+    rc = wh_Server_SetConnected(&g_server, WH_COMM_CONNECTED);
+    if (rc != WH_ERROR_OK) {
+        wolfBoot_panic();
     }
-    (void)wh_Server_SetConnected(&g_server, WH_COMM_CONNECTED);
     g_wolfhsm_ready = 1;
 }
 
-/* Single NSC veneer. Per call: validate the NS pointers/sizes (single-fetch
- * defeats TOCTOU on *rspSz), park the buffers in the secure-side transport
- * context, run wh_Server_HandleRequestMessage exactly once, write back the
- * captured response size. */
 int CSME_NSE_API wcs_wolfhsm_transmit(const uint8_t *cmd, uint32_t cmdSz,
         uint8_t *rsp, uint32_t *rspSz)
 {
@@ -123,9 +125,8 @@ int CSME_NSE_API wcs_wolfhsm_transmit(const uint8_t *cmd, uint32_t cmdSz,
     if (cmd == NULL || rsp == NULL || rspSz == NULL) {
         return WH_ERROR_BADARGS;
     }
-    /* Single fetch of the caller-supplied capacity; subsequent code uses
-     * only this local copy. The NS caller cannot mutate it under us. */
-    rsp_capacity = *rspSz;
+    /* volatile read forbids the compiler from re-fetching *rspSz later. */
+    rsp_capacity = *(volatile const uint32_t *)rspSz;
 
     if (cmdSz == 0U || cmdSz > WH_COMM_MTU) {
         return WH_ERROR_BADARGS;
@@ -151,6 +152,13 @@ int CSME_NSE_API wcs_wolfhsm_transmit(const uint8_t *cmd, uint32_t cmdSz,
     } else {
         *rspSz = 0;
     }
+
+    g_srv_tx_ctx.req_buf         = NULL;
+    g_srv_tx_ctx.req_size        = 0;
+    g_srv_tx_ctx.rsp_buf         = NULL;
+    g_srv_tx_ctx.rsp_capacity    = 0;
+    g_srv_tx_ctx.request_pending = 0;
+
     return rc;
 }
 
diff --git a/src/wolfhsm_flash_hal.c b/src/wolfhsm_flash_hal.c
@@ -13,6 +13,9 @@
 #include "hal.h"
 #include "wolfboot/wolfhsm_flash_hal.h"
 
+#include "wolfssl/wolfcrypt/settings.h"
+#include "wolfssl/wolfcrypt/misc.h"
+
 #include "wolfhsm/wh_error.h"
 #include "wolfhsm/wh_flash.h"
 
@@ -28,20 +31,22 @@ static uint8_t cached_sector[WHFH5_SECTOR_SIZE];
 
 static int _Init(void *context, const void *config)
 {
-    whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
+    const whFlashH5Ctx *cfg = (const whFlashH5Ctx *)config;
+    whFlashH5Ctx       *ctx = (whFlashH5Ctx *)context;
 
-    if (ctx == NULL || config == NULL) {
+    if (ctx == NULL || cfg == NULL) {
         return WH_ERROR_BADARGS;
     }
-    *ctx = *((const whFlashH5Ctx *)config);
 
-    if (ctx->base == 0U || ctx->size == 0U || ctx->partition_size == 0U ||
-        (ctx->base % WHFH5_SECTOR_SIZE) != 0U ||
-        (ctx->size % WHFH5_SECTOR_SIZE) != 0U ||
-        (ctx->partition_size % WHFH5_SECTOR_SIZE) != 0U ||
-        ctx->size < (uint32_t)2 * ctx->partition_size) {
+    if (cfg->base == 0U || cfg->size == 0U || cfg->partition_size == 0U ||
+        (cfg->base % WHFH5_SECTOR_SIZE) != 0U ||
+        (cfg->size % WHFH5_SECTOR_SIZE) != 0U ||
+        (cfg->partition_size % WHFH5_SECTOR_SIZE) != 0U ||
+        cfg->partition_size > cfg->size / 2U) {
         return WH_ERROR_BADARGS;
     }
+
+    *ctx = *cfg;
     return WH_ERROR_OK;
 }
 
@@ -96,6 +101,7 @@ static int _Program(void *context, uint32_t offset, uint32_t size,
 {
     whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
     uint32_t      written = 0U;
+    int           hrc     = 0;
 
     if (ctx == NULL || (size != 0U && data == NULL)) {
         return WH_ERROR_BADARGS;
@@ -107,28 +113,38 @@ static int _Program(void *context, uint32_t offset, uint32_t size,
         return WH_ERROR_OK;
     }
 
+    hal_flash_unlock();
     while (written < size) {
         uint32_t in_sector_off = (offset + written) % WHFH5_SECTOR_SIZE;
-        uint32_t sector_base   = (offset + written) - in_sector_off;
+        uint32_t sector_offset = (offset + written) - in_sector_off;
         uint32_t chunk         = WHFH5_SECTOR_SIZE - in_sector_off;
         if (chunk > size - written) {
             chunk = size - written;
         }
 
         memcpy(cached_sector,
-               (const uint8_t *)(ctx->base + sector_base),
+               (const uint8_t *)(ctx->base + sector_offset),
                WHFH5_SECTOR_SIZE);
         memcpy(cached_sector + in_sector_off, data + written, chunk);
 
-        hal_flash_unlock();
-        hal_flash_erase(ctx->base + sector_base, WHFH5_SECTOR_SIZE);
-        hal_flash_write(ctx->base + sector_base, cached_sector,
-                        WHFH5_SECTOR_SIZE);
-        hal_flash_lock();
+        hrc = hal_flash_erase(ctx->base + sector_offset, WHFH5_SECTOR_SIZE);
+        if (hrc == 0) {
+            hrc = hal_flash_write(ctx->base + sector_offset, cached_sector,
+                                  WHFH5_SECTOR_SIZE);
+        }
+
+        /* Per-iteration wipe so a fault between sectors doesn't strand
+         * plaintext keystore bytes in the static cache. */
+        wc_ForceZero(cached_sector, sizeof(cached_sector));
 
+        if (hrc != 0) {
+            break;
+        }
         written += chunk;
     }
-    return WH_ERROR_OK;
+    hal_flash_lock();
+
+    return (hrc == 0) ? WH_ERROR_OK : WH_ERROR_ABORTED;
 }
 
 static int _Erase(void *context, uint32_t offset, uint32_t size)
diff --git a/test-app/Makefile b/test-app/Makefile
@@ -5,6 +5,7 @@
 
 WOLFBOOT_LIB_WOLFSSL?=../lib/wolfssl
 WOLFBOOT_LIB_WOLFTPM?=../lib/wolfTPM
+WOLFBOOT_LIB_WOLFHSM?=../lib/wolfHSM
 WOLFSSL_LOCAL_OBJDIR?=wolfssl_obj
 WOLFTPM_LOCAL_OBJDIR?=wolftpm_obj
 WOLFHSM_LOCAL_OBJDIR?=wolfhsm_obj
diff --git a/test-app/wcs/wolfhsm_test.c b/test-app/wcs/wolfhsm_test.c
@@ -21,6 +21,7 @@
 #include "wolfhsm/wh_keyid.h"
 
 #include "wolfssl/wolfcrypt/aes.h"
+#include "wolfssl/wolfcrypt/misc.h"
 #include "wolfssl/wolfcrypt/random.h"
 #include "wolfssl/wolfcrypt/sha256.h"
 
@@ -124,7 +125,8 @@ static int wolfhsm_test_aes_cached(whClientContext *client)
     static const uint8_t iv[16] = { 0 };
     Aes      aes;
     uint8_t  ct[16];
-    uint16_t keyId = WH_KEYID_ERASED;
+    uint16_t keyId      = WH_KEYID_ERASED;
+    int      aes_inited = 0;
     int      rc;
 
     memset(&aes, 0, sizeof(aes));
@@ -142,6 +144,7 @@ static int wolfhsm_test_aes_cached(whClientContext *client)
         printf("wolfHSM AesInit failed: %d\r\n", rc);
         goto out;
     }
+    aes_inited = 1;
 
     rc = wh_Client_AesSetKeyId(&aes, keyId);
     if (rc != WH_ERROR_OK) {
@@ -166,7 +169,9 @@ static int wolfhsm_test_aes_cached(whClientContext *client)
     printf("wolfHSM AES ok\r\n");
 
 out:
-    wc_AesFree(&aes);
+    if (aes_inited) {
+        wc_AesFree(&aes);
+    }
     (void)wh_Client_KeyEvict(client, keyId);
     return rc;
 }
@@ -188,11 +193,12 @@ static int wolfhsm_test_persist(whClientContext *client, int *boot_state)
         memcmp(out, persist_key, sizeof(persist_key)) == 0) {
         printf("wolfHSM second boot path, restored persisted key\r\n");
         *boot_state = WOLFHSM_TEST_SECOND_BOOT_OK;
+        wc_ForceZero(out, sizeof(out));
         return 0;
     }
+    wc_ForceZero(out, sizeof(out));
 
     printf("wolfHSM first boot path, committing key to NVM\r\n");
-    keyId = WH_MAKE_KEYID(0, WCS_WOLFHSM_CLIENT_ID, 1);
     rc = wh_Client_KeyCache(client, WH_NVM_FLAGS_USAGE_ENCRYPT, NULL, 0,
                             persist_key, (uint16_t)sizeof(persist_key),
                             &keyId);
@@ -205,13 +211,14 @@ static int wolfhsm_test_persist(whClientContext *client, int *boot_state)
         printf("wolfHSM persist KeyCommit failed: %d\r\n", rc);
         return rc;
     }
+    (void)wh_Client_KeyEvict(client, keyId);
     *boot_state = WOLFHSM_TEST_FIRST_BOOT_OK;
     return 0;
 }
 
 int cmd_wolfhsm_test(const char *args)
 {
-    static const whTransportNscClientConfig nsc_cfg = { 0 };
+    static const whTransportNscClientConfig nsc_cfg = { { 0 } };
     whCommClientConfig comm_cfg;
     whClientConfig     cfg;
     whClientContext    client;
diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile
@@ -51,7 +51,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128
        unit-enc-nvm-flagshome unit-delta unit-update-flash unit-update-flash-delta \
        unit-update-flash-hook \
        unit-update-flash-self-update \
-       unit-update-flash-enc unit-update-ram unit-update-ram-nofixed unit-pkcs11_store unit-psa_store unit-disk \
+       unit-update-flash-enc unit-update-ram unit-update-ram-nofixed unit-pkcs11_store unit-psa_store unit-wolfhsm_flash_hal unit-disk \
        unit-update-disk unit-multiboot unit-boot-x86-fsp unit-loader-tpm-init unit-qspi-flash unit-fwtpm-stub unit-tpm-rsa-exp \
        unit-image-nopart unit-image-sha384 unit-image-sha3-384 unit-store-sbrk \
        unit-tpm-blob unit-policy-create unit-policy-sign unit-rot-auth unit-sdhci-response-bits \
@@ -357,6 +357,12 @@ unit-pkcs11_store: ../../include/target.h unit-pkcs11_store.c
 unit-psa_store: ../../include/target.h unit-psa_store.c
 	gcc -o $@ $(WOLFCRYPT_SRC) unit-psa_store.c $(CFLAGS) $(WOLFCRYPT_CFLAGS) $(LDFLAGS)
 
+unit-wolfhsm_flash_hal:CFLAGS+=-I$(WOLFBOOT_LIB_WOLFHSM) -DWOLFCRYPT_TZ_WOLFHSM -DWOLFHSM_CFG_NO_SYS_TIME -DMOCK_PARTITIONS
+unit-wolfhsm_flash_hal:WOLFCRYPT_SRC:=$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/memory.c \
+                                     $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/misc.c
+unit-wolfhsm_flash_hal: ../../include/target.h unit-wolfhsm_flash_hal.c
+	gcc -o $@ $(WOLFCRYPT_SRC) unit-wolfhsm_flash_hal.c $(CFLAGS) $(WOLFCRYPT_CFLAGS) $(LDFLAGS)
+
 gpt-sfdisk-test.h:
 	truncate -s 131072 .gpt-tmp.img
 	printf 'label: gpt\nfirst-lba: 34\nstart=34, size=67, name="boot"\nstart=101, size=100, name="rootfs"\n' \
diff --git a/tools/unit-tests/unit-wolfhsm_flash_hal.c b/tools/unit-tests/unit-wolfhsm_flash_hal.c
