Skip to content

Check workspace bounds in Comba squaring and Montgomery reduction - #618

Open
PetarMax wants to merge 1 commit into
libtom:developfrom
PetarMax:fix-comba-workspace-bounds
Open

PetarMax wants to merge 1 commit into
libtom:developfrom
PetarMax:fix-comba-workspace-bounds

Conversation

@PetarMax

Copy link
Copy Markdown

The private Comba helpers can overrun their workspace when called directly with oversized inputs. With the default 60-bit digits and MP_WARRAY = 512:

  • s_mp_sqr_comba with a->used = 257 writes past the end of W. Reject a->used > MP_WARRAY / 2 before doubling the size or growing the destination.
  • s_mp_montgomery_reduce_comba with n->used = 256 needs 513 workspace entries. Extend the existing input-size check to reject n->used > (MP_WARRAY - 1) / 2 before growing the destination or zeroing the workspace.

Both cases return MP_VAL with the input numbers and destination unchanged. Release the temporary workspace on these exits when MP_SMALL_STACK_SIZE is enabled. Express the limits using MP_WARRAY so they follow the digit configuration.

Add regression coverage for the first rejected sizes, rejected in-place squaring, the largest supported Montgomery modulus, and the existing oversized-input rejection.

The current public dispatch paths already check these Comba size limits. This change makes the private helpers enforce their own workspace bounds. The patch is limited to bounds validation and regression tests.

Validation against develop at ae40a87a920099a7d9d00979570e0c8d917a1fd7, using GCC 11.4.0:

  • Reproduced both invalid accesses on the unpatched sources with sanitizers; both reproducers return MP_VAL after the patch.
  • All 51 tests pass with 60-bit digits, 28-bit digits, and MP_SMALL_STACK_SIZE.
  • The three relevant test groups pass with AddressSanitizer and UndefinedBehaviorSanitizer for 15-bit, 31-bit, and 60-bit digits.
  • The source-style check and git diff --check pass.

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