harden forcezero and compiler barriers - #10935
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
fdce988 to
c0a7910
Compare
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
c0a7910 to
69e535e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
|
jenkins retest this please |
69e535e to
6fcdbd1
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
6fcdbd1 to
b371392
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
1844d19 to
6bf96b1
Compare
6bf96b1 to
74486d3
Compare
|
jenkins retest this please |
74486d3 to
84e4744
Compare
|
jenkins retest this please |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 25 total — 8 posted, 17 skipped
Posted findings
- [High] Gating out ForceZero() in the CryptoCb free callbacks breaks the mock-SE zeroization assertions —
tests/api/test_aes.c:10586-10588 - [High] WOLF_C99 branch precedes the KEIL branch, making the new no-clobber carve-out unreachable —
wolfssl/wolfcrypt/wc_port.h:1921-1933 - [Medium] Gating out ForceZero() defeats the stated WOLFSSL_CHECK_MEM_ZERO purpose in the TLS 1.3 ticket test —
tests/api/test_tls13.c:7112-7125 - [Medium] Replacing XFENCE() with WC_BARRIER() silently drops the hardware fence from ForceZero() —
wolfcrypt/src/misc.c:696-724 - [Medium] New forcezero_test() duplicates weaker coverage and tests nothing the PR actually changed —
wolfcrypt/test/test.c:4112-4149 - [Medium] WOLFSSL_NO_FORCE_ZERO gating in tests rests on an incorrect premise - ForceZero() is always declared —
tests/api/test_aes.c:11626-11630 - [Medium] XASM_VOLATILE converted from basic to extended asm, changing '%' semantics and requiring clobber-list support —
wolfssl/wolfcrypt/wc_port.h:1923 - [Low] New XASM_VOLATILE lines exceed the 80-column convention —
wolfssl/wolfcrypt/wc_port.h:1932
Skipped findings
- [Medium] WOLFSSL_NO_FORCE_ZERO guard removes zeroization of preMasterSecret and Arrays before XFREE
- [Medium] ForceZero() loses its hardware memory/speculation fence on all GCC and clang targets
- [Medium] New WOLFSSL_NO_FORCE_ZERO guard in test_aes.c breaks an unguarded assertion in the same test
- [Medium] New KEIL carve-out for GCC clobber syntax is unreachable because WOLF_C99 is tested first
- [Low] forcezero_test buffer has no guaranteed alignment, so the 'unaligned' offsets are not deterministic
- [Low] Guarded-out ForceZero leaves TLS 1.3 traffic key in static storage and orphans the slot
- [Low] Guarded-out ForceZero leaves simulated-SE key populated and breaks the asserting test
- [Low] Early return in ForceZero alignment loop bypasses the trailing WC_BARRIER()
- [Low] WOLFSSL_NO_FORCE_ZERO guard disables TLS 1.3 crypto-callback slot reclamation, exhausting the pool
- [Low] WOLFSSL_NO_FORCE_ZERO guard removes the zeroing the comment says is required to avoid a WOLFSSL_CHECK_MEM_ZERO abort
- [Low] IAR branch of XASM_VOLATILE left without the memory clobber, silently weakening XFENCE() on IAR
- [Low] New WOLFSSL_NO_FORCE_ZERO guards are unnecessary; ForceZero() is declared in both misc.h branches
- [Info] forcezero_test() does not exercise the dead-store elimination it was added to verify
- [Info] ForceZero no longer emits a hardware fence on GNU toolchains; barrier strength now varies by compiler
- [Info] XASM_VOLATILE now appends ::: "memory", breaking any caller that supplies its own operands
- [Info] forcezero_test() is compiled out under NO_INLINE, skipping the new coverage in --disable-inline builds
- [Info] forcezero_test() enshrines an undocumented ForceZero(NULL, 0) contract
Review generated by Skoll via Claude/Codex
84e4744 to
5d4a4be
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10935
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 11 total — 8 posted, 3 skipped
Posted findings
- [Medium] Early return in the alignment loop bypasses the trailing WC_BARRIER() —
wolfcrypt/src/misc.c:703-708 - [Medium] Re-adding volatile reverts a deliberate performance decision and blocks memset/vector idiom recognition —
wolfcrypt/src/misc.c:696-722 - [Medium] Follow-up on XFENCE() to WC_BARRIER(): the speculation-barrier case is not covered by the ordering argument —
wolfcrypt/src/misc.c:701 - [Low] PR checklist still marks 'added tests' after the test was withdrawn —
wolfcrypt/src/misc.c:694-725 - [Medium] XFENCE() silently keeps the un-hardened (no memory clobber) form on IAR and Keil, based on a rationale contradicted by wolfSSL's own SP assembly —
wolfssl/wolfcrypt/wc_port.h:1925-1934 - [Low] XASM_VOLATILE_MB()'s generic #else fallback now requires GCC extended-asm support from any unrecognized compiler that reaches an inline-asm XFENCE branch —
wolfssl/wolfcrypt/wc_port.h:1938-1941 - [Info] XASM_VOLATILE() now has zero in-tree users —
wolfssl/wolfcrypt/wc_port.h:1925-1941 - [Info] ForceZero comment overstates the WC_BARRIER() guarantee on non-GNUC / no-fence configurations —
wolfcrypt/src/misc.c:696-701
Skipped findings
- [Low] Misaligned line-continuation backslashes in the ARMASM_BARRIER_DETECT XFENCE macro
- [Low] ForceZero loses its hardware fence on GCC/clang after XFENCE() to WC_BARRIER() swap
- [Low] ForceZero() swapped XFENCE() for WC_BARRIER(), removing the speculation barrier on all GCC/clang targets
Review generated by Skoll via Claude/Codex
| XFENCE(); | ||
| WC_BARRIER(); | ||
|
|
||
| while ((wc_ptr_t)zb & (wc_ptr_t)(sizeof(unsigned long) - 1U)) { |
There was a problem hiding this comment.
🟡 [Medium] Early return in the alignment loop bypasses the trailing WC_BARRIER()
💡 SUGGEST bug
The PR adds the comment WC_BARRIER() prevents compiler reordering and brackets the whole body with WC_BARRIER() at entry and exit, but the head-alignment loop still contains an unconditional return that escapes before the closing barrier. For a misaligned mem with a small len (e.g. ForceZero(p + 3, 3) on a 8-byte-aligned p), the loop writes real bytes and then returns via if (len == 0) return; with no trailing barrier at all. The volatile stores still guarantee the writes happen, but volatile accesses may still be reordered relative to non-volatile accesses, so surrounding code can be scheduled across the zeroization on exactly this path. The structure predates the PR, but the PR explicitly re-states the barrier-bracketing contract on these lines and leaves the one path that violates it.
Measured: running this exact function body with WC_BARRIER() replaced by a counter gives 2 barriers on the normal path but only 1 whenever the head-alignment loop consumes the whole length:
aligned, len=16 (normal path) -> 2 barrier(s)
unaligned buf+1, len=3 (small secret) -> 1 barrier(s) <== trailing barrier SKIPPED
unaligned buf+5, len=1 -> 1 barrier(s) <== trailing barrier SKIPPED
unaligned buf+1, len=7 (fills to align) -> 2 barrier(s)
Low severity in practice, since the volatile stores carry the anti-DSE guarantee on their own - but the trailing barrier is the one that matters for DSE, and it is the path that skips it.
Suggestion:
| while ((wc_ptr_t)zb & (wc_ptr_t)(sizeof(unsigned long) - 1U)) { | |
| while ((len > 0) && | |
| ((wc_ptr_t)zb & (wc_ptr_t)(sizeof(unsigned long) - 1U))) { | |
| *zb++ = 0; | |
| --len; | |
| } | |
| /* remaining loops are already no-ops when len == 0, so control always | |
| * reaches the trailing WC_BARRIER() */ |
| { | ||
| byte *zb = (byte *)mem; | ||
| unsigned long *zl; | ||
| /* Volatile pointers prevent dead-store elimination. |
There was a problem hiding this comment.
🟡 [Medium] Re-adding volatile reverts a deliberate performance decision and blocks memset/vector idiom recognition
💡 SUGGEST question
Commit 6617f2edf (Aug 2025, douzzer) deliberately removed volatile from ForceZero and replaced it with XFENCE() bracketing — the pre-6617f2edf implementation used volatile byte* / volatile word64* exactly as this PR reintroduces. This PR reverses that decision without referencing it. The practical cost is real: with volatile unsigned long *zl, the compiler must emit one store instruction per word and cannot recognize the loop as a memset idiom or vectorize it. ForceZero has ~1036 call sites in-tree, including large-buffer calls such as ForceZero(ssl->arrays, sizeof(Arrays)) and ForceZero(ssl_c->arrays->preMasterSecret, ENCRYPT_LEN). The PR description reports cross-compilation and static analysis but no benchmark data. Note that reverting to volatile is a defensible hardening trade — the concern is that it silently undoes a recent, intentional tuning change by the maintainer.
Measured on x86_64 (clang -O2), both function bodies copied verbatim from their respective revisions, ns per call:
bytes volatile ns plain ns ratio
128 4.2 1.3 3.2x
512 15.2 3.7 4.1x
4096 120.1 28.5 4.2x
65536 1807.6 306.8 5.9x
At 64 KB the non-volatile version runs at memset speed (~213 GB/s, vectorized) while the volatile version is pinned to scalar 8-byte stores (~36 GB/s), which is exactly the idiom-recognition loss described above. At 16-32 bytes the two are within call overhead of each other, so short wipes are unaffected. Typical hot call sites such as ForceZero(ssl->arrays, sizeof(Arrays)) land in the 3-4x band.
Recommendation: Confirm with the author of 6617f2edf that the volatile-vs-fence trade-off is being intentionally reversed, and include throughput numbers for a large ForceZero (e.g. 4 KB) at -O2 on x86_64 and aarch64 in the PR description. If the regression is material, consider keeping non-volatile word stores for the bulk loop and relying on WC_BARRIER() plus a volatile-pointer read of mem after the loop to defeat DSE.
There was a problem hiding this comment.
STM32's forcezero seemed to be getting DSEd, I think we should keep volatile.
| volatile unsigned long *zl; | ||
|
|
||
| XFENCE(); | ||
| WC_BARRIER(); |
There was a problem hiding this comment.
🟡 [Medium] Follow-up on XFENCE() to WC_BARRIER(): the speculation-barrier case is not covered by the ordering argument
💡 SUGGEST question
Following up on your reply that a hardware fence "would matter if there were a second thread/core about to read that memory" and is therefore out of scope. Agreed for the ordering/visibility case - making the zeros visible to another core is the caller's problem, not ForceZero()'s, and the restored volatile stores carry the anti-DSE guarantee on their own. This is not a zeroization-correctness regression.
The one sub-case that framing does not reach: the architecture-specific XFENCE() definitions in this header do not select cross-core ordering fences, they select speculation barriers - lfence on x86, isb (or sb) on ARM, isync; sync on PPC. Their purpose is bounding speculative execution, not inter-thread visibility, so the second-core argument does not apply to them. Concretely, on aarch64 the WOLFSSL_ARMASM_BARRIER_SB / WOLFSSL_ARMASM_BARRIER_DETECT machinery (wc_port.h:1970-1985) exists specifically to emit sb, and after this change it no longer applies to ForceZero() at all.
Two smaller notes in the same area:
- The strength is now inverted across platforms. On GCC/clang
WC_BARRIER()is__asm__ __volatile__("" ::: "memory")(wc_port.h:1999-2000) - compiler-only, no instruction emitted. On non-GNUC compilers the fallback at wc_port.h:2005 still callsXFENCE()and keeps the full fence. So the mainstream builds are the ones that lose it. WC_BARRIERis user-overridable via#ifdef WC_BARRIER(wc_port.h:1997), separately fromXFENCE's own override at wc_port.h:1936. A downstream no-opWC_BARRIERoverride now silently removes both barriers fromForceZero().
No change requested if the speculation barrier is deliberately considered out of scope too - but it is worth one line in the code comment or PR description saying so, since the swap is otherwise indistinguishable from an oversight to the next reader.
Recommendation: State explicitly in the PR description and in the code comment why a compiler barrier now suffices (volatile stores supply the DSE guarantee that the fence previously supplied), so the difference from the XFENCE() speculation-barrier uses in asn.c/ssl_sess.c is documented rather than implied.
There was a problem hiding this comment.
@douzzer Questions about this one:
- Do any of wolfSSL's own internal ForceZero() call sites lack a lock before the memory becomes cross-thread-visible? I didn't see any call sites that weren't routed through a mutex.
- Should the public wc_ForceZero() API provide a hardware fence as part of its documented contract, independent of whether wolfSSL's internal callers need it?
| @@ -693,10 +693,12 @@ WC_MISC_STATIC WC_INLINE void xorbuf(void* buf, const void* mask, word32 count) | |||
| with zeros. It ensures compiler optimization doesn't skip it. */ | |||
| WC_MISC_STATIC WC_INLINE void ForceZero(void* mem, size_t len) | |||
There was a problem hiding this comment.
🔵 [Low] PR checklist still marks 'added tests' after the test was withdrawn
🔧 NIT test
Minor bookkeeping: the checklist still marks [X] added tests, but the diff is now wolfcrypt/src/misc.c and wolfssl/wolfcrypt/wc_port.h only (2 files, 36 insertions, 22 deletions). That is a direct consequence of removing forcezero_test() in response to earlier review feedback, so the code change is right - only the checkbox is stale.
Worth noting for the record what is now untested: the existing test_ForceZero (tests/api.c:27312) verifies functional zeroing and non-overrun across offsets and lengths, but cannot observe either property this PR is about - that the stores survive dead-store elimination at -O2/-O3, or that the "memory" clobber reaches XFENCE(). Nothing exercises XASM_VOLATILE_MB() or the reordered __IAR_SYSTEMS_ICC__ / __KEIL__ / WOLF_C99 arms. Untick the box, or note in the description that the test was dropped deliberately.
Suggestion:
| WC_MISC_STATIC WC_INLINE void ForceZero(void* mem, size_t len) | |
| /* Suggested addition to test_ForceZero: DSE-resistance check. | |
| * A stack buffer zeroed then never read is the classic case an | |
| * optimizer removes; scan the frame via a noinline helper. */ | |
| static WC_NOINLINE void zero_stack_secret(void) | |
| { | |
| unsigned char secret[64]; | |
| unsigned int i; | |
| for (i = 0; i < sizeof(secret); i++) | |
| secret[i] = (unsigned char)(i + 1); | |
| ForceZero(secret, sizeof(secret)); | |
| } | |
| /* Also add an unaligned small-len case that exercises the | |
| * early-return path: */ | |
| ForceZero(data + 1, 1); | |
| ForceZero(data + 3, 3); |
There was a problem hiding this comment.
Suggested test would be very compiler dependent, just updated pr summary.
| * it requires '%%' escaping. IAR/KEIL checked before WOLF_C99 since both may | ||
| * define __STDC_VERSION__ under --c99. */ | ||
| /* NOLINTBEGIN(bugprone-macro-parentheses) */ | ||
| #if defined(__IAR_SYSTEMS_ICC__) |
There was a problem hiding this comment.
🟡 [Medium] XFENCE() silently keeps the un-hardened (no memory clobber) form on IAR and Keil, based on a rationale contradicted by wolfSSL's own SP assembly
💡 SUGGEST
The PR's entire purpose for XASM_VOLATILE_MB() is to add a "memory" clobber so the compiler cannot move loads/stores across XFENCE(). For the __IAR_SYSTEMS_ICC__ and __KEIL__ branches, XASM_VOLATILE_MB(a) is defined identically to XASM_VOLATILE(a) — no clobber — justified by the in-tree comments "IAR's asm volatile() doesn't take a GCC-style clobber list" and "ARM Compiler 5 inline asm rejects GCC clobber syntax".
That rationale is contradicted by shipped wolfSSL code compiled with those exact toolchains. wolfcrypt/src/sp_cortexm.c, sp_arm32.c, and sp_arm64.c each begin with:
#ifdef __IAR_SYSTEMS_ICC__
#define __asm__ asm
#define __volatile__ volatile
#endif
#ifdef __KEIL__
#define __asm__ __asm
#define __volatile__ volatile
#endif
and then emit 241 / 250 / 206 inline-asm blocks respectively that all end in : "memory", "cc", ... clobber lists (e.g. sp_cortexm.c:671). Those expand to exactly asm volatile(... ::: "memory", ...) for IAR and __asm volatile(... ::: "memory", ...) for Keil. The WOLFSSL_NO_VAR_ASSIGN_REG guard in those files only changes operand binding — it never strips the clobber list. So both toolchains demonstrably accept GCC-style clobbers in this codebase.
Reachability: IAR EWARM/EWRISCV do not define __GNUC__, and under --c99 __STDC_VERSION__ is 199901L, so they fall past every atomics/GNU branch of the XFENCE() chain straight into the inline-asm branches (__arm__ && __ARM_ARCH > 6 → "isb", __riscv → "fence", __aarch64__ → "isb"). Keil/armcc likewise. The result is that on precisely the toolchains that do reach the inline-asm path, XFENCE() remains a bare instruction-stream barrier the compiler is free to reorder memory accesses across, while every other toolchain got hardened. This affects the existing XFENCE() hardening sites wolfcrypt/src/asn.c:1812,1821,1890 (commented "Prevent memory access") and src/ssl_sess.c:1772,1786.
Recommendation: Add the "memory" clobber on IAR and Keil too, matching the SP asm precedent:
#if defined(__IAR_SYSTEMS_ICC__)
#define XASM_VOLATILE(a) asm volatile(a)
#define XASM_VOLATILE_MB(a) asm volatile(a ::: "memory")
#elif defined(__KEIL__)
#define XASM_VOLATILE(a) __asm volatile(a)
#define XASM_VOLATILE_MB(a) __asm volatile(a ::: "memory")
If a specific IAR or armcc version genuinely rejects this, narrow the exception with a version guard (the file already uses __VER__ < 9000000 for IAR elsewhere, e.g. sp_cortexm.c:2384) rather than disabling the clobber for the whole toolchain, and correct the comments to name the version. Either way, leaving XASM_VOLATILE_MB identical to XASM_VOLATILE makes the _MB suffix actively misleading at the call sites.
There was a problem hiding this comment.
Gave IAR and KIEL the clobber following the precedent
| #elif defined(WOLF_C99) | ||
| #define XASM_VOLATILE(a) __asm__ volatile(a) | ||
| #define XASM_VOLATILE_MB(a) __asm__ volatile(a ::: "memory") | ||
| #else |
There was a problem hiding this comment.
🔵 [Low] XASM_VOLATILE_MB()'s generic #else fallback now requires GCC extended-asm support from any unrecognized compiler that reaches an inline-asm XFENCE branch
💡 SUGGEST
Before this PR, every inline-asm XFENCE() variant used XASM_VOLATILE(), i.e. basic asm (__asm__ __volatile__("isync; sync")), which is accepted by a wide range of non-GNU compilers. After this PR all of them use XASM_VOLATILE_MB(), whose generic #else arm emits GCC extended asm with a clobber list (__asm__ __volatile__(a ::: "memory")). Basic and extended asm are distinct language extensions; a compiler can support the first and not the second.
The compilers that actually reach these branches are, by construction, non-GNU: the XFENCE() chain checks C11 <stdatomic.h>, then __GNUC__ 4.1+, then __GNUC__ >= 5 || __clang__ before falling through to inline asm. So GCC and clang never get here; only toolchains such as Green Hills (__PPC__), Wind River Diab (__PPC__), TI, and TASKING do. The PR author recognized this class of problem — that is exactly why the IAR and Keil escape hatches were added — but only those two got an escape. Any other non-GNU compiler that previously compiled the basic-asm form will now hit a syntax error on the ::: clobber list, and the only remedy is a new compiler-specific #elif in this header.
Severity is held at Low because these branches are near-dead for mainstream toolchains and no in-tree build configuration is known to hit them; the concern is that the required compiler capability was silently raised with no fallback.
Recommendation: Make the clobbered form conditional on a compiler that is actually known to support extended asm, and degrade gracefully otherwise:
#elif defined(__GNUC__) || defined(__clang__)
#define XASM_VOLATILE(a) __asm__ __volatile__(a)
#define XASM_VOLATILE_MB(a) __asm__ __volatile__(a ::: "memory")
#else
/* Unknown compiler: basic asm only, no clobber list. */
#define XASM_VOLATILE(a) __asm__ __volatile__(a)
#define XASM_VOLATILE_MB(a) XASM_VOLATILE(a)
#endif
Alternatively, expose a WOLFSSL_NO_ASM_CLOBBER opt-out so a port can fall back without patching wc_port.h.
There was a problem hiding this comment.
Added suggested else. Didn't add the opt-out, since else will degrade any unrecognized compiler to basic asm.
| * it requires '%%' escaping. IAR/KEIL checked before WOLF_C99 since both may | ||
| * define __STDC_VERSION__ under --c99. */ | ||
| /* NOLINTBEGIN(bugprone-macro-parentheses) */ | ||
| #if defined(__IAR_SYSTEMS_ICC__) |
There was a problem hiding this comment.
⚪ [Info] XASM_VOLATILE() now has zero in-tree users
🔧 NIT question
Before this PR, XASM_VOLATILE() was used only by the XFENCE() definitions. After the switch to XASM_VOLATILE_MB(), a grep across the whole tree finds no remaining use of XASM_VOLATILE() outside its own definition in wc_port.h. It stays defined for out-of-tree port/user code, which is reasonable, but the result is two near-identical macros where only one is exercised by any build — so the no-clobber variant will get no CI coverage and can silently rot. The header comment also says XASM_VOLATILE_MB() is XFENCE()-internal only, but nothing enforces that; both are plain public macros in a public header.
Recommendation: Either note in the comment that XASM_VOLATILE() is retained purely for backward compatibility with out-of-tree ports, or drop it if no such consumers are supported.
There was a problem hiding this comment.
Just added a comment, probably should keep for compatibility purposes
| { | ||
| byte *zb = (byte *)mem; | ||
| unsigned long *zl; | ||
| /* Volatile pointers prevent dead-store elimination. |
There was a problem hiding this comment.
⚪ [Info] ForceZero comment overstates the WC_BARRIER() guarantee on non-GNUC / no-fence configurations
🔧 NIT
The new comment states WC_BARRIER() prevents compiler reordering, but on the non-GNUC fallback (wc_port.h:2005-2006) WC_BARRIER() expands to do { volatile byte _xfence = 0; (void)_xfence; XFENCE(); } while(0). When XFENCE() itself degenerates to WC_DO_NOTHING — which happens under WOLFSSL_NO_FENCE (wc_port.h:1994), WOLFSSL_NO_STDATOMIC_FENCE (set for a platform at settings.h:330), WOLFSSL_NO_ASM on a non-GNUC compiler, or any unrecognized architecture reaching the final #else at wc_port.h:1991 — the whole macro reduces to a store to a function-local volatile byte. A volatile access to a local object the compiler can prove is not otherwise referenced imposes no ordering constraint on unrelated memory, so no barrier is actually established on those targets. This is not a missed zeroization: the volatile byte * / volatile unsigned long * qualifiers the PR adds independently guarantee every zero store is emitted, which is the load-bearing part of the hardening. The issue is that the newly added comment asserts a property the macro does not deliver on every supported configuration, which invites future callers to rely on it. The WC_BARRIER() definition itself is pre-existing (also used by blake2b.c:128,147 and blake2s.c:125,145); what is new here is ForceZero() depending on it and documenting a guarantee around it.
Recommendation: Soften the comment at misc.c:696-697 to state what is actually guaranteed everywhere — e.g. 'Volatile pointers guarantee the zero stores are emitted; WC_BARRIER() prevents compiler reordering on targets where a barrier is available.' No code change is required in ForceZero(), since correctness of the clear rests on the volatile qualifiers, not on the barrier.
|
Tagging @douzzer to take a look at the volatile and barrier issues as well. |
5d4a4be to
7f754a2
Compare
7f754a2 to
c32d33f
Compare
4 participants
Description
Hardens ForceZero() against dead-store elimination by using volatile pointers and WC_BARRIER(). Also strengthens XASM_VOLATILE's asm barriers with a "memory" clobber, via a new internal XASM_VOLATILE_MB() macro used by XFENCE().
Testing
Cross compiled architectures: x86_64, aarch64, arm32, and riscv64. Statically analyzed and passed forcezero testing.
Checklist