Skip to content

Commit 0c7b556

Browse files
aidangarskeaidangarske
committed
WOLFCRYPT_TZ_WOLFHSM: review fixes and unit-test coverage
- options.mk: drop duplicate WOLFHSM_CLIENT_OBJS / WOLFHSM_SERVER_OBJS block; top-of-file definitions are reached before all consumers. - wolfhsm_callable.c: idempotency guard in wcs_wolfhsm_init; defensive memset of g_srv_tx_ctx; move g_flash_cfg to a stack local; clamp rsp_size to rsp_capacity before publishing *rspSz; set *rspSz = 0 on every early-validation error path; use { 0 } initializer; switch to #ifdef WOLF_CRYPTO_CB. - wolfhsm_flash_hal.c: rename _Foo callbacks to whFlashH5_Foo to avoid C-reserved leading-underscore-uppercase identifiers; constant-time compare in Verify since data may be key material; defensive wc_ForceZero(cached_sector) on entry to Program; Erase short-circuits size == 0 before the alignment check for consistency. - test-app/wcs/wolfhsm_test.c: split AesSetIV vs AesCbcEncrypt error diagnostics; KeyEvict the cached key when KeyCommit fails; use { 0 } initializer for nsc_cfg. - tools/unit-tests/Makefile: add WOLFBOOT_LIB_WOLFHSM default and external-libs fallback so unit-wolfhsm_flash_hal finds wolfhsm headers in CI. - .github/workflows/test-external-library-paths.yml: pass WOLFBOOT_LIB_WOLFHSM to the unit-test matrix entry. - unit-wolfhsm_flash_hal.c: cover Cleanup, erase-failure propagation, Read happy path, multi-sector Program, NULL context for Read/PartitionSize/Program/Erase/Verify/BlankCheck, NULL data, and WriteLock/WriteUnlock; use MAP_FIXED_NOREPLACE when available.
1 parent b1c5d33 commit 0c7b556

7 files changed

Lines changed: 247 additions & 95 deletions

File tree

‎.github/workflows/test-external-library-paths.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,5 @@ jobs:
8686
echo "=== Building unit tests with external paths ==="
8787
make -C tools/unit-tests \
8888
WOLFBOOT_LIB_WOLFSSL="$(realpath ../external-libs/wolfssl)" \
89-
WOLFBOOT_LIB_WOLFPKCS11="$(realpath ../external-libs/wolfPKCS11)"
89+
WOLFBOOT_LIB_WOLFPKCS11="$(realpath ../external-libs/wolfPKCS11)" \
90+
WOLFBOOT_LIB_WOLFHSM="$(realpath ../external-libs/wolfHSM)"

‎options.mk‎

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1393,44 +1393,6 @@ ifeq ($(WOLFBOOT_TEST_SIM_CRYPTOCB),1)
13931393
endif
13941394
endif
13951395

1396-
# Shared wolfHSM client/server object lists. Both the legacy WOLFHSM_CLIENT=1 /
1397-
# WOLFHSM_SERVER=1 flags and the WOLFCRYPT_TZ_WOLFHSM=1 TZ engine reference
1398-
# these to avoid object-list duplication.
1399-
WOLFHSM_CLIENT_OBJS := \
1400-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_client.o \
1401-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_client_nvm.o \
1402-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_client_cryptocb.o \
1403-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_client_crypto.o \
1404-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_client_dma.o \
1405-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_crypto.o \
1406-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_dma.o \
1407-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_utils.o \
1408-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_comm.o \
1409-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_comm.o \
1410-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_nvm.o \
1411-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_customcb.o
1412-
1413-
WOLFHSM_SERVER_OBJS := \
1414-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_utils.o \
1415-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_comm.o \
1416-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_nvm.o \
1417-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_nvm_flash.o \
1418-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_keyid.o \
1419-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_flash_unit.o \
1420-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_crypto.o \
1421-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_server.o \
1422-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_server_nvm.o \
1423-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_server_crypto.o \
1424-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_server_counter.o \
1425-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_server_keystore.o \
1426-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_server_customcb.o \
1427-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_customcb.o \
1428-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_keystore.o \
1429-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_crypto.o \
1430-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_counter.o \
1431-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_nvm.o \
1432-
$(WOLFBOOT_LIB_WOLFHSM)/src/wh_message_comm.o
1433-
14341396
# wolfHSM client options
14351397
ifeq ($(WOLFHSM_CLIENT),1)
14361398
WOLFCRYPT_OBJS += \

