Skip to content

Commit 6b92961

Browse files
committed
[secp256k1] ct: Use volatile "trick" in all fe/scalar cmov implementations
Summary: ``` Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls). This is just a quick fix. We should still look into other methods, e.g., asm and #457. We should also consider not caring about constant-time in scalar_low_impl.h We should also consider testing on very new compilers in nightly CI, see bitcoin-core/secp256k1#864 (comment) ``` Backport of [[bitcoin-core/secp256k1#1257 | secp256k1#1257]] and [[bitcoin-core/secp256k1#1303 | secp256k1#1303]]. Depends on D18157. Test Plan: See CI (the issue occurs after the CI migration to bookworm) here: https://cirrus-ci.com/build/6479968802177024 ninja check-secp256k1 Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D18158
1 parent e7a0cc4 commit 6b92961

File tree

8 files changed

+58
-41
lines changed

8 files changed

+58
-41
lines changed

src/secp256k1/src/ecmult_const_impl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
#define ECMULT_CONST_TABLE_GET_GE(r,pre,n,w) do { \
1717
int m = 0; \
1818
/* Extract the sign-bit for a constant time absolute-value. */ \
19-
int mask = (n) >> (sizeof(n) * CHAR_BIT - 1); \
19+
int volatile mask = (n) >> (sizeof(n) * CHAR_BIT - 1); \
2020
int abs_n = ((n) + mask) ^ mask; \
2121
int idx_n = abs_n >> 1; \
2222
secp256k1_fe neg_y; \

src/secp256k1/src/field_10x26_impl.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,8 +1098,9 @@ static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) {
10981098

10991099
static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) {
11001100
uint32_t mask0, mask1;
1101+
volatile int vflag = flag;
11011102
VG_CHECK_VERIFY(r->n, sizeof(r->n));
1102-
mask0 = flag + ~((uint32_t)0);
1103+
mask0 = vflag + ~((uint32_t)0);
11031104
mask1 = ~mask0;
11041105
r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
11051106
r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1);
@@ -1121,8 +1122,9 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
11211122

11221123
static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
11231124
uint32_t mask0, mask1;
1125+
volatile int vflag = flag;
11241126
VG_CHECK_VERIFY(r->n, sizeof(r->n));
1125-
mask0 = flag + ~((uint32_t)0);
1127+
mask0 = vflag + ~((uint32_t)0);
11261128
mask1 = ~mask0;
11271129
r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
11281130
r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1);

src/secp256k1/src/field_5x52_impl.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -450,8 +450,9 @@ static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) {
450450

451451
static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) {
452452
uint64_t mask0, mask1;
453+
volatile int vflag = flag;
453454
VG_CHECK_VERIFY(r->n, sizeof(r->n));
454-
mask0 = flag + ~((uint64_t)0);
455+
mask0 = vflag + ~((uint64_t)0);
455456
mask1 = ~mask0;
456457
r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
457458
r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1);
@@ -468,8 +469,9 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
468469

469470
static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
470471
uint64_t mask0, mask1;
472+
volatile int vflag = flag;
471473
VG_CHECK_VERIFY(r->n, sizeof(r->n));
472-
mask0 = flag + ~((uint64_t)0);
474+
mask0 = vflag + ~((uint64_t)0);
473475
mask1 = ~mask0;
474476
r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
475477
r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1);

