Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1169: Add support for msan instead of va…
Browse files Browse the repository at this point in the history
…lgrind (for memcheck and ctime test)

0f088ec Rename CTIMETEST -> CTIMETESTS (Pieter Wuille)
74b026f Add runtime checking for DECLASSIFY flag (Pieter Wuille)
5e2e6fc Run ctime test in Linux MSan CI job (Pieter Wuille)
1897406 Make ctime tests building configurable (Pieter Wuille)
5048be1 Rename valgrind_ctime_test -> ctime_tests (Pieter Wuille)
6eed6c1 Update error messages to suggest msan as well (Pieter Wuille)
8e11f89 Add support for msan integration to checkmem.h (Pieter Wuille)
8dc6407 Add compile-time error to valgrind_ctime_test (Pieter Wuille)
0db05a7 Abstract interactions with valgrind behind new checkmem.h (Pieter Wuille)
4f1a54e Move valgrind CPPFLAGS into SECP_CONFIG_DEFINES (Pieter Wuille)

Pull request description:

  This introduces an abstraction layer `src/checkmem.h`, which defines macros for interacting with memory checking tools. Depending on the environment, they're mapped to MemorySanitizer builtins, Valgrind integration macros, or nothing at all.

  This means that msan builds immediately benefit from existing undefined memory checks in the tests. It also means those builds result in a `ctime_tests` (new name for `valgrind_ctime_test`) binary that can usefully test constant-timeness (not inside Valgrind, and with the downside that it's not running against a production library build, but it's faster and available on more platforms).

  Such an msan-ctime test is added to the Linux x86_64 msan CI job, as an example. More CI cases could be added (e.g. for MacOs or ARM Linux) later.

ACKs for top commit:
  real-or-random:
    ACK 0f088ec
  hebasto:
    ACK 0f088ec, I have reviewed the code and it looks OK. Able to build `ctime_tests` using MSan.

Tree-SHA512: f4ffcc0c2ea794894662d9797b3a349770a4b361996f967f33d7d14b332171de5d525f50bcebaeaf7d0624957083380962079c75e490d1b7d71f8f9eb6211590
  • Loading branch information
real-or-random committed Jan 16, 2023
2 parents ff8edf8 + 0f088ec commit f29a327
Show file tree
Hide file tree
Showing 16 changed files with 279 additions and 191 deletions.
29 changes: 15 additions & 14 deletions .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ env:
SECP256K1_TEST_ITERS:
BENCH: yes
SECP256K1_BENCH_ITERS: 2
CTIMETEST: yes
CTIMETESTS: yes
# Compile and run the tests
EXAMPLES: yes

Expand All @@ -40,8 +40,8 @@ cat_logs_snippet: &CAT_LOGS
- cat noverify_tests.log || true
cat_exhaustive_tests_log_script:
- cat exhaustive_tests.log || true
cat_valgrind_ctime_test_log_script:
- cat valgrind_ctime_test.log || true
cat_ctime_tests_log_script:
- cat ctime_tests.log || true
cat_bench_log_script:
- cat bench.log || true
cat_config_log_script:
Expand Down Expand Up @@ -81,9 +81,9 @@ task:
- env: {WIDEMUL: int128, ECDH: yes, SCHNORRSIG: yes}
- env: {WIDEMUL: int128, ASM: x86_64}
- env: { RECOVERY: yes, SCHNORRSIG: yes}
- env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETEST: no, BENCH: no}
- env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETESTS: no, BENCH: no}
- env: {CPPFLAGS: -DDETERMINISTIC}
- env: {CFLAGS: -O0, CTIMETEST: no}
- env: {CFLAGS: -O0, CTIMETESTS: no}
- env: { ECMULTGENPRECISION: 2, ECMULTWINDOW: 2 }
- env: { ECMULTGENPRECISION: 8, ECMULTWINDOW: 4 }
matrix:
Expand Down Expand Up @@ -128,7 +128,7 @@ task:
env:
ASM: no
WITH_VALGRIND: no
CTIMETEST: no
CTIMETESTS: no
matrix:
- env:
CC: gcc
Expand All @@ -153,7 +153,7 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
<< : *MERGE_BASE
test_script:
# https://sourceware.org/bugzilla/show_bug.cgi?id=27008
Expand All @@ -172,7 +172,7 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
matrix:
- env: {}
- env: {EXPERIMENTAL: yes, ASM: arm}
Expand All @@ -192,7 +192,7 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
<< : *MERGE_BASE
test_script:
- ./ci/cirrus.sh
Expand All @@ -209,7 +209,7 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
<< : *MERGE_BASE
test_script:
- ./ci/cirrus.sh
Expand All @@ -223,7 +223,7 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
matrix:
- name: "x86_64 (mingw32-w64): Windows (Debian stable, Wine)"
env:
Expand All @@ -246,7 +246,7 @@ task:
RECOVERY: yes
EXPERIMENTAL: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
# Use a MinGW-w64 host to tell ./configure we're building for Windows.
# This will detect some MinGW-w64 tools but then make will need only
# the MSVC tools CC, AR and NM as specified below.
Expand Down Expand Up @@ -285,7 +285,7 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: no
matrix:
- name: "Valgrind (memcheck)"
container:
Expand Down Expand Up @@ -330,10 +330,11 @@ task:
ECDH: yes
RECOVERY: yes
SCHNORRSIG: yes
CTIMETEST: no
CTIMETESTS: yes
CC: clang
SECP256K1_TEST_ITERS: 32
ASM: no
WITH_VALGRIND: no
container:
memory: 2G
matrix:
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ tests
exhaustive_tests
precompute_ecmult_gen
precompute_ecmult
valgrind_ctime_test
ctime_tests
ecdh_example
ecdsa_example
schnorr_example
Expand Down
18 changes: 8 additions & 10 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ noinst_HEADERS += src/modinv64_impl.h
noinst_HEADERS += src/precomputed_ecmult.h
noinst_HEADERS += src/precomputed_ecmult_gen.h
noinst_HEADERS += src/assumptions.h
noinst_HEADERS += src/checkmem.h
noinst_HEADERS += src/util.h
noinst_HEADERS += src/int128.h
noinst_HEADERS += src/int128_impl.h
Expand Down Expand Up @@ -98,10 +99,6 @@ libsecp256k1_la_CPPFLAGS = $(SECP_INCLUDES) $(SECP_CONFIG_DEFINES)
libsecp256k1_la_LIBADD = $(SECP_LIBS) $(COMMON_LIB) $(PRECOMPUTED_LIB)
libsecp256k1_la_LDFLAGS = -no-undefined -version-info $(LIB_VERSION_CURRENT):$(LIB_VERSION_REVISION):$(LIB_VERSION_AGE)

