Skip to content

Fix: WolfCrypt Fenrir - 12 fixes - #10786

Open
aidankeefe2022 wants to merge 9 commits into
wolfSSL:masterfrom
aidankeefe2022:fenrir-fixes-jun24/26-ak
Open

Fix: WolfCrypt Fenrir - 12 fixes#10786
aidankeefe2022 wants to merge 9 commits into
wolfSSL:masterfrom
aidankeefe2022:fenrir-fixes-jun24/26-ak

Conversation

@aidankeefe2022

@aidankeefe2022 aidankeefe2022 commented Jun 25, 2026

Copy link
Copy Markdown
Member

Description

https://fenrir.wolfssl.com/finding/6145

wc_DsaVerify/wc_DsaVerify_ex leave *answer uninitialized on all error paths, unlike sibling ECC/ECCSI verify APIs that default to "not verified".

  • Initialized the answer parameter to zero so that on early exit the output parameter is defined as false.

https://fenrir.wolfssl.com/finding/5384

CAAM secure-memory addresses are truncated to 32 bits in ECC keys.

  • Changed the ecc_key fields to CAAM_ADDRESS when it is defined. This allows the address width to expand with the platform so addresses are not truncated.

https://fenrir.wolfssl.com/finding/4432

wc_DrbgState_MutexInit unsafe lazy mutex initialization without WOLFSSL_MUTEX_INITIALIZER.

  • Instead of checking a plain int, we use atomic operations when they are present to initialize the mutex. This ensures that no two threads can initialize the same mutex.

https://fenrir.wolfssl.com/finding/5392

DES key schedule branches on secret key bits.

  • Instead of an if statement, we use a mask to set bits in ks.

https://fenrir.wolfssl.com/finding/5994

Invalid free / use-after-free of embedded X509 NAME in the ESP32 cert-bundle verify callback on a lookup miss.

  • Confirmed and removed the double free in esp_crt_bundle.c.

https://fenrir.wolfssl.com/finding/4445

devcrypto wc_Sha256Copy produces a non-functional hash copy when WOLFSSL_DEVCRYPTO_HASH_KEEP is disabled.

  • Made the function match the rest of the file and return an error when a copy is not available. Also freed the destination before the copy to fix a memory leak.

https://fenrir.wolfssl.com/finding/4446

devcrypto wc_Sha256Final leaks the kernel hash session when GetDigest fails.

  • Added a free on error, which uncovered and fixed memory leaks caught by tests when devcrypto is enabled.

https://fenrir.wolfssl.com/finding/5418

FSPSM AES-GCM TLS key allocation failures return without unlocking hardware.

  • Added a mutex unlock on memory allocation failure.

https://fenrir.wolfssl.com/finding/5420

FSPSM hash Final/GetHash silently succeeds when hardware hash initialization fails.

  • Set ret to an error value so that on return the error is no longer silent.

https://fenrir.wolfssl.com/finding/5411

SipHash assembly paths load the caller key through word64 pointer casts.

  • Swapped multiple byte* to word64* casts to use the GET_U64() helper macro to protect against alignment issues.

https://fenrir.wolfssl.com/finding/5412

Intel RDSEED/RDRAND generators write arbitrary output buffers as word64.

  • Added a temporary word64 value, then used writeUnalignedWord64 to transfer it into the output without alignment issues.

https://fenrir.wolfssl.com/finding/5413

ML-KEM AArch64 noise helpers cast byte buffers and seeds to word64 pointers.

  • Used the alignment-protected helper function writeUnalignedWord64 instead of a byte* -> word64* cast.

@aidankeefe2022 aidankeefe2022 self-assigned this Jun 25, 2026
@aidankeefe2022 aidankeefe2022 changed the title https://fenrir.wolfssl.com/finding/6145 Fix: WolfCrypt Fenrir - 6145, 5384, 4432, 5392 Jun 25, 2026
@aidankeefe2022
aidankeefe2022 force-pushed the fenrir-fixes-jun24/26-ak branch from 9046e08 to 8968849 Compare June 26, 2026 18:34
@aidankeefe2022
aidankeefe2022 marked this pull request as ready for review June 26, 2026 18:47
@github-actions

Copy link
Copy Markdown

retest this please

@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@aidankeefe2022 aidankeefe2022 changed the title Fix: WolfCrypt Fenrir - 6145, 5384, 4432, 5392 Fix: WolfCrypt Fenrir - 6145, 5384, 4432, 5392, 5994, 4445, 4446, 5418, 5420, 5411, 5412, 5413 Jun 26, 2026
@aidankeefe2022
aidankeefe2022 force-pushed the fenrir-fixes-jun24/26-ak branch 3 times, most recently from f0db333 to 60b22a5 Compare June 26, 2026 21:10
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@aidankeefe2022 aidankeefe2022 changed the title Fix: WolfCrypt Fenrir - 6145, 5384, 4432, 5392, 5994, 4445, 4446, 5418, 5420, 5411, 5412, 5413 Fix: WolfCrypt Fenrir - 12 fixes Jun 26, 2026
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

1 similar comment
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@aidankeefe2022

Copy link
Copy Markdown
Member Author

rebased branch on to master

Comment thread wolfcrypt/src/port/devcrypto/devcrypto_hash.c Outdated
@aidankeefe2022
aidankeefe2022 force-pushed the fenrir-fixes-jun24/26-ak branch from 7a668b2 to f475fe0 Compare July 1, 2026 21:42
@Frauschi

Frauschi commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Jenkins retest this please.

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

  • [High] wc_Sha256Copy frees a zero-initialized dst and closes fd 0 (devcrypto)wolfcrypt/src/port/devcrypto/devcrypto_hash.c:231
  • [Medium] wc_Sha256Copy leaks the just-opened session on XMALLOC failurewolfcrypt/src/port/devcrypto/devcrypto_hash.c:238-241

Review generated by Skoll via Claude/Codex

Comment thread wolfcrypt/src/port/devcrypto/devcrypto_hash.c
Comment thread wolfcrypt/src/port/devcrypto/devcrypto_hash.c Outdated
@aidankeefe2022

Copy link
Copy Markdown
Member Author

retest this please

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Skoll Multi-Scan Review

Modes: review + review-securityOverall recommendation: REQUEST_CHANGES
Findings: 6 total — 6 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

  • [High] [review-security] ecc.h now includes wolfcaam.h before the ecc_key typedef, creating a circular include that breaks CAAM buildswolfssl/wolfcrypt/ecc.h:79-81
  • [Medium] [review-security] devcrypto wc_Sha256Copy frees the destination before initializing it, reading an uninitialized dst (arbitrary fd close / invalid free)wolfcrypt/src/port/devcrypto/devcrypto_hash.c:226-244
  • [Medium] [review] wc_Sha256Copy now returns NOT_COMPILED_IN without WOLFSSL_DEVCRYPTO_HASH_KEEPwolfcrypt/src/port/devcrypto/devcrypto_hash.c:245-252
  • [Low] [review+review-security] DRBG mutex init/free state-machine can busy-spin/hang (no CPU yield; non-volatile flag in non-atomic fallback)wolfcrypt/src/random.c:388-479
  • [Low] [review] New WC_CRYPTODEV.inited bitfield inserted mid-struct; word8 bitfield typewolfssl/wolfcrypt/port/devcrypto/wc_devcrypto.h:41
  • [Info] [review] Unreachable return after infinite loop in wc_DrbgState_MutexFreewolfcrypt/src/random.c:475

Review generated by Skoll

Comment thread wolfssl/wolfcrypt/ecc.h Outdated
#endif


#if defined(WOLFSSL_CAAM)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 [High] ecc.h now includes wolfcaam.h before the ecc_key typedef, creating a circular include that breaks CAAM builds · Regression

