aes.c: extend GCM H-skip guard to cover WOLF_CRYPTO_CB_SETKEY#10580
Open
MarkAtwood wants to merge 1 commit into
Open
aes.c: extend GCM H-skip guard to cover WOLF_CRYPTO_CB_SETKEY#10580MarkAtwood wants to merge 1 commit into
MarkAtwood wants to merge 1 commit into
Conversation
The existing guard in wc_AesGcmSetKey at aes.c:7855 says "if the SE owns the key, skip software-side H subkey and M-table generation," gated on WOLF_CRYPTO_CB_AES_SETKEY. Add WOLF_CRYPTO_CB_SETKEY to the same guard so generic SetKey cryptocb users get the same treatment. Without this change, a port that registers a generic SetKey callback (WOLF_CRYPTO_CB_SETKEY) and successfully imports an AES key into a secure element returns success from wc_AesSetKey without populating the software key schedule. wc_AesGcmSetKey then calls wc_AesEncrypt(aes, iv, aes->gcm.H) which checks 'r > 7 || r == 0' on aes->rounds and bails with KEYUSAGE_E (-226). Result: AES-GCM cannot be offloaded via the generic SetKey cryptocb at all. The WOLF_CRYPTO_CB_AES_SETKEY (AES-specific) path already handles this case correctly. WOLF_CRYPTO_CB_SETKEY is the newer generic dispatch (single hook for AES, HMAC, ECC, and future algorithms); any port adopting it hits the same condition for GCM and needs the same skip behavior. No behavior change for builds that don't define either macro, or for builds that only define WOLF_CRYPTO_CB_AES_SETKEY. Verified: - ./configure && make && make check 5 PASS, 4 SKIP, 0 FAIL - ./configure --enable-cryptocb CFLAGS=-DWOLF_CRYPTO_CB_SETKEY && make && make check 5 PASS, 4 SKIP, 0 FAIL - ./configure --enable-all && make && make check 22 TOTAL, 17 PASS, 5 SKIP, 0 FAIL
Contributor
Author
|
Discovered while bringing up the Caliptra cryptocb port; that work is in draft PR #10579 (held until this lands). |
Contributor
There was a problem hiding this comment.
Pull request overview
Extends the AES-GCM key-setup CryptoCB guard in wc_AesGcmSetKey() so that secure-element offload using the newer generic WOLF_CRYPTO_CB_SETKEY path skips software H/M-table generation (avoiding failure when the software key schedule is intentionally left unset).
Changes:
- Expand the existing “SE owns key, skip H/M generation” preprocessor guard from
WOLF_CRYPTO_CB_AES_SETKEYto also includeWOLF_CRYPTO_CB_SETKEY.
Comments suppressed due to low confidence (1)
wolfcrypt/src/aes.c:7859
- With WOLF_CRYPTO_CB_SETKEY builds where the SetKey callback offloads the key into a secure element (ret==0, devId valid, devCtx non-NULL), this new guard correctly skips software H/M-table generation. However, later in wc_AesGcmSetKey the raw key is still copied into aes->devKey unless WOLF_CRYPTO_CB_AES_SETKEY is defined, which leaves key material in process memory even though devCtx indicates the SE owns the key. Consider extending the later "SE owns key - don't copy to devKey" conditional to also cover WOLF_CRYPTO_CB_SETKEY so SE-owned keys aren’t persisted in aes->devKey.
#if defined(WOLF_CRYPTO_CB_AES_SETKEY) || defined(WOLF_CRYPTO_CB_SETKEY)
if ((ret == 0) && (aes->devId != INVALID_DEVID && aes->devCtx != NULL)) {
/* SE owns key - skip H and M table generation */
}
else
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+7855
to
7858
| #if defined(WOLF_CRYPTO_CB_AES_SETKEY) || defined(WOLF_CRYPTO_CB_SETKEY) | ||
| if ((ret == 0) && (aes->devId != INVALID_DEVID && aes->devCtx != NULL)) { | ||
| /* SE owns key - skip H and M table generation */ | ||
| } |
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.
aes.c: extend GCM H-skip guard to cover WOLF_CRYPTO_CB_SETKEY
What
One-line fix at
wolfcrypt/src/aes.c:7855: extend the existing"SE owns key, skip H and M-table generation" guard from
#ifdef WOLF_CRYPTO_CB_AES_SETKEYto#if defined(WOLF_CRYPTO_CB_AES_SETKEY) || defined(WOLF_CRYPTO_CB_SETKEY).Why
WOLF_CRYPTO_CB_AES_SETKEYis the older, AES-specific cryptocbmechanism.
WOLF_CRYPTO_CB_SETKEYis the newer generic SetKeydispatch — a single hook handles AES, HMAC, ECC, and any future
algorithm via
WC_SETKEY_AES,WC_SETKEY_HMAC, etc. tags(
cryptocb.c:2615,cryptocb.h::wc_SetKeyType).For AES specifically, both mechanisms have identical semantics: when
the registered callback successfully imports the key into a secure
element,
wc_AesSetKeyreturns 0 without populating the software keyschedule (
aes->rounds,aes->keylen, expanded key bytes). The SEowns the key; software state is intentionally left empty.
wc_AesGcmSetKeythen needs to generate the GCM hash subkeyHand(when enabled) the M0 table. The existing path at
aes.c:7861-7900does this by calling
wc_AesEncrypt(aes, iv_zeros, aes->gcm.H). Butwc_AesEncryptchecks the rounds count ataes.c:3148and bailswith
KEYUSAGE_E(-226) ifaes->rounds == 0. The H/M-skip guard atline 7855 exists precisely to avoid this dead-end.
The bug: that guard only checks the AES-specific macro. Any port using
the generic
WOLF_CRYPTO_CB_SETKEYmechanism — the wolfSSL-recommendeddirection for new ports — falls through to the broken path and gets
-226on everywc_AesGcmSetKeycall.Discovery
I hit this while bringing up a Caliptra (CHIPS Alliance hardware Root
of Trust) cryptocb port that uses
WOLF_CRYPTO_CB_SETKEY. The Caliptraport is on a separate branch and PR; this one-line fix is the
prerequisite that should land first.
Compatibility
WOLF_CRYPTO_CB_AES_SETKEY(the existing condition still triggersvia the left operand of the
||).WOLF_CRYPTO_CB_SETKEYis defined AND acryptocb is registered AND
devId != INVALID_DEVIDANDdevCtx != NULLAND the callback returned success — i.e., exactly the casethat's currently broken.
Testing
Three configurations verified, all
0 FAIL:./configure && make check./configure --enable-cryptocb CFLAGS=-DWOLF_CRYPTO_CB_SETKEY && make check./configure --enable-all && make checkThe 4-5 SKIPs across configurations are all network/external-dep
(openssl interop, google.com TLS, etc.) — standard for offline CI.
A full end-to-end demonstration that this enables HW-offloaded GCM via
WOLF_CRYPTO_CB_SETKEYis in the Caliptra port branch (separate PR);that branch with this fix applied passes 12 caliptra subtests
including AES-GCM round-trip, KAT, and authentication-failure
detection through the SetKey-dispatched path.