NVM locking: improve locking across the crypto layer#438
Open
rizlik wants to merge 6 commits into
Open
Conversation
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.
Contributor
There was a problem hiding this comment.
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*Enforcewrappers 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 *CacheExport*Enforce 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
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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
requested changes
Jul 14, 2026
bigbrett
left a comment
bigbrett
left a comment
Contributor
There was a problem hiding this comment.
First pass. Overall looks good and necessary to clean up some thread safety violations.
bigbrett
requested changes
Jul 16, 2026
bigbrett
left a comment
bigbrett
left a comment
Contributor
There was a problem hiding this comment.
one quick nit then I'm good with it.
| (void)wh_Server_KeystoreEvictKey(ctx, keyId); | ||
| (void)WH_SERVER_NVM_UNLOCK(ctx); | ||
| } /* WH_SERVER_NVM_LOCK() */ | ||
| else { |
Contributor
There was a problem hiding this comment.
did you forget to remove this debug statement in the last commit? Nowhere else do we have this in any of the other lock blocks. I don't want else{} clauses distracting.
bigbrett
approved these changes
Jul 20, 2026
bigbrett
left a comment
bigbrett
left a comment
Contributor
There was a problem hiding this comment.
@AlexLanzano over to you
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This address two classes of gap with current state of N locking:
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.