The PR moved #include \<wolfssl/wolfcrypt/port/caam/wolfcaam.h> out of ecc.c and INTO ecc.h at line 80, so that CAAM_ADDRESS is visible when struct ecc_key (line 502) is defined. However, the include is placed BEFORE the typedef struct ecc_key ecc_key; forward typedef (ecc.h line 448). For WOLFSSL_QNX_CAAM / WOLFSSL_SECO_CAAM / WOLFSSL_IMXRT1170_CAAM builds this creates a circular include with a fatal ordering problem: ecc.h(guard set) -> wolfcaam.h -> wolfcaam_qnx.h (defines CAAM_ADDRESS, then includes) -> wolfcaam_ecdsa.h -> #include ecc.h (guard already set, so SKIPPED) -> wolfcaam_ecdsa.h then declares prototypes such as wc_CAAM_EccVerify(..., ecc_key* key, ...) (lines 31-39). At that point ecc_key is NOT yet defined (we are still inside ecc.h at line 80, and the typedef is at line 448), producing a hard compile error "unknown type name 'ecc_key'". BEFORE the PR, ecc.c included wolfcaam.h only AFTER ecc.h had been fully processed, so ecc_key was already fully defined and the circular re-include was a harmless no-op. This regression breaks compilation of exactly the CAAM configurations the PR is intended to fix, for any translation unit that includes ecc.h before any wolfcaam header (i.e. essentially all of them).

Fix: Do not pull in the full wolfcaam.h (which drags in wolfcaam_ecdsa.h and its ecc_key-typed prototypes) before ecc_key is declared. Either (a) move the #include wolfcaam.h to AFTER the typedef struct ecc_key ecc_key; (line 448) but still before struct ecc_key { (line 502), or (b) include only the lightweight porting header that defines CAAM_ADDRESS (e.g. wolfcaam_qnx.h / wolfcaam_fsl_nxp.h / wolfcaam_seco.h) rather than the umbrella wolfcaam.h. Verify a clean build of WOLFSSL_QNX_CAAM / WOLFSSL_IMXRT1170_CAAM.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

}

wc_InitSha256_ex(dst, src->heap, 0);
#ifdef WOLFSSL_DEVCRYPTO_HASH_KEEP

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] devcrypto wc_Sha256Copy frees the destination before initializing it, reading an uninitialized dst (arbitrary fd close… · Security

BEFORE, devcrypto wc_Sha256Copy called wc_InitSha256_ex(dst, ...) FIRST, which does XMEMSET(sha, 0, sizeof(wc_Sha256)) and is therefore safe on an uninitialized/garbage dst. AFTER, the function calls wc_Sha256Free(dst) as its very first action (to fix a leak of a pre-existing dst session/msg). For WOLFSSL_DEVCRYPTO_HASH, wc_Sha256Free executes wc_DevCryptoFree(&dst->ctx) -> if the garbage dst->ctx.inited bit reads as 1 it runs ioctl(dst->ctx.cfd, ...) and close(dst->ctx.cfd) on a garbage descriptor (closing an unrelated live fd/socket in the process), and for WOLFSSL_DEVCRYPTO_HASH_KEEP it runs ForceZero(dst->msg, dst->len) then XFREE(dst->msg) on a garbage pointer/length (arbitrary write + invalid free / heap corruption). This changes the long-standing wc_XxxCopy contract ("the copy methods overwrite the entire dest low level hash struct"), which the generic sha256.c backend still honors via XMEMCPY(dst, src, sizeof) without touching dst first. wc_Sha256Copy is a public WOLFSSL_API. The one internal caller that could hit this (wc_Sha256GetHash) was patched in the same diff with XMEMSET(&cpy, 0, sizeof(cpy)), and the in-tree SHA-256 call sites zero dst first, so in-tree exploitability is low; but any external consumer that relied on the previous "copy overwrites everything" behavior and passed an uninitialized dst now triggers undefined behavior.

Fix: Zero/initialize dst before freeing it, or free the prior contents conditionally. Simplest: XMEMSET(dst, 0, sizeof(*dst)); (or restore the original order: init dst first) and only free a caller-supplied dst when the API contract explicitly guarantees dst is already initialized. Alternatively, document and enforce that dst must be a zeroed/initialized object and update the sha256.h contract for all backends.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All other wc_ShaXXXCopy functions assume the dst struct that does not have garbage data. This is not documented but before copy the dst is freed and the free function checks whether each allocation is != NULL before freeing. This change to copy in devcrypto is mirroring this behavior. I will add a documentation so this is not a surprise.


return 0;
return ret;
#else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] wc_Sha256Copy now returns NOT_COMPILED_IN without WOLFSSL_DEVCRYPTO_HASH_KEEP · api

When WOLFSSL_DEVCRYPTO_HASH_KEEP is not defined, wc_Sha256Copy now returns NOT_COMPILED_IN instead of silently succeeding. This is the intended fix (the previous copy produced a non-functional/empty context), and failing loudly is correct. However, it is a behavior change for a public API: any consumer that copies a running SHA-256 state in a devcrypto-hash build without HASH_KEEP (e.g. TLS transcript-hash duplication, PRF, wc_HashCopy) will now hard-fail rather than silently misbehave. This should be intentional and documented so integrators enable WOLFSSL_DEVCRYPTO_HASH_KEEP when copy-dependent operations (TLS) are used with devcrypto hashing.

Fix: Confirm this is intended and note in the devcrypto documentation/config that WOLFSSL_DEVCRYPTO_HASH_KEEP is required whenever hash-copy (e.g. TLS) is needed; consider a build-time guard/warning to catch the misconfiguration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

WOLFSSL_DEVCRYPTO_HASH_KEEP is only not included when ENABLED_CRYPTONLY is set to yes this makes it impossible to have the TLS issue. This also matches the pattern that was present in the port.

Comment thread wolfcrypt/src/random.c
wc_DrbgState_Mutex_Inited
};

#ifndef SINGLE_THREADED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] DRBG mutex init/free state-machine can busy-spin/hang (no CPU yield; non-volatile flag in non-atomic fallback) · Logic

Both modes flagged the new CAS-based mutex init/free state machine. review view (SUGGEST/Low): the for(;;) loops busy-wait on a competing thread's InitProgress/FreeProgress state using a bare continue with no sched_yield/pause/relax hint; the critical section is short so this is benign under preemptive multitasking, but on a single-core cooperatively-scheduled RTOS the spinning thread could starve the owner that must transition the state (potential livelock). review-security view (CWE-835/Low): the state machine uses wolfSSL_Atomic_Int_CompareExchange/Exchange unconditionally, but in the #else (no WOLFSSL_ATOMIC_OPS) path the backing variable is a plain static int drbgStateMutex_inited = 0; and the atomic calls resolve to non-atomic fallbacks; in that narrow config (multi-threaded, no atomic ops, no WOLFSSL_MUTEX_INITIALIZER) a losing thread spins reading a NON-volatile global with no memory barrier, so the compiler may cache the flag and the winner's Inited publish may never be observed, turning the previous benign double-init race into a potential spin/hang. The common WOLFSSL_ATOMIC_OPS path is correct.

Fix: Guard the atomic state-machine body with #ifdef WOLFSSL_ATOMIC_OPS and keep a simple if (!inited){init; inited=1;} (or a real mutex) for the non-atomic fallback, or at minimum make the fallback flag volatile. Additionally consider a yield/relax hint (or bounded backoff) in the spin path, or document that these routines assume a preemptive scheduler when WOLFSSL_ATOMIC_OPS is used without WOLFSSL_MUTEX_INITIALIZER. Confirm the intended set of build configs for this code path.

@aidankeefe2022 aidankeefe2022 Jul 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a Yeild macro to wc_port.h that resolves to supported platforms thread yield equivalent. This allows us to yeild when a thread does not win CAS and the mutex is in a transition state instead of spin.

