Skip to content

Commit 214cb3c

Browse files
real-or-randomreal-or-random
committed
Merge #772: Improve constant-timeness on PowerPC
67a429f Suppress a harmless variable-time optimization by clang in _int_cmov (Tim Ruffing) 5b19633 Remove redundant "? 1 : 0" after comparisons in scalar code (Tim Ruffing) Pull request description: Attempt at resolving #771 . This surprisingly seems to improve the situation at least for the compilers available on godbolt. ACKs for top commit: gmaxwell: ACK 67a429f elichai: tACK 67a429f Tree-SHA512: ee8b0c86831ec8c3d5a9abcad773ed8a0f267e5c47012e4e1423b10a64c26b4cf6e3c466c3df765ba7e636787a3fe134d633926d67b599287f12c51be924f478
2 parents 40412b1 + 67a429f commit 214cb3c

File tree

3 files changed

+26
-21
lines changed

3 files changed

+26
-21
lines changed

‎src/scalar_4x64_impl.h‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -192,9 +192,9 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
192192
tl = t; \
193193
} \
194194
c0 += tl; /* overflow is handled on the next line */ \
195-
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFFFFFFFFFF */ \
195+
th += (c0 < tl); /* at most 0xFFFFFFFFFFFFFFFF */ \
196196
c1 += th; /* overflow is handled on the next line */ \
197-
c2 += (c1 < th) ? 1 : 0; /* never overflows by contract (verified in the next line) */ \
197+
c2 += (c1 < th); /* never overflows by contract (verified in the next line) */ \
198198
VERIFY_CHECK((c1 >= th) || (c2 != 0)); \
199199
}
200200

@@ -207,7 +207,7 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
207207
tl = t; \
208208
} \
209209
c0 += tl; /* overflow is handled on the next line */ \
210-
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFFFFFFFFFF */ \
210+
th += (c0 < tl); /* at most 0xFFFFFFFFFFFFFFFF */ \
211211
c1 += th; /* never overflows by contract (verified in the next line) */ \
212212
VERIFY_CHECK(c1 >= th); \
213213
}
@@ -221,32 +221,32 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
221221
tl = t; \
222222
} \
223223
th2 = th + th; /* at most 0xFFFFFFFFFFFFFFFE (in case th was 0x7FFFFFFFFFFFFFFF) */ \
224-
c2 += (th2 < th) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
224+
c2 += (th2 < th); /* never overflows by contract (verified the next line) */ \
225225
VERIFY_CHECK((th2 >= th) || (c2 != 0)); \
226226
tl2 = tl + tl; /* at most 0xFFFFFFFFFFFFFFFE (in case the lowest 63 bits of tl were 0x7FFFFFFFFFFFFFFF) */ \
227-
th2 += (tl2 < tl) ? 1 : 0; /* at most 0xFFFFFFFFFFFFFFFF */ \
227+
th2 += (tl2 < tl); /* at most 0xFFFFFFFFFFFFFFFF */ \
228228
c0 += tl2; /* overflow is handled on the next line */ \
229-
th2 += (c0 < tl2) ? 1 : 0; /* second overflow is handled on the next line */ \
229+
th2 += (c0 < tl2); /* second overflow is handled on the next line */ \
230230
c2 += (c0 < tl2) & (th2 == 0); /* never overflows by contract (verified the next line) */ \
231231
VERIFY_CHECK((c0 >= tl2) || (th2 != 0) || (c2 != 0)); \
232232
c1 += th2; /* overflow is handled on the next line */ \
233-
c2 += (c1 < th2) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
233+
c2 += (c1 < th2); /* never overflows by contract (verified the next line) */ \
234234
VERIFY_CHECK((c1 >= th2) || (c2 != 0)); \
235235
}
236236

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

246246
/** Add a to the number defined by (c0,c1). c1 must never overflow, c2 must be zero. */
247247
#define sumadd_fast(a) { \
248248
c0 += (a); /* overflow is handled on the next line */ \
249-
c1 += (c0 < (a)) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
249+
c1 += (c0 < (a)); /* never overflows by contract (verified the next line) */ \
250250
VERIFY_CHECK((c1 != 0) | (c0 >= (a))); \
251251
VERIFY_CHECK(c2 == 0); \
252252
}

