Skip to content

Commit

Permalink
Improve constant-timeness on PowerPC
Browse files Browse the repository at this point in the history 
Summary:
 * Remove redundant "? 1 : 0" after comparisons in scalar code

This prevents GCC from generating branches on PowerPC in certain
cases.

Fixes #771.

 * Suppress a harmless variable-time optimization by clang in _int_cmov

Follow up on 52a03512c1d800603b5c923c1a28bdba12dadb30

This is a backport of libsecp256k1 [[bitcoin-core/secp256k1#772 | PR772]]

Depends on D7590

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D7597
@real-or-random @deadalnix
real-or-random authored and deadalnix committed Sep 27, 2020
1 parent f5b18bc commit 62fa9a6
Showing 3 changed files with 26 additions and 21 deletions.
20 changes: 10 additions & 10 deletions src/secp256k1/src/scalar_4x64_impl.h
View file
Original file line number Diff line number Diff line change
@@ -192,9 +192,9 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
tl = t; \
} \
c0 += tl; /* overflow is handled on the next line */ \
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFFFFFFFFFF */ \
th += (c0 < tl); /* at most 0xFFFFFFFFFFFFFFFF */ \
c1 += th; /* overflow is handled on the next line */ \
c2 += (c1 < th) ? 1 : 0; /* never overflows by contract (verified in the next line) */ \
c2 += (c1 < th); /* never overflows by contract (verified in the next line) */ \
VERIFY_CHECK((c1 >= th) || (c2 != 0)); \
}

@@ -207,7 +207,7 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
tl = t; \
} \
c0 += tl; /* overflow is handled on the next line */ \
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFFFFFFFFFF */ \
th += (c0 < tl); /* at most 0xFFFFFFFFFFFFFFFF */ \
c1 += th; /* never overflows by contract (verified in the next line) */ \
VERIFY_CHECK(c1 >= th); \
}
@@ -221,32 +221,32 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
tl = t; \
} \
th2 = th + th; /* at most 0xFFFFFFFFFFFFFFFE (in case th was 0x7FFFFFFFFFFFFFFF) */ \
c2 += (th2 < th) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
c2 += (th2 < th); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((th2 >= th) || (c2 != 0)); \
tl2 = tl + tl; /* at most 0xFFFFFFFFFFFFFFFE (in case the lowest 63 bits of tl were 0x7FFFFFFFFFFFFFFF) */ \
th2 += (tl2 < tl) ? 1 : 0; /* at most 0xFFFFFFFFFFFFFFFF */ \
th2 += (tl2 < tl); /* at most 0xFFFFFFFFFFFFFFFF */ \
c0 += tl2; /* overflow is handled on the next line */ \
th2 += (c0 < tl2) ? 1 : 0; /* second overflow is handled on the next line */ \
th2 += (c0 < tl2); /* second overflow is handled on the next line */ \
c2 += (c0 < tl2) & (th2 == 0); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((c0 >= tl2) || (th2 != 0) || (c2 != 0)); \
c1 += th2; /* overflow is handled on the next line */ \
c2 += (c1 < th2) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
c2 += (c1 < th2); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((c1 >= th2) || (c2 != 0)); \
}

/** Add a to the number defined by (c0,c1,c2). c2 must never overflow. */
#define sumadd(a) { \
unsigned int over; \
c0 += (a); /* overflow is handled on the next line */ \
over = (c0 < (a)) ? 1 : 0; \
over = (c0 < (a)); \
c1 += over; /* overflow is handled on the next line */ \
c2 += (c1 < over) ? 1 : 0; /* never overflows by contract */ \
c2 += (c1 < over); /* never overflows by contract */ \
}