If the macro is not defined or atomics are not avaible then the mutex init/free funcs revert to using a volatile int.


typedef struct WC_CRYPTODEV {
int cfd;
word8 inited : 1;/* is this object initialized (1) or not (0) */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] New WC_CRYPTODEV.inited bitfield inserted mid-struct; word8 bitfield type · convention

The new inited bit-field is inserted between cfd and sess rather than appended at the end of the struct. wolfSSL convention prefers adding new members at the end (mid-struct insertion changes offsets/ABI). Since WC_CRYPTODEV is internal and always recompiled with its embedders (Aes/Hmac/wc_Sha256/RsaKey), this is not a functional problem, only a convention/ABI-hygiene note. Additionally the documented bit-field style is unsigned int flag : 1;; a word8 : 1 bit-field is accepted by GCC/Clang but non-int bit-field types can warn under strict/portable settings (e.g. some MSVC/-Wpedantic configs).

Fix: Optionally move inited to the end of the struct and/or use unsigned int inited : 1; to match the documented convention.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This placement is intentional to take advantage of preexisting padding in the struct. The new field does not increase the size of the struct when it is placed here. Also changed the type to unsigned int, the struct had space for it so no increase in size from that!

Comment thread wolfcrypt/src/random.c
continue;
}

return 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚪ [Info] Unreachable return after infinite loop in wc_DrbgState_MutexFree · style

The for(;;) loop never falls through (every path returns or continues), so the return 0; immediately following it (line 475) is dead code and can trigger -Wunreachable-code under strict warning settings. wc_DrbgState_MutexInit does not have this issue because its loop is the last statement before the #endif.

Fix: Drop the redundant return 0; inside the #ifndef WOLFSSL_MUTEX_INITIALIZER block (the trailing return 0; after the #endifs already covers the other configurations).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

@aidankeefe2022
aidankeefe2022 force-pushed the fenrir-fixes-jun24/26-ak branch 2 times, most recently from 7b66811 to 1c8cade Compare July 15, 2026 19:18
@aidankeefe2022

Copy link
Copy Markdown
Member Author

retest this please

@Frauschi

Copy link
Copy Markdown
Contributor

Jenkins retest this please

1 similar comment
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@aidankeefe2022
aidankeefe2022 force-pushed the fenrir-fixes-jun24/26-ak branch from 30d05a6 to 2b3617a Compare July 16, 2026 17:02
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

1 similar comment
@aidankeefe2022

aidankeefe2022 commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Jenkins retest this please

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a set of WolfCrypt correctness, safety, and side-channel findings reported by Fenrir across multiple platform ports and crypto primitives.

Changes:

  • Harden thread-safety and output determinism (e.g., DRBG mutex lazy-init, DSA verify default output).
  • Fix portability and memory-safety issues (unaligned loads/stores in SipHash/IntelRD/ML-KEM noise helpers; ESP32 cert-bundle double-free; devcrypto session lifecycle leaks).
  • Improve platform correctness (CAAM secure-memory address width; Renesas FSPSM error handling and unlock-on-failure paths).

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
wolfssl/wolfcrypt/wc_port.h Adds a cross-platform thread-yield macro for spin-wait loops.
wolfssl/wolfcrypt/settings.h Forces WOLFSSL_CAAM on for SECO/IMXRT1170 CAAM configs.
wolfssl/wolfcrypt/port/devcrypto/wc_devcrypto.h Adds an inited flag to track devcrypto context initialization.
wolfssl/wolfcrypt/port/caam/wolfcaam.h Centralizes CAAM address type via new caam_type.h.
wolfssl/wolfcrypt/port/caam/wolfcaam_seco.h Switches CAAM address type definition to shared header.
wolfssl/wolfcrypt/port/caam/wolfcaam_qnx.h Switches CAAM address type definition to shared header.
wolfssl/wolfcrypt/port/caam/wolfcaam_fsl_nxp.h Switches CAAM address type definition to shared header.
wolfssl/wolfcrypt/port/caam/caam_type.h Introduces a single authoritative CAAM_ADDRESS definition.
wolfssl/wolfcrypt/port/caam/caam_qnx.h Uses shared CAAM address type definition.
wolfssl/wolfcrypt/include.am Installs the new CAAM type header when CAAM is built.
wolfssl/wolfcrypt/ecc.h Uses CAAM_ADDRESS for ecc_key CAAM address fields.
wolfcrypt/src/wc_mlkem_poly.c Removes unaligned word64* casts; uses safe helpers and explicit copies/zeroing.
wolfcrypt/src/siphash.c Replaces unaligned word64* key loads with GET_U64() helper usage.
wolfcrypt/src/random.c Adds atomic, state-machine-based lazy init/free for DRBG mutex; hardens Intel RDSEED/RDRAND writes.
wolfcrypt/src/port/Renesas/renesas_fspsm_sha.c Ensures FSPSM hash finalization returns an error on init failure (no silent success).
wolfcrypt/src/port/Renesas/renesas_fspsm_aes.c Fixes missing hardware unlock on TLS AES-GCM key allocation failure.
wolfcrypt/src/port/Espressif/esp_crt_bundle/esp_crt_bundle.c Removes double-free of embedded X509 NAME pointers on lookup miss.
wolfcrypt/src/port/devcrypto/wc_devcrypto.c Marks devcrypto contexts initialized and gates free on that state.
wolfcrypt/src/port/devcrypto/devcrypto_rsa.c Removes manual cfd=-1 pre-init in RSA paths (relies on centralized init/state).
wolfcrypt/src/port/devcrypto/devcrypto_hmac.c Ensures devcrypto HMAC context init flag is reset before creating sessions.
wolfcrypt/src/port/devcrypto/devcrypto_hash.c Fixes devcrypto SHA-256 copy/final error cleanup and leak paths; aligns copy behavior with feature availability.
wolfcrypt/src/port/devcrypto/devcrypto_aes.c Switches devcrypto AES session creation checks to use inited flag; tightens argument validation.
wolfcrypt/src/port/caam/wolfcaam_hmac.c Uses devcrypto inited flag to decide whether to initialize HMAC context.
wolfcrypt/src/port/caam/wolfcaam_ecdsa.c Updates CAAM address handling to the widened CAAM_ADDRESS type.
wolfcrypt/src/hmac.c Initializes devcrypto HMAC context inited flag in generic HMAC init.
wolfcrypt/src/ecc.c Uses CAAM_ADDRESS for CAAM key address storage and partition read/write calls.
wolfcrypt/src/dsa.c Initializes *answer to 0 in DSA verify to ensure defined output on error paths.
wolfcrypt/src/des3.c Removes key-dependent branching in DES key schedule via mask-based bit setting.
wolfcrypt/src/aes.c Initializes devcrypto AES context inited flag in generic AES setup paths.
tests/api/test_aes.c Adjusts AES-GCM tests for devcrypto buffer sizing and excludes unsupported NonStdNonce behavior.
doc/dox_comments/header_files/sha256.h Updates SHA256 copy documentation example to use a zero-initialized destination.

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

Comment thread wolfcrypt/src/random.c
WOLFSSL_MUTEX_INITIALIZER_CLAUSE(drbgStateMutex);
#ifndef WOLFSSL_MUTEX_INITIALIZER
static int drbgStateMutex_inited = 0;
#if defined(WOLFSSL_ATOMIC_OPS) && defined(WOLFSSL_THREAD_YIELD)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a trade-off we have to make I think... it is unfortunate, I cannot think of what a "safe default yield macro" would be and it cannot be just spin because that could cause all kinds of issues in environments we do not know about.

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I'd also like a review from @douzzer regarding the changes to the random.c code and the new yield macro.

@Frauschi
Frauschi requested a review from douzzer July 20, 2026 14:25

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 33 total — 33 posted, 0 skipped
19 finding(s) posted as inline comments (see file-level comments below)
14 finding(s) not tied to a diff line (full detail below)

