Skip to content

Fix Crash when using Sha224 Callback with MAX32666 #9712

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
night1rider wants to merge 3 commits into
base: master
Choose a base branch
from
+199 −13
Open

Fix Crash when using Sha224 Callback with MAX32666 #9712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6cbbb9
Improve logic behind copy and free for sha, add copy and free callbac…
night1rider Jan 22, 2026
23be39c
Add setting callback and MXC init when using arm asm with callbacks
night1rider Jan 27, 2026
de36166
Remove stdio inclusion and then revert removal of null check for MXC …
night1rider Jan 27, 2026
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
192 changes: 179 additions & 13 deletions wolfcrypt/src/port/maxim/max3266x.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,16 @@ int wc_MxcShaCryptoCb(wc_CryptoInfo* info)
int wc_MxcCryptoCb(int devIdArg, wc_CryptoInfo* info, void* ctx)
{
int ret;
#ifdef MAX3266X_SHA_CB
int savedDevId;
wc_MXC_Sha *srcMxcCtx;
wc_MXC_Sha *dstMxcCtx;
int *srcDevId;
int *dstDevId;
word32 copySize;
#endif
(void)ctx;
(void)devIdArg;

if (info == NULL) {
return BAD_FUNC_ARG;
Expand All @@ -265,6 +274,132 @@ int wc_MxcCryptoCb(int devIdArg, wc_CryptoInfo* info, void* ctx)
MAX3266X_MSG("Using MXC SHA HW Callback:");
ret = wc_MxcShaCryptoCb(info); /* Determine SHA HW or SW */
break;
case WC_ALGO_TYPE_COPY:
MAX3266X_MSG("Using MXC Copy Callback:");
if (info->copy.algo == WC_ALGO_TYPE_HASH) {
srcMxcCtx = NULL;
dstMxcCtx = NULL;
srcDevId = NULL;
dstDevId = NULL;
copySize = 0;
/* Get pointers and size based on hash type */
switch (info->copy.type) {
#ifndef NO_SHA
case WC_HASH_TYPE_SHA:
srcMxcCtx = &((wc_Sha*)info->copy.src)->mxcCtx;
dstMxcCtx = &((wc_Sha*)info->copy.dst)->mxcCtx;
srcDevId = &((wc_Sha*)info->copy.src)->devId;
dstDevId = &((wc_Sha*)info->copy.dst)->devId;
copySize = sizeof(wc_Sha);
break;
#endif
#ifdef WOLFSSL_SHA224
case WC_HASH_TYPE_SHA224:
srcMxcCtx = &((wc_Sha224*)info->copy.src)->mxcCtx;
dstMxcCtx = &((wc_Sha224*)info->copy.dst)->mxcCtx;
srcDevId = &((wc_Sha224*)info->copy.src)->devId;
dstDevId = &((wc_Sha224*)info->copy.dst)->devId;
copySize = sizeof(wc_Sha224);
break;
#endif
#ifndef NO_SHA256
case WC_HASH_TYPE_SHA256:
srcMxcCtx = &((wc_Sha256*)info->copy.src)->mxcCtx;
dstMxcCtx = &((wc_Sha256*)info->copy.dst)->mxcCtx;
srcDevId = &((wc_Sha256*)info->copy.src)->devId;
dstDevId = &((wc_Sha256*)info->copy.dst)->devId;
copySize = sizeof(wc_Sha256);
break;
#endif
#ifdef WOLFSSL_SHA384
case WC_HASH_TYPE_SHA384:
srcMxcCtx = &((wc_Sha384*)info->copy.src)->mxcCtx;
dstMxcCtx = &((wc_Sha384*)info->copy.dst)->mxcCtx;
srcDevId = &((wc_Sha384*)info->copy.src)->devId;
dstDevId = &((wc_Sha384*)info->copy.dst)->devId;
copySize = sizeof(wc_Sha384);
break;
#endif
#ifdef WOLFSSL_SHA512
case WC_HASH_TYPE_SHA512:
srcMxcCtx = &((wc_Sha512*)info->copy.src)->mxcCtx;
dstMxcCtx = &((wc_Sha512*)info->copy.dst)->mxcCtx;
srcDevId = &((wc_Sha512*)info->copy.src)->devId;
dstDevId = &((wc_Sha512*)info->copy.dst)->devId;
copySize = sizeof(wc_Sha512);
break;
#endif
default:
return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE);
}
/* Software copy */
savedDevId = *srcDevId;
XMEMCPY(info->copy.dst, info->copy.src, copySize);
*dstDevId = savedDevId;
/* Hardware copy - handles shallow copy from XMEMCPY */
ret = wc_MXC_TPU_SHA_Copy(srcMxcCtx, dstMxcCtx);
Comment on lines +335 to +340
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The WC_ALGO_TYPE_COPY handler does a raw XMEMCPY of the entire hash object and then only deep-copies the mxcCtx buffer. When WOLF_CRYPTO_CB_COPY is enabled, the generic wc_Sha224Copy/wc_Sha256Copy functions will return early after this callback and will skip their usual deep-copy steps (e.g., allocating a new dst->W under WOLFSSL_SMALL_STACK_CACHE and deep-copying msg under WOLFSSL_HASH_KEEP in wolfcrypt/src/sha256.c:2605-2612, 2636-2644, 2762-2769, 2800-2807). This can leave dst sharing pointers with src, leading to double-free/use-after-free when both contexts are freed. Suggest either (a) have the callback return CRYPTOCB_UNAVAILABLE after handling only MAX3266X-specific state so the normal copy path runs, plus add missing MAX3266X_SHA_CB mxcCtx copy into wc_Sha224Copy, or (b) update the callback to fully mirror the standard copy semantics for each supported hash type (W, msg, async ctx, etc.).

Suggested change
/* Software copy */
savedDevId = *srcDevId;
XMEMCPY(info->copy.dst, info->copy.src, copySize);
*dstDevId = savedDevId;
/* Hardware copy - handles shallow copy from XMEMCPY */
ret = wc_MXC_TPU_SHA_Copy(srcMxcCtx, dstMxcCtx);
/* Hardware copy of MAX3266X-specific SHA context.
* Return CRYPTOCB_UNAVAILABLE so core performs full
* software deep-copy (W, msg, async ctx, etc.). */
(void)srcDevId;
(void)dstDevId;
(void)copySize;
(void)savedDevId;
(void)copySize;
(void)savedDevId;
(void)srcDevId;
(void)dstDevId;
(void)copySize;
ret = wc_MXC_TPU_SHA_Copy(srcMxcCtx, dstMxcCtx);
/* Signal to use standard copy path for software state. */
ret = WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE);

Copilot uses AI. Check for mistakes.
}
else {
ret = WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE);
}
break;
case WC_ALGO_TYPE_FREE:
MAX3266X_MSG("Using MXC Free Callback:");
if (info->free.algo == WC_ALGO_TYPE_HASH) {
dstMxcCtx = NULL;
dstDevId = NULL;
copySize = 0;
/* Get pointers and size based on hash type */
switch (info->free.type) {
#ifndef NO_SHA
case WC_HASH_TYPE_SHA:
dstMxcCtx = &((wc_Sha*)info->free.obj)->mxcCtx;
dstDevId = &((wc_Sha*)info->free.obj)->devId;
copySize = sizeof(wc_Sha);
break;
#endif
#ifdef WOLFSSL_SHA224
case WC_HASH_TYPE_SHA224:
dstMxcCtx = &((wc_Sha224*)info->free.obj)->mxcCtx;
dstDevId = &((wc_Sha224*)info->free.obj)->devId;
copySize = sizeof(wc_Sha224);
break;
#endif
#ifndef NO_SHA256
case WC_HASH_TYPE_SHA256:
dstMxcCtx = &((wc_Sha256*)info->free.obj)->mxcCtx;
dstDevId = &((wc_Sha256*)info->free.obj)->devId;
copySize = sizeof(wc_Sha256);
break;
#endif
#ifdef WOLFSSL_SHA384
case WC_HASH_TYPE_SHA384:
dstMxcCtx = &((wc_Sha384*)info->free.obj)->mxcCtx;
dstDevId = &((wc_Sha384*)info->free.obj)->devId;
copySize = sizeof(wc_Sha384);
break;
#endif
#ifdef WOLFSSL_SHA512
case WC_HASH_TYPE_SHA512:
dstMxcCtx = &((wc_Sha512*)info->free.obj)->mxcCtx;
dstDevId = &((wc_Sha512*)info->free.obj)->devId;
copySize = sizeof(wc_Sha512);
break;
#endif
default:
return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE);
}
/* Hardware free */
wc_MXC_TPU_SHA_Free(dstMxcCtx);
/* Software free */
*dstDevId = INVALID_DEVID;
ForceZero(info->free.obj, copySize);
ret = 0;
Comment on lines +392 to +397
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The WC_ALGO_TYPE_FREE handler frees only the MAX3266X mxcCtx and then ForceZero()s the whole hash object and returns success. With WOLF_CRYPTO_CB_FREE enabled, wc_Sha224Free/wc_Sha256Free will return immediately after a successful callback (see wolfcrypt/src/sha256.c:2330-2333 and 2407-2409), skipping their normal cleanup (e.g., freeing sha224/sha256->W under WOLFSSL_SMALL_STACK_CACHE, async ctx, HASH_KEEP msg, etc.). This causes leaks and can leave hardware/software state inconsistent. A safer pattern is to free only the MAX3266X-specific allocations (mxcCtx), set obj->devId to INVALID_DEVID if needed, and return CRYPTOCB_UNAVAILABLE so the standard Free path runs (mxcCtx free is idempotent if msg is NULL). Alternatively, the callback must explicitly duplicate the full standard free logic for each hash type.

