Add CRC integrity check to NVM flash state - #503
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional CRC16 (CRC-16/CCITT-FALSE) integrity checking to the nvm_flash backend’s on-flash object state so the directory load, full-object reads, and compaction copies can detect corruption (gated by WOLFHSM_CFG_NVM_FLASH_CRC16), and wires this option through tests, tooling, docs, and CI.
Changes:
- Add
wh_Utils_Crc16()utility plusWH_UTILS_CRC16_INITseed constant. - Extend
nvm_flashon-flash format to embed CRC16 values in object start/count state words and verify metadata/data at key read/copy points. - Add CRC-enabled test coverage and build knobs for
test,test-refactor, andwhnvmtool, including CI jobs.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_utils.h | Declares CRC16 API and seed constant. |
| src/wh_utils.c | Implements CRC-16/CCITT-FALSE routine used by NVM flash CRC feature. |
| wolfhsm/wh_settings.h | Documents WOLFHSM_CFG_NVM_FLASH_CRC16 configuration macro and format incompatibility. |
| wolfhsm/wh_nvm_flash.h | Extends NVM flash in-memory state with CRC fields and adds NF_STATUS_CRC_BAD. |
| src/wh_nvm_flash.c | Stores CRCs in state words, verifies metadata/data, and updates directory parsing/accounting for CRC failures. |
| tools/whnvmtool/Makefile | Adds NVM_FLASH_CRC=1 build switch to compile tool with CRC-enabled format. |
| tools/whnvmtool/test/Makefile | Adds CRC build switch for whnvmtool tests. |
| tools/whnvmtool/README.md | Documents CRC format compatibility requirement between tool and server. |
| test/Makefile | Adds NVM_FLASH_CRC=1 build switch for CRC-enabled test runs. |
| test/wh_test_nvm_flash.h | Declares CRC16 integrity test entry point (guarded by macro). |
| test/wh_test_nvm_flash.c | Adds runtime tests for CRC vectors and corruption detection/handling in NVM flash backend. |
| test-refactor/README.md | Updates test mapping documentation to include CRC test coverage. |
| test-refactor/posix/Makefile | Adds NVM_FLASH_CRC=1 build switch for refactor POSIX test runs. |
| test-refactor/posix/wh_test_posix_main.c | Runs the refactor CRC test in the POSIX harness. |
| test-refactor/posix/wh_test_nvm_flash.c | Adds refactor CRC16 integrity test implementation. |
| docs/src/9-Configuration.md | Documents the new CRC config macro in the configuration reference. |
| docs/src/6-Utilities.md | Updates whnvmtool compatibility requirements to include CRC setting. |
| docs/src/5-Features.md | Documents CRC behavior/caveats for nvm_flash backend. |
| .github/workflows/build-and-test.yml | Adds CI job for CRC-enabled test build/run. |
| .github/workflows/build-and-test-whnvmtool.yml | Adds CI job for CRC-enabled whnvmtool build/tests. |
| .github/workflows/build-and-test-refactor.yml | Adds CI job for CRC-enabled refactor test build/run. |
Suppressed comments (1)
src/wh_nvm_flash.c:295
- The metadata CRC verification currently runs for both NF_STATUS_USED and NF_STATUS_DATA_BAD entries. For NF_STATUS_DATA_BAD (count word blank), failing the metadata CRC changes the status to NF_STATUS_CRC_BAD, but the directory parser assumes CRC_BAD entries have a valid state.count from the count word. This can mis-account reserved space (and can become undefined behavior if count was never set). Only apply the metadata CRC check when the object state is fully present (NF_STATUS_USED).
#ifdef WOLFHSM_CFG_NVM_FLASH_CRC16
/* Verify the metadata against the CRC in the start state word */
if (wh_Utils_Crc16(WH_UTILS_CRC16_INIT, &object->metadata,
sizeof(object->metadata)) !=
object->state.crc_meta) {
object->state.status = NF_STATUS_CRC_BAD;
clear_metadata = 1;
}
#endif
💡 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 #503
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #503
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 2
1 finding(s) posted as inline comments (see file-level comments below)
Low (1)
CRC-corrupted metadata resurrects superseded object version
File: src/wh_nvm_flash.c:927
Function: nfMemDirectory_Parse
Category: NV storage vulnerabilities
The duplicate-id reclaim loop only demotes an older NF_STATUS_USED entry when the newest same-id entry is also NF_STATUS_USED. If the newest entry is NF_STATUS_CRC_BAD (metadata failed CRC), the older entry is never marked bad and nfMemDirectory_FindObjectIndexById returns it, silently making a superseded object (e.g. a rotated key or cert) authoritative again.
Recommendation: Also treat NF_STATUS_CRC_BAD (and NF_STATUS_DATA_BAD) newest entries as superseding-but-invalid so older same-id entries stay reclaimed, or track ids independent of the newest entry's verification status.
Referenced code: src/wh_nvm_flash.c:927-931 (5 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #503
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
billphipps
left a comment
billphipps
left a comment
There was a problem hiding this comment.
Looks great! Can you modify the memDirectory per comments?
| * magic. The remaining 0x12/0x34 bytes still keep every state word distinct | ||
| * from erased flash. */ | ||
| static const whFlashUnit CRC_BASE_STATE = 0x1234000000000000ULL; | ||
| #define NF_STATE_CRC_PACK(_crc) (((whFlashUnit)(_crc)) << 32) |
There was a problem hiding this comment.
| #define NF_STATE_CRC_PACK(_crc) (((whFlashUnit)(_crc)) << 32) | |
| #define NF_STATE_CRC_PACK(_crc) (((whFlashUnit)((uint16_t)(_crc)) << 32) |
| if ((context->partition_units - NF_PARTITION_DATA_OFFSET) > | ||
| d->objects[d->next_free_object].state.start) { | ||
| d->reclaimable_data += | ||
| (context->partition_units - NF_PARTITION_DATA_OFFSET) - | ||
| d->objects[d->next_free_object].state.start; | ||
| } | ||
| d->next_free_data = context->partition_units; |
There was a problem hiding this comment.
Grumble. The flash context should NOT be necessary to parse the directory that was already read in. I agree with how you are handling this funky edge case, but this change really points out that we are missing some basic sanity checks in the start/count values anyway.
I recommend instead to add the max_count value into the memDirectory structure and set it within the ReadParseMemDirectory function, which needs the context value. Then, this function won't need the flash context to figure out the max_count (AND can make sure that the next_free_data NEVER exceeds max_count for ALL cases.
There is a minor bug here also in that this loop continues to update the memDirectory values after a failure is detected. That means that if the next object is good, the next_free_data WILL be set to the correct value, BUT the reclaimable_data that was computed here will be WRONG. I don't see an easy way around this, but maybe just be aware that reclaimable_data is a measurement and not necessarily gospel.
Also note that the deduplication loop below will also update reclaimable data after it is tweaked here.
Maybe we should consider a different method to compute/damage reclaimable data? Seems like we should be able to fully account for the usage of every sector in the data area. Dunno.
| *out_avail_size = (context->partition_units - | ||
| NF_PARTITION_DATA_OFFSET - d->next_free_data) * | ||
| WHFU_BYTES_PER_UNIT; | ||
| uint32_t data_units = |
There was a problem hiding this comment.
this could be replaced by memDirectory.max_count. Or a better name.
| uint32_t partition_units; /* Size of partition in units */ | ||
| int active; /* Which partition (0 or 1) is active */ | ||
| int initialized; | ||
| int directory_bad; /* Directory could not be reloaded from |
There was a problem hiding this comment.
Could this be eliminated and simply unset initialized? Or initialized could become a state instead of boolean? No fix.
Adds optional CRC16 integrity checking to the nvm_flash backend’s on-flash object state so the directory load, full-object reads, and compaction copies can detect corruption (gated by WOLFHSM_CFG_NVM_FLASH_CRC16), and wires this option through tests, tooling, docs, and CI.
Changes: