Zephyr fixes - #11401
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11401
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. Reported findings require changes before merge.
|
95cdbe5 to
883a111
Compare
3a29a95 to
33f3aa7
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11401
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
33f3aa7 to
8efa414
Compare
…x-M3 WOLFSSL_ARM_ARCH_7M selects the UMAAL-free variants in sp_cortexm.c and the thumb2 assembly, but was derived from __ARM_ARCH_7M__, which names the Cortex-M3 alone. UMAAL belongs to the DSP extension, optional on ARMv8-M, so a part like the Cortex-M33 in the NXP RW612 has neither UMAAL nor __ARM_ARCH_7M__ and took the UMAAL path anyway. Key on the absence of __ARM_FEATURE_DSP, which toolchains define exactly when the extension is present. Verified with gcc for M0, M3, M4, M7, M23, M33 and M33+nodsp. The __ARM_ARCH_7M__ arm stays, so nothing that worked before changes.
The WOLFSSL_ZEPHYR block includes <zephyr/kernel.h> and friends and declares z_realloc. Assembly reaches settings.h through libwolfssl_sources_asm.h, so the assembler got all of it and stopped at the first C declaration: stddef.h:160: Error: no such instruction: `typedef long int ptrdiff_t' That makes every wolfSSL .S file unbuildable on Zephyr, on any architecture - reproduced on x86_64 and on a Cortex-M33. Ports using the *_c.c inline variants never noticed, since those are compiled as C. Guard the block with __ASSEMBLER__, as settings.h already does for the STM32MP13 header. No effect on any C translation unit.
…s them
The nine div-word helpers bind a variable to rax with the bare asm keyword,
which is a GNU extension rather than an ISO C one and so is unavailable under
-std=c17:
sp_x86_64.c:597: error: expected '=', ',', ';', 'asm' or '__attribute__'
before 'asm'
Zephyr compiles with -std=c17, but any strict-ISO build hits it. __asm__ is
the spelling GCC and Clang keep regardless of -std, and is what the next line
of each function already uses.
CPUID's AVX and AVX-512 bits say the silicon has the unit. Executing those instructions also needs the OS to have enabled extended state - CR4.OSXSAVE plus the matching XCR0 components - and an OS that does not context-switch those registers leaves them clear, so the instruction raises #UD whatever CPUID says. cpuid_set_flags() tested the feature bits alone. Zephyr is such a system: its x86 context switch is fxsave/fxrstor and it never sets CR4.OSXSAVE, so the first vector SHA-256 or AES-GCM path crashes. Gate each family on OSXSAVE plus the XCR0 components it needs, in the order Intel documents - XGETBV is only legal once OSXSAVE is set. The masks have to differ because the AVX-512 dispatch sites branch on IS_INTEL_AVX512 && IS_INTEL_VAES without consulting AVX1 or AVX2, so gating only the 256-bit flags would leave the 512-bit paths reachable on exactly the systems this protects. Nothing changes where an OS enables XSAVE, which is every mainstream one.
The ARMv8 AES and SHA-256 ports emit aese, aesmc and the sha256 instructions
with no directive of their own, so they assemble only when the command line
already names a CPU that has the extension:
armv8-aes-asm.S: Error: selected processor does not support
`aese v0.16b,v1.16b'
Zephyr lands there: it derives -mcpu from the board and has no symbol for the
extension, and naming a CPU in the module would retarget every other board.
Add .arch_extension crypto. The .S and .asm outputs carry one at file scope;
the C output cannot, because clang gives each __asm__ block a fresh subtarget,
so there it leads every block, as the SHA-3 ports already do.
All three sit inside WOLFSSL_ARMASM_NO_HW_CRYPTO, and that guard now carries
the whole safety argument, which is the part worth recording. The directive
removes the assembler's refusal, so setting the macro becomes the integrator's
job rather than something the toolchain enforces: with no ID registers to
read, cpuid.c reports AES, PMULL and SHA-256 present whenever it is unset, and
a base-ARMv8 build then dispatches them on a core where FEAT_AES is optional.
FrodoKEM's aese deliberately gets no directive for the same reason - it has no
such guard to opt out through.
Regenerated from the scripts repository, which carries the matching change.
The SHA-3, SHA-512 and FrodoKEM ARM64 ports guard their .arch_extension sha3
with __APPLE__, assuming every other assembler learns the extension from the
command line. It does not hold:
armv8-sha3-asm.S: Error: selected processor does not support
`eor3 v31.16b,v0.16b,v5.16b,v10.16b'
Emit it unconditionally; it is a no-op where the extension is already on, so
the Apple path is unchanged. The AES port needs it too and had none: its
AES-GCM EOR3 variants emit eor3, while the only directive in the file is the
crypto one added earlier in this branch, which does not cover sha3.
Regenerated from the scripts repository, which carries the matching change.
The ARM64 ML-KEM port emits eor3, which is not in the base ARMv8-A a toolchain
may be invoked with, and the file carries no directive:
armv8-mlkem-asm.S: Error: selected processor does not support
`eor3 v31.16b,v0.16b,v5.16b,v10.16b'
The directive sits inside WOLFSSL_ARMASM_CRYPTO_SHA3, the guard selecting the
variant that needs it, so a build that has not opted in never asks for it.
The same file's sqrdmlsh deliberately gets none. RDMA is opt-out
(WOLFSSL_AARCH64_NO_SQRDMLSH) where SHA-3 is opt-in, and on a target with no
way to read the ID registers cpuid.c reports it present whenever that macro is
unset. A directive there would turn a build the assembler correctly rejects
into an image that traps on the first sqrdmlsh, so the mnemonic is left to
answer to -mcpu.
L_mlkem_aarch64_consts sat outside the WOLFSSL_HAVE_MLKEM conditional while
zetas, zetas_inv and q sat inside it. With ML-KEM off the C output declares a
static const nothing references:
armv8-mlkem-asm_c.c:37: error: 'L_mlkem_aarch64_consts' defined but not used
[-Werror=unused-const-variable=]
Zephyr compiles with -Werror, so armv8-mlkem-asm_c.c was unbuildable on any
aarch64 target without ML-KEM, which is every one by default.
The cause is in the generator: initialize() emitted the constant but write()
is what opens the guard and runs later. Fixed there with a define_consts()
called from write(), mirroring the existing define_q(); these files are the
regenerated result.
8efa414 to
cb95566
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11401
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfssl-src, wolfssl-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| #define MAX_ECC_BITS_USE SP_INT_BITS | ||
| #else | ||
| #define MAX_ECC_BITS_USE MAX_ECC_BITS_NEEDED | ||
| #define MAX_ECC_BITS_USE (MAX_ECC_BITS + MAX_ECC_BITS_EXTRA) |
There was a problem hiding this comment.
ECC ceiling outgrows deterministic-k buffers · Buffer overflows
The new MAX_ECC_BITS_USE lets a 256-bit custom key with a 257-bit order reach deterministic signing, where qLen==33 is written into x[MAX_ECC_BYTES] (32 bytes). Unlike #8192's timing leak, this is an OOB write.
Related known finding #8192 (similar but distinct): Both affect wc_ecc_gen_deterministic_k, but #8192's faulting operations are variable-time private-scalar sizing/serialization and its root cause is a timing leak. This candidate writes a 33-byte order-derived value into 32-byte buffers after the ECC bit ceiling expands; it is an OOB-write size-capacity defect requiring buffer sizing/bounds fixes, not #8192's constant-time serialization patch.
Suggested fix: Define a byte-capacity macro that includes MAX_ECC_BITS_EXTRA, use it for x and h1, and reject larger qLen values before serialization.
Basis: ISO/IEC 9899:2018 §§6.5.6¶8 and 6.5.3.2¶4 make dereferencing beyond an array object undefined.
There was a problem hiding this comment.
Real, reproduced, and fixed in 5e025375a (folded into the ceiling commit). secp160r1 is enough: 21 order bytes written into a 20-byte x, the last one the low byte of the private scalar.
It predates the PR - ASAN reports it on the merge base at 64-bit digits, where MP_BITS_CNT() rounds 160 and 161 to the same count. At 32-bit digits the old ceiling rejected the curve at wc_ecc_make_key() before signing reached it, so this commit is what makes it reachable, and it fixes it here.
x and h1 are now sized with MAX_ECC_ORDER_BYTES, plus a guard on the order size. It is never smaller than MAX_ECC_BYTES and larger only when MAX_ECC_BITS is a multiple of 8 and the extra bit applies, so a P-521 build stays at 66.
A WOLFSSL_CUSTOM_CURVES build fails at runtime on its own largest enabled curve: ecc_test_curve_size 64 failed! (WC_KEY_SIZE_E, -234) Not a Brainpool quirk - a bp256-only build fails on bp256, bp256+bp384 on bp384. The ceiling is off by exactly one bit: ECC_KEY_MAX_BITS() takes a "dp->size * 8 + 1" variant for orders a bit larger than their prime, while MAX_ECC_BITS_NEEDED is the plain curve size, and MP_BITS_CNT() rounds up to whole digits, so that bit puts the key one digit past the ceiling. Name it MAX_ECC_BITS_EXTRA and build both sides from it. Custom curves are only one of three ways the variant gets selected: ECC_MIN_KEY_SZ at or below 160 takes it, as does HAVE_ECC_KOBLITZ at or below 224, the default for any Koblitz build. MAX_ECC_BITS keeps meaning the plain size of the largest curve, so a build that pins it to exactly what its curves need still passes the guard that rejects a too-small value. MAX_ECC_BITS_USE also derived from MAX_ECC_BITS_NEEDED, so an overridden MAX_ECC_BITS never reached the runtime guard. Derive it from MAX_ECC_BITS, which only ever widens, and is what an arbitrary curve passed to wc_ecc_set_custom_curve() needs. The SP_INT_BITS clamp above it is unchanged. Raising the ceiling reaches a latent overflow behind it. The curves the extra bit exists for have an order a byte longer than their prime, and wc_ecc_gen_deterministic_k() sizes x and h1 with MAX_ECC_BYTES, which comes from the prime, then writes mp_unsigned_bin_size(order) bytes into them. On secp160r1 that is 21 bytes into 20, the last of them the low byte of the private scalar. It is not new here - ASAN reports it on the merge base at 64-bit digits, where MP_BITS_CNT() rounds 160 and 161 to the same count - but at 32-bit digits the old ceiling rejected the curve at wc_ecc_make_key() before signing could reach it, so this commit is what makes it reachable. Size those buffers from the order with MAX_ECC_ORDER_BYTES, and reject an order larger than that. The macro is never smaller than MAX_ECC_BYTES, and only larger when MAX_ECC_BITS is a multiple of 8 and the extra bit applies, so a P-521 build stays at 66 bytes.
XHTONS and XNTOHS resolve to htons() and ntohs() when the socket I/O layer is compiled in. Zephyr 4.4 renamed those to net_htons() and net_ntohs(), and brings the unprefixed spellings back only under CONFIG_NET_NAMESPACE_COMPAT_MODE: wolfio.h:1060: error: implicit declaration of function 'htons' On a host-libc target such as native_sim it surfaces at link time instead, out of DefTicketEncCb(). That call site uses XHTONS on a session-ticket length field rather than on a socket port, so the macro is doing plain byte-order work; nothing had reached it before because session tickets require TLS 1.3. Zephyr 4.3 and older define the unprefixed names themselves, so neither spelling covers the supported range. Gate on KERNEL_VERSION_NUMBER, the way the 4.1 socket changes already are a few hundred lines above.
The module hard-coded ECC_USER_CURVES with SECP256R1 alone, so an application needing P-384, P-521 or Brainpool had to patch user_settings.h. Each curve is now its own option under WOLFSSL_ECC, with P-256 the default so existing configurations are unaffected. Two dependencies are encoded rather than left to be rediscovered, since both fail without pointing at the cause. Brainpool needs WOLFSSL_CUSTOM_CURVES, its curves not being prime-field NIST ones; custom curves in turn cannot coexist with the per-curve SP math this module selects, so asking for Brainpool moves the build onto the generic SP variant. That costs size and speed on the common curves, which is why it is opt-in. Brainpool does not drag another curve in with it, though it looks as though it must: on the generic variant the enabled-curve ceiling has to clear the largest Brainpool curve compiled in, and when it does not that curve fails with WC_KEY_SIZE_E. That ceiling was short by one bit, fixed earlier in this branch. Koblitz keeps its commented-out line rather than gaining an option, secp256k1 and secp224k1 having no TLS or wolfPSA use. P-256 keeps its inverted sense, being the one curve wolfCrypt enables by default. Verified on frdm_rw612: with P-384 and P-521 selected all three curves round-trip a signature on device; with the defaults the image is unchanged and 20 KB smaller.
The curve Kconfigs decide which curves wolfCrypt compiles and which SP implementation backs them, and getting that pairing wrong fails at runtime with WC_KEY_SIZE_E rather than at build time. Two wolfssl_test scenarios pin the combinations that matter: the NIST curves on their per-curve SP paths, and the Brainpool set that forces the generic backend, with no P-521 present to lend it a curve ceiling.
…s used The build-profile options only write the module's user_settings.h, which is never read when the application supplies its own settings file. So CONFIG_WOLFSSL_ECC_384=y alongside a settings file was accepted and silently ignored, as was every other feature option. They now depend on not having one. This needs a tracking bool, because WOLFSSL_SETTINGS_FILE is a string symbol and Kconfig evaluates a string in a logical context as always-false, which would have made every one of these dependencies quietly unsatisfiable. FIPS is gated on the master switch rather than each version: on the choice members alone the version prompt would stay visible with nothing selectable, while CMake still compiled the FIPS bundle. Kconfig.tls-generic is left alone here - only two of its 53 symbols reach any code, so gating the rest would make dead options look conditional, and the commit after this one deletes it. Options that drive the build rather than the wolfCrypt configuration stay selectable either way.
Kconfig.tls-generic is a copy of Zephyr's mbedTLS file with the symbol prefix renamed, which is why a wolfSSL module carries options named after mbedTLS internals. Renaming was all that happened: of its 53 config symbols, exactly two reach any code, WOLFSSL_TLS_VERSION_1_2 and _1_3. Checked every symbol against the whole workspace; the other 51 appear only in that file, so CONFIG_WOLFSSL_ECP_DP_SECP384R1_ENABLED=y has always done nothing. Three are worse than inert: WOLFSSL_TLS_VERSION_1_0, _1_1 and _1_3 select symbols that are defined nowhere, and Kconfig accepts that silently, so the reader sees a mechanism that does not exist. Delete the file and move the two live symbols into the module's own Kconfig, next to WOLFSSL_DTLS, keeping the settings-file dependency. The dead selects go with it, as do four dead options the TLS samples were setting and a stale entry in .wolfssl_known_macro_extras. TLS 1.3 gains a default of y. The old file had 1.2 on and 1.3 off, Zephyr's mbedTLS default from 2018, while wolfSSL's own configure enables both. Keeping 1.2 as well preserves interop with the TLS 1.2-only peers embedded deployments still meet; a 1.3-only build works by clearing the 1.2 option. That default is also what first reached DefTicketEncCb(), hence the wolfio byte-order fix earlier in this branch.
Four options guarded the assembly - WOLFCRYPT_ARMASM, its THUMB2 companion, WOLFCRYPT_INTELASM, and nothing at all for the single-precision math - and between them they could not express a working configuration on any target. WOLFCRYPT_ARMASM_THUMB2 selected the thumb2 sources but nothing defined WOLFSSL_ARMASM_THUMB2, so the link failed on AES_*_AARCH32. WOLFCRYPT_INTELASM named the 32-bit sources while user_settings.h declared WOLFSSL_X86_64_BUILD, so aes_asm.S compiled to an empty object and aes_gcm_x86_asm.S emitted 32-bit code into a 64-bit build. Only one combination is ever right for a given board, so the four collapse into WOLFCRYPT_ASM and the module derives the rest from the CPU Zephyr reports. ARMv6-M and ARMv8-M baseline get no symmetric assembly - the Thumb2 port uses UBFX and LDRD, which those cores do not have - and keep the Thumb math. Each ARM port sets two single-precision macros. WOLFSSL_SP_<cpu>_ASM compiles the per-size backend; the unsuffixed WOLFSSL_SP_<cpu> enables the inline-asm word primitives in sp_int.c, which is what the generic WOLFSSL_SP_MATH_ALL path uses for every other size and curve. Defining only the _ASM half, as this module did, left Brainpool, custom curves and any uncovered RSA or DH size with no assembly at all. Every ARM profile needs WOLFSSL_ARMASM_NO_HW_CRYPTO unless the toolchain says otherwise. The ARMv8-32 sources carry no .arch_extension, so a plain ARMv7 part rejects aese.8 outright; the AArch64 ones do carry it and always assemble, which moves the failure rather than removing it, since cpuid.c has no ID registers to read here and reports AES, PMULL and SHA-256 present whenever the macro is unset. Key it on __ARM_FEATURE_CRYPTO, and the ARMv8.1 RDMA opt-out on __ARM_FEATURE_QRDMX for the same reason. ML-KEM's small-memory key generation and encapsulation are dropped where the assembly is on for x86_64 or AArch64: wc_mlkem.c rejects that combination with an #error rather than falling back. Neither 32-bit x86 nor the x86_64 single-precision assembly is offered. The latter is worth recording: sp_x86_64_asm.S is AVX throughout and sp_x86_64.c calls into it with no CPUID dispatch and no scalar counterpart, while Zephyr never sets CR4.OSXSAVE, so those instructions fault. Both set(TOOLCHAIN_C_FLAGS ...) calls go: Zephyr applies that variable globally before it add_subdirectory's modules, so the assignment ran after the flags were consumed and never reached the parent scope either. HAVE___UINT128_T is defined wherever the compiler has the type, since with user settings nobody sets it and sp_int.c fails on an undeclared sp_int_word. One wolfssl_test scenario per port covers the option: qemu_x86_64 for Intel, qemu_cortex_a53 for AArch64, and mps2/an521/cpu0 for Thumb2. AArch64 gets a second at -mcpu=cortex-a53+crypto, since the guard above keeps the hardware blocks out of the first and the .arch_extension directives would otherwise never be assembled here. Measured on frdm_rw612, ops/sec: ECDSA P-256 sign 16 to 136, verify 10 to 102, RSA-2048 public 86 to 194.
cb95566 to
8d37db5
Compare
Summary
Nine fixes to wolfSSL proper, then five commits reworking the Zephyr module's configuration.
None of the nine is Zephyr-specific: each is a place wolfSSL assumes a normal toolchain, a POSIX libc, or an OS that enables vector state, so any bare-metal or RTOS target hits the same wall.
Library fixes
7f595dc3362737WOLFSSL_ZEPHYRblock hands C declarations to the assembler, so no wolfSSL.Sfile built on Zephyr, on any architecture4bc2165sp_x86_64.cbinds registers with the bareasmkeyword, which strict ISO C rejectsd069f5dcpuid_set_flags()tested the AVX bits but neverOSXSAVE/XGETBV, so wolfSSL dispatched vector code on an OS that never enabled the state and the image died on#UDab9a065armv8-{aes,sha256}-asmemitaese/sha256hwith no.arch_extension crypto5ddbe55.arch_extension sha3was emitted only for Apple's assembler7197713L_mlkem_aarch64_constssat outside itsWOLFSSL_HAVE_MLKEMguard, so a-Werrorbuild with ML-KEM off cannot compile the file9232649WOLFSSL_CUSTOM_CURVESbuild fails at runtime on its own largest enabled curve withWC_KEY_SIZE_E17081fdXHTONS/XNTOHSuse the POSIXhtons/ntohs, which Zephyr declares only underCONFIG_POSIX_API9232649is off by exactly one bit: under custom curvesECC_KEY_MAX_BITS()adds a bit for orders larger than the prime, the ceiling does not, andMP_BITS_CNT()rounds that into a whole extra digit. bp256 fails at a 256-bit ceiling, bp384 at 384, bp512 at 512 - always the largest curve in the set. It also makes the overridableMAX_ECC_BITSreach the runtime guard, which it never did.Four of the nine are regenerated output. The generator changes are in https://github.com/wolfSSL/scripts/pull/675, so regeneration cannot silently drop them.
Module changes
8a62e7b,bd6b6d0- ECC curves become Kconfig options instead of a hard-coded SECP256R1. P-256 stays the default; Brainpool implies custom curves and the generic SP backend, so it is opt-in. Two Twister scenarios pin the pairings that fail at runtime rather than at build time.81b3ca4- feature Kconfigs now depend on not having a settings file. They only write the module'suser_settings.h, which such a build never reads, so they were accepted and silently ignored.d893c9a- deletesKconfig.tls-generic, Zephyr's mbedTLS file with the prefix renamed. Of its 53 symbols two reach any code, and threeselectsymbols defined nowhere. The two live ones move into the module Kconfig; TLS 1.3 gainsdefault y.95cdbe5- four assembly options collapse intoWOLFCRYPT_ASM, which derives the port from the CPU Zephyr reports. None of the four could express a working configuration. Each ARM port also needs two SP macros, not one:WOLFSSL_SP_<cpu>_ASMforsp_<cpu>.c, the unsuffixed one for thesp_int.cprimitives the generic path uses. On frdm_rw612: ECDSA P-256 sign 16 to 136 ops/sec, verify 10 to 102, RSA-2048 public 86 to 194.Verification
13 of 13 Twister configurations across
qemu_x86,qemu_x86_64,qemu_cortex_a53,mps2/an521/cpu0andnative_sim/native/64, plus 8 of 8 wolfssl-tagged scenarios in the Zephyr fork.frdm_rw612builds and links withCONFIG_WOLFCRYPT_ASM=y.There is now one scenario per assembly port, so a regression in the generated sources fails in CI rather than on hardware - the aarch64 one found
7197713on its first run.Not included
32-bit x86 (only the AES sources exist at that width); the x86_64 SP assembly (AVX throughout, called with no CPUID check and no fallback); and the AES-XTS streaming regeneration, whose C wiring is in #11386.