Posted findings

  • [Medium] wc_Sha256Copy now returns NOT_COMPILED_IN in a reachable build configurationwolfcrypt/src/port/devcrypto/devcrypto_hash.c:217-248
  • [Medium] wc_Sha256Final destroys the caller's context and loses the heap hint on error pathswolfcrypt/src/port/devcrypto/devcrypto_hash.c:157-184
  • [Medium] wc_AesSetKey clears the devcrypto context without releasing an already-open sessionwolfcrypt/src/port/devcrypto/devcrypto_aes.c:131-132
  • [Medium] Non-atomic DRBG fallback is still racy with no warning or commentwolfcrypt/src/random.c:396-398,436-446,490-499
  • [Medium] Yield-macro branch order diverges from the wolfSSL_Mutex typedef orderwolfssl/wolfcrypt/wc_port.h:781-801
  • [Medium] caam_type.h's #ifndef guard defeats the ODR guarantee the header exists to providewolfssl/wolfcrypt/port/caam/caam_type.h:22-46
  • [Low] New 'inited' bit-field inserted mid-struct in WC_CRYPTODEV rather than appendedwolfssl/wolfcrypt/port/devcrypto/wc_devcrypto.h:39-43
  • [Low] des3.c: secret-dependent ternary remains in the pc1 bit-extraction loopwolfcrypt/src/des3.c:1557-1563
  • [Low] FSPSM_HashFinal maps Init/Update failures to WC_HW_E without a WOLFSSL_ERROR tracewolfcrypt/src/port/Renesas/renesas_fspsm_sha.c:436-438
  • [Low] ecc.h now hard-requires caam_type.h, which is only installed under BUILD_CAAMwolfssl/wolfcrypt/include.am:211-224
  • [Low] caam_type.h includes stdint.h unconditionally, bypassing the NO_STDINT_H conventionwolfssl/wolfcrypt/port/caam/caam_type.h:38
  • [Low] ML-KEM aarch64 eta2 helpers add 600 and 200 bytes of stackwolfcrypt/src/wc_mlkem_poly.c:4378,4514
  • [Low] des3.c: new line exceeds the 80-column limit and will fail check-source-textwolfcrypt/src/des3.c:1576-1581
  • [Low] dsa.c: added comment misstates what WC_MIN_DIGEST_SIZE_FOR_VERIFY expands towolfcrypt/src/dsa.c:1134-1137
  • [Low] Prefer XMEMSET over the aggregate '= {0}' initializer for wc_Sha256wolfcrypt/src/port/devcrypto/devcrypto_hash.c:196
  • [Low] sha256.h doc: contract change documented only for wc_Sha256Copy, and the example leaves src uninitializeddoc/dox_comments/header_files/sha256.h:368-374
  • [Low] Renesas FSPSM AES: inconsistent condition order and argument alignment between encrypt and decryptwolfcrypt/src/port/Renesas/renesas_fspsm_aes.c:413-421,644-652
  • [Low] esp_crt_bundle.c: missing space before comment terminatorwolfcrypt/src/port/Espressif/esp_crt_bundle/esp_crt_bundle.c:985-987
  • [Info] ML-KEM: '= {0}' on the 600-byte state arrays is provably dead work on a hot pathwolfcrypt/src/wc_mlkem_poly.c:4378,4411

Findings not tied to a diff line

Incomplete fix: FSPSM_HashGet still silently succeeds when hardware hash Init() fails

File: wolfcrypt/src/port/Renesas/renesas_fspsm_sha.c:525-541
Function: FSPSM_HashGet
Severity: High

