Skip to content

Zero DMA destination buffer on server WRITE_PRE - #486

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_4238
Open

Zero DMA destination buffer on server WRITE_PRE#486
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_4238

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Problem

WH_MESSAGE_CERT_ACTION_READTRUSTED_DMA passed req.cert_len to WRITE_POST
regardless of bytes written, so an authenticated client received up to
cert_len bytes of uninitialised server memory — when the read was denied
(WH_ERROR_ACCESS), when the id was absent, or past the end of a short
certificate. NVM READDMA had the identical defect. Addressed by f-4238.

Fix (src/wh_server_cert.c)

  • Deny before mapping — the flag check and metadata lookup precede
    WRITE_PRE, so a refused request never maps client memory.
    wh_Server_CertReadTrusted() calls the unchecked wh_Nvm_Read(), so this is
    the only gate.
  • Zero-fill on WRITE_PRE — the full mapped length is zeroed before the
    read, so the tail past a short object is zeros, not residue.
  • NULL mapping fails WH_ERROR_BADARGS with no paired POST; a zero-length
    request stays a no-op.
  • cert_len bounded by WOLFHSM_CFG_MAX_CERT_SIZE. NVM objects have no
    size cap, so the DMA allowlist bounds that handler's write extent — stated in
    wolfhsm/wh_client.h.

src/wh_server_nvm.c gets the same deny-before-map and zero-fill. Both handlers
keep main's per-handler lock layout.

Path Before After
denied / absent id cert_len of server heap never mapped
short read trailing heap disclosed zero-filled

Client-visible: a denied id with an out-of-range offset now returns ACCESS
rather than BADARGS, closing a length oracle on protected objects; and the
full requested length is written, not the clamped read length.

Closes f-4238.

Tests

Reworked per review. test-refactor/server/wh_test_dma_zerofill.c (777 lines)
drove the handlers with hand-built packets, so it is deleted along with its two
wh_test_list.c lines. Replaced by 146 lines in two existing client-server
files — wh_Client_* only, no new file, no new entry point:

File Sub-test
wh_test_nvm_ops.c _whTest_NvmDmaTailZeroed — 32-byte object read into a 256-byte poisoned buffer; the 224-byte tail must be zero
wh_test_nvm_ops.c _whTest_NvmDmaDeniedUntouchedNONEXPORTABLEACCESS with the buffer byte-for-byte unchanged; same at offset >= len
wh_test_client_certs.c _whTest_CertReadTrustedDmaTailZeroed — tail zero, and cert_len > WOLFHSM_CFG_MAX_CERT_SIZEBADARGS with the buffer unchanged

The cert case goes slightly past "what wh_Client_NvmReadDma can show" so that
src/wh_server_cert.c — the handler the finding is about — is not left
untested. whTest_CryptoAesDmaInPlace is unchanged. Coverage that needed the
deleted harness (bounce-buffer port shape, PRE/POST pairing counters, allowlist
accept/reject, NULL-mapping callback) is dropped deliberately; none of it is
reachable from a client.

Verification

  • test-refactor DMA=1 49/22/0 of 71; default 45/26/0; NOCRYPTO=1 16/55/0.
    Clean under -std=c90 -Werror -Wall -Wextra, zero warnings.
  • Negative controls: reverting either zero-fill, or the NVM deny-check hoist,
    fails the corresponding test.
  • ASan + UBSan under DMA=1: 0 ASan reports. UBSan hits are pre-existing
    message-struct alignment and one wolfSSL wc_mldsa.c overflow — nothing in
    either DMA handler.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 24, 2026
Copilot AI review requested due to automatic review settings July 24, 2026 01:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR closes a DMA bounce-buffer disclosure class in server-side DMA READ handlers by ensuring client destination buffers are either never mapped on refusal paths or are fully zero-filled before any partial writes, and it documents the resulting client-visible buffer contract.

Changes:

  • Reorders READDMA / READTRUSTED_DMA logic to deny-before-map and adds destination-buffer zero-fill on successful WRITE_PRE mappings.
  • Updates client and server header documentation to describe the destination-buffer contract and DMA callback locking constraints.
  • Adds focused regression tests for zero-fill behavior (NVM + cert) and an AES DMA in-place aliasing guard test.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
wolfhsm/wh_server.h Documents that DMA callbacks may run while the server NVM lock is held.
wolfhsm/wh_client.h Documents the “write all bytes (data + zero tail), or untouched if refused” destination-buffer contract for DMA reads.
src/wh_server_cert.c Implements deny-before-map, bounds cert_len, zero-fills mapped destination, and moves WRITE_POST outside the NVM lock for READTRUSTED_DMA.
src/wh_server_nvm.c Implements deny-before-map and zero-fills mapped destination for NVM READDMA.
test-refactor/wh_test_list.c Registers the new server DMA zero-fill test.
test-refactor/server/wh_test_dma_zerofill.c New regression test suite covering refusal paths, short reads, carry-over, oversized lengths, and null mapping/address cases.
test-refactor/client-server/wh_test_crypto_aes.c Adds AES DMA in-place aliasing test to guard against centralizing zero-fill in generic DMA address processing.

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

Comment thread src/wh_server_nvm.c Outdated

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #486

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

No new issues found in the changed files. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants