Fix false positives in constant time tests using MSan with Clang 16

CMakeLists.txt

@@ -175,7 +175,7 @@ find_package(Threads)
 # If this is the root project add longer list of available CMAKE_BUILD_TYPE values
 if(NOT TF_PSA_CRYPTO_AS_SUBPROJECT)
     set(CMAKE_BUILD_TYPE ${CMAKE_BUILD_TYPE}
-        CACHE STRING "Choose the type of build: None Debug Release Coverage ASan ASanDbg MemSan MemSanDbg Check CheckFull TSan TSanDbg"
+        CACHE STRING "Choose the type of build: None Debug Release Coverage ASan ASanDbg CFMemSan CFMemSanDbg MemSan MemSanDbg Check CheckFull TSan TSanDbg"
         FORCE)
 endif()
 
@@ -318,10 +318,23 @@ function(set_clang_base_compile_options target)
     set_target_properties(${target} PROPERTIES LINK_FLAGS_ASAN "-fsanitize=address -fsanitize=undefined")
     target_compile_options(${target} PRIVATE $<$<CONFIG:ASanDbg>:-fsanitize=address -fno-common -fsanitize=undefined -fno-sanitize-recover=all -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls>)
     set_target_properties(${target} PROPERTIES LINK_FLAGS_ASANDBG "-fsanitize=address -fsanitize=undefined")
+
+    set(sanitize_memory_debug_flags "-O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2")
     target_compile_options(${target} PRIVATE $<$<CONFIG:MemSan>:-fsanitize=memory>)
     set_target_properties(${target} PROPERTIES LINK_FLAGS_MEMSAN "-fsanitize=memory")
-    target_compile_options(${target} PRIVATE $<$<CONFIG:MemSanDbg>:-fsanitize=memory -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2>)
+    target_compile_options(${target} PRIVATE $<$<CONFIG:MemSanDbg>:-fsanitize=memory ${sanitize_memory_debug_flags}>)
     set_target_properties(${target} PROPERTIES LINK_FLAGS_MEMSANDBG "-fsanitize=memory")