The PR description states it fixes 'FSPSM hash Final/GetHash silently succeeds when hardware hash initialization fails' (finding 5420), but only FSPSM_HashFinal was fixed. FSPSM_HashGet is declared with int ret = FSP_SUCCESS; (line 489, i.e. 0) and still uses the un-captured form if (Init(&handle) == FSP_SUCCESS) {, with no trailing if (ret != FSP_SUCCESS) ret = WC_HW_E; mapping. When Init() fails the whole block is skipped, ret stays 0, and the function returns success with the out digest buffer never written. FSPSM_HashGet backs every wc_*GetHash entry point on this port: wc_ShaGetHash (line 613), wc_Sha224GetHash (644), wc_Sha256GetHash (677), wc_Sha384GetHash (709), wc_Sha512GetHash (742), wc_Sha512_224GetHash (767), wc_Sha512_256GetHash (792). This is the exact defect class the PR set out to close, left open in half the surface it names.

Recommendation: Apply the same (ret = Init(&handle)) capture and trailing error mapping to FSPSM_HashGet that was applied to FSPSM_HashFinal, so the PR actually closes finding 5420 for both functions it names.

Referenced code: wolfcrypt/src/port/Renesas/renesas_fspsm_sha.c:525-534 (10 lines)


devcrypto AES-GCM out-of-bounds writes are masked by enlarging the unit-test buffer rather than fixed

File: wolfcrypt/src/port/devcrypto/devcrypto_aes.c:319-348, tests/api/test_aes.c:3262-3271
Function: wc_DevCrypto_AesGcm / test_wc_AesGcmEncryptDecrypt_Sizes
Severity: High

The PR enlarges the test cipher buffer by WC_AES_BLOCK_SIZE with the comment 'enlarged size is to accommodate devcrypto build which assumes space in buffer to append auth tag'. That comment correctly identifies a real out-of-bounds write which the PR then leaves in place, removing the test that was detecting it. Three overflows remain: (1) on encrypt authTagSz is unconditionally forced to WC_AES_BLOCK_SIZE (line 326), then XMEMCPY(authTag, out + sz, authTagSz) (line 347) writes a full 16 bytes into the caller's authTag — but wc_local_AesGcmCheckTagSz (wolfcrypt/src/aes.c:8037-8063) accepts 4/8/12/13/14/15/16 subject to WOLFSSL_MIN_AUTH_TAG_SZ (default 12, settings.h:3883), so a conforming caller with a 12-byte tag buffer gets a 4-byte overflow; (2) the same line reads out[sz..sz+15], requiring out to be sz+16 — a contract no other AES-GCM backend imposes and that is absent from the public docs; (3) on decrypt, XMEMCPY(in + sz, authTag, authTagSz) (line 321) writes 16 bytes past the caller's const byte* in. The PR's own commit message claims 'fixed bug in aes where the authTag was appended past the end of the cypher text', but the only change made was to the test.

Recommendation: Fix the port, not the test: bounce the tag through a local 16-byte scratch and copy back only authTagSz; use a private sz + authTagSz buffer on decrypt instead of writing into the caller's const input. Then revert the test buffer enlargement so the test keeps guarding the API contract.

Referenced code: wolfcrypt/src/port/devcrypto/devcrypto_aes.c:319-348, tests/api/test_aes.c:3262-3273 (12 lines)


New WOLFSSL_THREAD_YIELD guard disables the DRBG race fix on ~20 RTOS ports and duplicates the existing…

File: wolfcrypt/src/random.c:393,407,459; wolfssl/wolfcrypt/wc_port.h:778-801
Function: wc_DrbgState_MutexInit / wc_DrbgState_MutexFree
Severity: High

wolfSSL already has an established solution to this exact problem that the PR does not use. WC_RELAX_LONG_LOOP() is defined unconditionally in types.h:2291-2293 (defaulting to WC_DO_NOTHING) and is used by the two existing in-tree CAS spin loops in wc_port.c:303 and :389. Most tellingly, sp_c32.c:24056-24085 contains a near-identical tri-state lazy-mutex-init CAS machine for the same purpose, gated only by #ifndef WOLFSSL_MUTEX_INITIALIZER with no yield-macro requirement, using wolfSSL_Atomic_Uint_CompareExchange (which has a non-atomic inline fallback at wc_port.h:763-775) and WOLFSSL_ATOMIC_LOAD/STORE (defined even under WOLFSSL_NO_ATOMICS, wc_port.h:610-616). That version therefore compiles and runs on every platform with no #else branch at all. By contrast, the new code adds && defined(WOLFSSL_THREAD_YIELD) to its guard, and WOLFSSL_THREAD_YIELD is only defined for 7 platforms. Every port that has WOLFSSL_ATOMIC_OPS and lacks WOLFSSL_MUTEX_INITIALIZER — i.e. exactly the ports the fix is for — silently falls back to the racy volatile int path: FREERTOS_TCP, WOLFSSL_SAFERTOS, RTTHREAD, MICRIUM, WOLFSSL_DEOS, EBSNET, FREESCALE_MQX/KSDK_MQX, FREESCALE_FREE_RTOS, uITRON4, uTKERNEL2, CMSIS_RTOS/v2, MDK_ARM, MBED, TIRTOS, FROSTED, INTIME_RTOS, NUCLEUS_1_2, MYNEWT, TELIT_M2MB, EMBOS, WATCOMC/OS2, and Zephyr < 3.1. Note pthread builds define WOLFSSL_MUTEX_INITIALIZER (wc_port.h:446), so Linux/macOS never exercise this code at all — meaning the fix as written benefits almost no one. (I did verify the state machine itself is sound: all four…

Recommendation: Drop && defined(WOLFSSL_THREAD_YIELD) from both guards and call WC_RELAX_LONG_LOOP() in the spin loops, matching sp_c32.c and wc_port.c. That removes the entire non-atomic fallback branch, deletes the need for the new WOLFSSL_THREAD_YIELD macro, and makes the fix apply on every threaded port instead of seven.

Referenced code: wolfcrypt/src/random.c:393,407,459; wolfssl/wolfcrypt/wc_port.h:778-783 (6 lines)


CAAM address widening is undone downstream: the QNX driver IPC path still moves only 4 bytes

File: wolfcrypt/src/port/caam/wolfcaam_qnx.c:189,220; wolfcrypt/src/port/caam/wolfcaam_ecdsa.c:616,737
Function: SynchronousSendRequest / wc_CAAM_Ecdh / wc_CAAM_MakeEccKey
Severity: Medium

The point of key->blackKey = vaddr; (ecc.c:12027, previously (word32)vaddr) is to stop truncating a CAAM secure-memory address, and inside ecc.c that works — caamReadPartition/caamWriteToPartition take CAAM_ADDRESS (wolfcaam.h:75-76). But every crossing into the QNX driver re-truncates. wolfcaam_qnx.c:103 declares CAAM_ADDRESS privkey; and then lines 189 and 220 send it with SETIOV(&in[inIdx], &privkey, sizeof(unsigned int)) — 4 bytes — while the driver side reads the full width: caam_qnx.c:1088-1089 uses keySz = sizeof(CAAM_ADDRESS); SETIOV(&in_iovs[0], &blackKey, sizeof(CAAM_ADDRESS)); and :1217-1218 does the same for ECDH. wolfcaam_ecdsa.c:616 and :737 likewise declare the SM key buffer as sizeof(unsigned int), and :753 reassembles the keygen-returned address from exactly four bytes. So on 64-bit QNX/AArch64 (supported per caam_qnx.h:29 and caam_driver.h:34-37) the client sends 4 bytes where the driver expects 8. These specific lines are pre-existing and untouched by the diff, but they mean the PR's stated outcome for finding 5384 — 'addresses are not truncated' — is not reached on the paths that hand addresses to the hardware.

Recommendation: Either carry the width change through — replace sizeof(unsigned int) with sizeof(CAAM_ADDRESS) at wolfcaam_qnx.c:189/220 and wolfcaam_ecdsa.c:616/737, and make the 4-byte reassembly at wolfcaam_ecdsa.c:753 width-correct — or state in the PR that 64-bit CAAM is out of scope and leave a TODO, because as it stands the ecc_key type change is cosmetic on the paths that matter.

Referenced code: wolfcrypt/src/port/caam/wolfcaam_qnx.c:189,220; wolfcrypt/src/port/caam/wolfcaam_ecdsa.c:616,737 (7 lines)


New 'inited' flag is not consulted by the devcrypto hash and HMAC ioctl paths

File: wolfcrypt/src/port/devcrypto/devcrypto_hash.c:84,105; wolfcrypt/src/port/devcrypto/devcrypto_hmac.c:82,103
Function: HashUpdate / GetDigest / wc_DevCrypto_HmacUpdate / wc_DevCrypto_HmacFinal
Severity: Medium

The PR introduces WC_CRYPTODEV.inited as the canonical 'owns a live session' flag and converts the AES paths (devcrypto_aes.c:52, 85, 155, 225, 312) and wolfcaam_hmac.c:57 to test it, but the hash and HMAC data paths still dereference dev->cfd with no validity check. wc_Sha256Free() ForceZeros the whole struct, so a freed context has cfd == 0 (stdin), not -1. Combined with this PR's other change — wc_Sha256Final() now calls wc_Sha256Free(sha) on its error paths — a caller that ignores the Final error and reuses the context reaches ioctl(0 /* stdin */, CIOCCRYPT, &crt). Same for any Hmac that was memset-zeroed rather than wc_HmacInit'd. The PR added exactly the flag needed to prevent this but did not apply it here.

Recommendation: Add an inited == 0 guard at the top of HashUpdate, GetDigest, wc_DevCrypto_HmacUpdate and wc_DevCrypto_HmacFinal, mirroring how aes.c uses the new flag. This is what makes the new flag pay for itself.

Referenced code: wolfcrypt/src/port/devcrypto/devcrypto_hash.c:84,105; wolfcrypt/src/port/devcrypto/devcrypto_hmac.c:82,103 (4 lines)


RTOS yield primitives do not yield to lower-priority threads, so the spin loop can hang

File: wolfssl/wolfcrypt/wc_port.h:790-799; wolfcrypt/src/random.c:411-435,463-489
Function: wc_DrbgState_MutexInit / wc_DrbgState_MutexFree
Severity: Medium

taskYIELD() (FreeRTOS), tx_thread_relinquish() (ThreadX) and taskDelay(0) (VxWorks) all yield only among threads of equal-or-higher priority. The new for(;;) loops have no iteration bound and no timeout, so on a single-core RTOS a high-priority thread spinning while a lower-priority thread owns WC_DRBG_MUTEX_INITPROGRESS will never let the owner run — an unbounded hang rather than a race. The previous code, whatever its race, could not hang.

Recommendation: Bound the spin and fall back to a one-tick sleep, or use a priority-agnostic delay (vTaskDelay(1) / tx_thread_sleep(1) / taskDelay(1)) after N failed attempts. Also moot if WC_RELAX_LONG_LOOP() is adopted.

Referenced code: wolfssl/wolfcrypt/wc_port.h:790-799; wolfcrypt/src/random.c:411-435,463-489 (8 lines)


No CI coverage for devcrypto, CAAM, Renesas FSPSM, or aarch64 ARMASM ML-KEM

File: .github/workflows/
Function: N/A
Severity: Medium

A grep across all 110 workflow files returns zero matches for devcrypto/DEVCRYPTO and zero for caam, and there is no FSPSM/SCEPROTECT job. This PR's highest-risk changes sit almost entirely in those unbuilt ports: the new WC_CRYPTODEV.inited lifecycle flag threaded through aes.c/hmac.c/devcrypto_*.c, the wc_Sha256Copy/wc_Sha256Final semantic changes, the CAAM_ADDRESS retyping of two ecc_key fields, and the FSPSM lock/error-path changes. The aarch64 ML-KEM NEON helpers are similarly thin: workflows matching armasm (multi-arch.yml, windows-arm64.yml, aesgcm-siv.yml) do not obviously pair --enable-armasm with ML-KEM KATs, and a stride error there would be silent (wrong noise, still a well-formed key). None of this is compiled, let alone executed, by CI.

Recommendation: Add at minimum compile-only jobs for --enable-devcrypto=all, --enable-devcrypto=hash and --enable-cryptonly --enable-devcrypto, plus an --enable-caam compile job. A cryptodev-dkms runtime job on ubuntu-latest would additionally exercise the AES-GCM buffer contract. Confirm an aarch64 job builds --enable-armasm with ML-KEM and runs the KATs.

Referenced code: .github/workflows/ (2 lines)


No test asserts *answer == 0 on wc_DsaVerify_ex error paths

File: tests/api/test_dsa.c:812-818
Function: test_wc_DsaSign_bad_digestSz
Severity: Medium

The PR's fix for finding 6145 is *answer = 0;. The existing test exercises both length-check failures but only checks the return code, never answer. Worse, the ordering means it could not catch a regression even if it did: int answer = 0; is initialized at declaration (line 767) and both BAD_LENGTH_E calls happen before the successful verify that sets it to 1, so answer is 0 at those points regardless of whether wc_DsaVerify_ex writes it. The behaviour this PR adds is untested. The fix itself is complete — wc_DsaVerify (dsa.c:1106-1110) tail-calls wc_DsaVerify_ex, the assignment is correctly placed after the answer == NULL check, and the only later writes are the terminal *answer = 1/0 at 1243-1246.

Recommendation: Repeat a bad-length call after the successful verify (which leaves answer == 1) and assert answer == 0, so the new default is actually regression-protected.

Referenced code: tests/api/test_dsa.c:812-816 (5 lines)


QNX example printf format no longer matches the widened blackKey (latent UB on 64-bit)

File: IDE/QNX/example-server/server-tls.c:172-173
Function: cover
Severity: Low

keyOut->blackKey is now CAAM_ADDRESS but is still printed with %08X, which consumes an unsigned int. On a 64-bit QNX/AArch64 build that is a varargs type mismatch — undefined behaviour that prints garbage and desynchronises the remaining conversions — plus a -Wformat diagnostic. It is latent rather than live today because IDE/QNX/example-server/Makefile:4 pins PLATFORM = armv7le, where uintptr_t == unsigned int; but caam_qnx.h:29 and caam_driver.h:34-37 both explicitly support AArch64 QNX. This is the only printf-family use of either retyped field in the tree.

Recommendation: Print width-independently with an explicit cast so the example stays correct if the Makefile is retargeted to aarch64le.

Referenced code: IDE/QNX/example-server/server-tls.c:172-173 (2 lines)


siphash.c: GET_U16 still uses a raw misaligned word16 cast

File: wolfcrypt/src/siphash.c:83,914
Function: wc_SipHash / GET_U16
Severity: Low

The PR converts the word64 key loads to GET_U64 to avoid casting const unsigned char* to const word64*, but the little-endian GET_U16 macro on line 83 is still (*(const word16*)(a)), applied to the caller-supplied message pointer at line 914 (b |= (word64)GET_U16(in); in the 2-remaining-bytes tail). At that point in has advanced by a multiple of SIPHASH_BLOCK_SIZE from an arbitrary user pointer and can be odd-aligned — exactly the problem the PR fixes elsewhere in the same file. GET_U32 (line 76) and SET_U64 (line 90) already use the unaligned helpers, so GET_U16 is the odd one out. Pre-existing, but it leaves the PR's cleanup incomplete within the file it touches. No other (word64*)/(word32*) casts remain.

Recommendation: Make GET_U16 consistent with its siblings — either a readUnaligned word16 helper or the explicit byte assembly above, which is correct on LE and free in the 2-byte tail case.

Referenced code: wolfcrypt/src/siphash.c:83,914 (4 lines)


IDE/INTIME-RTOS/Makefile header manifest not updated with caam_type.h

File: IDE/INTIME-RTOS/Makefile:346-359
Function: INCL_TARGS
Severity: Low

The PR added caam_type.h to wolfssl/wolfcrypt/include.am, and CMake picks it up automatically because CMakeLists.txt globs the whole port/caam directory. The INtime RTOS Makefile, however, enumerates every header by name in INCL_TARGS (consumed by the header-publish loop at lines 450-456) and lists all fourteen CAAM headers explicitly — caam_driver.h, caam_error.h, caam_qnx.h, wolfcaam.h, wolfcaam_aes.h, wolfcaam_cmac.h, wolfcaam_ecdsa.h, wolfcaam_fsl_nxp.h, wolfcaam_hash.h, wolfcaam_qnx.h, wolfcaam_rsa.h, wolfcaam_seco.h, wolfcaam_sha.h, wolfcaam_x25519.h — but not caam_type.h. The published INtime header set is therefore incomplete. Impact is limited because INtime is not a CAAM target, but the manifest is meant to be a complete copy of the header tree. No other build file in the repo enumerates CAAM headers individually.

Recommendation: Add wolfssl/wolfcrypt/port/caam/caam_type.h \ to INCL_TARGS in alphabetical order, between caam_qnx.h and wolfcaam.h.

Referenced code: IDE/INTIME-RTOS/Makefile:346-349 (4 lines)


Implicit narrowing of CAAM_ADDRESS blackKey into the word32 args[] array

File: wolfcrypt/src/port/caam/wolfcaam_ecdsa.c:364,641,728
Function: wc_CAAM_EccSign / wc_CAAM_Ecdh / wc_CAAM_MakeEccKey
Severity: Low

All three functions declare word32 args[4] = {0}; (lines 303, 552, 681) and assign key->blackKey into it. With blackKey now CAAM_ADDRESS rather than word32, these become implicit 64→32-bit narrowing conversions on any LP64 target. Semantically benign in every reachable case, because these branches only carry the small black-key type enumeration (CAAM_BLACK_KEY_SM=1, CCM=2, ECB=3, or 0 — wolfcaam.h:49-55) and never a real address (the address travels via buf[idx].TheAddress). But they are new implicit-truncation sites that warn under -Wconversion, which wolfSSL's multi-test configurations enable. Under WOLFSSL_SECO_CAAM the source is additionally signed, making it a signed→unsigned narrowing too.

Recommendation: Add an explicit documenting cast at wolfcaam_ecdsa.c:364, :641 and :728.

Referenced code: wolfcrypt/src/port/caam/wolfcaam_ecdsa.c:364,641,728 (3 lines)


siphash.c: GET_U64(key) vs GET_U64(key + 0) inconsistency between the two asm hunks

File: wolfcrypt/src/siphash.c:414,643
Function: wc_SipHash
Severity: Low

The x86-64 hunk writes k0 = GET_U64(key); while the aarch64 hunk writes k0 = GET_U64(key + 0);; the C reference implementation at line 869 uses key + 0. Cosmetic only. The substantive change is verified correct and is an improvement beyond alignment safety: on LITTLE_ENDIAN_ORDER GET_U64 is readUnalignedWord64 (line 69), bit-for-bit identical to the old cast on LE targets but without the strict-aliasing/alignment violation; on a big-endian target the macro (lines 98-105) performs an explicit little-endian load, which is what the SipHash spec requires and which the old native-endian cast got wrong.

Recommendation: Use GET_U64(key + 0) in both so the k0/k1 pair reads symmetrically and matches the C implementation.

Referenced code: wolfcrypt/src/siphash.c:414,643 (2 lines)


Verified correct: changes that were checked in depth and need no action

File: wolfcrypt/src/random.c:3811-3915, wolfssl/wolfcrypt/settings.h:3146-3157, wolfcrypt/src/ecc.c:10404,12020-12035
Function: multiple
Severity: Info

Recording these so they are not re-litigated. (1) Intel RDSEED/RDRANDwriteUnalignedWord64 exists (misc.h:92, misc.c:380-393), pointer arithmetic is unchanged, discarding the return matches existing call sites, and the in-loop declaration is C89-legal. The ForceZero(&rndTmp,...) at :3913 is not an inconsistency: it was already present in the seed function at the merge base, so the PR fixes the asymmetry. One residual gap: the trailing-remainder error paths at :3826-3828 and :3908-3910 still return ret; without zeroing rndTmp, though the asm writes *rnd via "=r"(*rnd) regardless of the carry flag. (2) settings.h #undef WOLFSSL_CAAM — benign; WOLFSSL_CAAM is a bare presence flag never defined with a value, each #undef is immediately followed by the #define, and it matches the pre-existing WOLFSSL_IMX6Q_CAAM block at :3136-3138. All three CAAM blocks are now consistent. (3) ecc.c hunks — type-correct and they fix real bugs: vaddr is CAAM_ADDRESS (ecc.c:12020) so (word32)vaddr was a genuine truncation, and (word32)vaddr + privSz was truncate-then-add. All CAAM_BLACK_KEY_* comparisons are against small non-negative constants so no -Wsign-compare issue arises, and no serialization or 4-byte-assuming XMEMSET of ecc_key exists. (4) DRBG state machine liveness — every transient state is owned by exactly one thread that always terminates it; no deadlock or ABA. (5) sizeof(ecc_key) growth — no compile-time size assertion anywhere in the tree; all uses are sizeof()-relative and recompile correctly. (6) **test_aes.c…

Recommendation: No action needed except optionally adding ForceZero(&rndTmp, ...) to the trailing-remainder error returns at random.c:3826-3828 and :3908-3910 for symmetry with the loop bodies.

Referenced code: wolfcrypt/src/random.c:3811-3915, wolfssl/wolfcrypt/settings.h:3146-3157, wolfcrypt/src/ecc.c:10404,12020-12035 (7 lines)


Review generated by Skoll

@@ -219,22 +216,35 @@ int wc_Sha256GetHash(wc_Sha256* sha, byte* hash)

int wc_Sha256Copy(wc_Sha256* src, wc_Sha256* dst)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] wc_Sha256Copy now returns NOT_COMPILED_IN in a reachable build configuration

