Add suppoort for running wolfSSL on CHERIoT#10653
Add suppoort for running wolfSSL on CHERIoT#10653wbeasley-thegoodpenguin wants to merge 2 commits into
Conversation
The same branchless bitmask arithmetic pattern encountered in sp_int.c causes CHERI tag violation in sp_c32.c. The same fix has been applied to XOR the data rather than the pointers. This will have a performance impact so it has been guarded with WC_NO_PTR_INT_CAST. Signed-off-by: William Beasley (The Capable Hub) <wbeasley@thegoodpenguin.co.uk>
ASN template setters (GetASN_Int8Bit, GetASN_Buffer, etc.) will cause a tag violation as they will store stack derived capabilities on the heap. This will silently wipe the tag on CHERIoT as it does not have StoreLocal permission. Added WOLFSSL_ASN_TEMPLATE_STACK_ALLOC to override the WOLFSSL_SMALL_STACK heap allocation path. Signed-off-by: William Beasley (The Capable Hub) <wbeasley@thegoodpenguin.co.uk>
|
Can one of the admins verify this patch? |
|
Okay to test. Contributor agreement on file. Thank you @wbeasley-thegoodpenguin |
|
|
||
| #ifdef WOLFSSL_ASN_TEMPLATE | ||
| #ifdef WOLFSSL_SMALL_STACK | ||
| #if defined(WOLFSSL_SMALL_STACK) && !defined(WOLFSSL_ASN_TEMPLATE_STACK_ALLOC) |
There was a problem hiding this comment.
This is a new build option being introduced.
- Make sure its documented at top of asn.c
- Please add to
.wolfssl_known_macro_extras:
unrecognized macros used:
WOLFSSL_ASN_TEMPLATE_STACK_ALLOC
add well-formed but unknown macros to .wolfssl_known_macro_extras at top of source tree.
| &ctx->t[ctx->y^1], &ctx->t[0], &ctx->t[1], ctx->tmp); | ||
| if (err == MP_OKAY) { | ||
| #ifdef WC_NO_PTR_INT_CAST | ||
| sp_cond_copy_bytes(&ctx->t[0], ctx->y^1, &ctx->t[2], sizeof(sp_point_256)); |
There was a problem hiding this comment.
overlong lines added:
wolfcrypt/src/sp_c32.c:22583 sp_cond_copy_bytes(&ctx->t[0], ctx->y^1, &ctx->t[2], sizeof(sp_point_256));
wolfcrypt/src/sp_c32.c:22584 sp_cond_copy_bytes(&ctx->t[1], ctx->y, &ctx->t[2], sizeof(sp_point_256));
wolfcrypt/src/sp_c32.c:22599 sp_cond_copy_bytes(&ctx->t[2], ctx->y^1, &ctx->t[0], sizeof(sp_point_256));
[...and more...]
| #ifdef NEED_ADDR_MASK | ||
| /* Mask for address to obfuscate which of the two address will be used. */ | ||
| static const size_t addr_mask[2] = { 0, (size_t)-1 }; | ||
| #ifdef WC_NO_PTR_INT_CAST |
There was a problem hiding this comment.
Any changes to this file will need an wolfSSL engineer involved since this code is generated from internal scripts. It will also need careful review by @SparkiDev
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
7 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] New static conditional-copy helpers can be unused in valid configs (-Wunused-function under -Werror) —
wolfcrypt/src/sp_c32.c:229-247 - [Medium] addr_mask[] array becomes unused in all WC_NO_PTR_INT_CAST builds —
wolfcrypt/src/sp_c32.c:228 - [Medium] Hand edits to a generated file (sp_c32.c) not reflected in the SP generator or sibling C files —
wolfcrypt/src/sp_c32.c:229-247 - [Medium] New WC_NO_PTR_INT_CAST C-path branches lack CI/test coverage —
wolfcrypt/src/sp_c32.c:229-247 - [Medium] New build macro WOLFSSL_ASN_TEMPLATE_STACK_ALLOC is undocumented and silently raises stack usage —
wolfcrypt/src/asn.c:523 - [Low] for-loop bodies omit braces, inconsistent with the _sp_cond_copy reference —
wolfcrypt/src/sp_c32.c:235-236,244-245 - [Low] Several new call sites exceed the 80-column limit —
wolfcrypt/src/sp_c32.c:22583-22584 (and the sp_point_384/521/1024 _nb variants)
Review generated by Skoll
| #ifdef NEED_ADDR_MASK | ||
| /* Mask for address to obfuscate which of the two address will be used. */ | ||
| static const size_t addr_mask[2] = { 0, (size_t)-1 }; | ||
| #ifdef WC_NO_PTR_INT_CAST |
There was a problem hiding this comment.
🔴 [High] New static conditional-copy helpers can be unused in valid configs (-Wunused-function under -Werror)
Both helpers are defined together under #ifdef NEED_ADDR_MASK + #ifdef WC_NO_PTR_INT_CAST, but they have disjoint users: sp_cond_copy_digits is only called from the RSA/DH sp_*_mod_exp_* functions, while sp_cond_copy_bytes is only called from the WOLFSSL_SP_SMALL ECC sp_*_ecc_mulmod_* functions. NEED_ADDR_MASK is, however, defined by EITHER path independently (see lines 213-218 for the ECC-small path and 219-224 for the RSA/DH path). Therefore: (a) an ECC-small-only CHERI build (WOLFSSL_HAVE_SP_ECC + WOLFSSL_SP_SMALL, no SP RSA/DH) defines sp_cond_copy_digits but never references it; (b) an RSA/DH-only CHERI build (no small-stack SP ECC) defines sp_cond_copy_bytes but never references it. A defined-but-unreferenced static function triggers -Wunused-function (in -Wall, both GCC and Clang/CHERI), which becomes a hard build break under -Werror. The author's full wolfcryptest config happens to use both, so the defect is latent and was not caught in testing. The reference implementation _sp_cond_copy in sp_int.c does not have this problem because it has a single, guard-aligned set of callers.
Fix: Mark both helpers with the existing WC_MAYBE_UNUSED macro (defined in wolfssl/wolfcrypt/wc_port.h), or split their #ifdef guards so sp_cond_copy_digits is only compiled under the RSA/DH NEED_ADDR_MASK condition and sp_cond_copy_bytes only under the ECC-small condition. Verify with an ECC-only and an RSA-only build that also defines WC_NO_PTR_INT_CAST.
| @@ -226,6 +226,25 @@ sp_uint64 __muldi3(sp_uint64 a, sp_uint64 b) | |||
| #ifdef NEED_ADDR_MASK | |||
| /* Mask for address to obfuscate which of the two address will be used. */ | |||
| static const size_t addr_mask[2] = { 0, (size_t)-1 }; | |||
There was a problem hiding this comment.
🟠 [Medium] addr_mask[] array becomes unused in all WC_NO_PTR_INT_CAST builds
When WC_NO_PTR_INT_CAST is defined, every reference to addr_mask is moved into the #else branches and compiled out, so the file-scope static const size_t addr_mask[2] is never referenced in any CHERI build (including the full config the author tested). GCC does not warn about unused static const file-scope arrays, but Clang/LLVM (the CHERIoT toolchain) can emit -Wunused-const-variable under -Wall, which would break -Werror builds. This is a smaller-impact cousin of the unused-function issue above.
Fix: Guard the addr_mask definition with #ifndef WC_NO_PTR_INT_CAST (i.e. only define it when it is actually used) so CHERI/Clang builds do not carry a dead static const array.
| #ifdef NEED_ADDR_MASK | ||
| /* Mask for address to obfuscate which of the two address will be used. */ | ||
| static const size_t addr_mask[2] = { 0, (size_t)-1 }; | ||
| #ifdef WC_NO_PTR_INT_CAST |
There was a problem hiding this comment.
🟠 [Medium] Hand edits to a generated file (sp_c32.c) not reflected in the SP generator or sibling C files
sp_c32.c is a generated file (header banner: 'Implementation by Sean Parkinson.', part of the auto-generated SP family). The new helpers and the many #ifdef WC_NO_PTR_INT_CAST blocks are inserted by hand; on the next regeneration of the SP sources they would be lost unless the generator template is updated as well. Additionally, the same addr_mask/pointer-cast pattern exists in the sibling pure-C file sp_c64.c (and sp_dsp32.c), which this PR does not touch. CHERIoT (RV32) uses sp_c32.c, so the target build is covered, but any other word-size or DSP C build under CHERI would still hit the tag-stripping XMEMCPY.
Fix: Apply the equivalent change to the SP code generator (scripts/sp) so the edits survive regeneration, and mirror the fix into sp_c64.c/sp_dsp32.c for completeness even if CHERIoT only consumes the 32-bit C path.
| #ifdef NEED_ADDR_MASK | ||
| /* Mask for address to obfuscate which of the two address will be used. */ | ||
| static const size_t addr_mask[2] = { 0, (size_t)-1 }; | ||
| #ifdef WC_NO_PTR_INT_CAST |
There was a problem hiding this comment.
🟠 [Medium] New WC_NO_PTR_INT_CAST C-path branches lack CI/test coverage
The new constant-time copy branches are only compiled when WC_NO_PTR_INT_CAST is defined (CHERI). There is no native CI configuration that defines WC_NO_PTR_INT_CAST against the C SP backend, so these branches (and the latent unused-function break described above) are not exercised by the standard test matrix. The behavior is validated only by the author's out-of-tree CHERIoT wolfcryptest run. Because the logic is behavior-preserving, no new unit test of the helper output is strictly required, but the build/run path should be covered.
Fix: Add a CI build that compiles the pure-C SP path with -DWC_NO_PTR_INT_CAST (RSA-only, ECC-only, and combined) under -Werror, and runs RSA/ECC/DH self-tests, to lock in both correctness and the unused-symbol guards.
|
|
||
| #ifdef WOLFSSL_ASN_TEMPLATE | ||
| #ifdef WOLFSSL_SMALL_STACK | ||
| #if defined(WOLFSSL_SMALL_STACK) && !defined(WOLFSSL_ASN_TEMPLATE_STACK_ALLOC) |
There was a problem hiding this comment.
🟠 [Medium] New build macro WOLFSSL_ASN_TEMPLATE_STACK_ALLOC is undocumented and silently raises stack usage
WOLFSSL_ASN_TEMPLATE_STACK_ALLOC is introduced here but appears nowhere else in the tree (no settings.h documentation, configure option, or comment). When it is defined together with WOLFSSL_SMALL_STACK, the entire ASN template macro family falls through to the #else (stack) branch, so ASNGetData/ASNSetData arrays are declared on the stack via Type name[cnt] instead of being heap-allocated. The macro switch is internally consistent (DECL/ALLOC/CALLOC/FREE all flip together, so there is no decl/alloc/free mismatch and no regression when the macro is undefined). However, the option deliberately re-introduces potentially large stack arrays that WOLFSSL_SMALL_STACK exists to avoid - a non-obvious tradeoff that callers must understand.
Fix: Document WOLFSSL_ASN_TEMPLATE_STACK_ALLOC (settings.h comment block and/or configure/README) including its purpose (CHERI: keep stack-derived capabilities off the heap) and the explicit warning that it increases stack consumption despite WOLFSSL_SMALL_STACK being set.
| { | ||
| sp_digit mask = (sp_digit)0 - (sp_digit)(cond & 1); | ||
| word32 i; | ||
| for (i = 0; i < n; i++) |
There was a problem hiding this comment.
🔵 [Low] for-loop bodies omit braces, inconsistent with the _sp_cond_copy reference
The two new helpers write single-statement for bodies without braces. The equivalent reference function _sp_cond_copy in sp_int.c (lines 5113-5115) and the surrounding wolfSSL code use braces around loop bodies. Minor consistency point.
Fix: Add braces to the loop bodies to match repo style.
| &ctx->t[ctx->y^1], &ctx->t[0], &ctx->t[1], ctx->tmp); | ||
| if (err == MP_OKAY) { | ||
| #ifdef WC_NO_PTR_INT_CAST | ||
| sp_cond_copy_bytes(&ctx->t[0], ctx->y^1, &ctx->t[2], sizeof(sp_point_256)); |
There was a problem hiding this comment.
🔵 [Low] Several new call sites exceed the 80-column limit
Lines such as sp_cond_copy_bytes(&ctx->t[0], ctx->y^1, &ctx->t[2], sizeof(sp_point_256)); at a 12-space indent are ~87 columns; the sp_point_1024 variants are longer. wolfSSL generally keeps lines at/under 80 columns.
Fix: Wrap the over-length calls to stay within 80 columns.
Description
Whilst porting wolfSSL to CHERIoT I encountered a few CHERI-related issues. These commits are intended to address them.
The CHERIoT changes to support this so far are here. Note: This is yet to be formally reviewed by the CHERIoT team and is for indication only.
Getting feedback from the wolfSSL team on the settings setup would also be appreciated here, as i'm not certain where they should live (in this repo vs theirs).
Commit 1: Fix CHERI tag violation in sp_c32.c
This is similar to previous CHERI changes where pointers are manipulated as integers and then cast back to pointers. This strips the CHERI validation tag and causes a tag violation on dereference.
Commit 2: Add ability to force ASN template setters to use stack over heap
On CHERIoT, stack derived capabilities cannot be stored on the heap. When this happens the hardware silently clears the tag bit, causing the pointer to become invalid without raising an immediate fault.
I found this when running wolfcryptest on the CHERIoT target with WOLFSSL_SMALL_STACK defined. When it gets to the RSA test it fails, and I think this is because of storing things like a pointer to
versionin the ASNData.Testing
Ran wolfcryptest and wolfcryptbenchmark on the Sonata dev board. You can see the results here
Checklist