Fix Crash when using Sha224 Callback with MAX32666 #9712

night1rider · 2026-01-24T02:29:56Z

Found that when using sha224 with callbacks on program/system would crash due to missing initialization.

Update MAX32666 to use Copy and Free Callbacks as well, with some general improvement to copy and free logic.

Crash bug is present in v5.8.4, but not previous stable v5.8.2. Was found when adding #9070 specifically commit 07b3695 which added callback for sha224.

Also found that when commit 9c1462a entered, there was no initialization for the callback structure and the MXC init call when the new wc_sha256_init_ex was added.

…k functions, fix sha224 crashing when using callbacks for MAX32666 due to unitialized struct.

night1rider · 2026-01-26T20:34:02Z

Jenkins retest this please

Jenkins disconnect from server during PRB-generic-config-parser

night1rider · 2026-01-27T02:39:26Z

Jenkins retest this please

JacobBarthelmeh · 2026-01-27T20:31:45Z

wolfcrypt/src/port/maxim/max3266x.c


 #include <stdint.h>
 #include <stdarg.h>
+#include <stdio.h>


Is the stdio include really needed here?

JacobBarthelmeh · 2026-01-27T20:36:24Z

wolfcrypt/src/port/maxim/max3266x.c

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


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.

JacobBarthelmeh · 2026-01-27T20:38:22Z

wolfcrypt/src/port/maxim/max3266x.c

-        return; /* Hash Struct is Null already, dont edit potentially */
-                /* undefined memory */
+    /* Securely zero the buffer before freeing */
+    if (hash->msg != NULL) {


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

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…free

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T22:37:15Z

wolfssl/wolfcrypt/port/maxim/max3266x.h

+    #define WOLF_CRYPTO_CB_COPY /* Enable copy callback for deep copy */
+    #define WOLF_CRYPTO_CB_FREE /* Enable free callback for proper cleanup */


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 · 2026-01-27T22:37:16Z

wolfcrypt/src/port/maxim/max3266x.c

+                /* 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);


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 · 2026-01-27T22:37:16Z

wolfcrypt/src/port/maxim/max3266x.c

+                /* Hardware free */
+                wc_MXC_TPU_SHA_Free(dstMxcCtx);
+                /* Software free */
+                *dstDevId = INVALID_DEVID;
+                ForceZero(info->free.obj, copySize);
+                ret = 0;


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 · 2026-01-27T22:37:16Z

wolfcrypt/src/port/maxim/max3266x.c

        return; /* Hash Struct is Null already, dont edit potentially */
-                /* undefined memory */
+                /* undefined memory by accident */
    }


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

Improve logic behind copy and free for sha, add copy and free callbac…

d6cbbb9

…k functions, fix sha224 crashing when using callbacks for MAX32666 due to unitialized struct.

Add setting callback and MXC init when using arm asm with callbacks

23be39c

night1rider assigned wolfSSL-Bot Jan 27, 2026

JacobBarthelmeh requested changes Jan 27, 2026

View reviewed changes

JacobBarthelmeh requested a review from Copilot January 27, 2026 20:40

JacobBarthelmeh assigned night1rider and unassigned wolfSSL-Bot Jan 27, 2026

Copilot started reviewing on behalf of JacobBarthelmeh January 27, 2026 20:41 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

Remove stdio inclusion and then revert removal of null check for MXC …

de36166

…free

JacobBarthelmeh requested a review from Copilot January 27, 2026 22:32

Copilot started reviewing on behalf of JacobBarthelmeh January 27, 2026 22:32 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Crash when using Sha224 Callback with MAX32666 #9712

Fix Crash when using Sha224 Callback with MAX32666 #9712

night1rider commented Jan 24, 2026 •

edited

Loading

Uh oh!

night1rider commented Jan 26, 2026 •

edited

Loading

Uh oh!

night1rider commented Jan 27, 2026

Uh oh!

JacobBarthelmeh Jan 27, 2026

Uh oh!

JacobBarthelmeh Jan 27, 2026

Uh oh!

JacobBarthelmeh Jan 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		#define WOLF_CRYPTO_CB_COPY /* Enable copy callback for deep copy */
		#define WOLF_CRYPTO_CB_FREE /* Enable free callback for proper cleanup */

-    #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.
+     */

-                /* 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);

Fix Crash when using Sha224 Callback with MAX32666 #9712

Are you sure you want to change the base?

Fix Crash when using Sha224 Callback with MAX32666 #9712

Conversation

night1rider commented Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

night1rider commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

night1rider commented Jan 27, 2026

Uh oh!

JacobBarthelmeh Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

JacobBarthelmeh Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

JacobBarthelmeh Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

night1rider commented Jan 24, 2026 •

edited

Loading

night1rider commented Jan 26, 2026 •

edited

Loading