Re: [PATCH] [RFC] security: allow using Clang's zero initialization for stack variables

On Sun, Jun 14, 2020 at 07:16:33PM +0200, Alexander Potapenko wrote:

On Sun, Jun 14, 2020 at 7:04 PM Kees Cook [off-list ref] wrote:

quoted

On Sun, Jun 14, 2020 at 04:45:34PM +0200, glider@google.com wrote:

quoted

In addition to -ftrivial-auto-var-init=pattern (used by
CONFIG_INIT_STACK_ALL now) Clang also supports zero initialization for
locals enabled by -ftrivial-auto-var-init=zero.
The future of this flag is still being debated, see
https://bugs.llvm.org/show_bug.cgi?id=45497
Right now it is guarded by another flag,
-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang,
which means it may not be supported by future Clang releases.
Another possible resolution is that -ftrivial-auto-var-init=zero will
persist (as certain users have already started depending on it), but the
name of the guard flag will change.

In the meantime, zero initialization has proven itself as a good
production mitigation measure against uninitialized locals. Unlike
pattern initialization, which has a higher chance of triggering existing
bugs, zero initialization provides safe defaults for strings, pointers,
indexes, and sizes. On the other hand, pattern initialization remains
safer for return values.
Performance-wise, the difference between pattern and zero initialization
is usually negligible, although the generated code for zero
initialization is more compact.

The proposed config, CONFIG_USE_CLANG_ZERO_INITIALIZATION, makes
CONFIG_INIT_STACK_ALL use zero initialization if the corresponding flags
are supported by Clang.

Cc: Kees Cook <redacted>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 Makefile                   | 15 ++++++++++++++-
 security/Kconfig.hardening | 16 ++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index fd31992bf918..2860bad7e39a 100644
--- a/Makefile
+++ b/Makefile

@@ -802,9 +802,22 @@ KBUILD_CFLAGS    += -fomit-frame-pointer
 endif
 endif

-# Initialize all stack variables with a pattern, if desired.
+# Initialize all stack variables, if desired.
 ifdef CONFIG_INIT_STACK_ALL
+
+# Use pattern initialization by default.
+ifndef CONFIG_USE_CLANG_ZERO_INITIALIZATION
 KBUILD_CFLAGS        += -ftrivial-auto-var-init=pattern
+else
+
+ifdef CONFIG_CC_HAS_AUTO_VAR_ZERO_INIT
+# Future support for zero initialization is still being debated, see
+# https://bugs.llvm.org/show_bug.cgi?id=45497. These flags are subject to being
+# renamed or dropped.
+KBUILD_CFLAGS        += -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
+endif
+
+endif
 endif

I'd prefer this be split instead of built as a nested if (i.e. entirely
control section via the Kconfig -- see below).

quoted

 DEBUG_CFLAGS := $(call cc-option, -fno-var-tracking-assignments)

diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index af4c979b38ee..299d27c6d78c 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening

@@ -22,6 +22,9 @@ menu "Memory initialization"
 config CC_HAS_AUTO_VAR_INIT
      def_bool $(cc-option,-ftrivial-auto-var-init=pattern)

+config CC_HAS_AUTO_VAR_ZERO_INIT
+     def_bool $(cc-option,-ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang)
+

I'd like to be more specific here. Let's rename CC_HAS_AUTO_VAR_INIT to
CC_HAS_AUTO_VAR_INIT_PATTERN, and change the other to
CC_HAS_AUTO_VAR_INIT_ZERO (they then both match the word order of the
option, and the thing that changes is the last word).

quoted

 choice
      prompt "Initialize kernel stack variables at function entry"
      default GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if COMPILE_TEST && GCC_PLUGINS

@@ -100,6 +103,19 @@ choice

 endchoice

+config USE_CLANG_ZERO_INITIALIZATION
+     bool "Use Clang's zero initialization for local variables"
+     depends on CC_HAS_AUTO_VAR_ZERO_INIT
+     depends on INIT_STACK_ALL
+     help
+       If set, uses zeros instead of 0xAA to initialize local variables in
+       INIT_STACK_ALL. Zeroing the stack provides safer defaults for strings,
+       pointers, indexes, and sizes. The downsides are less-safe defaults for
+       return values, and exposing fewer bugs where the underlying code
+       relies on zero initialization.
+       The corresponding flag isn't officially supported by Clang and may
+       sooner or later go away or get renamed.
+

Similarly, I'd like to rename INIT_STACK_ALL to INIT_STACK_ALL_PATTERN
and then add INIT_STACK_ALL_ZERO.

What are the policies regarding keeping the existing config flags?
Don't we need to preserve INIT_STACK_ALL?

There isn't a strong policy -- it's mostly a question of "how painful
will it be for end-users to change this?" Given the users of this are
the Clang folks, and it's a very new option, I would prefer we just
change it and carry on.

That said, I'd also like at some point do:

 choice
 	prompt "Initialize kernel stack variables at function entry"
 	default GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if COMPILE_TEST && GCC_PLUGINS
-	default INIT_STACK_ALL_ZERO if COMPILE_TEST && CC_HAS_AUTO_VAR_INIT_ZERO
+	default INIT_STACK_ALL_ZERO if CC_HAS_AUTO_VAR_INIT
 	default INIT_STACK_NONE
 	help
 	  This option enables initialization of stack variables at

But not at this time...

-- 
Kees Cook