Improvements for AES operations #282

Frauschi · 2026-02-05T16:13:19Z

This PR reworks a large part of the AES-DMA code and adds DMA support for the three remaining AES modes:

ECB
CBC
CTR

The existing AES-GCM DMA functionality has also been rewritten.

General

For all modes, only the input and output data is transferred using DMA buffers (exception: for AES-GCM, the AAD data is also transferred via DMA). All other variable data like a key, IV or other internal data with a (semi) fixed and short length are transferred by value within the request/response messages.

For all four modes, individual request/response message structures have been created for these AES DMA operations, as mapping them to the existing whMessageCrypto_AesDmaRequest/whMessageCrypto_AesGcmDmaResponse structures would increase complexity and also the number of transmitted bytes unnecessarily.

In the non-DMA implementations, some minor bugs have been fixed and the error handling has been cleaned up regarding usage of WH_ERROR_OK.

AES-ECB

As ECB does not an IV (each block is processed independently), only the key is transferred by value. Also, there is no internal AES state that has to be stored.

AES-CBC

Next to the key, an IV is transferred (IV is fixed block size length). This IV is also modified during the operation and has to be saved in the caller's AES structure for subsequent calls. Hence, an IV is transferred by value both in the request and in the response. This subsequent processing is also now tested in the unit tests.

AES-CTR

CTR mode also uses an IV (fixed block size length), which is transferred in both the request and the response. However, CTR also supports input/output data that is not a multiple of the AES block size. In this case, the last partial block is stored in the AES structure as internal state for subsequent calls (in the tmp array). Hence, this state must also be transferred between client and server. Subsequent processing for both whole blocks and partial blocks is now also tested in the unit tests to ensure this state handling is working.

AES-GCM

The AAD is also transferred via DMA, as this is variable length. Next to the variable length IV and the key, the authentication tag is also transferred by value in the request (for decryption) and the response (for encryption).

Benchmark

All AES modes with DMA are now executed in the benchmark in addition to the non-DMA versions. Furthermore, the module handling of the benchmarking application has been slightly reworked to get rid of the MAX_BENCH_OPS define.

JacobBarthelmeh

Initially was going to keep all AES operations under one "message" struct. I can see the merit in having separate ones for each mode though.

Copilot

Pull request overview