‎src/scalar_8x32_impl.h‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,9 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
271271
tl = t; \
272272
} \
273273
c0 += tl; /* overflow is handled on the next line */ \
274-
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFF */ \
274+
th += (c0 < tl); /* at most 0xFFFFFFFF */ \
275275
c1 += th; /* overflow is handled on the next line */ \
276-
c2 += (c1 < th) ? 1 : 0; /* never overflows by contract (verified in the next line) */ \
276+
c2 += (c1 < th); /* never overflows by contract (verified in the next line) */ \
277277
VERIFY_CHECK((c1 >= th) || (c2 != 0)); \
278278
}
279279

@@ -286,7 +286,7 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
286286
tl = t; \
287287
} \
288288
c0 += tl; /* overflow is handled on the next line */ \
289-
th += (c0 < tl) ? 1 : 0; /* at most 0xFFFFFFFF */ \
289+
th += (c0 < tl); /* at most 0xFFFFFFFF */ \
290290
c1 += th; /* never overflows by contract (verified in the next line) */ \
291291
VERIFY_CHECK(c1 >= th); \
292292
}
@@ -300,32 +300,32 @@ static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
300300
tl = t; \
301301
} \
302302
th2 = th + th; /* at most 0xFFFFFFFE (in case th was 0x7FFFFFFF) */ \
303-
c2 += (th2 < th) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
303+
c2 += (th2 < th); /* never overflows by contract (verified the next line) */ \
304304
VERIFY_CHECK((th2 >= th) || (c2 != 0)); \
305305
tl2 = tl + tl; /* at most 0xFFFFFFFE (in case the lowest 63 bits of tl were 0x7FFFFFFF) */ \
306-
th2 += (tl2 < tl) ? 1 : 0; /* at most 0xFFFFFFFF */ \
306+
th2 += (tl2 < tl); /* at most 0xFFFFFFFF */ \
307307
c0 += tl2; /* overflow is handled on the next line */ \
308-
th2 += (c0 < tl2) ? 1 : 0; /* second overflow is handled on the next line */ \
308+
th2 += (c0 < tl2); /* second overflow is handled on the next line */ \
309309
c2 += (c0 < tl2) & (th2 == 0); /* never overflows by contract (verified the next line) */ \
310310
VERIFY_CHECK((c0 >= tl2) || (th2 != 0) || (c2 != 0)); \
311311
c1 += th2; /* overflow is handled on the next line */ \
312-
c2 += (c1 < th2) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
312+
c2 += (c1 < th2); /* never overflows by contract (verified the next line) */ \
313313
VERIFY_CHECK((c1 >= th2) || (c2 != 0)); \
314314
}
315315

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

325325
/** Add a to the number defined by (c0,c1). c1 must never overflow, c2 must be zero. */
326326
#define sumadd_fast(a) { \
327327
c0 += (a); /* overflow is handled on the next line */ \
328-
c1 += (c0 < (a)) ? 1 : 0; /* never overflows by contract (verified the next line) */ \
328+
c1 += (c0 < (a)); /* never overflows by contract (verified the next line) */ \
329329
VERIFY_CHECK((c1 != 0) | (c0 >= (a))); \
330330
VERIFY_CHECK(c2 == 0); \
331331
}

‎src/util.h‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,15 @@ static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
197197
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized and non-negative.*/
198198
static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag) {
199199
unsigned int mask0, mask1, r_masked, a_masked;
200+
/* Access flag with a volatile-qualified lvalue.
201+
This prevents clang from figuring out (after inlining) that flag can
202+
take only be 0 or 1, which leads to variable time code. */
203+
volatile int vflag = flag;
204+
200205
/* Casting a negative int to unsigned and back to int is implementation defined behavior */
201206
VERIFY_CHECK(*r >= 0 && *a >= 0);
202207

203-
mask0 = (unsigned int)flag + ~0u;
208+
mask0 = (unsigned int)vflag + ~0u;
204209
mask1 = ~mask0;
205210
r_masked = ((unsigned int)*r & mask0);
206211
a_masked = ((unsigned int)*a & mask1);

0 commit comments

Comments
 (0)