if VALGRIND_ENABLED
libsecp256k1_la_CPPFLAGS += -DVALGRIND
endif

noinst_PROGRAMS =
if USE_BENCHMARK
noinst_PROGRAMS += bench bench_internal bench_ecmult
Expand All @@ -124,12 +121,6 @@ noverify_tests_SOURCES = src/tests.c
noverify_tests_CPPFLAGS = $(SECP_INCLUDES) $(SECP_TEST_INCLUDES) $(SECP_CONFIG_DEFINES)
noverify_tests_LDADD = $(SECP_LIBS) $(SECP_TEST_LIBS) $(COMMON_LIB) $(PRECOMPUTED_LIB)
noverify_tests_LDFLAGS = -static
if VALGRIND_ENABLED
noverify_tests_CPPFLAGS += -DVALGRIND
noinst_PROGRAMS += valgrind_ctime_test
valgrind_ctime_test_SOURCES = src/valgrind_ctime_test.c
valgrind_ctime_test_LDADD = libsecp256k1.la $(SECP_LIBS) $(COMMON_LIB)
endif
if !ENABLE_COVERAGE
TESTS += tests
noinst_PROGRAMS += tests
Expand All @@ -140,6 +131,13 @@ tests_LDFLAGS = $(noverify_tests_LDFLAGS)
endif
endif

if USE_CTIME_TESTS
noinst_PROGRAMS += ctime_tests
ctime_tests_SOURCES = src/ctime_tests.c
ctime_tests_LDADD = libsecp256k1.la $(SECP_LIBS) $(COMMON_LIB)
ctime_tests_CPPFLAGS = $(SECP_CONFIG_DEFINES)
endif