This pull request adds DMA (Direct Memory Access) support for three additional AES cipher modes: ECB, CBC, and CTR. The implementation follows the existing AES-GCM DMA pattern and includes proper cleanup of IV handling for ECB mode (which doesn't use IVs). The PR also refactors the existing AES-GCM DMA message structures for better clarity by renaming them from generic AesDmaRequest/Response to mode-specific AesGcmDmaRequest/Response.

Changes:

Added DMA support for AES-ECB, AES-CBC, and AES-CTR modes with new message request/response structures and translation functions
Removed unnecessary IV handling from ECB mode in both DMA and non-DMA implementations
Added comprehensive benchmark coverage for all new DMA operations

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
wolfhsm/wh_message_crypto.h	Added new DMA request/response structures for ECB, CBC, and CTR; renamed AES-GCM DMA structures for clarity; removed IV from ECB request structure
wolfhsm/wh_client_crypto.h	Added function declarations and documentation for new DMA operations
test/wh_test_crypto.c	Removed DMA exclusions for CTR and CBC tests; corrected ECB IV handling to use NULL
src/wh_server_crypto.c	Implemented DMA handlers for ECB, CBC, and CTR; removed IV handling from ECB; added default cases to switch statements; updated GCM handler to use renamed structures
src/wh_message_crypto.c	Added translation functions for new DMA structures; renamed GCM translation functions
src/wh_client_cryptocb.c	Added crypto callback handlers for new DMA cipher types
src/wh_client_crypto.c	Implemented client-side DMA functions for ECB, CBC, and CTR; removed IV handling from non-DMA ECB
benchmark/wh_bench_ops.h	Increased MAX_BENCH_OPS to accommodate new benchmark operations
benchmark/wh_bench.c	Added benchmark module entries for all new DMA operations
benchmark/bench_modules/wh_bench_mod_all.h	Added function declarations for new benchmark modules
benchmark/bench_modules/wh_bench_mod_aes.c	Implemented benchmark functions for all new DMA operations; added IV reset for CBC benchmarks

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

src/wh_server_crypto.c

src/wh_client_crypto.c

benchmark/bench_modules/wh_bench_mod_aes.c

Frauschi · 2026-02-06T16:20:30Z

I fixed all the Copilot findings. The failing test is related to unrelated changes in wolfssl upstream.

bigbrett

Fantastic work @Frauschi!

A few nits, as well as a couple issues that need to be fixed, most of which are actually issues with the original code that propagated into your new implementation due to following the same patterns.

wolfhsm/wh_message_crypto.h

wolfhsm/wh_client_crypto.h

src/wh_message_crypto.c

src/wh_client_cryptocb.c

benchmark/wh_bench_ops.h

benchmark/bench_modules/wh_bench_mod_aes.c

src/wh_server_crypto.c

.gitignore

Frauschi · 2026-02-10T14:52:21Z

I addressed all comments and reworked the DMA logic to only transfer input/output data via DMA, with all other parameters by value in the request/response.

src/wh_client_crypto.c

src/wh_message_crypto.c

bigbrett · 2026-02-10T19:35:55Z

src/wh_server_crypto.c

+        }
+    }
+    /* Verify key size is valid for AES */
+    if (keyLen != AES_128_KEY_SIZE && keyLen != AES_192_KEY_SIZE &&


this should probably be checked earlier in the function before any keystore lookups occur? And would WH_ERROR_BADARGS be more appropriate here?

This comment applies to all the new DMA handlers as the same logic exists in all of them.

I think the placement right after the keystore lookup is better, as the lookup sets keyLen to the size of the key from the store. Hence, both directly provided keys as well as keys from a store are checked. I looked into the keystore logic and there is no “logical” length check already present for the specific AES key sizes (probably because the store doesn't know about the intended usage for AES in any way).

But I changed the error code to WH_ERROR_BADARGS, as this is more appropriate as you said. I also added a debug message to identify the error.

OK, should this be protected by WH_ERROR_OK then? Specifically I don't want a keystore failure or lookup to be "masked" by a badargs here. We should return the first error that occurs

src/wh_server_crypto.c

benchmark/wh_bench_ops.c

Also minor fixes to the non-DMA AES-ECB code

bigbrett

Super close, a few more issues I found diving into the code.

Could you go back through the server-side handling and ensure any sizes/offsets/indices in client-controlled values are properly checked before usage? While this is somewhat beyond our threat model (we assume the comm buffers are "safe"), we still want to be robust against these things.

bigbrett · 2026-02-11T16:08:58Z

benchmark/bench_modules/wh_bench_mod_aes.c

+    ret = wc_AesInit(aes, NULL, WH_DEV_ID_DMA);
+    if (ret != 0) {
+        WH_BENCH_PRINTF("Failed to wc_AesInit %d\n", ret);
+        return ret;


This should goto exit too, as in the POSIX test case it will leak the DMA buffers? Not a big deal since typically everything is going to shut down anyway, but the other algos handle this case

bigbrett · 2026-02-11T16:23:16Z

src/wh_server_crypto.c

+        }
+    }
+    /* Verify key size is valid for AES */
+    if (keyLen != AES_128_KEY_SIZE && keyLen != AES_192_KEY_SIZE &&


OK, should this be protected by WH_ERROR_OK then? Specifically I don't want a keystore failure or lookup to be "masked" by a badargs here. We should return the first error that occurs

bigbrett · 2026-02-11T16:24:10Z

src/wh_server_crypto.c

+    uint32_t keyLen      = req.keySz;
+    uint32_t len         = req.input.sz;
+    uint32_t left        = req.left;
+    uint16_t needed_size = sizeof(whMessageCrypto_AesCtrDmaRequest) + keyLen +


I think there could be a "bug" here. Quotes because you would need to manually craft a packet to do this, not sure it is reachable through the client API or wolfCrypt, but still worth fixing.

needed_size needs to be uint64_t (like the non-DMA handlers). req.keyLen is a uint32_t so the addition can overflow uint16_t and bypass this bounds check. Pretty sure that no crypto is actually affected since the later keyLen length check against AES sizes catches this but I believe it still could cause WH_DEBUG_VERBOSE_HEXDUMP to do an out-of-bounds read. Same issue in all the other handlers. I would just change these to be safe.

bigbrett · 2026-02-11T16:33:03Z

src/wh_server_crypto.c

+    }
+
+    uint32_t keyLen      = req.keySz;
+    uint32_t len         = req.input.sz;


I think we also need to validate that input.sz == output.sz or an OOB write could occur? Similar to my other comment, I don't believe this is triggerable via proper use of the client API, but we do want to ensure the appropriate input validation is present on the server such that a crafted message can't crash it or cause a memory error.

The same issue appears to be present in the other handlers for CBC, CTR etc

Consider this case:

req.input.sz = 16

req.output.sz = 0 (null-write case) or 4 (overflow case)

valid req.input.addr, key, etc.

Server parses it and sets len = req.input.sz
In _HandleAesEcbDma, len comes from input size: len = req.input.sz

Output pointer handling depends only on req.output.sz
Output DMA mapping runs only if req.output.sz > 0
outAddr starts as NULL

Crypto call always uses len bytes
Server calls wc_AesEcbEncrypt/Decrypt(..., outAddr, ..., len) with no check that output.sz >= len.

So pretty sure this allows the two cases:

Null write path
If output.sz = 0, step 3 skips mapping, so outAddr stays NULL.
Step 4 still tries to write 16 bytes to outAddr, meaning null write

Overflow path
If output.sz = 4 and len = 16, mapping validates only a 4-byte writable region.
Step 4 writes 16 bytes anyway, so 12 bytes go past the validated region. So this writes past the region that the DMA callback would cover, possibly leading to unmapped or garbage addresses?

bigbrett · 2026-02-11T16:41:17Z

src/wh_client_crypto.c

+                if (enc != 0 && res->authTagSz > 0) {
+                    uint8_t* res_tag = (uint8_t*)res +
+                                       sizeof(whMessageCrypto_AesGcmDmaResponse);
+                    memcpy(enc_tag, res_tag, tag_len);


Doesn't look like we ever check enc_tag != NULL here, or clamp to tag_len, like the non-DMA wh_Client_AesGcm path does. So tag_len > 0 and enc_tag == NULL would lead to a null dereference.

I'd check server side too to make sure we are similarly robust

Frauschi requested a review from bigbrett February 5, 2026 16:37

JacobBarthelmeh self-assigned this Feb 5, 2026

JacobBarthelmeh previously approved these changes Feb 5, 2026

View reviewed changes

JacobBarthelmeh requested a review from Copilot February 5, 2026 21:46

JacobBarthelmeh removed their assignment Feb 5, 2026

Copilot started reviewing on behalf of JacobBarthelmeh February 5, 2026 21:46 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

Frauschi dismissed JacobBarthelmeh’s stale review via 8f2047e February 6, 2026 16:07

Frauschi force-pushed the aes_dma branch from d8f3885 to 8f2047e Compare February 6, 2026 16:07

bigbrett requested changes Feb 6, 2026

View reviewed changes

bigbrett assigned Frauschi Feb 9, 2026

Frauschi force-pushed the aes_dma branch 2 times, most recently from c3d0830 to 64007c0 Compare February 10, 2026 14:01

Frauschi changed the title ~~Add DMA support for remaining AES modes~~ Improvements for AES operations Feb 10, 2026

Frauschi requested review from JacobBarthelmeh and bigbrett February 10, 2026 14:52

Frauschi mentioned this pull request Feb 10, 2026

Fix potential DMA buffer handling errors #284

Merged

Frauschi assigned wolfSSL-Bot and unassigned Frauschi Feb 10, 2026

bigbrett requested changes Feb 10, 2026

View reviewed changes

Frauschi added 5 commits February 11, 2026 09:58

Add DMA support for AES-CTR

ba86088

Add DMA support for AES-CBC

e5b0440

Add DMA support for AES-ECB

1dd7b18

Also minor fixes to the non-DMA AES-ECB code

Improvements for AES-GCM

0fe8c78

AES Test improvements

41dbc38

Frauschi force-pushed the aes_dma branch from 64007c0 to 41dbc38 Compare February 11, 2026 09:16

Frauschi requested a review from bigbrett February 11, 2026 11:59

bigbrett requested changes Feb 11, 2026

View reviewed changes

bigbrett assigned Frauschi and unassigned wolfSSL-Bot Feb 11, 2026

Improvements for AES operations #282

Are you sure you want to change the base?

Improvements for AES operations #282

Conversation

Frauschi commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General

AES-ECB

AES-CBC

AES-CTR

AES-GCM

Benchmark

Uh oh!

JacobBarthelmeh left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Frauschi commented Feb 6, 2026

Uh oh!

bigbrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Frauschi commented Feb 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bigbrett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Frauschi commented Feb 5, 2026 •

edited

Loading