Fix TI port AES-CTR streaming and hashCopy correctness bugs - #10986
Fix TI port AES-CTR streaming and hashCopy correctness bugs#10986dgarske wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes two correctness bugs in the TI (TM4C129/TivaWare) hardware-accelerated crypto port that could silently produce incorrect output: AES-CTR streaming with partial buffered blocks, and hash context copying losing the accumulated message.
Changes:
- Fix
wc_AesCtrEncryptstreaming behavior when a follow-up chunk still doesn’t complete the buffered block (encrypt/emit bytes and advanceaes->leftwithout counter advance). - Fix TI
hashCopyto deep-copy the accumulated message buffer so copied contexts finalize correctly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
wolfcrypt/src/port/ti/ti-aes.c |
Adds missing handling for the “still not a full block” carry-over path in CTR streaming. |
wolfcrypt/src/port/ti/ti-hash.c |
Updates TI hash context copying to include buffered message data via independent allocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* copy the accumulated message into a fresh buffer so each descriptor | ||
| * owns its own allocation and can be freed independently */ | ||
| dst->used = src->used; | ||
| dst->len = src->len; | ||
| if ((src->msg != NULL) && (src->len > 0)) { | ||
| dst->msg = (byte*)XMALLOC(src->len, NULL, DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (dst->msg == NULL) | ||
| return MEMORY_E; | ||
| XMEMCPY(dst->msg, src->msg, src->len); | ||
| } | ||
| else { | ||
| dst->msg = NULL; | ||
| } |
|
Jenkins retest this please. valgrind with openssl-all |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 9 total — 3 posted, 6 skipped
Posted findings
- [Medium] hashCopy leaves dst in an inconsistent state when XMALLOC fails —
wolfcrypt/src/port/ti/ti-hash.c:132-138 - [Medium] hashCopy overwrites a pre-existing dst->msg without freeing it —
wolfcrypt/src/port/ti/ti-hash.c:130 - [Low] wc_AesCtrEncrypt(sz == 0) with buffered bytes now performs a pointless HW AES block —
wolfcrypt/src/port/ti/ti-aes.c:254
Skipped findings
- [Medium] TI port fixes are not exercised by CI; confirm on-target validation
- [Low] Copying src->len instead of src->used couples the copy to the allocator's capacity
- [Low] New CTR carry-over branch duplicates the existing tail-fragment block
- [Low] hashCopy leaves dst in an inconsistent state when the new allocation fails
- [Info] Added lines exceed the 80-column limit
- [Info] New AES-CTR carry-over branch runs a hardware AES pass for a zero-length call
Review generated by Skoll via Claude/Codex
| dst->len = 0; | ||
| /* copy the accumulated message into a fresh buffer so each descriptor | ||
| * owns its own allocation and can be freed independently */ | ||
| dst->used = src->used; |
There was a problem hiding this comment.
🟡 [Medium] hashCopy leaves dst in an inconsistent state when XMALLOC fails
💡 SUGGEST bug
dst->used and dst->len are assigned before the allocation is attempted. If XMALLOC fails, the function returns MEMORY_E with dst->msg == NULL but dst->used/dst->len set to non-zero values from src. That state is unsafe: hashGetHash()/hashFinal() on such a context calls ROM_SHAMD5DataProcess(SHAMD5_BASE, (uint32_t*)hash->msg /* NULL */, hash->used /* non-zero */, h), i.e. a DMA read from address 0, and hashUpdate() would append at hash->msg + hash->used over a freshly-allocated buffer whose first used bytes are garbage, silently producing a wrong digest. Before this PR hashCopy could not fail after the NULL check, so no caller has ever had to defend against a half-copied context. Every in-tree caller does check the return code, so this only bites under OOM plus an ignoring caller — but the fix is two lines and the invariant is worth keeping.
Suggested shape (replaces lines 132-142, ordering fix only):
if ((src->msg != NULL) && (src->len > 0)) {
dst->msg = (byte*)XMALLOC(src->len, NULL, DYNAMIC_TYPE_TMP_BUFFER);
if (dst->msg == NULL) {
dst->used = 0;
dst->len = 0;
return MEMORY_E;
}
XMEMCPY(dst->msg, src->msg, src->len);
}
else {
dst->msg = NULL;
}
dst->used = src->used;
dst->len = src->len;Suggestion: Publish dst->used/dst->len only after the allocation succeeds, and zero them on the MEMORY_E path, so a failed copy leaves dst fully reset.
| dst->msg = NULL; | ||
| dst->used = 0; | ||
| dst->len = 0; | ||
| /* copy the accumulated message into a fresh buffer so each descriptor |
There was a problem hiding this comment.
🟡 [Medium] hashCopy overwrites a pre-existing dst->msg without freeing it
💡 SUGGEST bug
The new comment states the intent is that "each descriptor owns its own allocation and can be freed independently", but dst->msg is assigned unconditionally without releasing whatever dst already owned. A dst that has been updated before the copy leaks its buffer. This is not a new leak — the previous code did dst->msg = NULL and leaked the same buffer — but the PR is precisely a rework of this ownership contract, and the contract is not complete without releasing dst. The path is live in-tree: wolfcrypt/test/test.c sha256_copy_test() does wc_Sha256Update(shaCopy, "abc", 3) and then wc_Sha256Copy(sha, shaCopy), i.e. it copies into a context that already holds an allocation. The reference implementation the PR description cites, wc_Sha256Copy() in wolfcrypt/src/port/devcrypto/devcrypto_hash.c:220, calls wc_InitSha256_ex(dst, ...) first for exactly this reason (note that a plain re-init would still leak here, since TI's hashInit only zeroes msg; TI needs hashFree(dst)).
Suggested shape (insert after the NULL check at line 129):
if (src == dst)
return 0;
/* release any buffer dst already owns before taking a new one */
XFREE(dst->msg, NULL, DYNAMIC_TYPE_TMP_BUFFER);
dst->msg = NULL;Suggestion: Free whatever dst already owns before taking a new allocation, and short-circuit the src == dst case.
| aes->left = 0; | ||
| XMEMSET(tmp, 0x0, WC_AES_BLOCK_SIZE); | ||
| } | ||
| else { |
There was a problem hiding this comment.
🔵 [Low] wc_AesCtrEncrypt(sz == 0) with buffered bytes now performs a pointless HW AES block
🔧 NIT bug
When the function is called with sz == 0 while aes->left != 0, the first branch is entered, odd is computed as sz (0), and control now falls into the new else. That runs a full AesProcess() — CCM mutex lock, ROM_AESReset, key/IV load, a 16-byte block through the HW engine — and then does XMEMCPY(out, out_block + aes->left, 0) and aes->left += 0. The result is functionally correct (nothing is written, no counter movement, state unchanged), but it is a wasted hardware round-trip and a wasted mutex acquisition on a call that previously did nothing. Reachability is narrow: no in-tree caller hits it. The zero-length calls in tests/api/test_aes.c:2447-2459 all pass at least one NULL pointer and return BAD_FUNC_ARG at the argument check, and wolfcrypt/src/evp.c:631 is only reached from evpCipherBlock() with ctx->block_size or blocks * ctx->block_size, never 0. It takes a direct wc_AesCtrEncrypt(aes, out, in, 0) from application code, which the public API permits.
Suggestion:
| else { | |
| else if (odd > 0) { |
Description
Fixes two bugs in the TI (TivaWare TM4C129 AES/SHA-MD5 hardware) port. Both silently produce wrong output rather than failing, and both trace to PR #7018.
F-3078 -
wc_AesCtrEncryptdrops partial streaming input (wolfcrypt/src/port/ti/ti-aes.c)When AES-CTR is called in streaming fashion and a follow-up chunk does not complete the buffered block (
aes->left + sz < WC_AES_BLOCK_SIZE), the leading carry-over branch buffered the new plaintext but never encrypted it, never wrote the caller's output, and never advancedaes->left. The caller saw uninitialized/stale output, and the next call overwrote the still-buffered bytes, silently losing plaintext. Fixed by adding theelsepath that mirrors the existing tail-handling block: encrypt the partial block withAES_CFG_MODE_CTR_NOCTR(no counter advance), emit the new bytes, and doaes->left += odd.F-2646 -
hashCopydiscards accumulated message (wolfcrypt/src/port/ti/ti-hash.c)hashCopyzeroedmsg/used/lenand copied only the never-populatedhashfield, so a copied context finalized to the hash of an empty message instead of the source data. Affectswc_Md5Copy,wc_ShaCopy,wc_Sha224Copy, andwc_Sha256Copy. Fixed by allocating a freshdst->msgand copyingsrc->lenbytes (independent allocation preserves the original anti-double-free intent), matching the referencewc_Sha256Copyindevcrypto_hash.c.