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 }} diff --git a/.github/actions/run-in-docker-action/action.yml b/.github/actions/run-in-docker-action/action.yml index dbfaa4fe..74933686 100644 --- a/.github/actions/run-in-docker-action/action.yml +++ b/.github/actions/run-in-docker-action/action.yml @@ -36,6 +36,11 @@ runs: load: true 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 + - # Tell Docker to pass environment variables in `env` into the container. run: > docker run \ diff --git a/ci/ci.sh b/ci/ci.sh index 47c4ae67..15f489af 100755 --- a/ci/ci.sh +++ b/ci/ci.sh @@ -18,7 +18,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 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 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. */ diff --git a/src/modules/ellswift/tests_impl.h b/src/modules/ellswift/tests_impl.h index fc163f39..440bcf06 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/scalar_4x64_impl.h b/src/scalar_4x64_impl.h index 6d97bce2..7ef726f7 100644 --- a/src/scalar_4x64_impl.h +++ b/src/scalar_4x64_impl.h @@ -493,6 +493,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 */ @@ -572,6 +580,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 */ @@ -617,6 +631,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; @@ -694,7 +712,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__( @@ -709,7 +727,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" @@ -727,7 +745,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 */ @@ -748,7 +766,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 */ @@ -777,7 +795,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 */ @@ -798,7 +816,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 */ @@ -813,51 +831,54 @@ 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"); + + SECP256K1_CHECKMEM_MSAN_DEFINE(l8, sizeof(*l8) * 8); + #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 } diff --git a/src/tests.c b/src/tests.c index 7c2f30e3..f56cb216 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2981,18 +2981,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); @@ -3013,7 +3009,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); @@ -3150,7 +3146,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 @@ -3169,7 +3165,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)); } } @@ -3196,7 +3192,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)); @@ -3256,27 +3252,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)); } } @@ -3345,18 +3341,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)); } } @@ -3678,9 +3687,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(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(fe_equal(&x_fe, &fe_cases[i][0])); } } for (i = 0; (size_t)i < sizeof(scalar_cases)/sizeof(scalar_cases[0]); ++i) { @@ -4737,7 +4746,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. */