Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the change reducing ECC curve-parameter stack usage in SP math builds.
No open issues 🎉
What changed in this PR
Reduces ECC curve-parameter stack usage in SP math builds by sizing temporary integers for the largest supported curve rather than the full SP integer capacity.
Changes:
- Extends sized curve-parameter storage to
WOLFSSL_SP_MATH, capped atSP_INT_BITS. - Uses an
sp_int_digitarray to align stack storage. - Preserves
MAX_ECC_BITS_USEand existing key-size checks.
| File | Description |
|---|---|
wolfcrypt/src/ecc.c |
Updates curve-parameter allocation, alignment, and initialization for sized SP integers. |
Review effort: Balanced
|
|
@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 didn't find any bugs. It changes how ECC curve-parameter mp_ints are laid out in memory in WOLFSSL_SP_MATH builds, which is core crypto code, so a maintainer should still take a look.
What was reviewed:
- Per-slot stride and alignment. MP_INT_SIZEOF is sizeof(sp_int_minimal) plus whole digits, so each slot stays a multiple of the digit size. That matches the MP_INT_SIZEOF_DIGITS stack array and the XMALLOC size.
- Sizing. MP_BITS_CNT(SP_INT_BITS) equals SP_INT_DIGITS, so capping at SP_INT_BITS keeps sp_init_size within its
size > SP_INT_DIGITScheck. In SP_MATH builds this is the same capacity mp_init gave before. - SP_MATH-only builds. MP_INT_SIZEOF, MP_INT_SIZEOF_DIGITS, MP_BITS_CNT and the mp_init_size -> sp_init_size mapping are defined unconditionally in sp_int.h.
Extended reasoning...
The change touches only wolfcrypt/src/ecc.c. It extends the sized curve-spec storage that sp-math-all builds already use to WOLFSSL_SP_MATH builds, and changes the stack buffer to an sp_int_digit array. It is core crypto code, and the right memory layout depends on build-configuration macros (SP_INT_BITS, MAX_ECC_BITS_USE, WOLFSSL_SMALL_STACK), so a mismatch could compile cleanly and still corrupt memory at runtime. The stride, alignment and size checks I ran all hold. Because this changes memory layout in crypto paths across several build configurations, a maintainer should still look before merge rather than relying on an automated approval.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12042
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
Coverage: 1 of 1 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
DECLARE_CURVE_SPECS keeps up to six curve parameters on the stack. With WOLFSSL_SP_MATH_ALL they are sized for the largest ECC curve, but with WOLFSSL_SP_MATH they are full mp_ints of SP_INT_BITS, which RSA makes 3072 or 4096 bits. wc_ecc_verify_hash_ex() then needs 7.5 KB of stack for a P-256 signature even though the SP code it calls never touches the parameters. Use the sized layout for both sp_int builds, with the digit count for MAX_ECC_BITS_USE capped at SP_INT_DIGITS, so a build that defines its own SP_INT_DIGITS still loads its curves. MAX_ECC_BITS_USE itself is unchanged, so the key size checks in sign and verify behave as before. The stack buffer is now an sp_int_digit array, so the mp_ints carved out of it are aligned. With --enable-sp-math --enable-sp on aarch64: - wc_ecc_verify_hash_ex: 7584 -> 1424 bytes - wc_ecc_make_pub_ex: 6384 -> 1104 bytes - wc_ecc_point_is_on_curve: 3264 -> 624 bytes Stack use in ECC-only, sp-math-all and small-stack builds is unchanged.
3c0e544 to
4aae476
Compare
Description
With
WOLFSSL_SP_MATH,DECLARE_CURVE_SPECSkeeps up to six fullSP_INT_BITSmp_ints on the stack, so an RSA-enabled build needs 7.5 KB of stack to verify a P-256 signature. This extends the sized curve-parameter layout sp-math-all already uses toWOLFSSL_SP_MATH.Stack usage in bytes with
--enable-sp-math --enable-spon aarch64:wc_ecc_verify_hash_exwc_ecc_make_pub_exwc_ecc_point_is_on_curveChanges
MAX_ECC_BITS_USEandSP_INT_BITSin both sp_int buildssp_int_digitarray so the carved-out mp_ints are alignedMAX_ECC_BITS_USEunchanged, so sign and verify key size checks behave as beforeTesting
testwolfcryptandunit.testcover the changed path*-o2-sp-mathlegs (arm64, armhf, armel) build sp-math and runtestwolfcrypt