+
+    set(cf_sanitize_memory_flags -fsanitize=memory -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
+    if(CMAKE_C_COMPILER_VERSION VERSION_GREATER 16.0.0)
+        set(cf_sanitize_memory_flags ${cf_sanitize_memory_flags} -fno-sanitize-memory-param-retval)
+    endif()
+    string(REPLACE ";" " " cf_sanitize_memory_flags_joined "${cf_sanitize_memory_flags}")
+    target_compile_options(${target} PRIVATE $<$<CONFIG:CFMemSan>:${cf_sanitize_memory_flags}>)
+    set_target_properties(${target} PROPERTIES LINK_FLAGS_CFMEMSAN "${cf_sanitize_memory_flags_joined}")
+    target_compile_options(${target} PRIVATE $<$<CONFIG:CFMemSanDbg>:${cf_sanitize_memory_flags} ${sanitize_memory_debug_flags}>)
+    set_target_properties(${target} PROPERTIES LINK_FLAGS_CFMEMSANDBG "${cf_sanitize_memory_flags_joined}")
+
     target_compile_options(${target} PRIVATE $<$<CONFIG:TSan>:-fsanitize=thread -O3>)
     set_target_properties(${target} PROPERTIES LINK_FLAGS_TSAN "-fsanitize=thread")
     target_compile_options(${target} PRIVATE $<$<CONFIG:TSanDbg>:-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls>)

include/psa/crypto_config.h

@@ -636,8 +636,10 @@
  * clang's MemorySanitizer. This causes some existing tests to also test
  * this non-functional property of the code under test.
  *
- * This setting requires compiling with clang -fsanitize=memory. The test
- * suites can then be run normally.
+ * This setting requires compiling with
+ * `clang -fsanitize=memory -fno-sanitize-memory-param-retval` with Clang 16
+ * and above, or `clang -fsanitize=memory` with older Clang.
+ * The test suites can then be run normally.
  *
  * \warning This macro is only used for extended testing; it is not considered
  * part of the library's API, so it may change or disappear at any time.

tests/scripts/components-sanitizers.sh

@@ -33,8 +33,8 @@ skip_all_except_given_suite () {
 component_tf_psa_crypto_test_memsan_constant_flow_psa () {
     # This tests both (1) accesses to undefined memory, and (2) branches or
     # memory access depending on secret values. To distinguish between those:
-    # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist?
-    # - or alternatively, change the build type to MemSanDbg, which enables
+    # - change the build type to MemSan - does the failure persist?
+    # - or alternatively, change the build type to CFMemSanDbg, which enables
     # origin tracking and nicer stack traces (which are useful for debugging
     # anyway), and check if the origin was TEST_CF_SECRET() or something else.
     msg "build: cmake MSan (clang), full config with constant flow testing"
@@ -45,8 +45,7 @@ component_tf_psa_crypto_test_memsan_constant_flow_psa () {
     cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=ON "$TF_PSA_CRYPTO_ROOT_DIR"
     make
 
-    $TF_PSA_CRYPTO_ROOT_DIR/scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN
-    cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=OFF -DCMAKE_BUILD_TYPE:String=MemSan "$TF_PSA_CRYPTO_ROOT_DIR"
+    cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=OFF -DCMAKE_BUILD_TYPE:String=CFMemSan "$TF_PSA_CRYPTO_ROOT_DIR"
     make
 
     msg "test: main suites (Msan + constant flow)"

tests/suites/test_suite_constant_time.data

@@ -1,3 +1,30 @@
+Constant-flow instrumentation: NOP
+cf_instrumentation:CF_INSTRUMENTATION_NOP
+
+Constant-flow instrumentation: memcpy
+cf_instrumentation:CF_INSTRUMENTATION_MEMCPY
+
+Constant-flow instrumentation: memmove
+cf_instrumentation:CF_INSTRUMENTATION_MEMMOVE
+
+Constant-flow instrumentation: copy with for loop
+cf_instrumentation:CF_INSTRUMENTATION_COPY_LOOP
+
+# Detect sanitizers where a secret value may not be passed to a function.
+# Such sanitizers cause false positives in most of the other constant-flow
+# tests.
+# This is the case with MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN when building
+# with Clang >=16 with just -fsanitize=memory. The fix is to pass the
+# additional option -fno-sanitize-memory-param-retval to clang.
+Constant-flow instrumentation: copy through function
+cf_instrumentation:CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION
+
+Constant-flow instrumentation: memset
+cf_instrumentation:CF_INSTRUMENTATION_MEMSET
+
+Constant-flow instrumentation: mbedtls_platform_zeroize
+cf_instrumentation:CF_INSTRUMENTATION_ZEROIZE
+
 # these are the numbers we'd get with an empty plaintext and truncated HMAC
 Constant-flow memcpy from offset: small
 mbedtls_ct_memcpy_offset:0:5:10

tests/suites/test_suite_constant_time.function

@@ -19,8 +19,98 @@
 #include <constant_time_internal.h>
 
 #include <test/constant_flow.h>
+
+/* Which kind of smoke test cf_instrumentation will do. */
+typedef enum {
+    CF_INSTRUMENTATION_NOP,
+    CF_INSTRUMENTATION_MEMCPY,
+    CF_INSTRUMENTATION_MEMMOVE,
+    CF_INSTRUMENTATION_COPY_LOOP,
+    CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION,
+    CF_INSTRUMENTATION_MEMSET,
+    CF_INSTRUMENTATION_ZEROIZE,
+} cf_instrumentation_method_t;
+
+/* A function that is non-inlined on GCC-like compilers, including Clang.
+ * Use this to implement CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION.
+ */
+#if defined(__has_attribute)
+#if __has_attribute(noinline)
+__attribute__((noinline))
+#endif
+#endif
+static unsigned char u8_identity_function(unsigned char c)
+{
+    return c;
+}
+
 /* END_HEADER */
 
+/* BEGIN_CASE */
+/* Test the constant-flow instrumentation. If this test function fails,
+ * something is wrong with the constant-flow instrumentation. */
+void cf_instrumentation(int method)
+{
+    unsigned char a[42] = { 0 };
+    unsigned char *p = NULL;
+
+    /* Set the buffer at p to some not-all-zero pattern,
+     * unless using the NOP method. */
+    TEST_CALLOC(p, sizeof(a));
+    if (method != CF_INSTRUMENTATION_NOP) {
+        for (size_t i = 0; i < sizeof(a); i++) {
+            p[i] = i & 0xff;
+        }
+    }
+
+    TEST_CF_SECRET(&a, sizeof(a));
+    TEST_CF_SECRET(p, sizeof(a));
+
+    /* Set the buffer at p to all-zero, possibly using a, in constant time. */
+    switch (method) {
+        case CF_INSTRUMENTATION_NOP:
+            /* p is already zero per above. */
+            break;
+        case CF_INSTRUMENTATION_MEMCPY:
+            memcpy(p, a, sizeof(a));
+            break;
+        case CF_INSTRUMENTATION_MEMMOVE:
+            memmove(p, a, sizeof(a));
+            break;
+        case CF_INSTRUMENTATION_COPY_LOOP:
+            for (size_t i = 0; i < sizeof(a); i++) {
+                p[i] = a[i];
+            }
+            break;
+        case CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION:
+            for (size_t i = 0; i < sizeof(a); i++) {
+                p[i] = u8_identity_function(a[i]);
+            }
+            break;
+        case CF_INSTRUMENTATION_MEMSET:
+            memset(p, 0, sizeof(a));
+            break;
+        case CF_INSTRUMENTATION_ZEROIZE:
+            mbedtls_platform_zeroize(p, sizeof(a));
+            break;
+        default:
+            TEST_CF_PUBLIC(&a, sizeof(a));
+            TEST_CF_PUBLIC(p, sizeof(a));
+            TEST_FAIL("Unknown instrumentation method");
+    }
+
+    /* Check that the buffer at p is now all-zero, not in constant time.
+     * If this triggers a constant-flow violation, TEST_CF_PUBLIC isn't
+     * doing its job. */
+    TEST_CF_PUBLIC(&a, sizeof(a));
+    TEST_CF_PUBLIC(p, sizeof(a));
+    ASSERT_COMPARE(a, sizeof(a), p, sizeof(a));
+
+exit:
+    mbedtls_free(p);
+}
+/* END_CASE */
+
 /* BEGIN_CASE */
 void mbedtls_ct_memcmp_null()
 {