From cac30a436cab3641bba3b774d3d3ddbc426e7908 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 5 Jun 2019 22:43:28 +0200 Subject: [PATCH 1/2] Clean up logic in memory_cleanse() for MSVC Commit fbf327b13868861c2877c5754caf5a9816f2603c ("Minimal code changes to allow msvc compilation.") was indeed minimal in terms of lines touched. But as a result of that minimalism it changed the logic in memory_cleanse() to first call std::memset() and then additionally the MSVC-specific SecureZeroMemory() function, and it also moved a comment to the wrong location. This commit removes the superfluous call to std::memset() on MSVC and ensures that the comment is in the right position again. --- src/support/cleanse.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index 17a4a4c2b21..f895e96568c 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -30,14 +30,14 @@ */ void memory_cleanse(void *ptr, size_t len) { +#if defined(_MSC_VER) + SecureZeroMemory(ptr, len); +#else std::memset(ptr, 0, len); /* As best as we can tell, this is sufficient to break any optimisations that might try to eliminate "superfluous" memsets. If there's an easy way to detect memset_s, it would be better to use that. */ -#if defined(_MSC_VER) - SecureZeroMemory(ptr, len); -#else __asm__ __volatile__("" : : "r"(ptr) : "memory"); #endif } From f53a70ce95231d34bb14cd6c58503927e8d7ff59 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 5 Jun 2019 22:44:04 +0200 Subject: [PATCH 2/2] Improve documentation of memory_cleanse() So far, the documentation of memory_cleanse() is a verbatim copy of the commit message in BoringSSL, where this code was originally written. However, our code evolved since then, and the commit message is not particularly helpful in the code but is rather of historical interested in BoringSSL only. This commit improves improves the comments around memory_cleanse() and gives a better rationale for the method that we use. This commit touches only comments. --- src/support/cleanse.cpp | 32 ++++++++++++-------------------- src/support/cleanse.h | 3 ++- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/support/cleanse.cpp b/src/support/cleanse.cpp index f895e96568c..ecb00510f78 100644 --- a/src/support/cleanse.cpp +++ b/src/support/cleanse.cpp @@ -11,33 +11,25 @@ #include // For SecureZeroMemory. #endif -/* Compilers have a bad habit of removing "superfluous" memset calls that - * are trying to zero memory. For example, when memset()ing a buffer and - * then free()ing it, the compiler might decide that the memset is - * unobservable and thus can be removed. - * - * Previously we used OpenSSL which tried to stop this by a) implementing - * memset in assembly on x86 and b) putting the function in its own file - * for other platforms. - * - * This change removes those tricks in favour of using asm directives to - * scare the compiler away. As best as our compiler folks can tell, this is - * sufficient and will continue to be so. - * - * Adam Langley - * Commit: ad1907fe73334d6c696c8539646c21b11178f20f - * BoringSSL (LICENSE: ISC) - */ void memory_cleanse(void *ptr, size_t len) { #if defined(_MSC_VER) + /* SecureZeroMemory is guaranteed not to be optimized out by MSVC. */ SecureZeroMemory(ptr, len); #else std::memset(ptr, 0, len); - /* As best as we can tell, this is sufficient to break any optimisations that - might try to eliminate "superfluous" memsets. If there's an easy way to - detect memset_s, it would be better to use that. */ + /* Memory barrier that scares the compiler away from optimizing out the memset. + * + * Quoting Adam Langley in commit ad1907fe73334d6c696c8539646c21b11178f20f + * in BoringSSL (ISC License): + * As best as we can tell, this is sufficient to break any optimisations that + * might try to eliminate "superfluous" memsets. + * This method is used in memzero_explicit() the Linux kernel, too. Its advantage is that it + * is pretty efficient because the compiler can still implement the memset() efficiently, + * just not remove it entirely. See "Dead Store Elimination (Still) Considered Harmful" by + * Yang et al. (USENIX Security 2017) for more background. + */ __asm__ __volatile__("" : : "r"(ptr) : "memory"); #endif } diff --git a/src/support/cleanse.h b/src/support/cleanse.h index 5298214e447..b03520315d3 100644 --- a/src/support/cleanse.h +++ b/src/support/cleanse.h @@ -8,7 +8,8 @@ #include -// Attempt to overwrite data in the specified memory span. +/** Secure overwrite a buffer (possibly containing secret data) with zero-bytes. The write + * operation will not be optimized out by the compiler. */ void memory_cleanse(void *ptr, size_t len); #endif // BITCOIN_SUPPORT_CLEANSE_H