From 620ba3d74bed3095cec7cd8877c8ce14cbf5e329 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Fri, 9 Dec 2022 22:31:15 +0000 Subject: [PATCH 1/3] benchmarks: fix bench_scalar_split scalar_split_lambda requires that the input pointer is different to both output pointers. Without this fix, the internal benchmarks crash when compiled with -DVERIFY. This was introduced in commit 362bb25608dbcd724a07dd5170c4ebe081c3dd84 (which requires configuring with --enable-endomorphism to exhibit the crash). --- src/bench_internal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bench_internal.c b/src/bench_internal.c index fd794a1c9dd13..e1ef14fdb014a 100644 --- a/src/bench_internal.c +++ b/src/bench_internal.c @@ -110,10 +110,11 @@ static void bench_scalar_mul(void* arg, int iters) { static void bench_scalar_split(void* arg, int iters) { int i, j = 0; bench_inv *data = (bench_inv*)arg; + secp256k1_scalar tmp; for (i = 0; i < iters; i++) { - secp256k1_scalar_split_lambda(&data->scalar[0], &data->scalar[1], &data->scalar[0]); - j += secp256k1_scalar_add(&data->scalar[0], &data->scalar[0], &data->scalar[1]); + secp256k1_scalar_split_lambda(&tmp, &data->scalar[1], &data->scalar[0]); + j += secp256k1_scalar_add(&data->scalar[0], &tmp, &data->scalar[1]); } CHECK(j <= iters); } From 7f49aa7f2dca595ac8b58d0268dc46a9dfff1e38 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Fri, 9 Dec 2022 23:04:57 +0000 Subject: [PATCH 2/3] ci: add test job with -DVERIFY This detects benchmarks that crash when VERIFY is defined. --- .cirrus.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.cirrus.yml b/.cirrus.yml index ac682e50a2d4b..a4182e1d011d1 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -81,6 +81,7 @@ task: - env: {WIDEMUL: int128, ECDH: yes, SCHNORRSIG: yes} - env: {WIDEMUL: int128, ASM: x86_64} - env: { RECOVERY: yes, SCHNORRSIG: yes} + - env: {CTIMETESTS: no, RECOVERY: yes, ECDH: yes, SCHNORRSIG: yes, CPPFLAGS: -DVERIFY} - env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETESTS: no, BENCH: no} - env: {CPPFLAGS: -DDETERMINISTIC} - env: {CFLAGS: -O0, CTIMETESTS: no} From eb6bebaee3947f8bca46816fa6bf6182085f1b56 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Wed, 4 Jan 2023 11:21:04 +0000 Subject: [PATCH 3/3] scalar: restrict split_lambda args, improve doc and VERIFY_CHECKs VERIFY_CHECK(r1 != r2) is added because otherwise the verify_scalar_split fails. --- src/scalar.h | 7 ++++--- src/scalar_impl.h | 8 ++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/scalar.h b/src/scalar.h index b9cb6b059c06d..63c0d646a3c26 100644 --- a/src/scalar.h +++ b/src/scalar.h @@ -88,9 +88,10 @@ static int secp256k1_scalar_eq(const secp256k1_scalar *a, const secp256k1_scalar /** Find r1 and r2 such that r1+r2*2^128 = k. */ static void secp256k1_scalar_split_128(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k); -/** Find r1 and r2 such that r1+r2*lambda = k, - * where r1 and r2 or their negations are maximum 128 bits long (see secp256k1_ge_mul_lambda). */ -static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k); +/** Find r1 and r2 such that r1+r2*lambda = k, where r1 and r2 or their + * negations are maximum 128 bits long (see secp256k1_ge_mul_lambda). It is + * required that r1, r2, and k all point to different objects. */ +static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT r1, secp256k1_scalar * SECP256K1_RESTRICT r2, const secp256k1_scalar * SECP256K1_RESTRICT k); /** Multiply a and b (without taking the modulus!), divide by 2**shift, and round to the nearest integer. Shift must be at least 256. */ static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r, const secp256k1_scalar *a, const secp256k1_scalar *b, unsigned int shift); diff --git a/src/scalar_impl.h b/src/scalar_impl.h index 9e72df2e50e6b..bed7f95fcb0df 100644 --- a/src/scalar_impl.h +++ b/src/scalar_impl.h @@ -52,7 +52,10 @@ static int secp256k1_scalar_set_b32_seckey(secp256k1_scalar *r, const unsigned c * nontrivial to get full test coverage for the exhaustive tests. We therefore * (arbitrarily) set r2 = k + 5 (mod n) and r1 = k - r2 * lambda (mod n). */ -static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k) { +static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT r1, secp256k1_scalar * SECP256K1_RESTRICT r2, const secp256k1_scalar * SECP256K1_RESTRICT k) { + VERIFY_CHECK(r1 != k); + VERIFY_CHECK(r2 != k); + VERIFY_CHECK(r1 != r2); *r2 = (*k + 5) % EXHAUSTIVE_TEST_ORDER; *r1 = (*k + (EXHAUSTIVE_TEST_ORDER - *r2) * EXHAUSTIVE_TEST_LAMBDA) % EXHAUSTIVE_TEST_ORDER; } @@ -119,7 +122,7 @@ static void secp256k1_scalar_split_lambda_verify(const secp256k1_scalar *r1, con * * See proof below. */ -static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar *r2, const secp256k1_scalar *k) { +static void secp256k1_scalar_split_lambda(secp256k1_scalar * SECP256K1_RESTRICT r1, secp256k1_scalar * SECP256K1_RESTRICT r2, const secp256k1_scalar * SECP256K1_RESTRICT k) { secp256k1_scalar c1, c2; static const secp256k1_scalar minus_b1 = SECP256K1_SCALAR_CONST( 0x00000000UL, 0x00000000UL, 0x00000000UL, 0x00000000UL, @@ -139,6 +142,7 @@ static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar ); VERIFY_CHECK(r1 != k); VERIFY_CHECK(r2 != k); + VERIFY_CHECK(r1 != r2); /* these _var calls are constant time since the shift amount is constant */ secp256k1_scalar_mul_shift_var(&c1, k, &g1, 384); secp256k1_scalar_mul_shift_var(&c2, k, &g2, 384);