Skip to content

Improve QAT AES GCM tag checking #10143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dgarske wants to merge 1 commit into
base: master
Choose a base branch
from
+13 −2
Open

Improve QAT AES GCM tag checking #10143

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions wolfcrypt/src/port/intel/quickassist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2214,9 +2214,7 @@ static void IntelQaSymCipherCallback(void *pCallbackTag, CpaStatus status,
int ret = ASYNC_OP_E;

(void)opData;
(void)verifyResult;
(void)pDstBuffer;
(void)operationType;

#ifdef QAT_DEBUG
printf("IntelQaSymCipherCallback: dev %p, type %d, status %d, "
Expand Down Expand Up @@ -2270,6 +2268,16 @@ static void IntelQaSymCipherCallback(void *pCallbackTag, CpaStatus status,

/* mark event result */
ret = 0; /* success */

/* check verify result for authenticated ciphers (GCM/CCM) */
if (operationType == CPA_CY_SYM_OP_ALGORITHM_CHAINING &&
verifyResult == CPA_FALSE) {
/* wipe output - do not return unauthenticated plaintext */
if (dev->qat.out) {
ForceZero(dev->qat.out, dev->qat.outLen);
}
Comment on lines +2276 to +2278
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

The wipe targets dev->qat.out/dev->qat.outLen rather than the callback-provided destination buffer (pDstBuffer). If dev->qat.out doesn’t exactly correspond to the buffer QAT wrote into (or if outLen differs from what was actually produced), this can fail to wipe the plaintext (data exposure) or wipe the wrong region (potential memory corruption). Prefer wiping the actual destination buffer provided to the callback (and using the actual produced length/message length associated with this operation). This also allows removing (void)pDstBuffer since it becomes used.

Suggested change
if (dev->qat.out) {
ForceZero(dev->qat.out, dev->qat.outLen);
}
if (pDstBuffer && pDstBuffer->numBuffers >= 1 &&
pDstBuffer->pBuffers &&
pDstBuffer->pBuffers->pData) {
ForceZero(pDstBuffer->pBuffers->pData, outLen);
}
if (dev->qat.out &&
dev->qat.out != pDstBuffer->pBuffers->pData) {
ForceZero(dev->qat.out, outLen);
}

Copilot uses AI. Check for mistakes.
ret = AES_GCM_AUTH_E;
Comment on lines +2272 to +2279
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

The condition operationType == CPA_CY_SYM_OP_ALGORITHM_CHAINING is broader than the comment implies; algorithm chaining can be used for modes beyond GCM/CCM (e.g., cipher+HMAC flows). As written, any chaining verify failure will be surfaced as AES_GCM_AUTH_E, which is potentially the wrong error code and can misclassify failures. Consider checking the actual auth/cipher mode configured for the session (GCM vs CCM vs other) and mapping to the appropriate verification error (or a generic auth-failure error) rather than hard-coding AES_GCM_AUTH_E for all chaining verify failures.

Suggested change
/* check verify result for authenticated ciphers (GCM/CCM) */
if (operationType == CPA_CY_SYM_OP_ALGORITHM_CHAINING &&
verifyResult == CPA_FALSE) {
/* wipe output - do not return unauthenticated plaintext */
if (dev->qat.out) {
ForceZero(dev->qat.out, dev->qat.outLen);
}
ret = AES_GCM_AUTH_E;
/* check verify result for authenticated algorithm chaining */
if (operationType == CPA_CY_SYM_OP_ALGORITHM_CHAINING &&
verifyResult == CPA_FALSE) {
/* wipe output - do not return unauthenticated plaintext */
if (dev->qat.out) {
ForceZero(dev->qat.out, dev->qat.outLen);
}
ret = MAC_CMP_FAILED_E;

Copilot uses AI. Check for mistakes.
}
}
}

Expand Down Expand Up @@ -2399,6 +2407,9 @@ static int IntelQaSymCipher(WC_ASYNC_DEV* dev, byte* out, const byte* in,
setup.hashSetupData.authModeSetupData.aadLenInBytes = authInSz;

setup.digestIsAppended = CPA_TRUE;
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

setup.verifyDigest is only conditionally assigned here. If setup isn’t guaranteed to be fully zero-initialized on all paths before this block, verifyDigest could be left with an indeterminate/stale value for encrypt operations. To make the behavior deterministic and self-contained, explicitly set setup.verifyDigest = CPA_FALSE in the non-decrypt case (or set it unconditionally before the if).

Suggested change
setup.digestIsAppended = CPA_TRUE;
setup.digestIsAppended = CPA_TRUE;
setup.verifyDigest = CPA_FALSE;

Copilot uses AI. Check for mistakes.
if (cipherDirection == CPA_CY_SYM_CIPHER_DIRECTION_DECRYPT) {
setup.verifyDigest = CPA_TRUE;
}
}

/* open session */
Expand Down
Loading