Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens DMA request handling in the certificate and NVM server handlers by ensuring the DMA *_POST operation is executed whenever the corresponding *_PRE succeeded, regardless of the main operation outcome.
Changes:
- Added
*_dma_pre_okflags to track successful DMA*_PREsteps in cert and NVM DMA request handlers. - Moved DMA
*_POSTcalls out of the “main operation success” path so cleanup always runs after a successful*_PRE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/wh_server_nvm.c | Tracks DMA PRE success for metadata/data and always executes corresponding POST cleanup. |
| src/wh_server_cert.c | Tracks DMA PRE success for cert buffers across multiple DMA actions and always executes corresponding POST cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Always call POST for successful PREs, regardless of operation | ||
| * result */ | ||
| if (metadata_dma_pre_ok) { | ||
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.metadata_hostaddr, &metadata, sizeof(whNvmMetadata), | ||
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); | ||
| } | ||
| if (resp.rc == 0) { | ||
| resp.rc = wh_Server_DmaProcessClientAddress( | ||
| if (data_dma_pre_ok) { | ||
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.data_hostaddr, &data, req.data_len, | ||
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); | ||
| } |
There was a problem hiding this comment.
The return code from the DMA *_POST operation is being discarded. If the main operation succeeds but the POST callback fails (e.g., cache flush/unmap error or allowlist rejection), the handler will still return success, which can leave the client with an inconsistent view of the operation. Consider capturing the POST rc and, if the current resp.rc is still WH_ERROR_OK, propagate the POST failure (or at least log it).
| /* Always call POST for successful PRE, regardless of read | ||
| * result */ | ||
| if (data_dma_pre_ok) { | ||
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.data_hostaddr, &data, req.data_len, | ||
| WH_DMA_OPER_CLIENT_WRITE_POST, (whServerDmaFlags){0}); | ||
| } |
There was a problem hiding this comment.
The DMA WRITE_POST return code is ignored. This can cause the server to report success even if the POST step fails, which is especially risky for write operations (e.g., cache maintenance not completed). Consider storing the POST rc and returning it when rc is still WH_ERROR_OK (or logging it if you intentionally want to preserve the read rc).
| /* Always call POST for successful PRE, regardless of operation | ||
| * result */ | ||
| if (cert_dma_pre_ok) { | ||
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.cert_addr, &cert_data, req.cert_len, | ||
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); | ||
| } |
There was a problem hiding this comment.
The DMA READ_POST return code is discarded via a void cast. If the POST callback fails after the certificate is added successfully, this handler will still return WH_ERROR_OK, potentially leaving DMA/cache state inconsistent. Consider propagating POST errors when resp.rc is still WH_ERROR_OK (or logging POST failures).
| /* Always call POST for successful PRE, regardless of operation | ||
| * result */ | ||
| if (cert_dma_pre_ok) { | ||
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.cert_addr, &cert_data, req.cert_len, | ||
| WH_DMA_OPER_CLIENT_WRITE_POST, (whServerDmaFlags){0}); | ||
| } |
There was a problem hiding this comment.
The DMA WRITE_POST return code is ignored. If the certificate read succeeded but POST fails, the handler will still return success, which may mean the client buffer wasn't correctly finalized (e.g., cache flush). Consider capturing the POST rc and using it when resp.rc is still WH_ERROR_OK (or logging failures if you need to preserve the main rc).
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.cert_addr, &cert_data, req.cert_len, | ||
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); |
There was a problem hiding this comment.
The DMA READ_POST return code is discarded. If cert verification succeeds but POST fails, the response will still indicate success even though the DMA cleanup step failed. Consider propagating POST errors when resp.rc is still WH_ERROR_OK (or logging POST failures).
| (void)wh_Server_DmaProcessClientAddress( | |
| server, req.cert_addr, &cert_data, req.cert_len, | |
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); | |
| int post_rc = wh_Server_DmaProcessClientAddress( | |
| server, req.cert_addr, &cert_data, req.cert_len, | |
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); | |
| if ((resp.rc == WH_ERROR_OK) && (post_rc != WH_ERROR_OK)) { | |
| resp.rc = post_rc; | |
| } |
| /* Always call POST for successful PRE, regardless of operation | ||
| * result */ | ||
| if (cert_dma_pre_ok) { | ||
| (void)wh_Server_DmaProcessClientAddress( | ||
| server, req.cert_addr, &cert_data, req.cert_len, | ||
| WH_DMA_OPER_CLIENT_READ_POST, (whServerDmaFlags){0}); | ||
| } |
There was a problem hiding this comment.
The DMA READ_POST return code is discarded. If the verify action succeeds but POST fails, this path will still report success (resp.rc remains WH_ERROR_OK), potentially masking DMA/cache maintenance failures. Consider capturing the POST rc and surfacing it when rc/resp.rc is still WH_ERROR_OK (or logging it).
|
CI failures look to be due to wolfSSL submodule |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resp.rc = wh_Server_DmaProcessClientAddress( | ||
| server, req.metadata_hostaddr, &metadata, sizeof(whNvmMetadata), | ||
| WH_DMA_OPER_CLIENT_READ_PRE, (whServerDmaFlags){0}); | ||
| if (resp.rc == 0) { | ||
| metadata_dma_pre_ok = 1; | ||
| } |
There was a problem hiding this comment.
This code mixes literal 0 checks with WH_ERROR_OK checks used elsewhere in the same file. Use WH_ERROR_OK consistently (e.g., resp.rc == WH_ERROR_OK) to avoid ambiguity and make the success condition resilient if the error-code scheme ever changes.
| /* If no allowlist is registered, deny all operations (fail-closed) */ | ||
| if (allowlist == NULL) { | ||
| return WH_ERROR_OK; | ||
| return WH_ERROR_ACCESS; | ||
| } |
There was a problem hiding this comment.
This PR also changes the DMA allowlist contract from 'NULL allowlist allows all' to 'NULL allowlist denies all' (fail-closed). That’s a significant behavioral/API change beyond the described PRE/POST cleanup adjustments; consider explicitly calling this out in the PR description (and any release notes/changelog if applicable) so downstream users aren’t surprised by the new default-deny behavior.
| /* Use static buffers so addresses are stable for DMA allow list */ | ||
| static uint8_t keyData1[32]; | ||
| static uint8_t keyData2[32]; | ||
| static uint8_t outBuf[32]; | ||
| uint8_t label[WH_NVM_LABEL_LEN]; | ||
| uint16_t labelSz = sizeof(label); | ||
| uint16_t outSz; | ||
|
|
||
| static whServerDmaAddrAllowList dmaAllowList = {0}; |
There was a problem hiding this comment.
Using static buffers makes the test non-reentrant and can introduce data races/flakiness if the test binary runs this test concurrently (e.g., parallel test execution). If stable addresses are required for the server’s registered allowlist lifetime, consider allocating per-test/per-thread buffers (heap or fixture-owned storage) and unregistering/resetting the allowlist after the test, rather than sharing statics across invocations.
| /* Use static buffers so addresses are stable for DMA allow list */ | |
| static uint8_t keyData1[32]; | |
| static uint8_t keyData2[32]; | |
| static uint8_t outBuf[32]; | |
| uint8_t label[WH_NVM_LABEL_LEN]; | |
| uint16_t labelSz = sizeof(label); | |
| uint16_t outSz; | |
| static whServerDmaAddrAllowList dmaAllowList = {0}; | |
| /* Per-invocation buffers keep stable addresses for this test run only. */ | |
| uint8_t keyData1[32]; | |
| uint8_t keyData2[32]; | |
| uint8_t outBuf[32]; | |
| uint8_t label[WH_NVM_LABEL_LEN]; | |
| uint16_t labelSz = sizeof(label); | |
| uint16_t outSz; | |
| whServerDmaAddrAllowList dmaAllowList = {0}; |
test/wh_test_multiclient.c
Outdated
| memcpy(keyData1, "GlobalDmaCacheTestKey123456!", 28); | ||
| memcpy(keyData2, "GlobalDmaExportTestKey12345!", 28); |
There was a problem hiding this comment.
The hard-coded length '28' is a magic number and can silently become wrong if the string constants change. Use a length derived from the source (e.g., sizeof("...") - 1) and/or explicitly initialize the whole 32-byte buffers (e.g., memset then memcpy) so the key material remains intentional and self-maintaining.
| memcpy(keyData1, "GlobalDmaCacheTestKey123456!", 28); | |
| memcpy(keyData2, "GlobalDmaExportTestKey12345!", 28); | |
| memset(keyData1, 0, sizeof(keyData1)); | |
| memcpy(keyData1, "GlobalDmaCacheTestKey123456!", | |
| sizeof("GlobalDmaCacheTestKey123456!") - 1); | |
| memset(keyData2, 0, sizeof(keyData2)); | |
| memcpy(keyData2, "GlobalDmaExportTestKey12345!", | |
| sizeof("GlobalDmaExportTestKey12345!") - 1); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| /* Register DMA allow list for the buffers used in DMA NVM tests. | ||
| * meta is declared here so its address is stable for the allow list */ | ||
| whNvmMetadata meta = {0}; | ||
| whServerDmaAddrAllowList nvm_dma_allow = {0}; | ||
| nvm_dma_allow.readList[0].addr = send_buffer; | ||
| nvm_dma_allow.readList[0].size = sizeof(send_buffer); | ||
| nvm_dma_allow.readList[1].addr = &meta; | ||
| nvm_dma_allow.readList[1].size = sizeof(meta); | ||
| nvm_dma_allow.writeList[0].addr = recv_buffer; | ||
| nvm_dma_allow.writeList[0].size = sizeof(recv_buffer); | ||
| WH_TEST_RETURN_ON_FAIL( | ||
| wh_Server_DmaRegisterAllowList(server, &nvm_dma_allow)); |
There was a problem hiding this comment.
The DMA allow list (nvm_dma_allow) and the address it contains (&meta) are stack variables whose lifetime ends at the closing brace, but the server likely retains a pointer to the registered allow list (as evidenced by other changes using static or struct-owned storage for allow list buffers). This can leave the server with a dangling allow-list pointer and/or dangling meta address, causing incorrect allow/deny behavior or crashes in later DMA operations. Fix by ensuring the allow list and any referenced buffers have a lifetime at least as long as the server usage (e.g., move whServerDmaAddrAllowList nvm_dma_allow and whNvmMetadata meta to the full function scope, store them in a longer-lived struct, or use static storage if appropriate for the test).
| { | |
| /* Register DMA allow list for the buffers used in DMA NVM tests. | |
| * meta is declared here so its address is stable for the allow list */ | |
| whNvmMetadata meta = {0}; | |
| whServerDmaAddrAllowList nvm_dma_allow = {0}; | |
| nvm_dma_allow.readList[0].addr = send_buffer; | |
| nvm_dma_allow.readList[0].size = sizeof(send_buffer); | |
| nvm_dma_allow.readList[1].addr = &meta; | |
| nvm_dma_allow.readList[1].size = sizeof(meta); | |
| nvm_dma_allow.writeList[0].addr = recv_buffer; | |
| nvm_dma_allow.writeList[0].size = sizeof(recv_buffer); | |
| WH_TEST_RETURN_ON_FAIL( | |
| wh_Server_DmaRegisterAllowList(server, &nvm_dma_allow)); | |
| /* Register DMA allow list for the buffers used in DMA NVM tests. | |
| * Keep both the allow list and meta at the surrounding function scope | |
| * so their addresses remain valid for as long as the server may use the | |
| * registered allow list. */ | |
| whNvmMetadata meta = {0}; | |
| whServerDmaAddrAllowList nvm_dma_allow = {0}; | |
| nvm_dma_allow.readList[0].addr = send_buffer; | |
| nvm_dma_allow.readList[0].size = sizeof(send_buffer); | |
| nvm_dma_allow.readList[1].addr = &meta; | |
| nvm_dma_allow.readList[1].size = sizeof(meta); | |
| nvm_dma_allow.writeList[0].addr = recv_buffer; | |
| nvm_dma_allow.writeList[0].size = sizeof(recv_buffer); | |
| WH_TEST_RETURN_ON_FAIL( | |
| wh_Server_DmaRegisterAllowList(server, &nvm_dma_allow)); |
| WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); | ||
| WH_TEST_ASSERT_RETURN(avail_objects == WOLFHSM_CFG_NVM_OBJECT_COUNT); | ||
|
|
||
| } /* DMA allow list scope */ |
There was a problem hiding this comment.
The DMA allow list (nvm_dma_allow) and the address it contains (&meta) are stack variables whose lifetime ends at the closing brace, but the server likely retains a pointer to the registered allow list (as evidenced by other changes using static or struct-owned storage for allow list buffers). This can leave the server with a dangling allow-list pointer and/or dangling meta address, causing incorrect allow/deny behavior or crashes in later DMA operations. Fix by ensuring the allow list and any referenced buffers have a lifetime at least as long as the server usage (e.g., move whServerDmaAddrAllowList nvm_dma_allow and whNvmMetadata meta to the full function scope, store them in a longer-lived struct, or use static storage if appropriate for the test).
src/wh_server_crypto.c
Outdated
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha256->devId = devId; | ||
| /* Validate buffLen from untrusted context */ | ||
| if (sha256->buffLen > WC_SHA256_BLOCK_SIZE) { |
There was a problem hiding this comment.
The buffLen upper-bound checks allow buffLen == WC_SHA*_*_BLOCK_SIZE. For SHA contexts, buffLen typically indexes into an internal buffer sized exactly WC_SHA*_*_BLOCK_SIZE, so permitting equality can enable one-past-the-end indexing/writes when the context is later used. Change these validations from > to >= for all four handlers to reject buffLen == block_size as well.
| if (sha256->buffLen > WC_SHA256_BLOCK_SIZE) { | |
| if (sha256->buffLen >= WC_SHA256_BLOCK_SIZE) { |
src/wh_server_crypto.c
Outdated
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha224->devId = devId; | ||
| /* Validate buffLen from untrusted context */ | ||
| if (sha224->buffLen > WC_SHA224_BLOCK_SIZE) { |
There was a problem hiding this comment.
The buffLen upper-bound checks allow buffLen == WC_SHA*_*_BLOCK_SIZE. For SHA contexts, buffLen typically indexes into an internal buffer sized exactly WC_SHA*_*_BLOCK_SIZE, so permitting equality can enable one-past-the-end indexing/writes when the context is later used. Change these validations from > to >= for all four handlers to reject buffLen == block_size as well.
| if (sha224->buffLen > WC_SHA224_BLOCK_SIZE) { | |
| if (sha224->buffLen >= WC_SHA224_BLOCK_SIZE) { |
src/wh_server_crypto.c
Outdated
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha384->devId = devId; | ||
| /* Validate buffLen from untrusted context */ | ||
| if (sha384->buffLen > WC_SHA384_BLOCK_SIZE) { |
There was a problem hiding this comment.
The buffLen upper-bound checks allow buffLen == WC_SHA*_*_BLOCK_SIZE. For SHA contexts, buffLen typically indexes into an internal buffer sized exactly WC_SHA*_*_BLOCK_SIZE, so permitting equality can enable one-past-the-end indexing/writes when the context is later used. Change these validations from > to >= for all four handlers to reject buffLen == block_size as well.
| if (sha384->buffLen > WC_SHA384_BLOCK_SIZE) { | |
| if (sha384->buffLen >= WC_SHA384_BLOCK_SIZE) { |
src/wh_server_crypto.c
Outdated
| /* retrieve hash Type to handle 512, 512-224, or 512-256 */ | ||
| hashType = sha512->hashType; | ||
| /* Validate buffLen from untrusted context */ | ||
| if (sha512->buffLen > WC_SHA512_BLOCK_SIZE) { |
There was a problem hiding this comment.
The buffLen upper-bound checks allow buffLen == WC_SHA*_*_BLOCK_SIZE. For SHA contexts, buffLen typically indexes into an internal buffer sized exactly WC_SHA*_*_BLOCK_SIZE, so permitting equality can enable one-past-the-end indexing/writes when the context is later used. Change these validations from > to >= for all four handlers to reject buffLen == block_size as well.
| if (sha512->buffLen > WC_SHA512_BLOCK_SIZE) { | |
| if (sha512->buffLen >= WC_SHA512_BLOCK_SIZE) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/wh_server_dma.c:139
- With the new default-deny behavior (NULL allow list => WH_ERROR_ACCESS), wh_Server_DmaProcessClientAddress() can return ACCESS after invoking server->dma.cb (it runs the callback before the allow list check). That means CopyFromClient/CopyToClient may execute callback side effects (e.g., allocations/cache ops) and then fail, without the caller getting a chance to run the matching *_POST for cleanup. Consider short-circuiting these DMA paths when no allow list is registered (return WH_ERROR_ACCESS before invoking the callback), or otherwise ensuring callback cleanup runs on failure.
/* Process the client address pre-read (includes allow list check) */
rc = wh_Server_DmaProcessClientAddress(
server, clientAddr, &transformedAddr, len, WH_DMA_OPER_CLIENT_READ_PRE,
flags);
if (rc != WH_ERROR_OK) {
return rc;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Same writeback test, but with DMA. | ||
| * Note: this is a client-only test where the server runs separately, | ||
| * so we cannot register a DMA allow list here. The server must have | ||
| * its allow list configured externally. */ | ||
| for (counter = 0; counter < 5; counter++) { |
There was a problem hiding this comment.
This client-only test block still asserts DMA operations succeed, but it also states the client cannot register a DMA allow list and the server must be configured externally. With the new default-deny behavior, this will fail against the example/benchmark servers that currently configure dmaAddrAllowList = NULL. Either skip the DMA portion in client-only mode unless an allow list is known to be configured, or update assertions to expect WH_ERROR_ACCESS when the server has no allow list.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Save the client devId to be restored later, when the context | ||
| * is copied back into client memory. */ | ||
| clientDevId = sha512->devId; | ||
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha512->devId = devId; | ||
| /* retrieve hash Type to handle 512, 512-224, or 512-256 */ | ||
| hashType = sha512->hashType; | ||
| /* Validate hashType from untrusted context */ | ||
| if (hashType != WC_HASH_TYPE_SHA512 && | ||
| hashType != WC_HASH_TYPE_SHA512_224 && | ||
| hashType != WC_HASH_TYPE_SHA512_256) { | ||
| ret = WH_ERROR_BADARGS; | ||
| } |
There was a problem hiding this comment.
In _HandleSha512Dma, sha512->devId is overwritten before validating the untrusted hashType. If hashType is invalid, this temporarily mutates the client context in a way the other SHA DMA handlers avoid. Consider validating hashType (and buffLen) before mutating the context, or explicitly reverting sha512->devId immediately when validation fails to keep error paths consistent and safer.
| /* Use static buffers so addresses are stable for DMA allow list */ | ||
| static uint8_t keyData1[32]; | ||
| static uint8_t keyData2[32]; | ||
| static uint8_t outBuf[32]; |
There was a problem hiding this comment.
Using static buffers/allowlist here makes the test non-reentrant and can introduce cross-test coupling if the test is ever run concurrently or invoked multiple times in the same process. Prefer allocating these as part of a per-test fixture/context that remains alive for the test duration (heap or a higher-level struct), and explicitly re-zero the allow list each invocation to avoid accidental carry-over.
| /* Process the client address pre-read (includes allow list check) */ | ||
| rc = wh_Server_DmaProcessClientAddress( | ||
| server, clientAddr, &transformedAddr, len, WH_DMA_OPER_CLIENT_READ_PRE, | ||
| flags); |
There was a problem hiding this comment.
The PR description’s 'Files Changed' list does not mention src/wh_server_dma.c (and also omits test/wh_test_clientserver.c), but both are modified in this PR. Please update the PR description to accurately reflect all changed files, especially since this file affects DMA access-control enforcement paths.
| @@ -351,18 +351,6 @@ static int _testDma(whServerContext* server, whClientContext* client) | |||
| WH_TEST_ASSERT_RETURN(0 == memcmp(testMem.srvBufAllow, testMem.cliBuf, | |||
| sizeof(testMem.srvBufAllow))); | |||
|
|
|||
There was a problem hiding this comment.
The prior assertions that DMA copies involving denylisted buffers return WH_ERROR_ACCESS were removed in this hunk. Given the allow-list behavior change (now fail-closed) and refactoring in whServerDma_CopyFromClient/ToClient, it would be good to add/retain a test that explicitly verifies access is denied for an out-of-allowlist DMA operation (ideally by exercising an unallowlisted clientAddr case) so allow-list enforcement remains covered.
| /* Check that out-of-allowlist client addresses are rejected */ | |
| WH_TEST_ASSERT_RETURN(WH_ERROR_ACCESS == whServerDma_CopyFromClient( | |
| server, testMem.srvBufAllow, | |
| (uintptr_t)testMem.srvBufDeny, | |
| sizeof(testMem.srvBufAllow), | |
| (whServerDmaFlags){0})); | |
| WH_TEST_ASSERT_RETURN(WH_ERROR_ACCESS == | |
| whServerDma_CopyToClient( | |
| server, (uintptr_t)testMem.srvBufDeny, | |
| testMem.srvBufAllow, | |
| sizeof(testMem.srvBufAllow), | |
| (whServerDmaFlags){0})); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Validate buffLen from untrusted context */ | ||
| if (sha256->buffLen >= WC_SHA256_BLOCK_SIZE) { | ||
| ret = WH_ERROR_BADARGS; | ||
| } | ||
| else { | ||
| /* Save the client devId to be restored later, when the context | ||
| * is copied back into client memory. */ | ||
| clientDevId = sha256->devId; | ||
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha256->devId = devId; | ||
| } |
There was a problem hiding this comment.
res is not initialized, and the new buffLen validation introduces an error path where no fields of res are written before translating it into the response. This can leak uninitialized stack data to the client. Initialize res (e.g., zero it at declaration) and ensure res.dmaAddrStatus is set to a deterministic value on all non-OK returns (including WH_ERROR_BADARGS for invalid buffLen).
| /* Validate buffLen from untrusted context */ | ||
| if (sha224->buffLen >= WC_SHA224_BLOCK_SIZE) { | ||
| ret = WH_ERROR_BADARGS; | ||
| } | ||
| else { | ||
| /* Save the client devId to be restored later, when the context | ||
| * is copied back into client memory. */ | ||
| clientDevId = sha224->devId; | ||
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha224->devId = devId; | ||
| } |
There was a problem hiding this comment.
res is not initialized, and the new buffLen validation can return WH_ERROR_BADARGS without ever populating res before wh_MessageCrypto_TranslateSha2DmaResponse(). That risks leaking uninitialized stack contents in the response. Initialize res and set a deterministic dmaAddrStatus value for this failure path.
| /* Validate buffLen from untrusted context */ | ||
| if (sha384->buffLen >= WC_SHA384_BLOCK_SIZE) { | ||
| ret = WH_ERROR_BADARGS; | ||
| } | ||
| else { | ||
| /* Save the client devId to be restored later, when the context | ||
| * is copied back into client memory. */ | ||
| clientDevId = sha384->devId; | ||
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha384->devId = devId; | ||
| } |
There was a problem hiding this comment.
The added buffLen check can fail with WH_ERROR_BADARGS before any fields of res are assigned, but res is still translated into the output buffer. Initialize res (e.g., {0}) and ensure dmaAddrStatus is always set on error to avoid returning uninitialized data.
| /* Validate buffLen from untrusted context */ | ||
| if (sha512->buffLen >= WC_SHA512_BLOCK_SIZE) { | ||
| ret = WH_ERROR_BADARGS; | ||
| } | ||
| else { | ||
| /* Save the client devId to be restored later, when the context | ||
| * is copied back into client memory. */ | ||
| clientDevId = sha512->devId; | ||
| /* overwrite the devId to that of the server for local crypto */ | ||
| sha512->devId = devId; | ||
| /* retrieve hash Type to handle 512, 512-224, or 512-256 */ | ||
| hashType = sha512->hashType; | ||
| /* Validate hashType from untrusted context */ | ||
| if (hashType != WC_HASH_TYPE_SHA512 && | ||
| hashType != WC_HASH_TYPE_SHA512_224 && | ||
| hashType != WC_HASH_TYPE_SHA512_256) { | ||
| ret = WH_ERROR_BADARGS; | ||
| } | ||
| } |
There was a problem hiding this comment.
With the new buffLen/hashType validation, ret can become WH_ERROR_BADARGS without writing anything into res, but res is still translated into the response. Initialize res and ensure res.dmaAddrStatus is deterministic on these validation failures to prevent leaking uninitialized stack data.
…nforce default-deny allow list Fix DMA POST resource leaks (bugs #1566-#1574): - Ensure DMA POST is always called after successful PRE in ADDOBJECTDMA, READDMA, ADDTRUSTED_DMA, READTRUSTED_DMA, VERIFY_DMA, and VERIFY_ACERT_DMA handlers using tracking flags Validate SHA DMA contexts from untrusted client memory (bug #2009): - Add buffLen >= block_size validation in all four SHA DMA handlers - Validate hashType in SHA512 DMA handler Change DMA allow list default to fail-closed (bug #2279): - Return WH_ERROR_ACCESS when no allow list is registered instead of permitting all operations - Remove erroneous server-side allow list check in CopyFromClient/CopyToClient (server buffers are trusted; client addresses are validated separately) Update tests for new DMA security defaults: - Register server and client DMA allow lists in stress, multiclient, and clientserver tests - Update DMA unit test to expect fail-closed behavior - Add client-address deny test for allow list enforcement
Summary
*_dma_pre_oktracking flags to all affected handlers._checkMemOperAgainstAllowListto returnWH_ERROR_ACCESSwhen no allow list is registered (allowlist == NULL), instead of silently permitting all operations. This is a breaking behavioral change — deployments that rely on DMA without an explicit allow list will now have DMA operations denied.buffLenvalidation to all four SHA DMA handlers (_HandleSha256Dma,_HandleSha224Dma,_HandleSha384Dma,_HandleSha512Dma) to prevent buffer overflow from malicious client context. Also validateshashTypein the SHA512 handler.wh_test_dma.c,wh_test_posix_threadsafe_stress.c, andwh_test_multiclient.cto register DMA allow lists and expect the new fail-closed behavior.Breaking Change: DMA Allow List Default
Previously, when
WOLFHSM_CFG_DMAwas enabled but no allow list was registered, all DMA operations were silently permitted (fail-open). This has been changed to deny all DMA operations when no allow list is configured (fail-closed). Any integration using DMA must now explicitly register an allow list viawh_Server_DmaRegisterAllowList().Files Changed
src/wh_server_nvm.c— DMA POST cleanup fix for ADDOBJECTDMA, READDMAsrc/wh_server_cert.c— DMA POST cleanup fix for ADDTRUSTED_DMA, READTRUSTED_DMA, VERIFY_DMA, VERIFY_ACERT_DMAsrc/wh_server_crypto.c— SHA DMA contextbuffLenandhashTypevalidationsrc/wh_dma.c— Default-deny when allow list is NULLwolfhsm/wh_server.h— Updated doc comment for new default-deny behaviortest/wh_test_dma.c— Updated NULL allowlist test expectationtest/wh_test_posix_threadsafe_stress.c— Added DMA allow list registrationtest/wh_test_multiclient.c— Added DMA allow list registration for global key DMA tests🤖 Generated with Claude Code