Merge #1169: Add support for msan instead of valgrind (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

Author: real-or-random

.cirrus.yml

@@ -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
 
@@ -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:
@@ -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:
@@ -128,7 +128,7 @@ task:
   env:
     ASM: no
     WITH_VALGRIND: no
-    CTIMETEST: no
+    CTIMETESTS: no
   matrix:
     - env:
         CC: gcc
@@ -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
@@ -172,7 +172,7 @@ task:
     ECDH: yes
     RECOVERY: yes
     SCHNORRSIG: yes
-    CTIMETEST: no
+    CTIMETESTS: no
   matrix:
     - env: {}
     - env: {EXPERIMENTAL: yes, ASM: arm}
@@ -192,7 +192,7 @@ task:
     ECDH: yes
     RECOVERY: yes
     SCHNORRSIG: yes
-    CTIMETEST: no
+    CTIMETESTS: no
   << : *MERGE_BASE
   test_script:
     - ./ci/cirrus.sh
@@ -209,7 +209,7 @@ task:
     ECDH: yes
     RECOVERY: yes
     SCHNORRSIG: yes
-    CTIMETEST: no
+    CTIMETESTS: no
   << : *MERGE_BASE
   test_script:
     - ./ci/cirrus.sh
@@ -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:
@@ -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.
@@ -285,7 +285,7 @@ task:
     ECDH: yes
     RECOVERY: yes
     SCHNORRSIG: yes
-    CTIMETEST: no
+    CTIMETESTS: no
   matrix:
     - name: "Valgrind (memcheck)"
       container:
@@ -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:

.gitignore

@@ -6,7 +6,7 @@ tests
 exhaustive_tests
 precompute_ecmult_gen
 precompute_ecmult
-valgrind_ctime_test
+ctime_tests
 ecdh_example
 ecdsa_example
 schnorr_example

Makefile.am

@@ -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
@@ -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
@@ -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
@@ -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

ci/cirrus.sh

@@ -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
@@ -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
 
@@ -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).

configure.ac

@@ -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])])
@@ -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"
@@ -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).
@@ -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"])
@@ -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"

doc/safegcd_implementation.md

@@ -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

src/checkmem.h

@@ -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 */