Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves QAT AES-GCM/CCM decryption authenticity handling by enabling digest verification on decrypt and propagating verify failures back to the caller (including wiping decrypted output on auth failure).
Changes:
- Enable
setup.verifyDigestfor QAT decrypt operations where a digest is appended. - In the QAT symmetric cipher callback, detect authentication verify failures and return an auth error.
- Wipe decrypted output on authentication failure to avoid returning unauthenticated plaintext.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* 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; |
There was a problem hiding this comment.
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.
| /* 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; |
| if (dev->qat.out) { | ||
| ForceZero(dev->qat.out, dev->qat.outLen); | ||
| } |
There was a problem hiding this comment.
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.
| 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); | |
| } |
| @@ -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; | |||
There was a problem hiding this comment.
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).
| setup.digestIsAppended = CPA_TRUE; | |
| setup.digestIsAppended = CPA_TRUE; | |
| setup.verifyDigest = CPA_FALSE; |
Description
Improve QAT AES GCM tag checking
Fixes ZD 21457 report 12
Testing
QAT 8970 PCIe card and latest QAT driver.
Checklist