‎src/wolfhsm_callable.c‎

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,12 @@ extern uint32_t _flash_keyvault;
5050
extern uint32_t _flash_keyvault_size;
5151

5252
static whFlashH5Ctx g_flash_ctx;
53-
/* Fields filled at runtime in wcs_wolfhsm_init: pointer-to-integer casts of
54-
* linker symbols are not strictly conforming static initializers. */
55-
static whFlashH5Ctx g_flash_cfg;
5653

5754
static whNvmFlashContext g_nvm_flash_ctx;
5855
static whNvmFlashConfig g_nvm_flash_cfg = {
5956
.cb = &whFlashH5_Cb,
6057
.context = &g_flash_ctx,
61-
.config = &g_flash_cfg,
58+
/* .config is set at runtime in wcs_wolfhsm_init */
6259
};
6360
static whNvmCb g_nvm_flash_cb = WH_NVM_FLASH_CB;
6461
static whNvmContext g_nvm_ctx;
@@ -70,7 +67,7 @@ static whNvmConfig g_nvm_cfg = {
7067

7168
static whServerCryptoContext g_crypto_ctx;
7269
static whTransportNscServerContext g_srv_tx_ctx;
73-
static whTransportNscServerConfig g_srv_tx_cfg = { { 0 } };
70+
static whTransportNscServerConfig g_srv_tx_cfg = { 0 };
7471
static whCommServerConfig g_comm_cfg = {
7572
.transport_context = &g_srv_tx_ctx,
7673
.transport_cb = &whTransportNscServer_Cb,
@@ -81,7 +78,7 @@ static whServerConfig g_server_cfg = {
8178
.comm_config = &g_comm_cfg,
8279
.nvm = &g_nvm_ctx,
8380
.crypto = &g_crypto_ctx,
84-
#if defined WOLF_CRYPTO_CB
81+
#ifdef WOLF_CRYPTO_CB
8582
.devId = INVALID_DEVID,
8683
#endif
8784
};
@@ -91,13 +88,22 @@ static int g_wolfhsm_ready;
9188

9289
void wcs_wolfhsm_init(void)
9390
{
94-
int rc;
91+
whFlashH5Ctx flash_cfg;
92+
int rc;
93+
94+
if (g_wolfhsm_ready) {
95+
return;
96+
}
97+
98+
memset(&g_srv_tx_ctx, 0, sizeof(g_srv_tx_ctx));
9599

96-
g_flash_cfg.base = (uint32_t)&_flash_keyvault;
97-
g_flash_cfg.size = (uint32_t)&_flash_keyvault_size;
98-
g_flash_cfg.partition_size = WCS_WOLFHSM_PARTITION_SIZE;
100+
flash_cfg.base = (uint32_t)&_flash_keyvault;
101+
flash_cfg.size = (uint32_t)&_flash_keyvault_size;
102+
flash_cfg.partition_size = WCS_WOLFHSM_PARTITION_SIZE;
103+
g_nvm_flash_cfg.config = &flash_cfg;
99104

100-
rc = wc_InitRng(g_crypto_ctx.rng);
105+
/* g_crypto_ctx.rng is an embedded WC_RNG[1] (see wh_server.h) */
106+
rc = wc_InitRng(&g_crypto_ctx.rng[0]);
101107
if (rc != 0) {
102108
wolfBoot_panic();
103109
}
@@ -125,16 +131,19 @@ int CSME_NSE_API wcs_wolfhsm_transmit(const uint8_t *cmd, uint32_t cmdSz,
125131
if (cmd == NULL || rsp == NULL || rspSz == NULL) {
126132
return WH_ERROR_BADARGS;
127133
}
128-
/* volatile read forbids the compiler from re-fetching *rspSz later. */
134+
/* single-fetch *rspSz so it cannot be re-read after validation */
129135
rsp_capacity = *(volatile const uint32_t *)rspSz;
130136

131137
if (cmdSz == 0U || cmdSz > WH_COMM_MTU) {
138+
*rspSz = 0;
132139
return WH_ERROR_BADARGS;
133140
}
134141
if (rsp_capacity == 0U || rsp_capacity > WH_COMM_MTU) {
142+
*rspSz = 0;
135143
return WH_ERROR_BADARGS;
136144
}
137145
if (!g_wolfhsm_ready) {
146+
*rspSz = 0;
138147
return WH_ERROR_NOTREADY;
139148
}
140149

@@ -148,8 +157,16 @@ int CSME_NSE_API wcs_wolfhsm_transmit(const uint8_t *cmd, uint32_t cmdSz,
148157
rc = wh_Server_HandleRequestMessage(&g_server);
149158

150159
if (rc == WH_ERROR_OK) {
151-
*rspSz = (uint32_t)g_srv_tx_ctx.rsp_size;
152-
} else {
160+
/* clamp: dispatcher must honor rsp_capacity; defend against regression */
161+
if ((uint32_t)g_srv_tx_ctx.rsp_size > rsp_capacity) {
162+
*rspSz = 0;
163+
rc = WH_ERROR_ABORTED;
164+
}
165+
else {
166+
*rspSz = (uint32_t)g_srv_tx_ctx.rsp_size;
167+
}
168+
}
169+
else {
153170
*rspSz = 0;
154171
}
155172

‎src/wolfhsm_flash_hal.c‎

Lines changed: 60 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,17 @@
2121

2222
#define WHFH5_SECTOR_SIZE (8U * 1024U)
2323

24-
/* Sector-cached read-modify-erase-write, mirroring psa_store.c. STM32H5
25-
* flash programs in 16-byte quad-words with ECC; each quad-word can be
26-
* programmed exactly once between erases. wolfHSM issues 8-byte unit
27-
* writes which would otherwise re-program neighbouring qwords, so every
28-
* Program call here loads the affected sector into RAM, modifies it, and
29-
* rewrites the whole 8 KiB sector after an erase. */
24+
/* Sector-cached RMW, mirrors psa_store.c. H5 flash programs as 16-byte
25+
* qwords ECC; one program per erase. wolfHSM 8-byte writes would clobber
26+
* neighbours, so Program loads the sector, modifies, erases, rewrites.
27+
*
28+
* Single static cache: non-reentrant by design (one secure-side server,
29+
* synchronous per-NSC dispatch, no threads or interrupts on this path).
30+
* Residual plaintext may remain in cache for one programming cycle if a
31+
* synchronous fault aborts hal_flash_write; disable debug in production. */
3032
static uint8_t cached_sector[WHFH5_SECTOR_SIZE];
3133

32-
static int _Init(void *context, const void *config)
34+
static int whFlashH5_Init(void *context, const void *config)
3335
{
3436
const whFlashH5Ctx *cfg = (const whFlashH5Ctx *)config;
3537
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
@@ -50,37 +52,38 @@ static int _Init(void *context, const void *config)
5052
return WH_ERROR_OK;
5153
}
5254

53-
static int _Cleanup(void *context)
55+
static int whFlashH5_Cleanup(void *context)
5456
{
5557
if (context == NULL) {
5658
return WH_ERROR_BADARGS;
5759
}
5860
return WH_ERROR_OK;
5961
}
6062

61-
static uint32_t _PartitionSize(void *context)
63+
static uint32_t whFlashH5_PartitionSize(void *context)
6264
{
6365
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
6466
return (ctx == NULL) ? 0U : ctx->partition_size;
6567
}
6668

67-
static int _WriteLock(void *context, uint32_t offset, uint32_t size)
69+
static int whFlashH5_WriteLock(void *context, uint32_t offset, uint32_t size)
6870
{
6971
(void)context;
7072
(void)offset;
7173
(void)size;
7274
return WH_ERROR_OK;
7375
}
7476

75-
static int _WriteUnlock(void *context, uint32_t offset, uint32_t size)
77+
static int whFlashH5_WriteUnlock(void *context, uint32_t offset, uint32_t size)
7678
{
7779
(void)context;
7880
(void)offset;
7981
(void)size;
8082
return WH_ERROR_OK;
8183
}
8284

83-
static int _Read(void *context, uint32_t offset, uint32_t size, uint8_t *data)
85+
static int whFlashH5_Read(void *context, uint32_t offset, uint32_t size,
86+
uint8_t *data)
8487
{
8588
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
8689

@@ -96,8 +99,8 @@ static int _Read(void *context, uint32_t offset, uint32_t size, uint8_t *data)
9699
return WH_ERROR_OK;
97100
}
98101

99-
static int _Program(void *context, uint32_t offset, uint32_t size,
100-
const uint8_t *data)
102+
static int whFlashH5_Program(void *context, uint32_t offset, uint32_t size,
103+
const uint8_t *data)
101104
{
102105
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
103106
uint32_t written = 0U;
@@ -113,6 +116,9 @@ static int _Program(void *context, uint32_t offset, uint32_t size,
113116
return WH_ERROR_OK;
114117
}
115118

119+
/* defensive wipe in case a prior call faulted before the per-iter wipe */
120+
wc_ForceZero(cached_sector, sizeof(cached_sector));
121+
116122
hal_flash_unlock();
117123
while (written < size) {
118124
uint32_t in_sector_off = (offset + written) % WHFH5_SECTOR_SIZE;
@@ -147,7 +153,7 @@ static int _Program(void *context, uint32_t offset, uint32_t size,
147153
return (hrc == 0) ? WH_ERROR_OK : WH_ERROR_ABORTED;
148154
}
149155

150-
static int _Erase(void *context, uint32_t offset, uint32_t size)
156+
static int whFlashH5_Erase(void *context, uint32_t offset, uint32_t size)
151157
{
152158
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
153159
int rc;
@@ -158,39 +164,56 @@ static int _Erase(void *context, uint32_t offset, uint32_t size)
158164
if (offset > ctx->size || size > ctx->size - offset) {
159165
return WH_ERROR_BADARGS;
160166
}
167+
if (size == 0U) {
168+
return WH_ERROR_OK;
169+
}
161170
if ((offset % WHFH5_SECTOR_SIZE) != 0U ||
162171
(size % WHFH5_SECTOR_SIZE) != 0U) {
163172
return WH_ERROR_BADARGS;
164173
}
165-
if (size == 0U) {
166-
return WH_ERROR_OK;
167-
}
168174

175+
/* hal_flash_erase takes int; loop over sector-sized chunks so the cast
176+
* stays well-defined regardless of how large size grows. */
169177
hal_flash_unlock();
170-
rc = hal_flash_erase(ctx->base + offset, (int)size);
178+
{
179+
uint32_t erased = 0U;
180+
rc = 0;
181+
while (erased < size) {
182+
rc = hal_flash_erase(ctx->base + offset + erased,
183+
(int)WHFH5_SECTOR_SIZE);
184+
if (rc != 0) {
185+
break;
186+
}
187+
erased += WHFH5_SECTOR_SIZE;
188+
}
189+
}
171190
hal_flash_lock();
172191
return (rc == 0) ? WH_ERROR_OK : WH_ERROR_ABORTED;
173192
}
174193

175-
static int _Verify(void *context, uint32_t offset, uint32_t size,
176-
const uint8_t *data)
194+
static int whFlashH5_Verify(void *context, uint32_t offset, uint32_t size,
195+
const uint8_t *data)
177196
{
178-
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
197+
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
198+
const uint8_t *p;
199+
uint8_t acc = 0;
200+
uint32_t i;
179201

180202
if (ctx == NULL || (size != 0U && data == NULL)) {
181203
return WH_ERROR_BADARGS;
182204
}
183205
if (offset > ctx->size || size > ctx->size - offset) {
184206
return WH_ERROR_BADARGS;
185207
}
186-
if (size > 0U &&
187-
memcmp((const uint8_t *)(ctx->base + offset), data, size) != 0) {
188-
return WH_ERROR_NOTVERIFIED;
208+
/* constant-time compare; verified data may be key material */
209+
p = (const uint8_t *)(ctx->base + offset);
210+
for (i = 0U; i < size; i++) {
211+
acc |= (uint8_t)(p[i] ^ data[i]);
189212
}
190-
return WH_ERROR_OK;
213+
return (acc == 0U) ? WH_ERROR_OK : WH_ERROR_NOTVERIFIED;
191214
}
192215

193-
static int _BlankCheck(void *context, uint32_t offset, uint32_t size)
216+
static int whFlashH5_BlankCheck(void *context, uint32_t offset, uint32_t size)
194217
{
195218
whFlashH5Ctx *ctx = (whFlashH5Ctx *)context;
196219
const uint8_t *p;
@@ -212,16 +235,16 @@ static int _BlankCheck(void *context, uint32_t offset, uint32_t size)
212235
}
213236

214237
const whFlashCb whFlashH5_Cb = {
215-
.Init = _Init,
216-
.Cleanup = _Cleanup,
217-
.PartitionSize = _PartitionSize,
218-
.WriteLock = _WriteLock,
219-
.WriteUnlock = _WriteUnlock,
220-
.Read = _Read,
221-
.Program = _Program,
222-
.Erase = _Erase,
223-
.Verify = _Verify,
224-
.BlankCheck = _BlankCheck,
238+
.Init = whFlashH5_Init,
239+
.Cleanup = whFlashH5_Cleanup,
240+
.PartitionSize = whFlashH5_PartitionSize,
241+
.WriteLock = whFlashH5_WriteLock,
242+
.WriteUnlock = whFlashH5_WriteUnlock,
243+
.Read = whFlashH5_Read,
244+
.Program = whFlashH5_Program,
245+
.Erase = whFlashH5_Erase,
246+
.Verify = whFlashH5_Verify,
247+
.BlankCheck = whFlashH5_BlankCheck,
225248
};
226249

227250
#endif /* WOLFCRYPT_TZ_WOLFHSM */

‎test-app/wcs/wolfhsm_test.c‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,11 @@ static int wolfhsm_test_aes_cached(whClientContext *client)
153153
}
154154

155155
rc = wc_AesSetIV(&aes, iv);
156-
if (rc == 0) {
157-
rc = wc_AesCbcEncrypt(&aes, ct, pt, (word32)sizeof(pt));
156+
if (rc != 0) {
157+
printf("wolfHSM AesSetIV failed: %d\r\n", rc);
158+
goto out;
158159
}
160+
rc = wc_AesCbcEncrypt(&aes, ct, pt, (word32)sizeof(pt));
159161
if (rc != 0) {
160162
printf("wolfHSM AES encrypt failed: %d\r\n", rc);
161163
goto out;
@@ -209,6 +211,7 @@ static int wolfhsm_test_persist(whClientContext *client, int *boot_state)
209211
rc = wh_Client_KeyCommit(client, keyId);
210212
if (rc != WH_ERROR_OK) {
211213
printf("wolfHSM persist KeyCommit failed: %d\r\n", rc);
214+
(void)wh_Client_KeyEvict(client, keyId);
212215
return rc;
213216
}
214217
(void)wh_Client_KeyEvict(client, keyId);
@@ -218,7 +221,7 @@ static int wolfhsm_test_persist(whClientContext *client, int *boot_state)
218221

219222
int cmd_wolfhsm_test(const char *args)
220223
{
221-
static const whTransportNscClientConfig nsc_cfg = { { 0 } };
224+
static const whTransportNscClientConfig nsc_cfg = { 0 };
222225
whCommClientConfig comm_cfg;
223226
whClientConfig cfg;
224227
whClientContext client;

0 commit comments

Comments
 (0)