Backport 3.6: Fix false positives in constant time tests using MSan with Clang 16 #9922
Conversation
In the documentation of `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN`, note that since Clang 16, an extra command line option `-fsanitize-memory-param-retval` is required. As documented in the release notes https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html#sanitizers since Clang 16, MSan forbids passing "uninitialized" values in and out of functions. In constant-flow testing, "uninitialized" values are actually secrets that must be manipulated with a data-independent flow, and it's perfectly fine to pass these in and out of functions. Fix Mbed-TLS#9921 Signed-off-by: Gilles Peskine <[email protected]>
New CMake build types CFMemSan, CFMemSanDbg to take care of differing compiler command lines with Clang <15 and Clang >=16. Signed-off-by: Gilles Peskine <[email protected]>
Support Clang >=16 in component_test_memsan_constant_flow and component_test_memsan_constant_flow_psa. Modern versions of Clang require an extra compiler option to avoid false positives. Fixes Mbed-TLS#9921. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
needs-backports
Add some basic checks of constant flow sanitizers. In particular, detect the specific way in which Clang 16 broke our constant-flow testing (by default, "uninitialized" values may not be passed to or returned from functions). Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
msancf-clang16-3.6
branch
from
afe5ac6 to
a95097f
Compare
gilles-peskine-arm
added
needs-review
| @@ -263,6 +263,14 @@ if(CMAKE_COMPILER_IS_CLANG) | |||
| set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -O3") | |||
| set(CMAKE_C_FLAGS_TSANDBG "-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls") | |||
| set(CMAKE_C_FLAGS_CHECK "-Os") | |||
|
|
|||
| set(CMAKE_C_FLAGS_CFMEMSAN "${CMAKE_C_FLAGS_MEMSAN} -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN") | |||
There was a problem hiding this comment.
Rather than add new build types for constant-flow msan, the right thing is probably to have CMake detect whether MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN is defined, and if so (and if the compiler needs it) add the extra -f flag.
(Maybe we should even enable MSan automatically based on MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN?)
The way to do that is with CheckCSourceCompiles:
include(CheckCSourceCompiles)
set(CMAKE_REQUIRED_INCLUDES …) # TODO: set this and create a scope for it
CheckCSourceCompiles("
#include <mbedtls/build_info.h>
#if !defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
#error MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN not defined
#endif
"
MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
if(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
…
endif()
There was a problem hiding this comment.
Actually Bence notes that CheckSymbolExists also works for macros.
include(CheckSymbolExists)
set(CMAKE_REQUIRED_INCLUDES …) # TODO: set this and create a scope for it
check_symbol_exists(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN
"mbedtls/build_info.h"
MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
There was a problem hiding this comment.
But that's not quite right because AFAICT it doesn't honor CFLAGS, so it could get the wrong configuration if CFLAGS includes e.g. -DMBEDTLS_CONFIG_FILE.
There was a problem hiding this comment.
Rather than add new build types for constant-flow msan, the right thing is probably to have CMake detect whether
MBEDTLS_TEST_CONSTANT_FLOW_MEMSANis defined, and if so (and if the compiler needs it) add the extra-fflag.
Is it really the right thing? I quite liked having separate build types, what's the problem with that?
There was a problem hiding this comment.
(Also, more down-to-earth, this is a relatively low priority thing that we're trying to address now while it's hot. We have a simple solution that works with separate build types. Not sure we should be spending more time working on another solution that might end up opening a can of worms.)
There was a problem hiding this comment.
I'm thinking of applications of the cmake training we just took. But I don't intend to change the approach here in the short term, it's working and I have higher priorities.
CMakeLists.txt
Outdated
| @@ -263,6 +263,14 @@ if(CMAKE_COMPILER_IS_CLANG) | |||
| set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -O3") | |||
| set(CMAKE_C_FLAGS_TSANDBG "-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls") | |||
| set(CMAKE_C_FLAGS_CHECK "-Os") | |||
|
|
|||
| set(CMAKE_C_FLAGS_CFMEMSAN "${CMAKE_C_FLAGS_MEMSAN} -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN") | |||
| set(CMAKE_C_FLAGS_CFMEMSANDBG "${CMAKE_C_FLAGS_CFMEMSANDBG} -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN") | |||
There was a problem hiding this comment.
${CMAKE_C_FLAGS_CFMEMSANDBG} should probably be ${CMAKE_C_FLAGS_MEMSANDBG} here.
| @@ -263,6 +263,14 @@ if(CMAKE_COMPILER_IS_CLANG) | |||
| set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -O3") | |||
| set(CMAKE_C_FLAGS_TSANDBG "-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls") | |||
| set(CMAKE_C_FLAGS_CHECK "-Os") | |||
|
|
|||
| set(CMAKE_C_FLAGS_CFMEMSAN "${CMAKE_C_FLAGS_MEMSAN} -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN") | |||
There was a problem hiding this comment.
Rather than add new build types for constant-flow msan, the right thing is probably to have CMake detect whether
MBEDTLS_TEST_CONSTANT_FLOW_MEMSANis defined, and if so (and if the compiler needs it) add the extra-fflag.
Is it really the right thing? I quite liked having separate build types, what's the problem with that?
Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
changed the title
waleed-elmelegy-arm
added
approved
Fix false positives of constant-flow testing when using
MBEDTLS_TEST_CONSTANT_FLOW_MEMSANon Clang 16 and above. Fixes #9921.This isn't needed for the CI (in the short to medium term) because still we do this testing on an old platform with an old Clang, but is needed on typical developer machines nowadays.
PR checklist