From 91ebd89d7657b66001bc976aff87a7ca1e7eb478 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Mon, 6 Jul 2026 09:04:16 +0200 Subject: [PATCH] Falcon: address review feedback (zephyr sources, configure sub-options, footprint) Four fixes from PR review: - zephyr/CMakeLists.txt: the native port split falcon.c into wc_falcon_*.c translation units; add the portable sources so a Zephyr build with Falcon links. x86-64 asm/AVX2 and the NEON backend are left out (not selected by any Zephyr config). - configure.ac: fold the standalone --enable-falcon-{asm,double,avx2,neon} switches into comma-separated sub-options of --enable-falcon (e.g. --enable-falcon=avx2), matching the common wolfSSL idiom. avx2/neon imply the double backend after arch-gating so ignoring an unsupported vector backend does not clobber an explicit 'double'. Sweep the qemu-falcon-neon doc to the new spelling. - configure.ac: align the Falcon line in the two feature summaries. - falcon.c/falcon.h: drop the duplicate public-key copy kept behind the private key. Its only remaining reader was wc_falcon_check_key, whose compare was against a copy of the same bytes and so could never detect a real mismatch; wc_falcon_export_private already rebuilds the concat layout on demand. Shrink key->k from FALCON_MAX_PRV_KEY_SIZE to FALCON_MAX_KEY_SIZE (saves 1793 bytes per key at level 5). check_key now verifies both halves are present and documents a full cryptographic cross-check as a follow-up. Update the unit test that relied on the old in-memory-copy compare. --- IDE/qemu-falcon-neon/README.md | 2 +- configure.ac | 132 +++++++++++++++++---------------- tests/api/test_falcon.c | 10 +-- wolfcrypt/src/falcon.c | 66 +++-------------- wolfssl/wolfcrypt/falcon.h | 5 +- zephyr/CMakeLists.txt | 9 +++ 6 files changed, 95 insertions(+), 129 deletions(-) diff --git a/IDE/qemu-falcon-neon/README.md b/IDE/qemu-falcon-neon/README.md index 4420a4f800..561624710e 100644 --- a/IDE/qemu-falcon-neon/README.md +++ b/IDE/qemu-falcon-neon/README.md @@ -33,4 +33,4 @@ grep -E 'fmla\s+v[0-9]+\.2d' IDE/qemu-falcon-neon/app-falcon-neon.dis ``` In a wolfSSL library build, the NEON FFT is enabled with -`--enable-falcon-neon` (AArch64; implies `--enable-falcon-double`). +`--enable-falcon=neon` (AArch64; implies the `double` fpr backend). diff --git a/configure.ac b/configure.ac index 91400a006b..7ea62eaa67 100644 --- a/configure.ac +++ b/configure.ac @@ -1816,87 +1816,89 @@ AC_ARG_WITH([liboqs], # section below). Because the algorithm is not yet standardized and its API name # is subject to change, it requires --enable-experimental (checked in the CFLAG # section, once all backend sub-options have been resolved). +# +# fpr/FFT backend selection follows the common wolfSSL comma-list idiom as +# sub-options of --enable-falcon (e.g. --enable-falcon=asm, --enable-falcon=avx2) +# rather than standalone --enable-falcon-* switches. Recognised sub-options: +# asm - per-architecture assembly fpr backend (x86-64 SSE2 only); the +# portable constant-time integer emulation remains the default. +# double - inline native-double fpr backend: the fpr ops become static-inline +# C double operations, so the FFT/sampler inline them and keep values +# in FP registers (no per-op call). Fastest backend on any platform +# with a constant-time hardware double FPU (signing ~2.5x faster than +# the out-of-line asm backend, ~16x faster than the portable +# emulation), but like asm it relies on the native FPU's rounding +# behavior. Mutually exclusive with asm. +# avx2 - AVX2 (4-wide __m256d + FMA) vectorized FFT for the signing path +# (x86-64); implies double. The AVX2 functions carry target("avx2,fma") +# attributes, so the TU builds under a baseline -march, but the host +# must support AVX2+FMA at run time. Signing is sampler-bound, so this +# is a modest (~1.1x) end-to-end speedup. +# neon - AArch64 NEON (2-wide float64x2_t + FMA) vectorized FFT; implies +# double. Advanced SIMD is part of the ARMv8-A baseline, so no special +# -march is required. AC_ARG_ENABLE([falcon], - [AS_HELP_STRING([--enable-falcon],[Enable Falcon post-quantum signatures (native, no liboqs; requires --enable-experimental) (default: disabled)])], + [AS_HELP_STRING([--enable-falcon@<:@=OPTS@:>@],[Enable Falcon post-quantum signatures (native, no liboqs; requires --enable-experimental). OPTS is a comma-separated list of fpr/FFT backends: asm, double, avx2, neon (default: disabled)])], [ ENABLED_FALCON=$enableval ], [ ENABLED_FALCON=no ]) -# --enable-falcon-asm selects the per-architecture assembly fpr backend for -# Falcon (currently x86-64 SSE2 only); the portable constant-time integer -# emulation remains the default. It implies --enable-falcon. -AC_ARG_ENABLE([falcon-asm], - [AS_HELP_STRING([--enable-falcon-asm],[Enable Falcon x86-64 assembly fpr backend (default: disabled)])], - [ ENABLED_FALCON_ASM=$enableval ], - [ ENABLED_FALCON_ASM=no ]) -if test "$ENABLED_FALCON_ASM" = "yes"; then - case $host_cpu in - *x86_64*|*amd64*) ENABLED_FALCON=yes ;; - *) AC_MSG_WARN([--enable-falcon-asm is only supported on x86-64; ignoring.]) - ENABLED_FALCON_ASM=no ;; - esac -fi +ENABLED_FALCON_ASM=no +ENABLED_FALCON_DOUBLE=no +ENABLED_FALCON_AVX2=no +ENABLED_FALCON_NEON=no -# --enable-falcon-double selects the inline native-double fpr backend: the fpr -# ops become static-inline C double operations, so the FFT/sampler inline them -# and keep values in FP registers (no per-op call). This is the fastest backend -# on any platform with a constant-time hardware double FPU (signing ~2.5x faster -# than the out-of-line asm backend, ~16x faster than the portable emulation), -# but like --enable-falcon-asm it relies on the native FPU's rounding behavior. -# It implies --enable-falcon and is mutually exclusive with --enable-falcon-asm. -AC_ARG_ENABLE([falcon-double], - [AS_HELP_STRING([--enable-falcon-double],[Enable Falcon inline native-double fpr backend (default: disabled)])], - [ ENABLED_FALCON_DOUBLE=$enableval ], - [ ENABLED_FALCON_DOUBLE=no ]) -if test "$ENABLED_FALCON_DOUBLE" = "yes"; then - if test "$ENABLED_FALCON_ASM" = "yes"; then - AC_MSG_ERROR([--enable-falcon-double and --enable-falcon-asm are mutually exclusive.]) - fi +# Parse the comma-separated backend sub-options. AC_ARG_ENABLE yields the plain +# "yes"/"no" for a bare --enable-falcon/--disable-falcon; anything else is a +# backend list. The double implication of avx2/neon is applied after arch-gating +# so ignoring an unsupported vector backend does not clobber an explicit 'double'. +if test "$ENABLED_FALCON" != "no" && test "$ENABLED_FALCON" != "yes"; then + OIFS="$IFS" + IFS=',' + for opt in $ENABLED_FALCON; do + case "$opt" in + yes) ;; + asm) ENABLED_FALCON_ASM=yes ;; + double) ENABLED_FALCON_DOUBLE=yes ;; + avx2) ENABLED_FALCON_AVX2=yes ;; + neon) ENABLED_FALCON_NEON=yes ;; + *) AC_MSG_ERROR([Unknown falcon option: $opt. Valid options: asm, double, avx2, neon]) ;; + esac + done + IFS="$OIFS" ENABLED_FALCON=yes fi -# --enable-falcon-avx2 adds the AVX2 (4-wide __m256d + FMA) vectorized FFT and -# FFT-domain polynomial operations for the signing path (x86-64). It composes -# with --enable-falcon-double (which it implies). The AVX2 functions carry -# target("avx2,fma") attributes, so the TU builds under a baseline -march; the -# host must support AVX2+FMA at run time. Signing is sampler-bound, so the FFT -# vectorization yields a modest (~1.1x) end-to-end speedup. -AC_ARG_ENABLE([falcon-avx2], - [AS_HELP_STRING([--enable-falcon-avx2],[Enable Falcon x86-64 AVX2 vectorized FFT (default: disabled)])], - [ ENABLED_FALCON_AVX2=$enableval ], - [ ENABLED_FALCON_AVX2=no ]) +# Resolve backend arch-gating: asm and avx2 are x86-64 only; neon is AArch64 +# only. An unsupported backend is warned and ignored. +if test "$ENABLED_FALCON_ASM" = "yes"; then + case $host_cpu in + *x86_64*|*amd64*) ;; + *) AC_MSG_WARN([falcon 'asm' backend is only supported on x86-64; ignoring.]) + ENABLED_FALCON_ASM=no ;; + esac +fi if test "$ENABLED_FALCON_AVX2" = "yes"; then case $host_cpu in - *x86_64*|*amd64*) - if test "$ENABLED_FALCON_ASM" = "yes"; then - AC_MSG_ERROR([--enable-falcon-avx2 and --enable-falcon-asm are mutually exclusive.]) - fi - ENABLED_FALCON=yes - ENABLED_FALCON_DOUBLE=yes ;; - *) AC_MSG_WARN([--enable-falcon-avx2 is only supported on x86-64; ignoring.]) + *x86_64*|*amd64*) ;; + *) AC_MSG_WARN([falcon 'avx2' backend is only supported on x86-64; ignoring.]) ENABLED_FALCON_AVX2=no ;; esac fi - -# --enable-falcon-neon adds the AArch64 NEON (2-wide float64x2_t + FMA) vectorized -# FFT for the signing path. It composes with --enable-falcon-double (which it -# implies). AArch64 Advanced SIMD is part of the ARMv8-A baseline, so no special -# -march is required. -AC_ARG_ENABLE([falcon-neon], - [AS_HELP_STRING([--enable-falcon-neon],[Enable Falcon AArch64 NEON vectorized FFT (default: disabled)])], - [ ENABLED_FALCON_NEON=$enableval ], - [ ENABLED_FALCON_NEON=no ]) if test "$ENABLED_FALCON_NEON" = "yes"; then case $host_cpu in - *aarch64*|*arm64*) - if test "$ENABLED_FALCON_ASM" = "yes"; then - AC_MSG_ERROR([--enable-falcon-neon and --enable-falcon-asm are mutually exclusive.]) - fi - ENABLED_FALCON=yes - ENABLED_FALCON_DOUBLE=yes ;; - *) AC_MSG_WARN([--enable-falcon-neon is only supported on AArch64; ignoring.]) + *aarch64*|*arm64*) ;; + *) AC_MSG_WARN([falcon 'neon' backend is only supported on AArch64; ignoring.]) ENABLED_FALCON_NEON=no ;; esac fi +# avx2/neon vectorized FFT requires the inline native-double fpr backend. +if test "$ENABLED_FALCON_AVX2" = "yes" || test "$ENABLED_FALCON_NEON" = "yes"; then + ENABLED_FALCON_DOUBLE=yes +fi +# asm and the double-based backends select different fpr representations. +if test "$ENABLED_FALCON_DOUBLE" = "yes" && test "$ENABLED_FALCON_ASM" = "yes"; then + AC_MSG_ERROR([falcon 'double' (also implied by 'avx2'/'neon') and 'asm' backends are mutually exclusive.]) +fi # MLKEM @@ -13209,7 +13211,7 @@ echo " * XMSS: $ENABLED_XMSS" echo " * SLH-DSA $ENABLED_SLHDSA" echo " * MLKEM: $ENABLED_MLKEM" echo " * ML-DSA: $ENABLED_MLDSA" -echo " * Falcon: $ENABLED_FALCON" +echo " * Falcon: $ENABLED_FALCON" echo " * ECCSI $ENABLED_ECCSI" echo " * SAKKE $ENABLED_SAKKE" echo " * ASN: $ENABLED_ASN" @@ -13267,7 +13269,7 @@ echo " * Persistent session cache: $ENABLED_SAVESESSION" echo " * Persistent cert cache: $ENABLED_SAVECERT" echo " * Atomic User Record Layer: $ENABLED_ATOMICUSER" echo " * Public Key Callbacks: $ENABLED_PKCALLBACKS" -echo " * Falcon: $ENABLED_FALCON" +echo " * Falcon: $ENABLED_FALCON" echo " * Whitewood netRandom: $ENABLED_WNR" echo " * Server Name Indication: $ENABLED_SNI" echo " * ALPN: $ENABLED_ALPN" diff --git a/tests/api/test_falcon.c b/tests/api/test_falcon.c index c41305b6fb..f8a181d149 100644 --- a/tests/api/test_falcon.c +++ b/tests/api/test_falcon.c @@ -451,10 +451,6 @@ int test_wc_falcon_check_key(void) prvLen = FALCON_MAX_KEY_SIZE; ExpectIntEQ(wc_falcon_export_private_only(&key, prv, &prvLen), 0); - /* Corrupt the standalone public copy: check_key compares it to the - * public bytes stored behind the private key, so this mismatches. */ - key.p[0] ^= 0xFF; - ExpectIntEQ(wc_falcon_check_key(&key), WC_NO_ERR_TRACE(PUBLIC_KEY_E)); wc_falcon_free(&key); /* Public only (no private) -> PUBLIC_KEY_E. */ @@ -473,10 +469,8 @@ int test_wc_falcon_check_key(void) ExpectIntEQ(wc_falcon_check_key(&key), WC_NO_ERR_TRACE(PUBLIC_KEY_E)); wc_falcon_free(&key); - /* Public imported FIRST, then a raw (non-concat) private key: the - * public copy stored behind the private key must be synced so - * check_key passes. Regression for the raw-import path not calling - * falcon_store_pub_behind_priv. */ + /* Public imported FIRST, then a raw (non-concat) private key: both + * halves are now present, so check_key passes. */ XMEMSET(&key, 0, sizeof(key)); ExpectIntEQ(wc_falcon_init(&key), 0); ExpectIntEQ(wc_falcon_set_level(&key, level), 0); diff --git a/wolfcrypt/src/falcon.c b/wolfcrypt/src/falcon.c index 29a2650214..2760ab1167 100644 --- a/wolfcrypt/src/falcon.c +++ b/wolfcrypt/src/falcon.c @@ -39,26 +39,6 @@ #include #endif -/* Store a second copy of the public key in key->k immediately after the private - * key, reproducing the historical concat(private,public) layout that - * wc_falcon_check_key compares against. No-op unless both halves are set. - * Defined unconditionally: wc_falcon_import_public (a verify-only operation) - * calls it. */ -static void falcon_store_pub_behind_priv(falcon_key* key) -{ - if (!key->pubKeySet || !key->prvKeySet) { - return; - } - if (key->level == 1) { - XMEMCPY(key->k + FALCON_LEVEL1_KEY_SIZE, key->p, - FALCON_LEVEL1_PUB_KEY_SIZE); - } - else if (key->level == 5) { - XMEMCPY(key->k + FALCON_LEVEL5_KEY_SIZE, key->p, - FALCON_LEVEL5_PUB_KEY_SIZE); - } -} - #ifndef WOLFSSL_FALCON_VERIFY_ONLY /* Generate a new Falcon key pair into key (key->level must be set first). * @@ -97,9 +77,6 @@ int wc_falcon_make_key(falcon_key* key, WC_RNG* rng) ret = NO_VALID_DEVID; #else ret = falcon_native_make_key(key, rng); - if (ret == 0) { - falcon_store_pub_behind_priv(key); - } #endif return ret; } @@ -448,8 +425,6 @@ int wc_falcon_import_public(const byte* in, word32 inLen, XMEMCPY(key->p, in, inLen); key->pubKeySet = 1; - /* Keep the concat(private,public) copy in sync if a private key is loaded. */ - falcon_store_pub_behind_priv(key); return 0; } @@ -505,13 +480,6 @@ int wc_falcon_import_private_only(const byte* priv, word32 privSz, key->pubKeySet = 1; } - /* Sync the public copy kept behind the private key whenever both halves are - * present. This also covers the raw-size case where a public key was - * imported first: without it key->k + KEY_SIZE would stay zero and - * wc_falcon_check_key would wrongly return PUBLIC_KEY_E. No-op when no - * public key is set. */ - falcon_store_pub_behind_priv(key); - return 0; } @@ -671,18 +639,23 @@ int wc_falcon_export_key(falcon_key* key, byte* priv, word32 *privSz, return ret; } -/* Check the public key of the falcon key matches the private key. +/* Check that the falcon key has a matching private/public key pair present. * * key [in] Falcon private/public key. - * returns BAD_FUNC_ARG when key is NULL, - * PUBLIC_KEY_E when the public key is not set or doesn't match, - * other -ve value on hash failure, + * returns BAD_FUNC_ARG when key is NULL or the level is unset, + * PUBLIC_KEY_E when either the public or private half is not set, * 0 otherwise. + * + * Note: this verifies both halves of the pair are loaded. It does not yet + * perform a full cryptographic cross-check (recomputing the public key h from + * the private (f, g) and comparing it against the stored public key); that is a + * TODO once a standalone public-key-from-private helper is exposed by the native + * core. The previous implementation compared the stored public key against a + * duplicate copy kept behind the private key, which was always a copy of the + * same bytes and so could never detect a mismatch. */ int wc_falcon_check_key(falcon_key* key) { - int ret = 0; - if (key == NULL) { return BAD_FUNC_ARG; } @@ -695,22 +668,7 @@ int wc_falcon_check_key(falcon_key* key) return PUBLIC_KEY_E; } - /* The public key is also decoded and stored within the private key buffer - * behind the private key. Hence, we can compare both stored public keys. */ - if (key->level == 1) { - ret = XMEMCMP(key->p, key->k + FALCON_LEVEL1_KEY_SIZE, - FALCON_LEVEL1_PUB_KEY_SIZE); - } - else if (key->level == 5) { - ret = XMEMCMP(key->p, key->k + FALCON_LEVEL5_KEY_SIZE, - FALCON_LEVEL5_PUB_KEY_SIZE); - } - - if (ret != 0) { - ret = PUBLIC_KEY_E; - } - - return ret; + return 0; } /* Returns the size of a falcon private key. diff --git a/wolfssl/wolfcrypt/falcon.h b/wolfssl/wolfcrypt/falcon.h index 15a0b5dffa..faaebbc9fc 100644 --- a/wolfssl/wolfcrypt/falcon.h +++ b/wolfssl/wolfcrypt/falcon.h @@ -125,7 +125,10 @@ struct falcon_key { #endif byte p[FALCON_MAX_PUB_KEY_SIZE]; - byte k[FALCON_MAX_PRV_KEY_SIZE]; + /* Private key only: the secret polynomials (header | f | g | F). The public + * key is held separately in p[]; the concat(priv,pub) layout is rebuilt on + * demand by wc_falcon_export_private, so no duplicate copy is kept here. */ + byte k[FALCON_MAX_KEY_SIZE]; }; #ifndef WC_FALCONKEY_TYPE_DEFINED diff --git a/zephyr/CMakeLists.txt b/zephyr/CMakeLists.txt index eaf69221ef..8835c000d0 100644 --- a/zephyr/CMakeLists.txt +++ b/zephyr/CMakeLists.txt @@ -79,6 +79,15 @@ if(CONFIG_WOLFSSL) zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/ed448.c) zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/error.c) zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/falcon.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_bigint.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_codec.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_fft.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_fpr.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_keygen.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_poly.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_sampler.c) + zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/wc_falcon_sign.c) zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/fe_448.c) zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/fe_low_mem.c) zephyr_library_sources(${ZEPHYR_CURRENT_MODULE_DIR}/wolfcrypt/src/fe_operations.c)