/** Add a to the number defined by (c0,c1). c1 must never overflow, c2 must be zero. */
#define sumadd_fast(a) { \
c0 += (a); /* overflow is handled on the next line */ \
c1 += (c0 < (a)) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
c1 += (c0 < (a)); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((c1 != 0) | (c0 >= (a))); \
VERIFY_CHECK(c2 == 0); \
}
20 changes: 10 additions & 10 deletions src/secp256k1/src/scalar_8x32_impl.h
View file
Original file line number Diff line number Diff line change
@@ -271,9 +271,9 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
tl = t; \
} \
c0 += tl; /* overflow is handled on the next line */ \
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFF */ \
th += (c0 < tl); /* at most 0xFFFFFFFF */ \
c1 += th; /* overflow is handled on the next line */ \
c2 += (c1 < th) ? 1 : 0; /* never overflows by contract (verified in the next line) */ \
c2 += (c1 < th); /* never overflows by contract (verified in the next line) */ \
VERIFY_CHECK((c1 >= th) || (c2 != 0)); \
}

@@ -286,7 +286,7 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
tl = t; \
} \
c0 += tl; /* overflow is handled on the next line */ \
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFF */ \
th += (c0 < tl); /* at most 0xFFFFFFFF */ \
c1 += th; /* never overflows by contract (verified in the next line) */ \
VERIFY_CHECK(c1 >= th); \
}
@@ -300,32 +300,32 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
tl = t; \
} \
th2 = th + th; /* at most 0xFFFFFFFE (in case th was 0x7FFFFFFF) */ \
c2 += (th2 < th) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
c2 += (th2 < th); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((th2 >= th) || (c2 != 0)); \
tl2 = tl + tl; /* at most 0xFFFFFFFE (in case the lowest 63 bits of tl were 0x7FFFFFFF) */ \
th2 += (tl2 < tl) ? 1 : 0; /* at most 0xFFFFFFFF */ \
th2 += (tl2 < tl); /* at most 0xFFFFFFFF */ \
c0 += tl2; /* overflow is handled on the next line */ \
th2 += (c0 < tl2) ? 1 : 0; /* second overflow is handled on the next line */ \
th2 += (c0 < tl2); /* second overflow is handled on the next line */ \
c2 += (c0 < tl2) & (th2 == 0); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((c0 >= tl2) || (th2 != 0) || (c2 != 0)); \
c1 += th2; /* overflow is handled on the next line */ \
c2 += (c1 < th2) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
c2 += (c1 < th2); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((c1 >= th2) || (c2 != 0)); \
}

/** Add a to the number defined by (c0,c1,c2). c2 must never overflow. */
#define sumadd(a) { \
unsigned int over; \
c0 += (a); /* overflow is handled on the next line */ \
over = (c0 < (a)) ? 1 : 0; \
over = (c0 < (a)); \
c1 += over; /* overflow is handled on the next line */ \
c2 += (c1 < over) ? 1 : 0; /* never overflows by contract */ \
c2 += (c1 < over); /* never overflows by contract */ \
}

/** Add a to the number defined by (c0,c1). c1 must never overflow, c2 must be zero. */
#define sumadd_fast(a) { \
c0 += (a); /* overflow is handled on the next line */ \
c1 += (c0 < (a)) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
c1 += (c0 < (a)); /* never overflows by contract (verified the next line) */ \
VERIFY_CHECK((c1 != 0) | (c0 >= (a))); \
VERIFY_CHECK(c2 == 0); \
}
7 changes: 6 additions & 1 deletion src/secp256k1/src/util.h
View file
Original file line number Diff line number Diff line change
@@ -197,10 +197,15 @@ static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized and non-negative.*/
static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag) {
unsigned int mask0, mask1, r_masked, a_masked;
/* Access flag with a volatile-qualified lvalue.
This prevents clang from figuring out (after inlining) that flag can
take only be 0 or 1, which leads to variable time code. */
volatile int vflag = flag;

/* Casting a negative int to unsigned and back to int is implementation defined behavior */
VERIFY_CHECK(*r >= 0 && *a >= 0);

mask0 = (unsigned int)flag + ~0u;
mask0 = (unsigned int)vflag + ~0u;
mask1 = ~mask0;
r_masked = ((unsigned int)*r & mask0);
a_masked = ((unsigned int)*a & mask1);

0 comments on commit 62fa9a6

Please sign in to comment.