NVM locking: improve locking across the crypto layer by rizlik · Pull Request #438 · wolfSSL/wolfHSM

rizlik · 2026-07-09T17:12:30Z

This address two classes of gap with current state of N locking:

crypto layer is now proper locking operation that may involve the global cache or the NVM
To avoid TOCTOU some operations must be consolidate under a single lock, for example: GetUniqueIq + KeyImporting, or UsageFlagEnforcing + KeyExporting

Part of this new enforcement is needed only for global shared cache, but I'm not sure if it's worth to have separate locking codepaths for local vs global keys.

I'm not sure this is the best way to test this kind of problem neither.

The crypto layer uses the keystore cache/nvm without locking. While the keystore cache is shared only for global keys, the NVM is shared across all clients for any keyId, so operations that might touch the NVM layer must be locked. Also, check + usage type of operations (check enforcement flags, then use the key) must held the lock over the all operation, otherwise a key can change flags between check and usage. This can happen only with global keys.

Copilot

Pull request overview

This PR strengthens thread-safety and TOCTOU resistance around global/shared NVM-backed key caching by making crypto-layer operations (key usage enforcement, key export/use, and key-id allocation + import) occur under a single NVM lock scope where necessary.

Changes:

Adds an atomic “read key + enforce usage policy” primitive and updates usage enforcement to avoid policy/key-generation races.
Introduces *Export*Enforce wrappers for multiple key types and applies them throughout server crypto handlers; also consolidates key-id allocation + cache import under one lock hold.
Adds POSIX-only concurrent stress tests (including TSAN support) to detect key-cache read mixups and unique-id collisions under contention.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
wolfhsm/wh_settings.h	Adds `WOLFHSM_CFG_SERVER_KDF_MAX_KEY_SIZE` to bound cached KDF input copies.
wolfhsm/wh_server_keystore.h	Declares `wh_Server_KeystoreReadKeyEnforce` and documents TOCTOU caveats for standalone policy checks.
wolfhsm/wh_server_crypto.h	Adds `CacheExportEnforce` APIs to enforce usage policy against a locked snapshot during export.
src/wh_server_keystore.c	Implements `wh_Server_KeystoreReadKeyEnforce`; updates usage-enforcement helper to lock around freshen+check.
src/wh_server_crypto.c	Applies atomic export/enforce patterns across handlers; adds locked eviction helper; adds KDF input copying + bounds + zeroization; locks keygen id-allocation+import.
src/wh_server_cert.c	Documents non-recursive lock behavior in cert path; avoids re-locking and fixes an error-path cleanup.
test/wh_test_crypto.c	Updates ML-DSA DMA verify test to import key with required verify usage flag.
test-refactor/posix/wh_test_posix_main.c	Registers the new concurrent stress tests in the POSIX test runner.
test-refactor/posix/wh_test_keyread_race.h	Declares a self-contained concurrent cached-key read race test.
test-refactor/posix/wh_test_keyread_race.c	Adds concurrent AES-ECB read/compare stress test to detect wrong-key cache reads under contention.
test-refactor/posix/wh_test_keygen_unique_id.h	Declares a self-contained concurrent unique-id allocation test.
test-refactor/posix/wh_test_keygen_unique_id.c	Adds concurrent cache-keygen test to detect duplicate id allocation under contention.
test-refactor/posix/Makefile	Adds TSAN build/run wiring and enables TSAN-specific transport shims via defines.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #438

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

No new issues found in the changed files. ✅

bigbrett

First pass. Overall looks good and necessary to clean up some thread safety violations.

bigbrett · 2026-07-14T17:03:05Z

+                /* The whole id allocation + cache import chain below is already
+                 * atomic: every caller of this function reaches it through the
+                 * cert request dispatch, which holds WH_SERVER_NVM_LOCK across
+                 * the entire verify call. The NVM lock is non-recursive, so we
+                 * must NOT re-acquire it here -- use the unlocked GetUniqueId +
+                 * GetCacheSlotChecked building blocks under the caller's lock.
+                 */


recommend removing overly verbose AI comment, doesn't really add anything to reader

bigbrett · 2026-07-14T17:04:29Z

+            }
+            else if (out != NULL) {
+                /* Don't hand back key material that failed the policy check */
+                memset(out, 0, *outSz);


probably should start using the new wh_Utils_ForceZero here

bigbrett · 2026-07-14T17:05:15Z

+                /* Don't hand back key material that failed the policy check */
+                memset(out, 0, *outSz);
+            }
+        }


Suggested change

}

} /* WH_SERVER_NVM_LOCK() */

bigbrett · 2026-07-14T17:05:59Z

+/* May be deprecated soon: see wh_server_keystore.h. Enforcing here and using
+ * the key in a later lock scope leaves a TOCTOU window; prefer
+ * wh_Server_KeystoreReadKeyEnforce or the typed wh_Server_*CacheExport*Enforce
+ * wrappers. */


Should we just rip it out?

bigbrett · 2026-07-14T17:06:25Z

+        if (ret == WH_ERROR_OK) {
+            /* Enforce the usage policy with the obtained metadata */
+            ret = wh_Server_KeystoreEnforceKeyUsage(meta, requiredUsage);
+        }


Suggested change

}

} /* WH_SERVER_NVM_LOCK() */

bigbrett · 2026-07-14T17:07:45Z

+    if (ret == WH_ERROR_OK) {
+        (void)wh_Server_KeystoreEvictKey(ctx, keyId);
+        (void)WH_SERVER_NVM_UNLOCK(ctx);
+    }


Suggested change

}

} /* WH_SERVER_NVM_LOCK() */

bigbrett · 2026-07-14T17:13:28Z

+            ret = wh_Crypto_RsaDeserializeKeyDer(cacheMeta->len, cacheBuf, key);
+        }
+        (void)WH_SERVER_NVM_UNLOCK(ctx);
+    }


Suggested change

}

} /* WH_SERVER_NVM_LOCK() */

I'll stop commenting these but plz do a pass through the file and make sure all the end if braces around the critical sections are commented as such

bigbrett · 2026-07-14T17:26:45Z

    }

    /* Extract parameters from translated request */
    int      op_type = (int)(req.opType);


plz run git-clang-format main

bigbrett · 2026-07-14T19:00:38Z

    return ret;
 }

+int wh_Server_CacheExportRsaKeyEnforce(whServerContext* ctx, whKeyId keyId,


does this (and the other algo export helpers) need to be public? Should only happen in response to client request so should keep static if possible.

bigbrett · 2026-07-14T19:10:50Z

is there a reason we wouldn't want to just add this to the existing multithreaded stress test as a test case?

rizlik added 2 commits July 9, 2026 16:51

mldsa: fix usage flags in ml-dsa DMA test

a3f1792

rizlik requested review from AlexLanzano, Frauschi, bigbrett and Copilot July 9, 2026 17:12

rizlik self-assigned this Jul 9, 2026

Copilot started reviewing on behalf of rizlik July 9, 2026 17:16 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

rizlik added 2 commits July 9, 2026 19:37

test: add USAGE flag in DSA tests

e3a6cb0

formatting fixes

2bd4968

bigbrett requested a review from wolfSSL-Fenrir-bot July 9, 2026 18:49

wolfSSL-Fenrir-bot reviewed Jul 9, 2026

View reviewed changes

rizlik assigned bigbrett and AlexLanzano and unassigned rizlik Jul 14, 2026

bigbrett requested changes Jul 14, 2026

View reviewed changes

bigbrett assigned rizlik and unassigned bigbrett Jul 14, 2026

Uh oh!

Conversation

rizlik commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #438

Uh oh!

bigbrett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants