ARM32 assembly: Get CPU Id flags to choose assembly.#10957
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10957
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
8f05e85 to
3883e89
Compare
|
Jenkins: retest this please |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10957
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
Added flag determination for Linux/Android/BSD/Windows/privilege-mode. Made AES and SHA-256 runtime dispatch. Updated the ARM32 assembly to support having multiple implementations compiled in.
3883e89 to
334bde6
Compare
|
Jenkins: retest this please |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Medium] OpenBSD grouped with FreeBSD but calls FreeBSD-only elf_aux_info() —
wolfcrypt/src/cpuid.c:719-767 - [Low] Android CPUID branch is unreachable (falls into the linux branch first) —
wolfcrypt/src/cpuid.c:639-718 - [Low] WOLFSSL_ARM32_AES_DISPATCH does not exclude Thumb-2 (unlike SHA256_ARM32_DISPATCH) —
wolfcrypt/src/aes.c:1094-1097
Review generated by Skoll via Claude/Codex
| (&cpuid_flags, &old_cpuid_flags, new_cpuid_flags); | ||
| } | ||
| } | ||
| #elif defined(__FreeBSD__) || defined(__OpenBSD__) |
There was a problem hiding this comment.
🟡 [Medium] OpenBSD grouped with FreeBSD but calls FreeBSD-only elf_aux_info()
💡 SUGGEST
The new AArch32 feature-detection block adds a branch #elif defined(__FreeBSD__) || defined(__OpenBSD__) that #include <sys/auxv.h> and calls elf_aux_info(AT_HWCAP, ...) / elf_aux_info(AT_HWCAP2, ...). elf_aux_info() is a FreeBSD/NetBSD interface; OpenBSD does not provide it (nor the same <sys/auxv.h>/elf_aux_info API for reading HWCAP). On a 32-bit ARM (armv7) OpenBSD build with WOLFSSL_ARMASM, this branch is selected and the translation unit fails to compile/link (implicit declaration of elf_aux_info, or missing header), breaking cpuid.c for that supported platform. FreeBSD is unaffected. Because __OpenBSD__ is grouped here rather than falling through to the safe #else compiled-features fallback, the run-time-dispatch feature turns a previously buildable configuration into a build break.
Recommendation: Restrict this branch to platforms that actually provide elf_aux_info() (e.g. #elif defined(__FreeBSD__)), and give OpenBSD its own branch using its supported mechanism (e.g. sysctl CTL_MACHDEP) or let OpenBSD fall through to the #else compiled-features fallback.
| (&cpuid_flags, &old_cpuid_flags, new_cpuid_flags); | ||
| } | ||
| } | ||
| #elif defined(__linux__) |
There was a problem hiding this comment.
🔵 [Low] Android CPUID branch is unreachable (falls into the linux branch first)
💡 SUGGEST
In the new HAVE_CPUID_ARM32 block the platform selection places #elif defined(__linux__) (getauxval, line 639) before #elif defined(__ANDROID__) || defined(ANDROID) (cpu-features.h / android_getCpuFeatures, line 689). Android NDK/bionic toolchains define __linux__ in addition to __ANDROID__, so the __linux__ branch always matches first and the dedicated Android branch is dead code that is never compiled. On modern Android the getauxval/AT_HWCAP2 path works (API 18+/21+), so run-time detection still functions, but the intended NDK cpu-features fallback (written for pre-API-18 bionic that lacked getauxval) can never be reached. Note this exactly mirrors the pre-existing AArch64 code (cpuid.c:267 vs 329), so it may be an accepted convention rather than an oversight. Severity assessed as Low by both modes; confidence bumped to High on cross-mode agreement.
Recommendation: Order the #elif defined(__ANDROID__) || defined(ANDROID) branch before the generic #elif defined(__linux__) branch (in both the ARM32 and, for consistency, the AArch64 blocks) so the Android-specific detection is actually selectable. If getauxval is preferred everywhere, consider dropping the dead Android branch instead.
| * base fallback can be dropped with WOLFSSL_ARMASM_NO_BASE_IMPL (crypto | ||
| * always present), and WOLFSSL_ARMASM_NO_HW_CRYPTO keeps only the base, both | ||
| * of which revert to direct calls with no run-time check. */ | ||
| #if !defined(__aarch64__) && !defined(WOLFSSL_ARMASM_NO_HW_CRYPTO) && \ |
There was a problem hiding this comment.
🔵 [Low] WOLFSSL_ARM32_AES_DISPATCH does not exclude Thumb-2 (unlike SHA256_ARM32_DISPATCH)
🔧 NIT
The SHA-256 dispatch guard explicitly excludes Thumb-2 (sha256.c:1350 uses !defined(WOLFSSL_ARMASM_THUMB2)), but the analogous AES guard WOLFSSL_ARM32_AES_DISPATCH (#if !defined(__aarch64__) && !defined(WOLFSSL_ARMASM_NO_HW_CRYPTO) && !defined(WOLFSSL_ARMASM_NO_BASE_IMPL)) does not. The Thumb-2 AES assembly (thumb2-aes-asm.S) provides only the base implementation and none of the crypto-extension AES_*_AARCH32 symbols, so a Thumb-2 build that reaches this dispatch path would fail to link against AES_encrypt_AARCH32. This is safe in autotools builds because configure always pairs WOLFSSL_ARMASM_THUMB2 with WOLFSSL_ARMASM_NO_HW_CRYPTO (configure.ac:4139-4140), which suppresses the dispatch macro. It could bite a hand-rolled user_settings.h that sets WOLFSSL_ARMASM_THUMB2 + crypto without NO_HW_CRYPTO (already broken pre-diff, so not a regression — just missing defense-in-depth/consistency with the SHA-256 guard).
Recommendation: Add !defined(WOLFSSL_ARMASM_THUMB2) to the WOLFSSL_ARM32_AES_DISPATCH definition to match the SHA-256 dispatch guard and make the Thumb-2 restriction explicit at the code level rather than relying solely on configure.
Description
Added flag determination for Linux/Android/BSD/Windows/privilege-mode.
Made AES and SHA-256 runtime dispatch.
Updated the ARM32 assembly to support having multiple implementations compiled in.
Testing
Tested ARM32 builds.