Conversation
…ful PRE in NVM and cert handlers
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 |
This pull request updates the DMA (Direct Memory Access) handling logic in both certificate and NVM (Non-Volatile Memory) server request handlers. The main improvement is ensuring that the DMA "POST" operation is always called if a corresponding "PRE" operation succeeded, regardless of the outcome of the main processing logic. This change increases robustness and correctness in resource management and cleanup. The implementation introduces flags to track the success of "PRE" operations and uses these flags to conditionally perform the "POST" step.
DMA Handling Improvements
cert_dma_pre_ok,metadata_dma_pre_ok,data_dma_pre_ok) to track the success of DMA "PRE" operations in both certificate and NVM request handlers (src/wh_server_cert.c,src/wh_server_nvm.c). [1] [2] [3] [4] [5] [6]src/wh_server_cert.c,src/wh_server_nvm.c). [1] [2] [3] [4] [5] [6]Request Handling Logic Adjustments
src/wh_server_cert.c,src/wh_server_nvm.c). [1] [2] [3] [4] [5]These changes make the DMA handling more robust and consistent across different server request types.