The rewrite moves wc_InitSha256_ex(dst, ...) inside the #ifdef WOLFSSL_DEVCRYPTO_HASH_KEEP block and returns NOT_COMPILED_IN in the new #else; previously it always initialized dst and returned 0. That configuration is reachable: configure.ac defines WOLFSSL_DEVCRYPTO_HASH_KEEP only inside if test "x$ENABLED_CRYPTONLY" = "xno" (configure.ac:9990-10007), so --enable-cryptonly --enable-devcrypto (or =hash) yields WOLFSSL_DEVCRYPTO_HASH without HASH_KEEP. Every caller then hard-fails: wc_HmacCopy (hmac.c:318), wolfSSL_HmacCopy (src/ssl_crypto.c:1464-1470), EVP_MD_CTX_copy_ex (evp.c:6027), SLH-DSA's hot path (wc_slhdsa.c:776, 830, 909, 993, 1041), and wolfcrypt/test/test.c:5978/6161. Returning an explicit error is the right direction — the old behaviour silently produced a non-functional copy, which is finding 4445 — but it converts a silent wrong-answer into a runtime hard-failure with no build-time guard. dst is also left completely untouched on that path, so a caller ignoring the return uses uninitialized memory.

Fix: Auto-define WOLFSSL_DEVCRYPTO_HASH_KEEP whenever WOLFSSL_DEVCRYPTO_HASH is set — in settings.h so it covers user_settings.h builds, not just configure — or add an #error for the unusable combination. At minimum still initialize dst before returning NOT_COMPILED_IN.

