Zero-copy encrypt of AEAD application data#10899
Open
julek-wolfssl wants to merge 2 commits into
Open
Conversation
BuildMessage copied the application data into the output buffer and encrypted it in place. For AEAD suites the cipher input is exactly the plaintext, so Encrypt can read it straight from the caller's buffer and write the record to the output buffer, skipping the copy. - EncryptDo AES-GCM/CCM: input == out keeps the in-place record layout [explicit IV | plaintext | tag space]; input != out means input is the plaintext itself. ChaCha20-Poly1305 already reads only the plaintext from input. Other suites keep the copy: CBC/stream append MAC/pad to the cipher input, CID appends the real content type. - Disabled for WOLFSSL_ASYNC_CRYPT and WOLFSSL_THREADED_CRYPT, where encryption can run after BuildMessage returns, and for debug hooks that expect the in-place layout. Define WOLFSSL_BUILD_MSG_NO_ZERO_COPY to force the old behavior. Measured with examples/benchmark/dtls_bench -z (DTLS 1.2, 1300 byte records, aesni + intelasm): +2.9% AES-256-GCM, +1.0% ChaCha20-Poly1305. The copy was ~3% of send-path CPU in perf. Also fix two bugs found while benchmarking with a larger STATIC_BUFFER_LEN: - The STATIC_BUFFER_LEN bounds check had an unbalanced paren and compared enum constants in a preprocessor #if, where they evaluate to 0, so any user supplied value failed to compile. Keep the lower bound as a static assert. Drop the upper bound: values past the largest record are never used by the record layer but can be useful for memory layout reasons. - ShrinkInputBuffer could run in the middle of record processing via FreeHandshakeResources on the first DTLS app data record, moving the record data and invalidating the idx based bookkeeping. The stale curStartIdx made the more-messages-per-record check dispatch a spurious second message over the AEAD tag bytes, handing garbage app data to the caller and corrupting the buffer state for the next read. Only reachable when the remaining record data fits in the static buffer, which it never does with the default STATIC_BUFFER_LEN. Only shrink between records; the deferred shrink happens at the next wolfSSL_read.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the TLS 1.2 record send path for AEAD application-data by enabling a zero-copy mode where encryption reads plaintext directly from the caller’s buffer and writes the record ciphertext into the output buffer, avoiding an intermediate memcpy. It also includes two correctness fixes discovered during benchmarking with larger STATIC_BUFFER_LEN values, affecting compile-time validation and DTLS record processing safety.
Changes:
- Add conditional “zero-copy” AEAD encryption in
BuildMessage()for AES-GCM/CCM and ChaCha20-Poly1305 (with config-based exclusions and an opt-out macro). - Fix
STATIC_BUFFER_LENvalidation by replacing a broken preprocessor check with a compile-time static assertion and removing the (unnecessary) upper bound. - Prevent
ShrinkInputBuffer()from moving the input buffer during in-flight record processing by gating shrinking onprocessReplystate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
wolfssl/internal.h |
Replaces invalid #if validation of STATIC_BUFFER_LEN with wc_static_assert() to ensure a usable minimum buffer size. |
src/internal.c |
Implements AEAD zero-copy encryption plumbing in BuildMessage()/EncryptDo() and hardens ShrinkInputBuffer() against mid-record buffer moves. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
retest this please |
|
check-source-text requires macros that are used but never defined in the tree to be listed in .wolfssl_known_macro_extras.
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.
Summary
BuildMessagecopied the application data into the output buffer and encrypted it in place. For AEAD suites the cipher input is exactly the plaintext, soEncryptcan read it straight from the caller's buffer and write the record to the output buffer, skipping the copy.EncryptDoAES-GCM/CCM:input == outkeeps the in-place record layout[explicit IV | plaintext | tag space];input != outmeans input is the plaintext itself. ChaCha20-Poly1305 already reads only the plaintext from input. Other suites keep the copy: CBC/stream append MAC/pad to the cipher input, CID appends the real content type.WOLFSSL_ASYNC_CRYPTandWOLFSSL_THREADED_CRYPT, where encryption can run afterBuildMessagereturns, and for debug hooks that expect the in-place layout. DefineWOLFSSL_BUILD_MSG_NO_ZERO_COPYto force the old behavior.examples/benchmark/dtls_bench -z(DTLS 1.2, 1300 byte records, aesni + intelasm): +2.9% AES-256-GCM, +1.0% ChaCha20-Poly1305. The copy was ~3% of send-path CPU in perf.Also fixes two bugs found while benchmarking with a larger
STATIC_BUFFER_LEN:STATIC_BUFFER_LENbounds check had an unbalanced paren and compared enum constants in a preprocessor#if, where they evaluate to 0, so any user supplied value failed to compile. Keep the lower bound as a static assert. Drop the upper bound: values past the largest record are never used by the record layer but can be useful for memory layout reasons.ShrinkInputBuffercould run in the middle of record processing viaFreeHandshakeResourceson the first DTLS app data record, moving the record data and invalidating the idx based bookkeeping. The stalecurStartIdxmade the more-messages-per-record check dispatch a spurious second message over the AEAD tag bytes, handing garbage app data to the caller and corrupting the buffer state for the next read. Only reachable when the remaining record data fits in the static buffer, which it never does with the defaultSTATIC_BUFFER_LEN. Only shrink between records; the deferred shrink happens at the nextwolfSSL_read.Test plan
examples/benchmark/dtls_bench -zshows no regressionSTATIC_BUFFER_LEN