Suggested change
/* Hardware free */
wc_MXC_TPU_SHA_Free(dstMxcCtx);
/* Software free */
*dstDevId = INVALID_DEVID;
ForceZero(info->free.obj, copySize);
ret = 0;
/* Hardware free: release MAX3266X-specific SHA context. */
wc_MXC_TPU_SHA_Free(dstMxcCtx);
/* Mark device as no longer using hardware so SW free can run. */
if (dstDevId != NULL) {
*dstDevId = INVALID_DEVID;
}
/* Return unavailable so the standard software Free path runs and
* performs any remaining cleanup (buffers, async state, etc.). */
ret = WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE);

Copilot uses AI. Check for mistakes.
}
else {
ret = WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE);
}
break;
#endif /* MAX3266X_SHA_CB */
default:
MAX3266X_MSG("Callback not support with MXC, using SW");
Expand Down Expand Up @@ -708,18 +843,42 @@ int wc_MXC_TPU_SHA_Copy(wc_MXC_Sha* src, wc_MXC_Sha* dst)
if (src == NULL || dst == NULL) {
return BAD_FUNC_ARG;
}
dst->used = src->used;
dst->size = src->size;
if (dst->msg == src->msg && src->msg != 0) {
/* Allocate new memory for dst->msg if it points to the same location */
/* as src->msg */
dst->msg = (unsigned char*)XMALLOC(src->size, NULL,
DYNAMIC_TYPE_TMP_BUFFER);
if (dst->msg == NULL) {
return MEMORY_E; /* Handle memory allocation failure */

/* Handle case where src has no data */
if (src->msg == NULL || src->size == 0) {
/* Free dst if it has different data, then zero it */
Copy link
Contributor

@JacobBarthelmeh JacobBarthelmeh Jan 27, 2026

Choose a reason for hiding this comment

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

The comment says "free then zero" but it looks like it conditionally frees here if not null and pointers are not the same, or it zeros if NULL or the pointers are the same.

if (dst->msg != NULL && dst->msg != src->msg) {
wc_MXC_TPU_SHA_Free(dst);
}
else {
dst->msg = NULL;
dst->used = 0;
dst->size = 0;
}
return 0;
}
XMEMCPY(dst->msg, src->msg, src->size);

/* Only free dst if it points to different memory than src */
if (dst->msg != NULL && dst->msg != src->msg) {
wc_MXC_TPU_SHA_Free(dst);
}
else {
/* Reset dst without freeing (would free src's buffer) */
dst->msg = NULL;
dst->used = 0;
dst->size = 0;
}

/* Allocate new buffer for dst */
dst->msg = (unsigned char*)XMALLOC(src->size, NULL,
DYNAMIC_TYPE_TMP_BUFFER);
if (dst->msg == NULL) {
return MEMORY_E;
}

XMEMCPY(dst->msg, src->msg, src->used);
dst->used = src->used;
dst->size = src->size;
return 0;
}

Expand All @@ -729,10 +888,17 @@ void wc_MXC_TPU_SHA_Free(wc_MXC_Sha* hash)
{
if (hash == NULL) {
return; /* Hash Struct is Null already, dont edit potentially */
/* undefined memory */
/* undefined memory by accident */
}
Comment on lines 890 to 892
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Minor: comment grammar/spelling — "dont" should be "don't", and the sentence can be simplified for clarity (e.g., avoid "by accident").

Copilot uses AI. Check for mistakes.
XFREE(hash->msg, NULL, DYNAMIC_TYPE_TMP_BUFFER);
wc_MXC_TPU_SHA_Init(hash); /* sets hash->msg to null + zero's attributes */
/* Securely zero the buffer before freeing */
if (hash->msg != NULL) {
Copy link
Contributor

@JacobBarthelmeh JacobBarthelmeh Jan 27, 2026

Choose a reason for hiding this comment

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

Why remove the null check on hash before accessing hash->msg?

ForceZero(hash->msg, hash->size);
XFREE(hash->msg, NULL, DYNAMIC_TYPE_TMP_BUFFER);
}
/* Reset struct members to initial state */
hash->msg = NULL;
hash->used = 0;
hash->size = 0;
return;
}

Expand Down
18 changes: 18 additions & 0 deletions wolfcrypt/src/sha256.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,19 @@ int wc_InitSha256_ex(wc_Sha256* sha256, void* heap, int devId)
return ret;

sha256->heap = heap;
#ifdef WOLF_CRYPTO_CB
sha256->devId = devId;
sha256->devCtx = NULL;
#else
(void)devId;
#endif

#ifdef MAX3266X_SHA_CB
ret = wc_MXC_TPU_SHA_Init(&(sha256->mxcCtx));
if (ret != 0) {
return ret;
}
#endif

#ifdef WOLFSSL_SMALL_STACK_CACHE
sha256->W = NULL;
Expand Down Expand Up @@ -2138,6 +2150,12 @@ static WC_INLINE int Transform_Sha256_Len(wc_Sha256* sha256, const byte* data,
sha224->devId = devId;
sha224->devCtx = NULL;
#endif
#ifdef MAX3266X_SHA_CB
ret = wc_MXC_TPU_SHA_Init(&(sha224->mxcCtx));
if (ret != 0) {
return ret;
}
#endif
#if defined(WOLFSSL_USE_ESP32_CRYPT_HASH_HW)
#if defined(NO_WOLFSSL_ESP32_CRYPT_HASH_SHA224)
/* We know this is a fresh, uninitialized item, so set to INIT */
Expand Down
2 changes: 2 additions & 0 deletions wolfssl/wolfcrypt/port/maxim/max3266x.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
/* Some extra conditions when using callbacks */
#if defined(WOLF_CRYPTO_CB)
#define MAX3266X_CB
#define WOLF_CRYPTO_CB_COPY /* Enable copy callback for deep copy */
#define WOLF_CRYPTO_CB_FREE /* Enable free callback for proper cleanup */
Comment on lines +36 to +37
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Defining WOLF_CRYPTO_CB_COPY and WOLF_CRYPTO_CB_FREE here changes behavior of the generic hash Copy/Free APIs: when enabled, wc_Sha224Copy/wc_Sha256Copy and wc_Sha224Free/wc_Sha256Free will call the CryptoCb copy/free hooks first and will return early when the callback reports success. The MAX3266X callback implementation added in wolfcrypt/src/port/maxim/max3266x.c does not replicate the normal deep-copy/free logic (e.g., WOLFSSL_SMALL_STACK_CACHE allocates a fresh dst->W in wc_Sha224Copy at wolfcrypt/src/sha256.c:2605-2612 and wc_Sha256Copy at 2762-2769; WOLFSSL_HASH_KEEP deep-copies msg at 2636-2644 / 2800-2807; frees also release these). As a result, enabling these macros for MAX3266X can introduce shared pointers, double-frees, and leaks. Consider not enabling these macros globally for the port; instead, add MAX3266X_SHA_CB-specific deep-copy/free handling inside the SHA224/SHA* Copy/Free implementations (like SHA256 already does for mxcCtx), or fully implement the same semantics in the callback.

Suggested change
#define WOLF_CRYPTO_CB_COPY /* Enable copy callback for deep copy */
#define WOLF_CRYPTO_CB_FREE /* Enable free callback for proper cleanup */
/*
* Do NOT define WOLF_CRYPTO_CB_COPY / WOLF_CRYPTO_CB_FREE here.
* Those macros change generic hash Copy/Free behavior and require
* callback implementations that fully mirror the normal deep-copy/free
* semantics. They must only be enabled in code that provides that logic.
*/

Copilot uses AI. Check for mistakes.
#ifdef MAX3266X_MATH
#error Cannot have MAX3266X_MATH and MAX3266X_CB
#endif
Expand Down