From 3777e3f36a61a05da19d5c6575fdd59288848948 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 17 Jan 2024 15:19:53 +0100 Subject: [PATCH 01/14] cmake: Recommend native CMake commands in README Resolves one item in #1235. Closes #1294. --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 4013e6a9..6e88eb4e 100644 --- a/README.md +++ b/README.md @@ -79,9 +79,9 @@ To maintain a pristine source tree, CMake encourages to perform an out-of-source $ mkdir build && cd build $ cmake .. - $ make - $ make check # run the test suite - $ sudo make install # optional + $ cmake --build . + $ ctest # run the test suite + $ sudo cmake --build . --target install # optional To compile optional modules (such as Schnorr signatures), you need to run `cmake` with additional flags (such as `-DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON`). Run `cmake .. -LH` to see the full list of available flags. From 51df2d9ab3a61dafee2c12b39e651d93a6b5be76 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 17 Jan 2024 16:54:04 +0100 Subject: [PATCH 02/14] tests: Drop redundant _scalar_check_overflow calls Redundant since d23da6d55714271c720fee58fbff5e5ef2fe193f. --- src/tests.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/tests.c b/src/tests.c index 32946439..e653aeea 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2927,18 +2927,14 @@ static void run_scalar_tests(void) { secp256k1_scalar_set_b32(&r2, res[i][1], &overflow); CHECK(!overflow); secp256k1_scalar_mul(&z, &x, &y); - CHECK(!secp256k1_scalar_check_overflow(&z)); CHECK(secp256k1_scalar_eq(&r1, &z)); if (!secp256k1_scalar_is_zero(&y)) { secp256k1_scalar_inverse(&zz, &y); - CHECK(!secp256k1_scalar_check_overflow(&zz)); secp256k1_scalar_inverse_var(&zzv, &y); CHECK(secp256k1_scalar_eq(&zzv, &zz)); secp256k1_scalar_mul(&z, &z, &zz); - CHECK(!secp256k1_scalar_check_overflow(&z)); CHECK(secp256k1_scalar_eq(&x, &z)); secp256k1_scalar_mul(&zz, &zz, &y); - CHECK(!secp256k1_scalar_check_overflow(&zz)); CHECK(secp256k1_scalar_eq(&secp256k1_scalar_one, &zz)); } secp256k1_scalar_mul(&z, &x, &x); From 94a14d5290e6ca805f23b1a7baca197d15e55f84 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 25 Jan 2024 12:09:47 +0000 Subject: [PATCH 03/14] ci: Update cache action This change fixes deprecation warnings for Node.js 16 actions in the GHA CI. See: - https://github.com/marketplace/actions/cache - https://github.com/actions/cache/releases/tag/v4.0.0 --- .github/actions/install-homebrew-valgrind/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/install-homebrew-valgrind/action.yml b/.github/actions/install-homebrew-valgrind/action.yml index 094ff891..ce10eb26 100644 --- a/.github/actions/install-homebrew-valgrind/action.yml +++ b/.github/actions/install-homebrew-valgrind/action.yml @@ -16,7 +16,7 @@ runs: cat valgrind_fingerprint shell: bash - - uses: actions/cache@v3 + - uses: actions/cache@v4 id: cache with: path: ${{ env.CI_HOMEBREW_CELLAR_VALGRIND }} From 00111c9c563367cf5163cda1ce6b9c6f032cea43 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 1 Feb 2024 00:47:12 +0100 Subject: [PATCH 04/14] tests: add missing fe comparison checks for inverse field test cases `check_fe_equal` is a wrapper around `secp256k1_fe_equal` that takes care of normalization. Since it doesn't check anything itself, the CHECK macro is needed at the call-sites to actually ensure equality. --- src/tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests.c b/src/tests.c index e653aeea..a64c3073 100644 --- a/src/tests.c +++ b/src/tests.c @@ -3618,9 +3618,9 @@ static void run_inverse_tests(void) for (i = 0; (size_t)i < sizeof(fe_cases)/sizeof(fe_cases[0]); ++i) { for (var = 0; var <= 1; ++var) { test_inverse_field(&x_fe, &fe_cases[i][0], var); - check_fe_equal(&x_fe, &fe_cases[i][1]); + CHECK(check_fe_equal(&x_fe, &fe_cases[i][1])); test_inverse_field(&x_fe, &fe_cases[i][1], var); - check_fe_equal(&x_fe, &fe_cases[i][0]); + CHECK(check_fe_equal(&x_fe, &fe_cases[i][0])); } } for (i = 0; (size_t)i < sizeof(scalar_cases)/sizeof(scalar_cases[0]); ++i) { From e7bdddd9c9c4b9f4f44420f3c7eeaef44f0c82ff Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 1 Feb 2024 15:33:02 +0100 Subject: [PATCH 05/14] refactor: rename `check_fe_equal` -> `fe_equal` As this function doesn't do any checking, it's better to rename it, so that it's less likely to miss the needed `CHECK`. --- src/modules/ellswift/tests_impl.h | 8 ++++---- src/tests.c | 28 ++++++++++++++-------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/modules/ellswift/tests_impl.h b/src/modules/ellswift/tests_impl.h index 7d1efbc4..f96e3a12 100644 --- a/src/modules/ellswift/tests_impl.h +++ b/src/modules/ellswift/tests_impl.h @@ -188,9 +188,9 @@ void run_ellswift_tests(void) { CHECK(ret == ((testcase->enc_bitmap >> c) & 1)); if (ret) { secp256k1_fe x2; - CHECK(check_fe_equal(&t, &testcase->encs[c])); + CHECK(fe_equal(&t, &testcase->encs[c])); secp256k1_ellswift_xswiftec_var(&x2, &testcase->u, &testcase->encs[c]); - CHECK(check_fe_equal(&testcase->x, &x2)); + CHECK(fe_equal(&testcase->x, &x2)); } } } @@ -203,7 +203,7 @@ void run_ellswift_tests(void) { CHECK(ret); ret = secp256k1_pubkey_load(CTX, &ge, &pubkey); CHECK(ret); - CHECK(check_fe_equal(&testcase->x, &ge.x)); + CHECK(fe_equal(&testcase->x, &ge.x)); CHECK(secp256k1_fe_is_odd(&ge.y) == testcase->odd_y); } for (i = 0; (unsigned)i < sizeof(ellswift_xdh_tests_bip324) / sizeof(ellswift_xdh_tests_bip324[0]); ++i) { @@ -290,7 +290,7 @@ void run_ellswift_tests(void) { secp256k1_ecmult(&resj, &decj, &sec, NULL); secp256k1_ge_set_gej(&res, &resj); /* Compare. */ - CHECK(check_fe_equal(&res.x, &share_x)); + CHECK(fe_equal(&res.x, &share_x)); } /* Verify the joint behavior of secp256k1_ellswift_xdh */ for (i = 0; i < 200 * COUNT; i++) { diff --git a/src/tests.c b/src/tests.c index a64c3073..1caee85e 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2953,7 +2953,7 @@ static void random_fe_non_square(secp256k1_fe *ns) { } } -static int check_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) { +static int fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) { secp256k1_fe an = *a; secp256k1_fe bn = *b; secp256k1_fe_normalize_weak(&an); @@ -3090,7 +3090,7 @@ static void run_field_half(void) { #endif secp256k1_fe_normalize_weak(&u); secp256k1_fe_add(&u, &u); - CHECK(check_fe_equal(&t, &u)); + CHECK(fe_equal(&t, &u)); /* Check worst-case input: ensure the LSB is 1 so that P will be added, * which will also cause all carries to be 1, since all limbs that can @@ -3109,7 +3109,7 @@ static void run_field_half(void) { #endif secp256k1_fe_normalize_weak(&u); secp256k1_fe_add(&u, &u); - CHECK(check_fe_equal(&t, &u)); + CHECK(fe_equal(&t, &u)); } } @@ -3136,7 +3136,7 @@ static void run_field_misc(void) { secp256k1_fe_add(&z, &q); /* z = x+v */ q = x; /* q = x */ secp256k1_fe_add_int(&q, v); /* q = x+v */ - CHECK(check_fe_equal(&q, &z)); + CHECK(fe_equal(&q, &z)); /* Test the fe equality and comparison operations. */ CHECK(secp256k1_fe_cmp_var(&x, &x) == 0); CHECK(secp256k1_fe_equal(&x, &x)); @@ -3196,27 +3196,27 @@ static void run_field_misc(void) { secp256k1_fe_add(&y, &x); z = x; secp256k1_fe_mul_int(&z, 3); - CHECK(check_fe_equal(&y, &z)); + CHECK(fe_equal(&y, &z)); secp256k1_fe_add(&y, &x); secp256k1_fe_add(&z, &x); - CHECK(check_fe_equal(&z, &y)); + CHECK(fe_equal(&z, &y)); z = x; secp256k1_fe_mul_int(&z, 5); secp256k1_fe_mul(&q, &x, &fe5); - CHECK(check_fe_equal(&z, &q)); + CHECK(fe_equal(&z, &q)); secp256k1_fe_negate(&x, &x, 1); secp256k1_fe_add(&z, &x); secp256k1_fe_add(&q, &x); - CHECK(check_fe_equal(&y, &z)); - CHECK(check_fe_equal(&q, &y)); + CHECK(fe_equal(&y, &z)); + CHECK(fe_equal(&q, &y)); /* Check secp256k1_fe_half. */ z = x; secp256k1_fe_half(&z); secp256k1_fe_add(&z, &z); - CHECK(check_fe_equal(&x, &z)); + CHECK(fe_equal(&x, &z)); secp256k1_fe_add(&z, &z); secp256k1_fe_half(&z); - CHECK(check_fe_equal(&x, &z)); + CHECK(fe_equal(&x, &z)); } } @@ -3618,9 +3618,9 @@ static void run_inverse_tests(void) for (i = 0; (size_t)i < sizeof(fe_cases)/sizeof(fe_cases[0]); ++i) { for (var = 0; var <= 1; ++var) { test_inverse_field(&x_fe, &fe_cases[i][0], var); - CHECK(check_fe_equal(&x_fe, &fe_cases[i][1])); + CHECK(fe_equal(&x_fe, &fe_cases[i][1])); test_inverse_field(&x_fe, &fe_cases[i][1], var); - CHECK(check_fe_equal(&x_fe, &fe_cases[i][0])); + CHECK(fe_equal(&x_fe, &fe_cases[i][0])); } } for (i = 0; (size_t)i < sizeof(scalar_cases)/sizeof(scalar_cases[0]); ++i) { @@ -4556,7 +4556,7 @@ static void ecmult_const_mult_xonly(void) { /* Check that resj's X coordinate corresponds with resx. */ secp256k1_fe_sqr(&v, &resj.z); secp256k1_fe_mul(&v, &v, &resx); - CHECK(check_fe_equal(&v, &resj.x)); + CHECK(fe_equal(&v, &resj.x)); } /* Test that secp256k1_ecmult_const_xonly correctly rejects X coordinates not on curve. */ From e7ea32e30a9f628855c4ddc7533ebd5d2013dc40 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 22 Feb 2024 20:11:51 +0000 Subject: [PATCH 06/14] msan: Add SECP256K1_CHECKMEM_MSAN_DEFINE which applies to memory sanitizer and not valgrind --- src/checkmem.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/checkmem.h b/src/checkmem.h index f2169dec..7e333ce5 100644 --- a/src/checkmem.h +++ b/src/checkmem.h @@ -30,6 +30,8 @@ * - SECP256K1_CHECKMEM_DEFINE(p, len): * - marks the len-byte memory pointed to by p as defined data (public data, in the * context of constant-time checking). + * - SECP256K1_CHECKMEM_MSAN_DEFINE(p, len): + * - Like SECP256K1_CHECKMEM_DEFINE, but applies only to memory_sanitizer. * */ @@ -48,11 +50,16 @@ # define SECP256K1_CHECKMEM_ENABLED 1 # define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len)) # define SECP256K1_CHECKMEM_DEFINE(p, len) __msan_unpoison((p), (len)) +# define SECP256K1_CHECKMEM_MSAN_DEFINE(p, len) __msan_unpoison((p), (len)) # define SECP256K1_CHECKMEM_CHECK(p, len) __msan_check_mem_is_initialized((p), (len)) # define SECP256K1_CHECKMEM_RUNNING() (1) # endif #endif +#if !defined SECP256K1_CHECKMEM_MSAN_DEFINE +# define SECP256K1_CHECKMEM_MSAN_DEFINE(p, len) SECP256K1_CHECKMEM_NOOP((p), (len)) +#endif + /* If valgrind integration is desired (through the VALGRIND define), implement the * SECP256K1_CHECKMEM_* macros using valgrind. */ #if !defined SECP256K1_CHECKMEM_ENABLED From 31ba40494428dcbf2eb5eb6f2328eca91b0b0746 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 21 Feb 2024 16:16:16 +0000 Subject: [PATCH 07/14] msan: notate variable assignments from assembly code msan isn't smart enough to see that these are set without some help. --- src/scalar_4x64_impl.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 7b9c542f..ede31a06 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -462,6 +462,14 @@ static void secp256k1_scalar_reduce_512(secp256k1_scalar *r, const uint64_t *l) : "S"(l), "i"(SECP256K1_N_C_0), "i"(SECP256K1_N_C_1) : "rax", "rdx", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "cc"); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m0, sizeof(m0)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m1, sizeof(m1)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m2, sizeof(m2)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m3, sizeof(m3)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m4, sizeof(m4)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m5, sizeof(m5)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&m6, sizeof(m6)); + /* Reduce 385 bits into 258. */ __asm__ __volatile__( /* Preload */ @@ -541,6 +549,12 @@ static void secp256k1_scalar_reduce_512(secp256k1_scalar *r, const uint64_t *l) : "g"(m0), "g"(m1), "g"(m2), "g"(m3), "g"(m4), "g"(m5), "g"(m6), "i"(SECP256K1_N_C_0), "i"(SECP256K1_N_C_1) : "rax", "rdx", "r8", "r9", "r10", "r11", "r12", "r13", "cc"); + SECP256K1_CHECKMEM_MSAN_DEFINE(&p0, sizeof(p0)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&p1, sizeof(p1)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&p2, sizeof(p2)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&p3, sizeof(p3)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&p4, sizeof(p4)); + /* Reduce 258 bits into 256. */ __asm__ __volatile__( /* Preload */ @@ -586,6 +600,10 @@ static void secp256k1_scalar_reduce_512(secp256k1_scalar *r, const uint64_t *l) : "=g"(c) : "g"(p0), "g"(p1), "g"(p2), "g"(p3), "g"(p4), "D"(r), "i"(SECP256K1_N_C_0), "i"(SECP256K1_N_C_1) : "rax", "rdx", "r8", "r9", "r10", "cc", "memory"); + + SECP256K1_CHECKMEM_MSAN_DEFINE(r, sizeof(*r)); + SECP256K1_CHECKMEM_MSAN_DEFINE(&c, sizeof(c)); + #else secp256k1_uint128 c128; uint64_t c, c0, c1, c2; From 11420a7a2836114393b70f4d10bb81921403a360 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 1 Feb 2024 00:28:45 +0100 Subject: [PATCH 08/14] tests: improve fe_sqr test Currently the `run_sqr` test doesn't do anything with the result of the `fe_sqr` call. Improve that by checking that the equation `(x+y)*(x-y) = x^2 - y^2` holds for some random values y, as suggested in issue #1471 by real-or-random. The existing loop for generating the x values is kept as-is. --- src/tests.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/tests.c b/src/tests.c index 1caee85e..85b48812 100644 --- a/src/tests.c +++ b/src/tests.c @@ -3285,18 +3285,31 @@ static void run_fe_mul(void) { } static void run_sqr(void) { - secp256k1_fe x, s; + int i; + secp256k1_fe x, y, lhs, rhs, tmp; - { - int i; - secp256k1_fe_set_int(&x, 1); - secp256k1_fe_negate(&x, &x, 1); + secp256k1_fe_set_int(&x, 1); + secp256k1_fe_negate(&x, &x, 1); - for (i = 1; i <= 512; ++i) { - secp256k1_fe_mul_int(&x, 2); - secp256k1_fe_normalize(&x); - secp256k1_fe_sqr(&s, &x); - } + for (i = 1; i <= 512; ++i) { + secp256k1_fe_mul_int(&x, 2); + secp256k1_fe_normalize(&x); + + /* Check that (x+y)*(x-y) = x^2 - y*2 for some random values y */ + random_fe_test(&y); + + lhs = x; + secp256k1_fe_add(&lhs, &y); /* lhs = x+y */ + secp256k1_fe_negate(&tmp, &y, 1); /* tmp = -y */ + secp256k1_fe_add(&tmp, &x); /* tmp = x-y */ + secp256k1_fe_mul(&lhs, &lhs, &tmp); /* lhs = (x+y)*(x-y) */ + + secp256k1_fe_sqr(&rhs, &x); /* rhs = x^2 */ + secp256k1_fe_sqr(&tmp, &y); /* tmp = y^2 */ + secp256k1_fe_negate(&tmp, &tmp, 1); /* tmp = -y^2 */ + secp256k1_fe_add(&rhs, &tmp); /* rhs = x^2 - y^2 */ + + CHECK(fe_equal(&lhs, &rhs)); } } From 2028069df2e16a05b332ae24c7ae63791f461063 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 1 Feb 2024 14:46:31 +0100 Subject: [PATCH 09/14] doc: clarify input requirements for secp256k1_fe_mul "... neither can be equal to b." could suggest that the values are not allowed to be identical, but what is meant here is that the mentioned inputs shouldn't point to the same object. --- src/field.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/field.h b/src/field.h index bd589bf8..8c65a3af 100644 --- a/src/field.h +++ b/src/field.h @@ -255,8 +255,8 @@ static void secp256k1_fe_add(secp256k1_fe *r, const secp256k1_fe *a); /** Multiply two field elements. * * On input, a and b must be valid field elements; r does not need to be initialized. - * r and a may point to the same object, but neither can be equal to b. The magnitudes - * of a and b must not exceed 8. + * r and a may point to the same object, but neither may point to the object pointed + * to by b. The magnitudes of a and b must not exceed 8. * Performs {r = a * b} * On output, r will have magnitude 1, but won't be normalized. */ From 84a93de4d2ba1063aa3d67d592db8ae0bb4a307e Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Tue, 19 Mar 2024 17:15:14 +0100 Subject: [PATCH 10/14] ci: Add workaround for ASLR bug in sanitizers Fixes #1506. --- .github/actions/run-in-docker-action/action.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/actions/run-in-docker-action/action.yml b/.github/actions/run-in-docker-action/action.yml index dbfaa4fe..126abc2d 100644 --- a/.github/actions/run-in-docker-action/action.yml +++ b/.github/actions/run-in-docker-action/action.yml @@ -36,6 +36,10 @@ runs: load: true cache-from: type=gha + - # Workaround for https://github.com/google/sanitizers/issues/1614 . + run: sudo sysctl -w vm.mmap_rnd_bits=28 + shell: bash + - # Tell Docker to pass environment variables in `env` into the container. run: > docker run \ From a5e8ab248447864873eae9b00f94175594477d66 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Tue, 19 Mar 2024 17:17:13 +0100 Subject: [PATCH 11/14] ci: Add sanitizer env variables to debug output --- ci/ci.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ci/ci.sh b/ci/ci.sh index 9cc71595..3999af4f 100755 --- a/ci/ci.sh +++ b/ci/ci.sh @@ -17,7 +17,8 @@ print_environment() { SECP256K1_TEST_ITERS BENCH SECP256K1_BENCH_ITERS CTIMETESTS\ EXAMPLES \ HOST WRAPPER_CMD \ - CC CFLAGS CPPFLAGS AR NM + CC CFLAGS CPPFLAGS AR NM \ + UBSAN_OPTIONS ASAN_OPTIONS LSAN_OPTIONS do eval "isset=\${$var+x}" if [ -n "$isset" ]; then From a61339149f1faae3c5d8e9ba533d96ff1ca2c32d Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Fri, 29 Mar 2024 15:04:39 +0000 Subject: [PATCH 12/14] change inconsistent array param to pointer The behavior is identical, but the former syntax suggests guarantees that don't actually exist. --- src/scalar_4x64_impl.h | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index ede31a06..835b7fa6 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -681,7 +681,7 @@ static void secp256k1_scalar_reduce_512(secp256k1_scalar *r, const uint64_t *l) secp256k1_scalar_reduce(r, c + secp256k1_scalar_check_overflow(r)); } -static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, const secp256k1_scalar *b) { +static void secp256k1_scalar_mul_512(uint64_t *l8, const secp256k1_scalar *a, const secp256k1_scalar *b) { #ifdef USE_ASM_X86_64 const uint64_t *pb = b->d; __asm__ __volatile__( @@ -696,7 +696,7 @@ static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, c /* (rax,rdx) = a0 * b0 */ "movq %%r15, %%rax\n" "mulq %%r11\n" - /* Extract l0 */ + /* Extract l8[0] */ "movq %%rax, 0(%%rsi)\n" /* (r8,r9,r10) = (rdx) */ "movq %%rdx, %%r8\n" @@ -714,7 +714,7 @@ static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, c "addq %%rax, %%r8\n" "adcq %%rdx, %%r9\n" "adcq $0, %%r10\n" - /* Extract l1 */ + /* Extract l8[1] */ "movq %%r8, 8(%%rsi)\n" "xorq %%r8, %%r8\n" /* (r9,r10,r8) += a0 * b2 */ @@ -735,7 +735,7 @@ static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, c "addq %%rax, %%r9\n" "adcq %%rdx, %%r10\n" "adcq $0, %%r8\n" - /* Extract l2 */ + /* Extract l8[2] */ "movq %%r9, 16(%%rsi)\n" "xorq %%r9, %%r9\n" /* (r10,r8,r9) += a0 * b3 */ @@ -764,7 +764,7 @@ static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, c "addq %%rax, %%r10\n" "adcq %%rdx, %%r8\n" "adcq $0, %%r9\n" - /* Extract l3 */ + /* Extract l8[3] */ "movq %%r10, 24(%%rsi)\n" "xorq %%r10, %%r10\n" /* (r8,r9,r10) += a1 * b3 */ @@ -785,7 +785,7 @@ static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, c "addq %%rax, %%r8\n" "adcq %%rdx, %%r9\n" "adcq $0, %%r10\n" - /* Extract l4 */ + /* Extract l8[4] */ "movq %%r8, 32(%%rsi)\n" "xorq %%r8, %%r8\n" /* (r9,r10,r8) += a2 * b3 */ @@ -800,51 +800,51 @@ static void secp256k1_scalar_mul_512(uint64_t l[8], const secp256k1_scalar *a, c "addq %%rax, %%r9\n" "adcq %%rdx, %%r10\n" "adcq $0, %%r8\n" - /* Extract l5 */ + /* Extract l8[5] */ "movq %%r9, 40(%%rsi)\n" /* (r10,r8) += a3 * b3 */ "movq %%r15, %%rax\n" "mulq %%r14\n" "addq %%rax, %%r10\n" "adcq %%rdx, %%r8\n" - /* Extract l6 */ + /* Extract l8[6] */ "movq %%r10, 48(%%rsi)\n" - /* Extract l7 */ + /* Extract l8[7] */ "movq %%r8, 56(%%rsi)\n" : "+d"(pb) - : "S"(l), "D"(a->d) + : "S"(l8), "D"(a->d) : "rax", "rbx", "rcx", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15", "cc", "memory"); #else /* 160 bit accumulator. */ uint64_t c0 = 0, c1 = 0; uint32_t c2 = 0; - /* l[0..7] = a[0..3] * b[0..3]. */ + /* l8[0..7] = a[0..3] * b[0..3]. */ muladd_fast(a->d[0], b->d[0]); - extract_fast(l[0]); + extract_fast(l8[0]); muladd(a->d[0], b->d[1]); muladd(a->d[1], b->d[0]); - extract(l[1]); + extract(l8[1]); muladd(a->d[0], b->d[2]); muladd(a->d[1], b->d[1]); muladd(a->d[2], b->d[0]); - extract(l[2]); + extract(l8[2]); muladd(a->d[0], b->d[3]); muladd(a->d[1], b->d[2]); muladd(a->d[2], b->d[1]); muladd(a->d[3], b->d[0]); - extract(l[3]); + extract(l8[3]); muladd(a->d[1], b->d[3]); muladd(a->d[2], b->d[2]); muladd(a->d[3], b->d[1]); - extract(l[4]); + extract(l8[4]); muladd(a->d[2], b->d[3]); muladd(a->d[3], b->d[2]); - extract(l[5]); + extract(l8[5]); muladd_fast(a->d[3], b->d[3]); - extract_fast(l[6]); + extract_fast(l8[6]); VERIFY_CHECK(c1 == 0); - l[7] = c0; + l8[7] = c0; #endif } From f7f0184ba1358cb8659607d2d0105628cb130e4f Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 27 Mar 2024 15:18:57 +0000 Subject: [PATCH 13/14] msan: notate more variable assignments from assembly code This was missed in 31ba40494428dcbf2eb5eb6f2328eca91b0b0746 because older versions of clang did not complain about it. But clang-17, at least, does. --- src/scalar_4x64_impl.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 835b7fa6..82cd957f 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -814,6 +814,9 @@ static void secp256k1_scalar_mul_512(uint64_t *l8, const secp256k1_scalar *a, co : "+d"(pb) : "S"(l8), "D"(a->d) : "rax", "rbx", "rcx", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15", "cc", "memory"); + + SECP256K1_CHECKMEM_MSAN_DEFINE(l8, sizeof(*l8) * 8); + #else /* 160 bit accumulator. */ uint64_t c0 = 0, c1 = 0; From a85e2233e701c3e3dd1673d187d7d8806de387ff Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 4 Apr 2024 11:40:19 +0200 Subject: [PATCH 14/14] ci: Note affected clangs in comment on ASLR quirk Resolves #1506. --- .github/actions/run-in-docker-action/action.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/run-in-docker-action/action.yml b/.github/actions/run-in-docker-action/action.yml index 126abc2d..74933686 100644 --- a/.github/actions/run-in-docker-action/action.yml +++ b/.github/actions/run-in-docker-action/action.yml @@ -37,6 +37,7 @@ runs: cache-from: type=gha - # Workaround for https://github.com/google/sanitizers/issues/1614 . + # The underlying issue has been fixed in clang 18.1.3. run: sudo sysctl -w vm.mmap_rnd_bits=28 shell: bash