Add DMA support for remaining AES modes #282

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

This PR adds DMA support for the three remaining AES modes:

ECB
CBC
CTR

The implementations are very similar to the existing AES-GCM DMA implementation. For CBC, there is #225 already, which is superseded by this PR (#225 seems stalled and doesn't pass tests). @JacobBarthelmeh if you are fine with it, you may close #225 once this is merged.

I added new message request/response structures 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 (although they seemed to be intended for all modes).

All the new implementations are tested in the unit tests and also executed in the benchmark.

I also did some minor cleanup work for non-DMA implementations (e.g. around CBC IV handling) and, most importantly, removed the IV handling for non-DMA ECB mode, as ECB does not use IV at all. This previous handling in code hasn't caused any bugs or problems, but for clarity I assumed the removal is beneficial anyway.

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.

bigbrett · 2026-02-05T23:27:35Z

wolfhsm/wh_message_crypto.h

+typedef struct {
+    uint32_t                  enc;      /* 1 for encrypt, 0 for decrypt */
+    uint32_t                  keyId;
+    whMessageCrypto_DmaBuffer key;      /* Key buffer */


I know this was based off of the GCM implementation where we already did this, but I'm realizing now it is a little silly to pass key and iv by reference, since the DMA pointer size is the same size as a 16-byte IV, and for AES 128 the key size will be too. So DMA actually doesn't improve anything for these fields. Our primary use case (keyId on HSM) doesn't even populate these fields anyway.

I think, in general, we really only need the (potentially large) input/output data to be DMA.

Just spitballing, no change necessary for now

bigbrett · 2026-02-05T23:37:28Z

wolfhsm/wh_client_crypto.h

 int wh_Client_AesCtr(whClientContext* ctx, Aes* aes, int enc, const uint8_t* in,
                     uint32_t len, uint8_t* out);
+
+#ifdef WOLFHSM_CFG_DMA


could you remove the macro protection here?

In wolfHSM we try not to conditionally declare functions in headers, but just gate their definitions in the corresponding source file.

This rule was implemented halfway through library development, so as you can see in this file, it is not consistently applied. Please just remove the DMA macro protection you added for now, and we can clean up and make more uniform in a subsequent PR>

bigbrett · 2026-02-05T23:42:41Z

src/wh_message_crypto.c


-    ret = wh_MessageCrypto_TranslateDmaBuffer(magic, &src->state, &dest->state);
+    ret = wh_MessageCrypto_TranslateDmaBuffer(magic, &src->key, &dest->key);
+    if (ret != 0) {


I know you didn't write this original code, but since we are touching it, could you pls change the ret != 0 to ret != WH_ERROR_OK?

We want all wolfHSM success values to be WH_ERROR_OK, which helps distinguish the intent from a wolfCrypt/wolfSSL error value (of which sometimes 0 is success, and sometimes not, depending on the API).

That said, since the DMA buffer translation can't fail when you control the arguments (already null checked), I'd be okay with you void-casting the return value here just to keep the function body smaller.

bigbrett · 2026-02-05T23:45:00Z

src/wh_client_cryptocb.c

+                uint8_t*       out = info->cipher.aesctr.out;
+
+                ret = wh_Client_AesCtrDma(ctx, aes, enc, in, len, out);
+


Suggested change

bigbrett · 2026-02-05T23:48:43Z

benchmark/wh_bench_ops.h


 /* Maximum number of operations that can be registered */
-#define MAX_BENCH_OPS 89
+#define MAX_BENCH_OPS 103


89 + 12 = 101?

Regardless, this really should be parameterized and not up to the developer to update each time...sigh

bigbrett · 2026-02-06T17:14:39Z

benchmark/bench_modules/wh_bench_mod_aes.c

+        if (encrypt) {
+            benchStartRet = wh_Bench_StartOp(ctx, id);
+            ret           = wc_AesCtrEncrypt(aes, out, in, inLen);
+            benchStopRet  = wh_Bench_StopOp(ctx, id);
+        }
+        else {
+            benchStartRet = wh_Bench_StartOp(ctx, id);
+            ret           = wc_AesCtrEncrypt(aes, out, in, inLen);
+            benchStopRet  = wh_Bench_StopOp(ctx, id);
+        }


do we even need separate paths for encrypt/decrypt here if they use the same underlying call?

EDIT: I see this is just the pattern carried over from existing non-DMA code. No change necessary, just pointing it out.

bigbrett · 2026-02-06T17:17:43Z

src/wh_server_crypto.c

+    }
+
+    /* Handle input data */
+    if (ret == WH_ERROR_OK && req.input.sz > 0) {


should we also check input and output sizes are multiples of WC_AES_BLOCK_SIZE like we do for non-DMA paths? Comment applies to all new DMA functions added in this file.

Consider sanity checks on all DMA input data sizes if we know what they should be.

bigbrett · 2026-02-06T17:22:45Z

src/wh_server_crypto.c

All the new handlers in this file are missing key usage policy enforcement. Including AES GCM (this is definitely a lingering bug, IIRC these two PRs landed at the same time, so might have been missed due to that). Understandable if GCM was then used as a template.

Please look at how the non-DMA functions apply key usage enforcement if used with an HSM keyId and apply. The enforcement should happen right after the requested key is read from the keystore.

This will require updating the usage flags in the new benchmark and test cases.

bigbrett · 2026-02-06T17:26:28Z

src/wh_server_crypto.c

+        keyId  = wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO,
+                                              ctx->comm->client_id, req.keyId);
+        keyLen = sizeof(tmpKey);
+        ret    = wh_Server_KeystoreReadKey(ctx, keyId, NULL, tmpKey, &keyLen);


Recommend using wh_Server_KeystoreFreshenKey() here to avoid the need for a temporary buffer. We have inconsistent usage of this function as it was introduced after a lot of crypto had initially been implemented. But this should let you obtain a pointer directly to the key in the keystore, which can then be passed to the wolfCrypt AES APIs, eliminating a copy.

Comment applies for all AES modes

bigbrett · 2026-02-06T17:27:22Z

.gitignore

 *.code-workspace
 .vscode
 compile_commands.json
+**/.cache


what is this?

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 added 3 commits February 6, 2026 17:06

Add DMA support for AES-CTR

45a9e0b

Add DMA support for AES-CBC

d1384f3

Add DMA support for AES-ECB

8f2047e

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

		uint8_t* out = info->cipher.aesctr.out;

		ret = wh_Client_AesCtrDma(ctx, aes, enc, in, len, out);

Add DMA support for remaining AES modes #282

Are you sure you want to change the base?

Add DMA support for remaining AES modes #282

Uh oh!

Conversation

Frauschi commented Feb 5, 2026

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!

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!

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

Uh oh!

3 participants