Make zap() more reliable

The gcc assembly version of zap() could still be optimized out under
gcc 5.1 or later, and the krb5int_zap() function could be optimized
out with link-time optimization.  Based on work by Zhaomo Yang and
Brian Johannesmeyer, use the C11 memset_s() when available, then fall
back to a memory barrier with gcc or clang, and finally fall back to
using krb5int_zap().  Modify krb5int_zap() to use a volatile pointer
in case link-time optimization is used.

(cherry picked from commit c163275f899b201dc2807b3ff2949d5e2ee7d838)

ticket: 8514
version_fixed: 1.15

3 files changed, 29 insertions, 21 deletions

diff --git a/src/aclocal.m4 b/src/aclocal.m4
index bd2eb48..9c46da4 100644
--- a/src/aclocal.m4
+++ b/src/aclocal.m4
@@ -53,6 +53,8 @@ AC_SUBST(EXTRA_FILES)
 dnl Consider using AC_USE_SYSTEM_EXTENSIONS when we require autoconf
 dnl 2.59c or later, but be sure to test on Solaris first.
 AC_DEFINE([_GNU_SOURCE], 1, [Define to enable extensions in glibc])
+AC_DEFINE([__STDC_WANT_LIB_EXT1__], 1, [Define to enable C11 extensions])
+
 WITH_CC dnl
 AC_REQUIRE_CPP
 if test -z "$LD" ; then LD=$CC; fi
diff --git a/src/include/k5-int.h b/src/include/k5-int.h
index 3cc32c3..6499173 100644
--- a/src/include/k5-int.h
+++ b/src/include/k5-int.h
@@ -652,30 +652,33 @@ k5_sha256(const krb5_data *in, uint8_t out[K5_SHA256_HASHLEN]);
  */
 #ifdef _WIN32
 # define zap(ptr, len) SecureZeroMemory(ptr, len)
-#elif defined(__GNUC__)
+#elif defined(__STDC_LIB_EXT1__)
+/*
+ * Use memset_s() which cannot be optimized out.  Avoid memset_s(NULL, 0, 0, 0)
+ * which would cause a runtime constraint violation.
+ */
 static inline void zap(void *ptr, size_t len)
 {
-    memset(ptr, 0, len);
-    /*
-     * Some versions of gcc have gotten clever enough to eliminate a
-     * memset call right before the block in question is released.
-     * This (empty) asm requires it to assume that we're doing
-     * something interesting with the stored (zero) value, so the
-     * memset can't be eliminated.
-     *
-     * An optimizer that looks at assembly or object code may not be
-     * fooled, and may still cause the memset to go away.  Address
-     * that problem if and when we encounter it.
-     *
-     * This also may not be enough if free() does something
-     * interesting like purge memory locations from a write-back cache
-     * that hasn't written back the zero bytes yet.  A memory barrier
-     * instruction would help in that case.
-     */
-    asm volatile ("" : : "g" (ptr), "g" (len));
+    if (len > 0)
+        memset_s(ptr, len, 0, len);
+}
+#elif defined(__GNUC__) || defined(__clang__)
+/*
+ * Use an asm statement which declares a memory clobber to force the memset to
+ * be carried out.  Avoid memset(NULL, 0, 0) which has undefined behavior.
+ */
+static inline void zap(void *ptr, size_t len)
+{
+    if (len > 0)
+        memset(ptr, 0, len);
+    __asm__ __volatile__("" : : "r" (ptr) : "memory");
 }
 #else
-/* Use a function from libkrb5support to defeat inlining. */
+/*
+ * Use a function from libkrb5support to defeat inlining unless link-time
+ * optimization is used.  The function uses a volatile pointer, which prevents
+ * current compilers from optimizing out the memset.
+ */
 # define zap(ptr, len) krb5int_zap(ptr, len)
 #endif
 
diff --git a/src/util/support/zap.c b/src/util/support/zap.c
index 48512a9..ed31630 100644
--- a/src/util/support/zap.c
+++ b/src/util/support/zap.c
@@ -34,5 +34,8 @@
 
 void krb5int_zap(void *ptr, size_t len)
 {
-    memset(ptr, 0, len);
+    volatile char *p = ptr;
+
+    while (len--)
+        *p++ = '\0';
 }