@@ -173,14 +167,16 @@ int wc_Sha256Final(wc_Sha256* sha, byte* hash)
#ifdef WOLFSSL_DEVCRYPTO_HASH_KEEP
/* keep full message to hash at end instead of incremental updates */
if ((ret = HashUpdate(sha, CRYPTO_SHA2_256, sha->msg, sha->used)) < 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] wc_Sha256Final destroys the caller's context and loses the heap hint on error paths

The PR adds wc_Sha256Free(sha) before both error returns. wc_Sha256Free() ForceZeros the entire wc_Sha256, wiping sha->heap, sha->msg/used/len and the cryptodev context, so the object silently goes from initialized to freed on a transient ioctl failure — behaviour no other wc_*Final backend has and the docs do not describe. There is no double-free (a later wc_Sha256Free is a no-op because msg is NULL and inited is 0 under the new gate, which is an improvement on the old cfd >= 0 gate). Separately, the success path at lines 182-183 reads sha->heap after wc_Sha256Free already zeroed it, so the heap hint is always lost — worth fixing while this function is being reworked.

Fix: Cache void* heap = sha->heap; at entry and use it for the trailing wc_InitSha256_ex(). On error paths prefer releasing only the KEEP buffer and leaving the context initialized; if full teardown is intended, re-init with the cached heap.

@@ -129,6 +129,7 @@ int wc_AesSetKey(Aes* aes, const byte* userKey, word32 keylen,
aes->left = 0;
#endif
aes->ctx.cfd = -1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] wc_AesSetKey clears the devcrypto context without releasing an already-open session

The PR touches these exact two lines (adding aes->ctx.inited = 0;). Calling wc_AesSetKey()/wc_AesGcmSetKey() twice on the same Aes — a supported pattern, e.g. TLS key update — resets cfd and inited without wc_DevCryptoFree(). The previously-opened cloned /dev/crypto descriptor and kernel session are orphaned: the fd leaks, and because the teardown guard is now inited, wc_AesFree() will not reclaim it either. Same pattern in wc_DevCrypto_HmacSetKey (devcrypto_hmac.c:53-54) and wc_AesSetKeyLocal (aes.c:5851-5852). The leak predates the PR, but the PR introduces the very lifecycle flag that should close it and instead cements it.

Fix: Replace the two assignments with wc_DevCryptoFree(&aes->ctx); (which already sets cfd = -1 and inited = 0 when a session exists) plus an explicit aes->ctx.cfd = -1; for the never-opened case. Apply the same to wc_DevCrypto_HmacSetKey.

Comment thread wolfcrypt/src/random.c
#if defined(WOLFSSL_ATOMIC_OPS) && defined(WOLFSSL_THREAD_YIELD)
static wolfSSL_Atomic_Int drbgStateMutex_inited =
WOLFSSL_ATOMIC_INITIALIZER(WC_DRBG_MUTEX_UNINITED);
#else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] Non-atomic DRBG fallback is still racy with no warning or comment

static volatile int drbgStateMutex_inited provides no atomicity — volatile is a compiler barrier, not a synchronization primitive. Two threads can both observe WC_DRBG_MUTEX_UNINITED and both call wc_InitMutex, producing a double InitializeCriticalSection on Windows or two FreeRTOS semaphores with one leaked, which is precisely finding 4432. A port lands on this path silently — there is no #warning and no comment. wc_port.h:670-675 explicitly requires 'local awareness of thread-unsafe semantics' for such fallbacks; that awareness is not documented here. If the guard is fixed per the BLOCK finding above this branch disappears entirely, which is the cleaner resolution.

Fix: Prefer removing the branch by adopting WC_RELAX_LONG_LOOP() as above. If it must stay, add a suppressible #warning and document that wolfCrypt_Init() must be called single-threaded on affected platforms.

/* Yield the CPU to another runnable thread. Used by spin-wait loops that are
* waiting on another thread to finish a short critical section. Ports may
* override. */
#ifndef WOLFSSL_THREAD_YIELD

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] Yield-macro branch order diverges from the wolfSSL_Mutex typedef order

