Skip to content

Use sizeof for SP digit memory operations on targets where a char is wider than 8 bits - #12094

Open
kojiws wants to merge 1 commit into
wolfSSL:masterfrom
kojiws:fix_sp_word_sizeof
Open

kojiws wants to merge 1 commit into
wolfSSL:masterfrom
kojiws:fix_sp_word_sizeof

Conversation

@kojiws

@kojiws kojiws commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Description

SP_WORD_SIZEOF holds the number of octets in an SP digit. This is confusing, because its name suggests a size in char units, like sizeof.

SP_WORD_SIZEOF was also used with two meanings: as a count of octets and as a size in char units. Functions such as sp_read_unsigned_bin() and sp_to_unsigned_bin_len_ct() work with octets, while memory functions such as ForceZero(), XMEMCPY(), XMEMMOVE() and XMEMSET() take a size in char units.

This is not a problem on targets with an 8-bit char, where the two values are the same. On targets with a 16-bit char, such as the TI C28x, they differ. For example, with SP_WORD_SIZE 32:

  • With an 8-bit char: SP_WORD_SIZEOF is 4 (octets) and sizeof(sp_int_digit) is 4 (chars).
  • With a 16-bit char: SP_WORD_SIZEOF is 4 (octets) and sizeof(sp_int_digit) is 2 (chars).

As a result, the memory function calls cover twice the intended region on such targets. For example, sp_forcezero() zeroes memory past the end of dp[].

Fixes ZD#22792

Changes

Replace SP_WORD_SIZEOF in wolfSSL according to how it is used:

  • Octet counts, XALIGNED() and preprocessor conditionals: use the new SP_WORD_OCTETS. It is an integer literal, so it can be used in XALIGNED() and #if.
  • Memory operations (ForceZero(), XMEMCPY(), XMEMMOVE(), XMEMSET()): use sizeof(sp_int_digit).

SP_WORD_SIZEOF remains only as a deprecated alias of SP_WORD_OCTETS. It is kept mainly for FIPS test code maintained outside of the wolfSSL repository, and also for user code. It is only defined where a char is 8 bits, where the two meanings agree, and it is not available when building wolfSSL itself.

Testing

  • macOS (arm64, clang), configured with
    --enable-cryptonly --enable-sp-math-all --enable-ecc --disable-sp-asm:
    • CFLAGS="-DSP_WORD_SIZE=32 -DWOLFSSL_SP_INT_NEGATIVE": testwolfcrypt passes.
    • Default SP_WORD_SIZE (64): testwolfcrypt passes.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

Copilot AI balanced review requested due to automatic review settings November 11, 2026 13:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reported tests cover only 8-bit-char hosts, leaving digit copying, shifting, and zeroization unverified on TI C28x after the cryptographic buffer-length changes.

No open issues 🎉

What changed in this PR

This PR corrects SP integer memory-operation sizes on targets with characters wider than 8 bits, such as TI C28x, while preserving behavior on 8-bit-character targets.

Changes:

  • Separates digit sizes in character units (SP_WORD_SIZEOF) from octet counts (SP_WORD_OCTETS).
  • Updates octet-based calculations and alignment declarations to use SP_WORD_OCTETS.
  • Adds a compile-time check that SP_WORD_SIZEOF matches sizeof(sp_int_digit).
File Description
wolfssl/​wolfcrypt/​sp_int.h Defines separate size macros, updates alignment, and adds a static assertion.
wolfcrypt/​test/​test.c Keeps the oversized-input test expressed in octets.
wolfcrypt/​src/​sp_int.c Uses octet counts for binary conversion and digit-capacity calculations.

Review effort: Balanced

@kojiws kojiws self-assigned this Nov 11, 2026
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

MemBrowse Memory Report · Explore what changed →

zephyr-frdm_rw612

  • FLASH: datas +16 B, log_const_area -8 B, rodata +28 B, text +11,728 B (+4.2%, 294,228 B / 67,108,864 B, total: 0% used)
  • RAM: bss +49 B, datas +16 B (+0.1%, 58,423 B / 983,040 B, total: 6% used)

No memory changes detected for:


Catch memory regressions in CI with MemBrowse. Free for open source.

@kojiws
kojiws force-pushed the fix_sp_word_sizeof branch 2 times, most recently from f8b0ff4 to 5acc102 Compare November 12, 2026 14:06
@kojiws kojiws changed the title Fix SP_WORD_SIZEOF on targets where a char is wider than 8 bits Use sizeof for SP digit memory operations on targets where a char is wider than 8 bits Nov 12, 2026
@kojiws
kojiws force-pushed the fix_sp_word_sizeof branch from 5acc102 to 6f0da66 Compare November 12, 2026 23:19
@kojiws
kojiws requested a balanced review from Copilot November 13, 2026 02:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The zeroization fix lacks a regression test on a CHAR_BIT == 16 target verifying that sp_forcezero() clears every allocated digit without overwriting an adjacent canary.

1 open issue

Review effort: Balanced

Comment thread wolfcrypt/src/sp_int.c Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The secure-erasure and memory-boundary changes on 16-bit-char targets need maintainer validation of the new canary tests with the C28x toolchain/runtime, since supplied results cover only 8-bit-char hosts.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@kojiws
kojiws force-pushed the fix_sp_word_sizeof branch from 9acbfce to 0cd364c Compare November 13, 2026 13:02
@kojiws
kojiws force-pushed the fix_sp_word_sizeof branch from 0cd364c to 6ed5c35 Compare November 13, 2026 13:20
@kojiws

kojiws commented Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Retest this please

1 similar comment
@kojiws

kojiws commented Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants