Repository navigation
Fix Thumb-1 sp_div_word - #12121
Fix Thumb-1 sp_div_word#12121yosuke-wolfssl wants to merge 3 commits into
Conversation
- Remove the movs r4 between the subtract and subtract-with-carry of the first correction round in the Thumb sp_div_word. - mp_test gains mp_test_div_word with an mp_div_d vector whose divisor has the top bit set. Issue: F-18180
- The final fix-up of the Thumb sp_div_word keeps d - rem in r5, adds 1 to the quotient when rem > d, then subtracts 1 from r5 and adds the borrow, so rem == d also adds 1. - mp_test_div_word gains two exact-division vectors. Issue: F-18181
- The Thumb sp_div_word shifts d left until its top bit is set, using a branch-free 16/8/4/2/1 search written once per compiler in place of the 0-or-8 bit pre-shift. - mp_test_div_word gains vectors with divisors in [2^16, 2^23) and [2^24, 2^31), and checks mp_mod_d for every vector. Issue: F-18179
|
@claude review |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and found no bugs. It rewrites hand-written Thumb-1 inline assembly in a core bignum division routine, in three syntax variants (Keil, clang, GCC), so a human should still look at it.
What I checked:
- The new 16/8/4/2/1 normalization uses
sub #1followed bysbc r6,r6. When the tested top bits are zero, this produces an all-ones mask, sodis shifted andr3adds up the shift without branching. r5 and r6 are already in the asm clobber list. - The final correction step:
d - r4now feeds a borrow-based +1 into the quotient before the existing subtract-1 step. Removing the deadmovs r4, %[hi]and the oldlsl d, d, r3matches the new flow. - I worked out 5 of the 6 new test vectors by hand and they check out; I didn't recompute the 64-bit top-bit-set vector. The new test's guard matches at the definition and the call site, and
mp_testitself already requires!WOLFSSL_RSA_VERIFY_ONLY, somp_div_d/mp_mod_dare available. - I didn't assemble or run the Thumb-1 builds or the qemu results described in the PR. Those claims are unverified here.
Extended reasoning...
The change rewrites the ARM Thumb-1 inline-asm sp_div_word in wolfcrypt/src/sp_int.c and adds a known-answer test in wolfcrypt/test/test.c. That routine is used by mp_div, mp_mod, mp_mulmod and gcd, which RSA/DH/ECC rely on. It is cryptographic arithmetic, and timing resistance matters there. The asm reads as correct and branch-free, but it is about 150 lines of hand-written assembly in three syntax variants that this review could not assemble or run. A human with Thumb-1 toolchain access should confirm it.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12121
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Max
Problem
The Thumb-1
sp_div_word(WOLFSSL_SP_ARM_THUMBwithWOLFSSL_SP_DIV_WORD_HALF, whichWOLFSSL_SP_DIV_32implies) returns wrong quotient digits in three independent ways. As a result,mp_div,mp_mod,mp_mulmod,mp_div_d,mp_mod_dandmp_gcd(e.g. the RSA key generation checkgcd(p-1, e)) return wrong results.Fix (
wolfcrypt/src/sp_int.c)One commit per finding:
movs r4, %[hi];r4is reloaded before use.d - remand also adds 1 when it is zero, as the ARM32sp_div_worddoes.duntil its top bit is set. This also removes the old branch ond.Closes f-18179, f-18180, f-18181.
Tests
New
mp_test_div_wordinwolfcrypt/test/test.c: 6 known-answer vectors throughmp_div_dandmp_mod_d, one group per finding.Verification
-DWOLFSSL_SP_DIV_32under qemu-arm 8.2.2, using GCC 13.3 and clang 18:mp_test_lcm_gcd); with this series it passes on both compilers.sp_div_wordagainst 64-bit division (3M random and edge inputs): 2,354,919 wrong on master (GCC), 0 wrong after the series (GCC and clang).-Werror.sp_div_wordis about 7% slower; RSA-2048, DH-2048 and P-256 operations change by less than 1%.The clang Thumb build only works with workarounds for problems already on master, which are tracked separately.