From 89f024f660793fdcef8059fde1610dd19e8d8306 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Wed, 1 Jul 2026 11:50:08 +0200 Subject: [PATCH] Falcon: address PR review feedback - cmake/functions.cmake: remove duplicated BUILD_FALCON conditional block; BUILD_FALCON is now set in one place. - wolfssl/wolfcrypt/include.am: list the internal wc_falcon_*.h headers under an "if BUILD_FALCON" block so they ship in "make dist" tarballs and install when Falcon is enabled (the public falcon.h was already listed). Automake distributes both branches of the conditional, so a Falcon-disabled dist still contains them. - wc_falcon_fft_neon.c: fix strict-aliasing UB in falcon_FFT/falcon_iFFT. fpr is a word64 bit pattern; casting fpr* to double* and using vld1q_f64/vst1q_f64 accesses the store through the wrong type and can miscompile at -O2/-O3. Load/store through uint64_t (the real object type) and reinterpret the vector register to/from f64 via new FALCON_VLD/FALCON_VST macros. Verified under qemu (NEON_FFT_PASS) with fmla v.2d still emitted. - wc_falcon_sampler.c / wc_falcon_sampler.h: falcon_prng_get_u8/get_u64 have no error return, so a SHAKE256 squeeze failure previously went unnoticed and stale buffer bytes could feed the signer. Add a sticky "err" field latched on the first refill failure. - wc_falcon_sign.c: falcon_sign_core now rejects the signature when the PRNG latched an error, and falcon_do_sign_tree bounds its restart loop (FALCON_SIGN_MAX_RESTARTS) so a wedged sampler terminates instead of spinning forever. The bound is far beyond any legitimate restart count. --- cmake/functions.cmake | 3 -- wolfcrypt/src/wc_falcon_fft_neon.c | 46 ++++++++++++++++----------- wolfcrypt/src/wc_falcon_sampler.c | 10 ++++++ wolfcrypt/src/wc_falcon_sign.c | 46 +++++++++++++++++++++------ wolfssl/wolfcrypt/include.am | 14 ++++++++ wolfssl/wolfcrypt/wc_falcon_sampler.h | 1 + 6 files changed, 88 insertions(+), 32 deletions(-) diff --git a/cmake/functions.cmake b/cmake/functions.cmake index 6fdfad8954..6294bf5b1b 100644 --- a/cmake/functions.cmake +++ b/cmake/functions.cmake @@ -262,9 +262,6 @@ function(generate_build_flags) if(WOLFSSL_FALCON OR WOLFSSL_USER_SETTINGS) set(BUILD_FALCON "yes" PARENT_SCOPE) endif() - if(WOLFSSL_FALCON OR WOLFSSL_USER_SETTINGS) - set(BUILD_FALCON "yes" PARENT_SCOPE) - endif() if(WOLFSSL_LMS OR WOLFSSL_USER_SETTINGS) set(BUILD_WC_LMS "yes" PARENT_SCOPE) endif() diff --git a/wolfcrypt/src/wc_falcon_fft_neon.c b/wolfcrypt/src/wc_falcon_fft_neon.c index c13a12796b..969d42571a 100644 --- a/wolfcrypt/src/wc_falcon_fft_neon.c +++ b/wolfcrypt/src/wc_falcon_fft_neon.c @@ -79,13 +79,22 @@ static WC_INLINE double falcon_neon_d(fpr x) (out_im) = vfmaq_f64(vmulq_f64((yr), (si)), (yi), (sr)); \ } while (0) +/* Aliasing-safe vector load/store. fpr is a word64 IEEE-754 bit pattern, so + * the backing store's real object type is the 64-bit integer, not double. + * Load/store through uint64_t (the actual type) and reinterpret the vector + * register to/from f64 -- casting fpr* to double* and using vld1q_f64/vst1q_f64 + * would be a strict-aliasing violation that can miscompile at -O2/-O3. */ +#define FALCON_VLD(pf) \ + vreinterpretq_f64_u64(vld1q_u64((const uint64_t*)(const void*)(pf))) +#define FALCON_VST(pf, v) \ + vst1q_u64((uint64_t*)(void*)(pf), vreinterpretq_u64_f64(v)) + /* ------------------------------------------------------------------------- */ /* Forward FFT */ /* ------------------------------------------------------------------------- */ void falcon_FFT(fpr* f, unsigned logn) { - double* fd = (double*)f; unsigned u; size_t t, n, hn, m; @@ -102,16 +111,16 @@ void falcon_FFT(fpr* f, unsigned logn) float64x2_t vsr = vdupq_n_f64(falcon_neon_d(s_re)); float64x2_t vsi = vdupq_n_f64(falcon_neon_d(s_im)); for (j = j1; j < j2; j += 2) { - float64x2_t xr = vld1q_f64(fd + j); - float64x2_t xi = vld1q_f64(fd + j + hn); - float64x2_t yr = vld1q_f64(fd + j + ht); - float64x2_t yi = vld1q_f64(fd + j + ht + hn); + float64x2_t xr = FALCON_VLD(f + j); + float64x2_t xi = FALCON_VLD(f + j + hn); + float64x2_t yr = FALCON_VLD(f + j + ht); + float64x2_t yi = FALCON_VLD(f + j + ht + hn); float64x2_t tr, ti; FALCON_VCMUL(tr, ti, yr, yi, vsr, vsi); - vst1q_f64(fd + j, vaddq_f64(xr, tr)); - vst1q_f64(fd + j + hn, vaddq_f64(xi, ti)); - vst1q_f64(fd + j + ht, vsubq_f64(xr, tr)); - vst1q_f64(fd + j + ht + hn, vsubq_f64(xi, ti)); + FALCON_VST(f + j, vaddq_f64(xr, tr)); + FALCON_VST(f + j + hn, vaddq_f64(xi, ti)); + FALCON_VST(f + j + ht, vsubq_f64(xr, tr)); + FALCON_VST(f + j + ht + hn, vsubq_f64(xi, ti)); } } else { @@ -135,7 +144,6 @@ void falcon_FFT(fpr* f, unsigned logn) void falcon_iFFT(fpr* f, unsigned logn) { - double* fd = (double*)f; int u; size_t n = (size_t)1 << logn, hn = n >> 1; @@ -151,18 +159,18 @@ void falcon_iFFT(fpr* f, unsigned logn) float64x2_t vsr = vdupq_n_f64(falcon_neon_d(s_re)); float64x2_t vsi = vdupq_n_f64(falcon_neon_d(s_im)); for (j = j1; j < j2; j += 2) { - float64x2_t ar = vld1q_f64(fd + j); - float64x2_t ai = vld1q_f64(fd + j + hn); - float64x2_t br = vld1q_f64(fd + j + t); - float64x2_t bi = vld1q_f64(fd + j + t + hn); + float64x2_t ar = FALCON_VLD(f + j); + float64x2_t ai = FALCON_VLD(f + j + hn); + float64x2_t br = FALCON_VLD(f + j + t); + float64x2_t bi = FALCON_VLD(f + j + t + hn); float64x2_t dr = vsubq_f64(ar, br); float64x2_t di = vsubq_f64(ai, bi); float64x2_t pr, pi; - vst1q_f64(fd + j, vaddq_f64(ar, br)); - vst1q_f64(fd + j + hn, vaddq_f64(ai, bi)); + FALCON_VST(f + j, vaddq_f64(ar, br)); + FALCON_VST(f + j + hn, vaddq_f64(ai, bi)); FALCON_VCMUL(pr, pi, dr, di, vsr, vsi); - vst1q_f64(fd + j + t, pr); - vst1q_f64(fd + j + t + hn, pi); + FALCON_VST(f + j + t, pr); + FALCON_VST(f + j + t + hn, pi); } } else { @@ -184,7 +192,7 @@ void falcon_iFFT(fpr* f, unsigned logn) float64x2_t vni = vdupq_n_f64(falcon_neon_d(ni)); size_t j; for (j = 0; j < n; j += 2) { - vst1q_f64(fd + j, vmulq_f64(vld1q_f64(fd + j), vni)); + FALCON_VST(f + j, vmulq_f64(FALCON_VLD(f + j), vni)); } } else { diff --git a/wolfcrypt/src/wc_falcon_sampler.c b/wolfcrypt/src/wc_falcon_sampler.c index d33a2564fc..8fece5e3c0 100644 --- a/wolfcrypt/src/wc_falcon_sampler.c +++ b/wolfcrypt/src/wc_falcon_sampler.c @@ -121,6 +121,12 @@ static int falcon_prng_refill(falcon_prng* p) int ret = wc_Shake256_SqueezeBlocks(&p->shake, p->buf, FALCON_PRNG_BLOCKS); p->ptr = 0; p->len = (ret == 0) ? (word32)FALCON_PRNG_BUFLEN : 0; + /* Latch the first failure. get_u8/get_u64 have no error return, so a squeeze + * failure is made sticky here and checked by the signer (falcon_sign_core), + * which rejects any signature produced from an invalid PRNG state instead of + * consuming stale buffer bytes. */ + if (ret != 0 && p->err == 0) + p->err = ret; return ret; } @@ -140,6 +146,7 @@ int falcon_prng_init(falcon_prng* p, WC_RNG* rng) p->ptr = 0; p->len = 0; + p->err = 0; ForceZero(seed, (word32)sizeof(seed)); if (ret == 0) @@ -152,6 +159,9 @@ byte falcon_prng_get_u8(falcon_prng* p) { byte v; + /* On a refill failure len becomes 0 and p->err is latched; the buffer read + * below stays in bounds (ptr reset to 0) but yields a discarded value -- + * falcon_sign_core checks p->err and rejects the resulting signature. */ if (p->ptr + 1U > p->len) (void)falcon_prng_refill(p); v = p->buf[p->ptr]; diff --git a/wolfcrypt/src/wc_falcon_sign.c b/wolfcrypt/src/wc_falcon_sign.c index e368c18889..ba1c6da2bb 100644 --- a/wolfcrypt/src/wc_falcon_sign.c +++ b/wolfcrypt/src/wc_falcon_sign.c @@ -44,6 +44,15 @@ #define FALCON_Q 12289 +/* Safety bound on the sign-tree restart loop. The per-iteration restart + * probability is well below 1 (expected iteration count ~1), so the chance of + * legitimately reaching even a few dozen restarts is astronomically small; this + * bound is never approached in normal operation. It only guarantees termination + * if the sampler can no longer produce accepting candidates (e.g. a failed PRNG + * squeeze), in which case falcon_sign_core additionally rejects the result via + * the PRNG's sticky error. */ +#define FALCON_SIGN_MAX_RESTARTS 4096UL + /* IEEE-754 binary64 bit patterns (the fpr seam carries doubles as word64). * These mirror named constants from the reference fpr.h that are not part of * the public wc_falcon_fpr.h API. fpr_invsqrt2 / fpr_invsqrt8 ARE exported by @@ -656,27 +665,44 @@ int falcon_do_sign_tree(falcon_samplerZ samp, void* samp_ctx, sword16* s2, } /* Loop until the candidate (s1, s2) is short enough. With degrees 512 and - * 1024 a restart is very rare. */ - for (;;) { - if (do_sign_tree_once(samp, samp_ctx, s2, expanded, hm, logn, tmp)) { - return 0; - } + * 1024 a restart is very rare (expected iteration count is ~1), so the + * bound below is astronomically beyond any legitimate run; it exists only + * so a wedged sampler -- e.g. a PRNG whose SHAKE256 squeeze started failing, + * yielding candidates that never pass the norm bound -- terminates instead + * of spinning forever. */ + { + unsigned long iter; + for (iter = 0; iter < FALCON_SIGN_MAX_RESTARTS; iter++) { + if (do_sign_tree_once(samp, samp_ctx, s2, expanded, hm, logn, tmp)) { + return 0; + } #ifdef WOLFSSL_FALCON_SIGN_STATS - /* Optional instrumentation for test harnesses: counts the rare - * ffSampling restarts. Not compiled into production builds. */ - extern unsigned long falcon_sign_restart_count; - falcon_sign_restart_count++; + /* Optional instrumentation for test harnesses: counts the rare + * ffSampling restarts. Not compiled into production builds. */ + extern unsigned long falcon_sign_restart_count; + falcon_sign_restart_count++; #endif + } } + /* Exhausted the restart bound: treat as an operational failure. */ + return WC_FAILURE; } int falcon_sign_core(falcon_sampler_ctx* spc, const fpr* expanded, const word16* c, sword16* s2, fpr* tmp, unsigned logn) { + int ret; + if (spc == NULL) { return BAD_FUNC_ARG; } - return falcon_do_sign_tree(falcon_sampler_z, spc, s2, expanded, c, logn, tmp); + ret = falcon_do_sign_tree(falcon_sampler_z, spc, s2, expanded, c, logn, tmp); + /* Reject the signature if the sampler's PRNG failed at any point: the + * squeezed bytes it consumed would be invalid, so the result is unsafe. */ + if (ret == 0 && spc->p.err != 0) { + ret = spc->p.err; + } + return ret; } #endif /* HAVE_FALCON && !WOLFSSL_FALCON_VERIFY_ONLY */ diff --git a/wolfssl/wolfcrypt/include.am b/wolfssl/wolfcrypt/include.am index d10b3cb3dd..28ac4c42f2 100644 --- a/wolfssl/wolfcrypt/include.am +++ b/wolfssl/wolfcrypt/include.am @@ -253,3 +253,17 @@ if BUILD_RISCV_ASM nobase_include_HEADERS+= wolfssl/wolfcrypt/port/riscv/riscv-64-asm.h endif +if BUILD_FALCON +# Internal Falcon headers (the public API is wolfssl/wolfcrypt/falcon.h, listed +# unconditionally above). Automake distributes both branches of a conditional, +# so these are still included in "make dist" when Falcon is disabled. +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_bigint.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_codec.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_fft.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_fpr.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_keygen.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_poly.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_sampler.h +nobase_include_HEADERS+= wolfssl/wolfcrypt/wc_falcon_sign.h +endif + diff --git a/wolfssl/wolfcrypt/wc_falcon_sampler.h b/wolfssl/wolfcrypt/wc_falcon_sampler.h index 8e8868cbf3..f8d9358f2a 100644 --- a/wolfssl/wolfcrypt/wc_falcon_sampler.h +++ b/wolfssl/wolfcrypt/wc_falcon_sampler.h @@ -70,6 +70,7 @@ typedef struct falcon_prng { byte buf[FALCON_PRNG_BUFLEN];/* squeezed stream buffer */ word32 ptr; /* index of next byte to consume */ word32 len; /* number of valid bytes in buf */ + int err; /* sticky: first refill error, or 0 */ } falcon_prng; /* Sampler context: the PRNG plus the parameter-set-dependent sigma_min. */