Skip to content

Commit e289a95

Browse files
committed
SP int: fixes from review by Claude
1. sp_cond_swap_ct_ex (line ~5524) — XOR typo: b->sign ^= b->sign always zeroed the sign. Fixed to b->sign ^= t->sign to correctly swap signs. 2. sp_mod_d (line ~7271) — Negative modulo correction was applied even when the remainder was 0. Added (*r != 0) guard to avoid producing d instead of 0. 3. sp_lshb (line ~8444) — Left-shift size check was off. Refactored to correctly distinguish between pure-digit shifts and bit-within-digit shifts when checking if the result fits, using separate overflow checks for each case. 4. sp_div (line ~9003) — Quotient size check could underflow (a->used - d->used) when a->used <= d->used. Added a->used > d->used guard before the subtraction. 5. _sp_mulmod_tmp (line ~12160) — Zero inputs caused an allocation of size 0, which is problematic. Added an early path: if either operand is zero, set result to zero and skip the allocation/multiply entirely. 6. sp_mod_2d — copy path (line ~14762) — XMEMCPY copied digits * SP_WORD_SIZEOF bytes but a may have fewer than digits used digits. Fixed to copy min(a->used, digits) digits to avoid reading uninitialized memory. 7. sp_mod_2d — negation loop (line ~14782) — Negation loop iterated over r->used, which could exceed digits. Fixed to loop over min(r->used, digits). 8. _sp_sqrmod (line ~17314) — Same zero-input issue as _sp_mulmod_tmp. Added early zero path to skip the allocation/squaring when input is zero. 9. sp_to_unsigned_bin_len_ct (line ~18312) — Constant-time mask update had undefined behavior when a->used == 0 (i < a->used - 1 would wrap). Rewritten as (i + 1) < a->used to be safe. 10. sp_lcm (line ~19838) — Typo in sign check: b->sign >= MP_NEG (comparing against a value that is 1, so >= 1 would also match MP_ZPOS) changed to b->sign == MP_NEG.
1 parent 20f640a commit e289a95

1 file changed

Lines changed: 56 additions & 37 deletions

File tree

wolfcrypt/src/sp_int.c

Lines changed: 56 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -5521,7 +5521,7 @@ int sp_cond_swap_ct_ex(sp_int* a, sp_int* b, int cnt, int swap, sp_int* t)
55215521
/* XOR temporary - when mask set then result will be a. */
55225522
b->used ^= t->used;
55235523
#ifdef WOLFSSL_SP_INT_NEGATIVE
5524-
b->sign ^= b->sign;
5524+
b->sign ^= t->sign;
55255525
#endif
55265526
for (i = 0; i < (unsigned int)cnt; i++) {
55275527
b->dp[i] ^= t->dp[i];
@@ -7268,7 +7268,7 @@ int sp_mod_d(const sp_int* a, sp_int_digit d, sp_int_digit* r)
72687268
}
72697269