The mutex typedef chain at wc_port.h:414-446 carries the explicit comment 'FREERTOS comes first to enable use of FreeRTOS Windows simulator only', but the new yield chain puts WOLFSSL_PTHREADS and USE_WINDOWS_API ahead of FREERTOS. Two mismatches result: (a) the FreeRTOS Windows simulator gets SemaphoreHandle_t mutexes but SwitchToThread() yields; (b) ESP-IDF built with wolfSSH defines WOLFSSL_PTHREADS alongside FREERTOS (settings.h:978-979), so it gets FreeRTOS mutexes but sched_yield(). Yielding via the wrong scheduler API on an RTOS is at best ineffective and at worst blocks. The Windows-vs-pthreads ordering itself is handled correctly and matches wc_port.h:433.

Fix: Mirror the ordering of the wolfSSL_Mutex typedef chain so the yield primitive always matches the mutex implementation. Moot if the macro is dropped in favour of WC_RELAX_LONG_LOOP().

@@ -366,11 +366,11 @@ int wc_Sha256_Grow(wc_Sha256* sha256, const byte* in, int inSz);
\return negative on error

\param src Source SHA256 structure

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] sha256.h doc: contract change documented only for wc_Sha256Copy, and the example leaves src uninitialized

The PR makes wc_Sha256Free(dst) the first operation on dst and codifies 'must be zeroed/initialized' in the dox comment. I verified every in-tree caller already satisfies that contract — src/internal.c:7670 XMEMSETs *destination, src/tls13.c:12893 XMEMSETs the Digest union, wolfcrypt/src/hmac.c:391 XMEMSETs dst->hash inside wc_HmacCopy, src/ssl_crypto.c, evp.c:6155, wc_slhdsa.c and the tests — and the generic implementation in sha256.c already behaved this way, so the contract is consistent repo-wide. Two documentation gaps remain: the updated example still leaves src uninitialized, and only the SHA-256 comment was touched though wc_ShaCopy, wc_Sha384Copy, wc_Sha512Copy and wc_Sm3Copy carry the identical contract.

Fix: Fix the example to initialize src, and propagate the must be zeroed/initialized wording to wc_ShaCopy / wc_Sha384Copy / wc_Sha512Copy / wc_Sm3Copy.

@@ -983,14 +983,12 @@ static CB_INLINE int wolfssl_ssl_conf_verify_cb_no_signer(int preverify,
/* Clean up and exit */
if ((_crt_found == 0) && (bundle_cert != NULL)) {
ESP_LOGW(TAG, "Cert not found, free bundle_cert");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] esp_crt_bundle.c: missing space before comment terminator

The fix is correct and I verified it thoroughly: this_issuer and this_subject are only ever assigned from wolfSSL_X509_get_issuer_name(bundle_cert) (line 778) and wolfSSL_X509_get_subject_name(bundle_cert) (line 788), which return &cert->issuer / &cert->subject (src/x509.c:6283-6298) — interior pointers, never dups. The removed wolfSSL_X509_NAME_free would have double-freed the name internals (since wolfSSL_X509_freeFreeX509 already calls FreeX509Name on both) and then XFREEd a pointer into the middle of the X509 allocation. It also ran after wolfSSL_X509_free(bundle_cert), and in the loop-exhausted case the pointers already dangled into a cert freed at line 862 — so it was a use-after-free too. The _crt_found != 0 branch correctly does not free them. Only the comment formatting is off: no space before */.

Fix: Add the missing space before */ and name wolfSSL_X509_free explicitly so the ownership reasoning is obvious to the next reader.

static void mlkem_get_noise_x3_eta2_aarch64(byte* rand, byte* seed, byte o)
{
word64* state = (word64*)rand;
word64 state[3 * 25] = {0};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚪ [Info] ML-KEM: '= {0}' on the 600-byte state arrays is provably dead work on a hot path

Both x3 helpers now zero-initialize a 600-byte array on entry, emitted as a 600-byte memset. The NEON routine (wolfcrypt/src/port/arm/armv8-mlkem-asm.S:10409+, inline twin at armv8-mlkem-asm_c.c:9739+) loads only state[4], state[29] and state[54] (offsets 32, 232, 432) and explicitly zeroes every other lane in registers before the 24 rounds, then stores all 75 words back — so the initializer is dead. mlkem_get_noise_x3_eta2_aarch64 runs up to three times per ML-KEM-1024 keygen/encaps, roughly 1.8 KB of pointless zeroing per operation. Not a correctness issue, and defensible as belt-and-braces, but it should be a conscious trade. This also confirms the = {0} added to the eta3 helper is not a behaviour change.

Fix: Either drop the = {0} and note why in a comment, or keep it with a comment saying it is defensive. Also consider a comment on the 25*8 copy stride noting it is the caller's layout and intentionally not ETA2_RAND_SIZE, since the adjacent eta3 helper packs at ETA3_RAND_SIZE and the difference reads like a typo.

Comment thread wolfcrypt/src/des3.c
@@ -1570,11 +1570,15 @@
pc1m[(l = j + totrot[i]) < (j < 28 ? 28 : 56) ? l : l-28];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] des3.c: secret-dependent ternary remains in the pc1 bit-extraction loop

The PR removes the secret-dependent branch from the pc2 selection loop, but the loop fifteen lines above — which produces the very values it consumes — still branches on a secret key bit via ? 1 : 0. That is the same construct class being eliminated. GCC and Clang both lower a ? 1 : 0 on a masked value to a branchless setne/cset, which is why it has not shown up; but the same was true of the if (pcr[...]) form just rewritten. Fixing one and leaving the other means the function is still branchless only by compiler goodwill, not by construction. (I verified the rewrite itself is bit-identical: pc1m[] is filled by this exact ? 1 : 0, and pcr[j] only copies pc1m[], so mask = (byte)(0 - bit) is provably 0x00/0xFF. bytebit appears nowhere else in the repo, so no duplicate loop was missed, and DES3 KATs at wolfcrypt/test/test.c:12588/12645 cover it.)

Fix: Convert to arithmetic extraction so the whole key schedule is branchless by construction rather than by optimizer behaviour.


Note: Referenced line (wolfcrypt/src/des3.c:1557-1563) is outside the diff hunk. Comment anchored to nearest changed region.

XFREE(plainBuf, aes->heap, DYNAMIC_TYPE_AES);
XFREE(cipherBuf, aes->heap, DYNAMIC_TYPE_AES);
XFREE(aTagBuf, aes->heap, DYNAMIC_TYPE_AES);
if (key_server_aes == NULL || key_client_aes == NULL) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] Renesas FSPSM AES: inconsistent condition order and argument alignment between encrypt and decrypt

The fix is correct and complete — I verified the lock is genuinely held (if ((ret = wc_fspsm_hw_lock()) == 0) at line 379 encrypt, 613 decrypt), that the pre-lock early returns at 354/359/363 and 588/593/598 correctly do not unlock, that this MEMORY_E return was the only unbalanced exit between lock and the shared unlock at 535/756, and that the new XFREEs can never free a wrapped_key alias because those assignments (445, 674) live in the mutually exclusive non-TLS branch reached only after this block. Only cosmetics: the encrypt path reorders the condition to key_server_aes == NULL || key_client_aes == NULL while decrypt keeps client-then-server, and the XFREE argument columns do not line up (key_client_aes, followed by 2 spaces, plainBuf, by 7).

Fix: Use client-then-server in both (matching declaration and allocation order) and align the XFREE arguments identically so the two blocks stay diffable.


Note: Referenced line (wolfcrypt/src/port/Renesas/renesas_fspsm_aes.c:413-421,644-652) is outside the diff hunk. Comment anchored to nearest changed region.

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants