From 4a496a36fb07d6cc8c99e591994f4ce0c3b1174c Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 1 Apr 2023 15:35:50 +0900 Subject: [PATCH] ct: Use volatile "trick" in all fe/scalar cmov implementations 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 https://github.com/bitcoin-core/secp256k1/pull/864#issuecomment-769211867 --- src/field_10x26_impl.h | 6 ++++-- src/field_5x52_impl.h | 6 ++++-- src/scalar_4x64_impl.h | 3 ++- src/scalar_8x32_impl.h | 3 ++- src/scalar_low_impl.h | 3 ++- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index d02fdddc69ac9..3b7f4d24807fb 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -1147,8 +1147,9 @@ static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) { static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) { uint32_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n)); - mask0 = flag + ~((uint32_t)0); + mask0 = vflag + ~((uint32_t)0); mask1 = ~mask0; r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1); r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1); @@ -1246,8 +1247,9 @@ static SECP256K1_INLINE void secp256k1_fe_half(secp256k1_fe *r) { static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) { uint32_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n)); - mask0 = flag + ~((uint32_t)0); + mask0 = vflag + ~((uint32_t)0); mask1 = ~mask0; r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1); r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1); diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index 4c4466ecebaaa..6b97157d0f032 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -487,8 +487,9 @@ static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) { static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag) { uint64_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n)); - mask0 = flag + ~((uint64_t)0); + mask0 = vflag + ~((uint64_t)0); mask1 = ~mask0; r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1); r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1); @@ -570,8 +571,9 @@ static SECP256K1_INLINE void secp256k1_fe_half(secp256k1_fe *r) { static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) { uint64_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r->n, sizeof(r->n)); - mask0 = flag + ~((uint64_t)0); + mask0 = vflag + ~((uint64_t)0); mask1 = ~mask0; r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1); r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1); diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 1b83575b3eb9f..1959dae986f95 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -811,8 +811,9 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r, static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) { uint64_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r->d, sizeof(r->d)); - mask0 = flag + ~((uint64_t)0); + mask0 = vflag + ~((uint64_t)0); mask1 = ~mask0; r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1); r->d[1] = (r->d[1] & mask0) | (a->d[1] & mask1); diff --git a/src/scalar_8x32_impl.h b/src/scalar_8x32_impl.h index c433adce75bc0..a2555cbbcd40f 100644 --- a/src/scalar_8x32_impl.h +++ b/src/scalar_8x32_impl.h @@ -632,8 +632,9 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r, static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) { uint32_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r->d, sizeof(r->d)); - mask0 = flag + ~((uint32_t)0); + mask0 = vflag + ~((uint32_t)0); mask1 = ~mask0; r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1); r->d[1] = (r->d[1] & mask0) | (a->d[1] & mask1); diff --git a/src/scalar_low_impl.h b/src/scalar_low_impl.h index e7800833395cd..bfd1139110b64 100644 --- a/src/scalar_low_impl.h +++ b/src/scalar_low_impl.h @@ -116,8 +116,9 @@ SECP256K1_INLINE static int secp256k1_scalar_eq(const secp256k1_scalar *a, const static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) { uint32_t mask0, mask1; + volatile int vflag = flag; SECP256K1_CHECKMEM_CHECK_VERIFY(r, sizeof(*r)); - mask0 = flag + ~((uint32_t)0); + mask0 = vflag + ~((uint32_t)0); mask1 = ~mask0; *r = (*r & mask0) | (*a & mask1); }