72707270
#ifdef WOLFSSL_SP_INT_NEGATIVE
7271-
if (a->sign == MP_NEG) {
7271+
if ((a->sign == MP_NEG) && (*r != 0)) {
72727272
*r = d - *r;
72737273
}
72747274
#endif
@@ -8441,14 +8441,17 @@ static int sp_lshb(sp_int* a, int n)
84418441
if (a->used != 0) {
84428442
/* Calculate number of digits to shift. */
84438443
sp_size_t s = (sp_size_t)n >> SP_WORD_SHIFT;
8444+
/* Get count of bits to move in digit. */
8445+
n &= (int)SP_WORD_MASK;
84448446

84458447
/* Ensure number has enough digits for result. */
8446-
if (a->used + s >= a->size) {
8448+
if ((n != 0) && (a->used + s >= a->size)) {
8449+
err = MP_VAL;
8450+
}
8451+
else if ((s > 0) && (a->used + s > a->size)) {
84478452
err = MP_VAL;
84488453
}
84498454
if (err == MP_OKAY) {
8450-
/* Get count of bits to move in digit. */
8451-
n &= (int)SP_WORD_MASK;
84528455
/* Check whether this is a complicated case. */
84538456
if (n != 0) {
84548457
unsigned int i;
@@ -8997,7 +9000,8 @@ int sp_div(const sp_int* a, const sp_int* d, sp_int* r, sp_int* rem)
89979000
err = MP_VAL;
89989001
}
89999002
/* Ensure quotient result has enough memory. */
9000-
if ((err == MP_OKAY) && (r != NULL) && (r->size < a->used - d->used + 2)) {
9003+
if ((err == MP_OKAY) && (r != NULL) && (a->used > d->used) &&
9004+
(r->size < a->used - d->used + 2)) {
90019005
err = MP_VAL;
90029006
}
90039007
if ((err == MP_OKAY) && (rem != NULL)) {
@@ -12153,24 +12157,30 @@ static int _sp_mulmod_tmp(const sp_int* a, const sp_int* b, const sp_int* m,
1215312157
sp_int* r)
1215412158
{
1215512159
int err = MP_OKAY;
12156-
/* Create temporary for multiplication result. */
12157-
DECL_SP_INT(t, a->used + b->used);
1215812160

12159-
ALLOC_SP_INT(t, a->used + b->used, err, NULL);
12160-
if (err == MP_OKAY) {
12161-
err = sp_init_size(t, (sp_size_t)(a->used + b->used));
12161+
if (sp_iszero(a) || sp_iszero(b)) {
12162+
_sp_zero(r);
1216212163
}
12164+
else {
12165+
/* Create temporary for multiplication result. */
12166+
DECL_SP_INT(t, a->used + b->used);
1216312167

12164-
/* Multiply and reduce. */
12165-
if (err == MP_OKAY) {
12166-
err = sp_mul(a, b, t);
12167-
}
12168-
if (err == MP_OKAY) {
12169-
err = sp_mod(t, m, r);
12170-
}
12168+
ALLOC_SP_INT(t, a->used + b->used, err, NULL);
12169+
if (err == MP_OKAY) {
12170+
err = sp_init_size(t, (sp_size_t)(a->used + b->used));
12171+
}
1217112172

12172-
/* Dispose of an allocated SP int. */
12173-
FREE_SP_INT(t, NULL);
12173+
/* Multiply and reduce. */
12174+
if (err == MP_OKAY) {
12175+
err = sp_mul(a, b, t);
12176+
}
12177+
if (err == MP_OKAY) {
12178+
err = sp_mod(t, m, r);
12179+
}
12180+
12181+
/* Dispose of an allocated SP int. */
12182+
FREE_SP_INT(t, NULL);
12183+
}
1217412184

1217512185
return err;
1217612186
}
@@ -14749,7 +14759,8 @@ int sp_mod_2d(const sp_int* a, int e, sp_int* r)
1474914759
if (err == MP_OKAY) {
1475014760
/* Copy a into r if not same pointer. */
1475114761
if (a != r) {
14752-
XMEMCPY(r->dp, a->dp, digits * (word32)SP_WORD_SIZEOF);
14762+
sp_size_t cnt = (a->used < digits) ? a->used : digits;
14763+
XMEMCPY(r->dp, a->dp, cnt * (word32)SP_WORD_SIZEOF);
1475314764
r->used = a->used;
1475414765
#ifdef WOLFSSL_SP_INT_NEGATIVE
1475514766
r->sign = a->sign;
@@ -14768,9 +14779,10 @@ int sp_mod_2d(const sp_int* a, int e, sp_int* r)
1476814779
if (a->sign == MP_NEG) {
1476914780
unsigned int i;
1477014781
sp_int_digit carry = 0;
14782+
sp_size_t cnt = (r->used < digits) ? r->used : digits;
1477114783

1477214784
/* Negate value. */
14773-
for (i = 0; i < r->used; i++) {
14785+
for (i = 0; i < cnt; i++) {
1477414786
sp_int_digit next = r->dp[i] > 0;
1477514787
r->dp[i] = (sp_int_digit)0 - r->dp[i] - carry;
1477614788
carry |= next;
@@ -17299,24 +17311,31 @@ int sp_sqr(const sp_int* a, sp_int* r)
1729917311
static int _sp_sqrmod(const sp_int* a, const sp_int* m, sp_int* r)
1730017312
{
1730117313
int err = MP_OKAY;
17302-
/* Create temporary for multiplication result. */
17303-
DECL_SP_INT(t, a->used * 2);
1730417314

17305-
ALLOC_SP_INT(t, a->used * 2, err, NULL);
17306-
if (err == MP_OKAY) {
17307-
err = sp_init_size(t, a->used * 2U);
17315+
if (sp_iszero(a)) {
17316+
_sp_zero(r);
1730817317
}
17318+
else {
17319+
/* Create temporary for multiplication result. */
17320+
DECL_SP_INT(t, a->used * 2);
1730917321

17310-
/* Square and reduce. */
17311-
if (err == MP_OKAY) {
17312-
err = sp_sqr(a, t);
17313-
}
17314-
if (err == MP_OKAY) {
17315-
err = sp_mod(t, m, r);
17322+
ALLOC_SP_INT(t, a->used * 2, err, NULL);
17323+
if (err == MP_OKAY) {
17324+
err = sp_init_size(t, a->used * 2U);
17325+
}
17326+
17327+
/* Square and reduce. */
17328+
if (err == MP_OKAY) {
17329+
err = sp_sqr(a, t);
17330+
}
17331+
if (err == MP_OKAY) {
17332+
err = sp_mod(t, m, r);
17333+
}
17334+
17335+
/* Dispose of an allocated SP int. */
17336+
FREE_SP_INT(t, NULL);
1731617337
}
1731717338

17318-
/* Dispose of an allocated SP int. */
17319-
FREE_SP_INT(t, NULL);
1732017339
return err;
1732117340
}
1732217341

@@ -18290,7 +18309,7 @@ int sp_to_unsigned_bin_len_ct(const sp_int* a, byte* out, int outSz)
1829018309
i = 0;
1829118310
for (j = outSz - 1; j >= 0; j--) {
1829218311
out[j] = a->dp[i] & mask;
18293-
mask &= (sp_int_digit)0 - (i < (unsigned int)a->used - 1);
18312+
mask &= (sp_int_digit)0 - ((i + 1) < (unsigned int)a->used);
1829418313
i += (unsigned int)(1 & mask);
1829518314
}
1829618315
}
@@ -19816,7 +19835,7 @@ int sp_lcm(const sp_int* a, const sp_int* b, sp_int* r)
1981619835
}
1981719836
#ifdef WOLFSSL_SP_INT_NEGATIVE
1981819837
/* Ensure a and b are positive. */
19819-
else if ((a->sign == MP_NEG) || (b->sign >= MP_NEG)) {
19838+
else if ((a->sign == MP_NEG) || (b->sign == MP_NEG)) {
1982019839
err = MP_VAL;
1982119840
}
1982219841
#endif

0 commit comments

Comments
 (0)