src/secp256k1/src/modinv32_impl.h

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ static void secp256k1_modinv32_normalize_30(secp256k1_modinv32_signed30 *r, int3
6464
const int32_t M30 = (int32_t)(UINT32_MAX >> 2);
6565
int32_t r0 = r->v[0], r1 = r->v[1], r2 = r->v[2], r3 = r->v[3], r4 = r->v[4],
6666
r5 = r->v[5], r6 = r->v[6], r7 = r->v[7], r8 = r->v[8];
67-
int32_t cond_add, cond_negate;
67+
volatile int32_t cond_add, cond_negate;
6868

6969
#ifdef VERIFY
7070
/* Verify that all limbs are in range (-2^30,2^30). */
@@ -186,7 +186,8 @@ static int32_t secp256k1_modinv32_divsteps_30(int32_t zeta, uint32_t f0, uint32_
186186
* being inside [-2^31,2^31) means that casting to signed works correctly.
187187
*/
188188
uint32_t u = 1, v = 0, q = 0, r = 1;
189-
uint32_t c1, c2, f = f0, g = g0, x, y, z;
189+
volatile uint32_t c1, c2;
190+
uint32_t mask1, mask2, f = f0, g = g0, x, y, z;
190191
int i;
191192

192193
for (i = 0; i < 30; ++i) {
@@ -195,23 +196,25 @@ static int32_t secp256k1_modinv32_divsteps_30(int32_t zeta, uint32_t f0, uint32_
195196
VERIFY_CHECK((q * f0 + r * g0) == g << i);
196197
/* Compute conditional masks for (zeta < 0) and for (g & 1). */
197198
c1 = zeta >> 31;
198-
c2 = -(g & 1);
199+
mask1 = c1;
200+
c2 = g & 1;
201+
mask2 = -c2;
199202
/* Compute x,y,z, conditionally negated versions of f,u,v. */
200-
x = (f ^ c1) - c1;
201-
y = (u ^ c1) - c1;
202-
z = (v ^ c1) - c1;
203+
x = (f ^ mask1) - mask1;
204+
y = (u ^ mask1) - mask1;
205+
z = (v ^ mask1) - mask1;
203206
/* Conditionally add x,y,z to g,q,r. */
204-
g += x & c2;
205-
q += y & c2;
206-
r += z & c2;
207-
/* In what follows, c1 is a condition mask for (zeta < 0) and (g & 1). */
208-
c1 &= c2;
207+
g += x & mask2;
208+
q += y & mask2;
209+
r += z & mask2;
210+
/* In what follows, mask1 is a condition mask for (zeta < 0) and (g & 1). */
211+
mask1 &= mask2;
209212
/* Conditionally change zeta into -zeta-2 or zeta-1. */
210-
zeta = (zeta ^ c1) - 1;
213+
zeta = (zeta ^ mask1) - 1;
211214
/* Conditionally add g,q,r to f,u,v. */
212-
f += g & c1;
213-
u += q & c1;
214-
v += r & c1;
215+
f += g & mask1;
216+
u += q & mask1;
217+
v += r & mask1;
215218
/* Shifts */
216219
g >>= 1;
217220
u <<= 1;

src/secp256k1/src/modinv64_impl.h

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ static int secp256k1_modinv64_mul_cmp_62(const secp256k1_modinv64_signed62 *a, i
6969
static void secp256k1_modinv64_normalize_62(secp256k1_modinv64_signed62 *r, int64_t sign, const secp256k1_modinv64_modinfo *modinfo) {
7070
const int64_t M62 = (int64_t)(UINT64_MAX >> 2);
7171
int64_t r0 = r->v[0], r1 = r->v[1], r2 = r->v[2], r3 = r->v[3], r4 = r->v[4];
72-
int64_t cond_add, cond_negate;
72+
volatile int64_t cond_add, cond_negate;
7373

7474
#ifdef VERIFY
7575
/* Verify that all limbs are in range (-2^62,2^62). */
@@ -165,7 +165,8 @@ static int64_t secp256k1_modinv64_divsteps_59(int64_t zeta, uint64_t f0, uint64_
165165
* being inside [-2^63,2^63) means that casting to signed works correctly.
166166
*/
167167
uint64_t u = 8, v = 0, q = 0, r = 8;
168-
uint64_t c1, c2, f = f0, g = g0, x, y, z;
168+
volatile uint64_t c1, c2;
169+
uint64_t mask1, mask2, f = f0, g = g0, x, y, z;
169170
int i;
170171

171172
for (i = 3; i < 62; ++i) {
@@ -174,23 +175,25 @@ static int64_t secp256k1_modinv64_divsteps_59(int64_t zeta, uint64_t f0, uint64_
174175
VERIFY_CHECK((q * f0 + r * g0) == g << i);
175176
/* Compute conditional masks for (zeta < 0) and for (g & 1). */
176177
c1 = zeta >> 63;
177-
c2 = -(g & 1);
178+
mask1 = c1;
179+
c2 = g & 1;
180+
mask2 = -c2;
178181
/* Compute x,y,z, conditionally negated versions of f,u,v. */
179-
x = (f ^ c1) - c1;
180-
y = (u ^ c1) - c1;
181-
z = (v ^ c1) - c1;
182+
x = (f ^ mask1) - mask1;
183+
y = (u ^ mask1) - mask1;
184+
z = (v ^ mask1) - mask1;
182185
/* Conditionally add x,y,z to g,q,r. */
183-
g += x & c2;
184-
q += y & c2;
185-
r += z & c2;
186+
g += x & mask2;
187+
q += y & mask2;
188+
r += z & mask2;
186189
/* In what follows, c1 is a condition mask for (zeta < 0) and (g & 1). */
187-
c1 &= c2;
190+
mask1 &= mask2;
188191
/* Conditionally change zeta into -zeta-2 or zeta-1. */
189-
zeta = (zeta ^ c1) - 1;
192+
zeta = (zeta ^ mask1) - 1;
190193
/* Conditionally add g,q,r to f,u,v. */
191-
f += g & c1;
192-
u += q & c1;
193-
v += r & c1;
194+
f += g & mask1;
195+
u += q & mask1;
196+
v += r & mask1;
194197
/* Shifts */
195198
g >>= 1;
196199
u <<= 1;

src/secp256k1/src/scalar_4x64_impl.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,9 @@ static int secp256k1_scalar_add(secp256k1_scalar *r, const secp256k1_scalar *a,
100100

101101
static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int flag) {
102102
uint128_t t;
103+
volatile int vflag = flag;
103104
VERIFY_CHECK(bit < 256);
104-
bit += ((uint32_t) flag - 1) & 0x100; /* forcing (bit >> 6) > 3 makes this a noop */
105+
bit += ((uint32_t) vflag - 1) & 0x100; /* forcing (bit >> 6) > 3 makes this a noop */
105106
t = (uint128_t)r->d[0] + (((uint64_t)((bit >> 6) == 0)) << (bit & 0x3F));
106107
r->d[0] = t & 0xFFFFFFFFFFFFFFFFULL; t >>= 64;
107108
t += (uint128_t)r->d[1] + (((uint64_t)((bit >> 6) == 1)) << (bit & 0x3F));
@@ -170,7 +171,8 @@ static int secp256k1_scalar_is_high(const secp256k1_scalar *a) {
170171
static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
171172
/* If we are flag = 0, mask = 00...00 and this is a no-op;
172173
* if we are flag = 1, mask = 11...11 and this is identical to secp256k1_scalar_negate */
173-
uint64_t mask = !flag - 1;
174+
volatile int vflag = flag;
175+
uint64_t mask = -vflag;
174176
uint64_t nonzero = (secp256k1_scalar_is_zero(r) != 0) - 1;
175177
uint128_t t = (uint128_t)(r->d[0] ^ mask) + ((SECP256K1_N_0 + 1) & mask);
176178
r->d[0] = t & nonzero; t >>= 64;
@@ -782,8 +784,9 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
782784

783785
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
784786
uint64_t mask0, mask1;
787+
volatile int vflag = flag;
785788
VG_CHECK_VERIFY(r->d, sizeof(r->d));
786-
mask0 = flag + ~((uint64_t)0);
789+
mask0 = vflag + ~((uint64_t)0);
787790
mask1 = ~mask0;
788791
r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1);
789792
r->d[1] = (r->d[1] & mask0) | (a->d[1] & mask1);

src/secp256k1/src/scalar_8x32_impl.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,9 @@ static int secp256k1_scalar_add(secp256k1_scalar *r, const secp256k1_scalar *a,
140140

141141
static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int flag) {
142142
uint64_t t;
143+
volatile int vflag = flag;
143144
VERIFY_CHECK(bit < 256);
144-
bit += ((uint32_t) flag - 1) & 0x100; /* forcing (bit >> 5) > 7 makes this a noop */
145+
bit += ((uint32_t) vflag - 1) & 0x100; /* forcing (bit >> 5) > 7 makes this a noop */
145146
t = (uint64_t)r->d[0] + (((uint32_t)((bit >> 5) == 0)) << (bit & 0x1F));
146147
r->d[0] = t & 0xFFFFFFFFULL; t >>= 32;
147148
t += (uint64_t)r->d[1] + (((uint32_t)((bit >> 5) == 1)) << (bit & 0x1F));
@@ -240,7 +241,8 @@ static int secp256k1_scalar_is_high(const secp256k1_scalar *a) {
240241
static int secp256k1_scalar_cond_negate(secp256k1_scalar *r, int flag) {
241242
/* If we are flag = 0, mask = 00...00 and this is a no-op;
242243
* if we are flag = 1, mask = 11...11 and this is identical to secp256k1_scalar_negate */
243-
uint32_t mask = !flag - 1;
244+
volatile int vflag = flag;
245+
uint32_t mask = -vflag;
244246
uint32_t nonzero = 0xFFFFFFFFUL * (secp256k1_scalar_is_zero(r) == 0);
245247
uint64_t t = (uint64_t)(r->d[0] ^ mask) + ((SECP256K1_N_0 + 1) & mask);
246248
r->d[0] = t & nonzero; t >>= 32;
@@ -631,8 +633,9 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
631633

632634
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
633635
uint32_t mask0, mask1;
636+
volatile int vflag = flag;
634637
VG_CHECK_VERIFY(r->d, sizeof(r->d));
635-
mask0 = flag + ~((uint32_t)0);
638+
mask0 = vflag + ~((uint32_t)0);
636639
mask1 = ~mask0;
637640
r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1);
638641
r->d[1] = (r->d[1] & mask0) | (a->d[1] & mask1);

src/secp256k1/src/scalar_low_impl.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,9 @@ SECP256K1_INLINE static int secp256k1_scalar_eq(const secp256k1_scalar *a, const
115115

116116
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
117117
uint32_t mask0, mask1;
118+
volatile int vflag = flag;
118119
VG_CHECK_VERIFY(r, sizeof(*r));
119-
mask0 = flag + ~((uint32_t)0);
120+
mask0 = vflag + ~((uint32_t)0);
120121
mask1 = ~mask0;
121122
*r = (*r & mask0) | (*a & mask1);
122123
}

0 commit comments

Comments
 (0)