if USE_EXHAUSTIVE_TESTS
noinst_PROGRAMS += exhaustive_tests
exhaustive_tests_SOURCES = src/tests_exhaustive.c
Expand Down
24 changes: 15 additions & 9 deletions ci/cirrus.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ print_environment() {
for var in WERROR_CFLAGS MAKEFLAGS BUILD \
ECMULTWINDOW ECMULTGENPRECISION ASM WIDEMUL WITH_VALGRIND EXTRAFLAGS \
EXPERIMENTAL ECDH RECOVERY SCHNORRSIG \
SECP256K1_TEST_ITERS BENCH SECP256K1_BENCH_ITERS CTIMETEST\
SECP256K1_TEST_ITERS BENCH SECP256K1_BENCH_ITERS CTIMETESTS\
EXAMPLES \
HOST WRAPPER_CMD \
CC CFLAGS CPPFLAGS AR NM
Expand Down Expand Up @@ -62,6 +62,7 @@ fi
--enable-module-ecdh="$ECDH" --enable-module-recovery="$RECOVERY" \
--enable-module-schnorrsig="$SCHNORRSIG" \
--enable-examples="$EXAMPLES" \
--enable-ctime-tests="$CTIMETESTS" \
--with-valgrind="$WITH_VALGRIND" \
--host="$HOST" $EXTRAFLAGS

Expand All @@ -78,24 +79,29 @@ export LOG_COMPILER="$WRAPPER_CMD"

make "$BUILD"

# Using the local `libtool` because on macOS the system's libtool has nothing to do with GNU libtool
EXEC='./libtool --mode=execute'
if [ -n "$WRAPPER_CMD" ]
then
EXEC="$EXEC $WRAPPER_CMD"
fi

if [ "$BENCH" = "yes" ]
then
# Using the local `libtool` because on macOS the system's libtool has nothing to do with GNU libtool
EXEC='./libtool --mode=execute'
if [ -n "$WRAPPER_CMD" ]
then
EXEC="$EXEC $WRAPPER_CMD"
fi
{
$EXEC ./bench_ecmult
$EXEC ./bench_internal
$EXEC ./bench
} >> bench.log 2>&1
fi

if [ "$CTIMETEST" = "yes" ]
if [ "$CTIMETESTS" = "yes" ]
then
./libtool --mode=execute valgrind --error-exitcode=42 ./valgrind_ctime_test > valgrind_ctime_test.log 2>&1
if [ "$WITH_VALGRIND" = "yes" ]; then
./libtool --mode=execute valgrind --error-exitcode=42 ./ctime_tests > ctime_tests.log 2>&1
else
$EXEC ./ctime_tests > ctime_tests.log 2>&1
fi
fi

# Rebuild precomputed files (if not cross-compiling).
Expand Down
13 changes: 11 additions & 2 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ AC_ARG_ENABLE(tests,
AS_HELP_STRING([--enable-tests],[compile tests [default=yes]]), [],
[SECP_SET_DEFAULT([enable_tests], [yes], [yes])])

AC_ARG_ENABLE(ctime_tests,
AS_HELP_STRING([--enable-ctime-tests],[compile constant-time tests [default=yes if valgrind enabled]]), [],
[SECP_SET_DEFAULT([enable_ctime_tests], [auto], [auto])])

AC_ARG_ENABLE(experimental,
AS_HELP_STRING([--enable-experimental],[allow experimental configure options [default=no]]), [],
[SECP_SET_DEFAULT([enable_experimental], [no], [yes])])
Expand Down Expand Up @@ -225,7 +229,10 @@ else
enable_valgrind=yes
fi
fi
AM_CONDITIONAL([VALGRIND_ENABLED],[test "$enable_valgrind" = "yes"])

if test x"$enable_ctime_tests" = x"auto"; then
enable_ctime_tests=$enable_valgrind
fi

if test x"$enable_coverage" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
Expand Down Expand Up @@ -344,7 +351,7 @@ case $set_ecmult_gen_precision in
esac

if test x"$enable_valgrind" = x"yes"; then
SECP_INCLUDES="$SECP_INCLUDES $VALGRIND_CPPFLAGS"
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES $VALGRIND_CPPFLAGS -DVALGRIND"
fi

# Add -Werror and similar flags passed from the outside (for testing, e.g., in CI).
Expand Down Expand Up @@ -407,6 +414,7 @@ AC_SUBST(SECP_CFLAGS)
AC_SUBST(SECP_CONFIG_DEFINES)
AM_CONDITIONAL([ENABLE_COVERAGE], [test x"$enable_coverage" = x"yes"])
AM_CONDITIONAL([USE_TESTS], [test x"$enable_tests" != x"no"])
AM_CONDITIONAL([USE_CTIME_TESTS], [test x"$enable_ctime_tests" = x"yes"])
AM_CONDITIONAL([USE_EXHAUSTIVE_TESTS], [test x"$enable_exhaustive_tests" != x"no"])
AM_CONDITIONAL([USE_EXAMPLES], [test x"$enable_examples" != x"no"])
AM_CONDITIONAL([USE_BENCHMARK], [test x"$enable_benchmark" = x"yes"])
Expand All @@ -428,6 +436,7 @@ echo "Build Options:"
echo " with external callbacks = $enable_external_default_callbacks"
echo " with benchmarks = $enable_benchmark"
echo " with tests = $enable_tests"
echo " with ctime tests = $enable_ctime_tests"
echo " with coverage = $enable_coverage"
echo " with examples = $enable_examples"
echo " module ecdh = $enable_module_ecdh"
Expand Down
2 changes: 1 addition & 1 deletion doc/safegcd_implementation.md
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ sufficient even. Given that every loop iteration performs *N* divsteps, it will

To deal with the branches in `divsteps_n_matrix` we will replace them with constant-time bitwise
operations (and hope the C compiler isn't smart enough to turn them back into branches; see
`valgrind_ctime_test.c` for automated tests that this isn't the case). To do so, observe that a
`ctime_tests.c` for automated tests that this isn't the case). To do so, observe that a
divstep can be written instead as (compare to the inner loop of `gcd` in section 1).

```python
Expand Down
88 changes: 88 additions & 0 deletions src/checkmem.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/***********************************************************************
* Copyright (c) 2022 Pieter Wuille *
* Distributed under the MIT software license, see the accompanying *
* file COPYING or https://www.opensource.org/licenses/mit-license.php.*
***********************************************************************/

/* The code here is inspired by Kris Kwiatkowski's approach in
* https://github.com/kriskwiatkowski/pqc/blob/main/src/common/ct_check.h
* to provide a general interface for memory-checking mechanisms, primarily
* for constant-time checking.
*/

/* These macros are defined by this header file:
*
* - SECP256K1_CHECKMEM_ENABLED:
* - 1 if memory-checking integration is available, 0 otherwise.
* This is just a compile-time macro. Use the next macro to check it is actually
* available at runtime.
* - SECP256K1_CHECKMEM_RUNNING():
* - Acts like a function call, returning 1 if memory checking is available
* at runtime.
* - SECP256K1_CHECKMEM_CHECK(p, len):
* - Assert or otherwise fail in case the len-byte memory block pointed to by p is
* not considered entirely defined.
* - SECP256K1_CHECKMEM_CHECK_VERIFY(p, len):
* - Like SECP256K1_CHECKMEM_CHECK, but only works in VERIFY mode.
* - SECP256K1_CHECKMEM_UNDEFINE(p, len):
* - marks the len-byte memory block pointed to by p as undefined data (secret data,
* in the context of constant-time checking).
* - 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).
*
*/

#ifndef SECP256K1_CHECKMEM_H
#define SECP256K1_CHECKMEM_H

/* Define a statement-like macro that ignores the arguments. */
#define SECP256K1_CHECKMEM_NOOP(p, len) do { (void)(p); (void)(len); } while(0)

/* If compiling under msan, map the SECP256K1_CHECKMEM_* functionality to msan.
* Choose this preferentially, even when VALGRIND is defined, as msan-compiled
* binaries can't be run under valgrind anyway. */
#if defined(__has_feature)
# if __has_feature(memory_sanitizer)
# include <sanitizer/msan_interface.h>
# 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_CHECK(p, len) __msan_check_mem_is_initialized((p), (len))
# define SECP256K1_CHECKMEM_RUNNING() (1)
# endif
#endif

/* If valgrind integration is desired (through the VALGRIND define), implement the
* SECP256K1_CHECKMEM_* macros using valgrind. */
#if !defined SECP256K1_CHECKMEM_ENABLED
# if defined VALGRIND
# include <stddef.h>
# include <valgrind/memcheck.h>
# define SECP256K1_CHECKMEM_ENABLED 1
# define SECP256K1_CHECKMEM_UNDEFINE(p, len) VALGRIND_MAKE_MEM_UNDEFINED((p), (len))
# define SECP256K1_CHECKMEM_DEFINE(p, len) VALGRIND_MAKE_MEM_DEFINED((p), (len))
# define SECP256K1_CHECKMEM_CHECK(p, len) VALGRIND_CHECK_MEM_IS_DEFINED((p), (len))
/* VALGRIND_MAKE_MEM_DEFINED returns 0 iff not running on memcheck.
* This is more precise than the RUNNING_ON_VALGRIND macro, which
* checks for valgrind in general instead of memcheck specifically. */
# define SECP256K1_CHECKMEM_RUNNING() (VALGRIND_MAKE_MEM_DEFINED(NULL, 0) != 0)
# endif
#endif

/* As a fall-back, map these macros to dummy statements. */
#if !defined SECP256K1_CHECKMEM_ENABLED
# define SECP256K1_CHECKMEM_ENABLED 0
# define SECP256K1_CHECKMEM_UNDEFINE(p, len) SECP256K1_CHECKMEM_NOOP((p), (len))
# define SECP256K1_CHECKMEM_DEFINE(p, len) SECP256K1_CHECKMEM_NOOP((p), (len))
# define SECP256K1_CHECKMEM_CHECK(p, len) SECP256K1_CHECKMEM_NOOP((p), (len))
# define SECP256K1_CHECKMEM_RUNNING() (0)
#endif

#if defined VERIFY
#define SECP256K1_CHECKMEM_CHECK_VERIFY(p, len) SECP256K1_CHECKMEM_CHECK((p), (len))
#else
#define SECP256K1_CHECKMEM_CHECK_VERIFY(p, len) SECP256K1_CHECKMEM_NOOP((p), (len))
#endif

#endif /* SECP256K1_CHECKMEM_H */
Loading

0 comments on commit f29a327

Please sign in to comment.