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

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

[glider]

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.

This patch renames CONFIG_INIT_STACK_ALL to
CONFIG_INIT_STACK_ALL_PATTERN and introduces another config option,
CONFIG_INIT_STACK_ALL_ZERO, that enables zero initialization for locals
if the corresponding flags are supported by Clang.

Cc: Kees Cook <keescook@chromium.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>

--
v2:
 - as suggested by Kees Cook, make CONFIG_INIT_STACK_ALL_PATTERN and
   CONFIG_INIT_STACK_ALL_ZERO separate options.
---
 Makefile                   | 12 ++++++++++--
 init/main.c                |  6 ++++--
 security/Kconfig.hardening | 29 +++++++++++++++++++++++++----
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index fd31992bf918..fa739995ee12 100644
--- a/Makefile
+++ b/Makefile
@@ -802,11 +802,19 @@ KBUILD_CFLAGS	+= -fomit-frame-pointer
 endif
 endif
 
-# Initialize all stack variables with a pattern, if desired.
-ifdef CONFIG_INIT_STACK_ALL
+# Initialize all stack variables with a 0xAA pattern.
+ifdef CONFIG_INIT_STACK_ALL_PATTERN
 KBUILD_CFLAGS	+= -ftrivial-auto-var-init=pattern
 endif
 
+# Initialize all stack variables with a zero pattern.
+ifdef CONFIG_INIT_STACK_ALL_ZERO
+# 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
+
 DEBUG_CFLAGS	:= $(call cc-option, -fno-var-tracking-assignments)
 
 ifdef CONFIG_DEBUG_INFO
diff --git a/init/main.c b/init/main.c
index 0ead83e86b5a..ee08cef4aa1a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -779,8 +779,10 @@ static void __init report_meminit(void)
 {
 	const char *stack;
 
-	if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
-		stack = "all";
+	if (IS_ENABLED(CONFIG_INIT_STACK_ALL_PATTERN))
+		stack = "all (pattern)";
+	else if (IS_ENABLED(CONFIG_INIT_STACK_ALL_ZERO))
+		stack = "all (zero)";
 	else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
 		stack = "byref_all";
 	else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index af4c979b38ee..7b705611ccaa 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -19,13 +19,16 @@ config GCC_PLUGIN_STRUCTLEAK
 
 menu "Memory initialization"
 
-config CC_HAS_AUTO_VAR_INIT
+config CC_HAS_AUTO_VAR_INIT_PATTERN
 	def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
 
+config CC_HAS_AUTO_VAR_INIT_ZERO
+	def_bool $(cc-option,-ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang)
+
 choice
 	prompt "Initialize kernel stack variables at function entry"
 	default GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if COMPILE_TEST && GCC_PLUGINS
-	default INIT_STACK_ALL if COMPILE_TEST && CC_HAS_AUTO_VAR_INIT
+	default INIT_STACK_ALL_PATTERN if COMPILE_TEST && CC_HAS_AUTO_VAR_INIT_PATTERN
 	default INIT_STACK_NONE
 	help
 	  This option enables initialization of stack variables at
@@ -88,15 +91,33 @@ choice
 		  of uninitialized stack variable exploits and information
 		  exposures.
 
-	config INIT_STACK_ALL
+	config INIT_STACK_ALL_PATTERN
 		bool "0xAA-init everything on the stack (strongest)"
-		depends on CC_HAS_AUTO_VAR_INIT
+		depends on CC_HAS_AUTO_VAR_INIT_PATTERN
 		help
 		  Initializes everything on the stack with a 0xAA
 		  pattern. This is intended to eliminate all classes
 		  of uninitialized stack variable exploits and information
 		  exposures, even variables that were warned to have been
 		  left uninitialized.
+		  Pattern initialization is known to provoke many existing bugs
+		  related to uninitialized locals, e.g. pointers receive
+		  non-NULL values, buffer sizes and indices are very big.
+
+	config INIT_STACK_ALL_ZERO
+		bool "zero-init everything on the stack (strongest and safest)"
+		depends on CC_HAS_AUTO_VAR_INIT_ZERO
+		help
+		  Initializes everything on the stack with a zero
+		  pattern. This is intended to eliminate all classes
+		  of uninitialized stack variable exploits and information
+		  exposures, even variables that were warned to have been
+		  left uninitialized.
+		  Zero initialization provides safe defaults for strings,
+		  pointers, indices and sizes, and is therefore more suitable as
+		  a security mitigation measure.
+		  The corresponding flag isn't officially supported by Clang and
+		  may sooner or later go away or get renamed.
 
 endchoice
 
-- 
2.27.0.290.gba653c62da-goog

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

[Maciej Żenczykowski]

> 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.
>
> This patch renames CONFIG_INIT_STACK_ALL to
> CONFIG_INIT_STACK_ALL_PATTERN and introduces another config option,
> CONFIG_INIT_STACK_ALL_ZERO, that enables zero initialization for locals
> if the corresponding flags are supported by Clang.

I'm a great fan of zero initialization as opposed to pattern.
I don't understand why clang is refusing to make this a supported option.

Anyway:

Reviewed-by: Maciej Żenczykowski <maze@google.com>

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

[Kees Cook]

On Tue, Jun 16, 2020 at 10:34:35AM +0200, glider@google.com wrote:
> 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.
> 
> This patch renames CONFIG_INIT_STACK_ALL to
> CONFIG_INIT_STACK_ALL_PATTERN and introduces another config option,
> CONFIG_INIT_STACK_ALL_ZERO, that enables zero initialization for locals
> if the corresponding flags are supported by Clang.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Thanks! I've applied this to my for-next/kspp tree (with a few small
tweaks).

> --

^^ note, this separator should be "---" for diff tools to do the right
thing, etc.

> v2:
>  - as suggested by Kees Cook, make CONFIG_INIT_STACK_ALL_PATTERN and
>    CONFIG_INIT_STACK_ALL_ZERO separate options.
> ---
>  Makefile                   | 12 ++++++++++--
>  init/main.c                |  6 ++++--
>  security/Kconfig.hardening | 29 +++++++++++++++++++++++++----
>  3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index fd31992bf918..fa739995ee12 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -802,11 +802,19 @@ KBUILD_CFLAGS	+= -fomit-frame-pointer
>  endif
>  endif
>  
> -# Initialize all stack variables with a pattern, if desired.
> -ifdef CONFIG_INIT_STACK_ALL
> +# Initialize all stack variables with a 0xAA pattern.
> +ifdef CONFIG_INIT_STACK_ALL_PATTERN
>  KBUILD_CFLAGS	+= -ftrivial-auto-var-init=pattern
>  endif
>  
> +# Initialize all stack variables with a zero pattern.
> +ifdef CONFIG_INIT_STACK_ALL_ZERO
> +# 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
> +
>  DEBUG_CFLAGS	:= $(call cc-option, -fno-var-tracking-assignments)
>  
>  ifdef CONFIG_DEBUG_INFO
> diff --git a/init/main.c b/init/main.c
> index 0ead83e86b5a..ee08cef4aa1a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -779,8 +779,10 @@ static void __init report_meminit(void)
>  {
>  	const char *stack;
>  
> -	if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
> -		stack = "all";
> +	if (IS_ENABLED(CONFIG_INIT_STACK_ALL_PATTERN))
> +		stack = "all (pattern)";
> +	else if (IS_ENABLED(CONFIG_INIT_STACK_ALL_ZERO))
> +		stack = "all (zero)";
>  	else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
>  		stack = "byref_all";
>  	else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index af4c979b38ee..7b705611ccaa 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -19,13 +19,16 @@ config GCC_PLUGIN_STRUCTLEAK
>  
>  menu "Memory initialization"
>  
> -config CC_HAS_AUTO_VAR_INIT
> +config CC_HAS_AUTO_VAR_INIT_PATTERN
>  	def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
>  
> +config CC_HAS_AUTO_VAR_INIT_ZERO
> +	def_bool $(cc-option,-ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang)
> +
>  choice
>  	prompt "Initialize kernel stack variables at function entry"
>  	default GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if COMPILE_TEST && GCC_PLUGINS
> -	default INIT_STACK_ALL if COMPILE_TEST && CC_HAS_AUTO_VAR_INIT
> +	default INIT_STACK_ALL_PATTERN if COMPILE_TEST && CC_HAS_AUTO_VAR_INIT_PATTERN
>  	default INIT_STACK_NONE
>  	help
>  	  This option enables initialization of stack variables at
> @@ -88,15 +91,33 @@ choice
>  		  of uninitialized stack variable exploits and information
>  		  exposures.
>  
> -	config INIT_STACK_ALL
> +	config INIT_STACK_ALL_PATTERN
>  		bool "0xAA-init everything on the stack (strongest)"
> -		depends on CC_HAS_AUTO_VAR_INIT
> +		depends on CC_HAS_AUTO_VAR_INIT_PATTERN
>  		help
>  		  Initializes everything on the stack with a 0xAA
>  		  pattern. This is intended to eliminate all classes
>  		  of uninitialized stack variable exploits and information
>  		  exposures, even variables that were warned to have been
>  		  left uninitialized.
> +		  Pattern initialization is known to provoke many existing bugs
> +		  related to uninitialized locals, e.g. pointers receive
> +		  non-NULL values, buffer sizes and indices are very big.
> +
> +	config INIT_STACK_ALL_ZERO
> +		bool "zero-init everything on the stack (strongest and safest)"
> +		depends on CC_HAS_AUTO_VAR_INIT_ZERO
> +		help
> +		  Initializes everything on the stack with a zero
> +		  pattern. This is intended to eliminate all classes
> +		  of uninitialized stack variable exploits and information
> +		  exposures, even variables that were warned to have been
> +		  left uninitialized.
> +		  Zero initialization provides safe defaults for strings,
> +		  pointers, indices and sizes, and is therefore more suitable as
> +		  a security mitigation measure.
> +		  The corresponding flag isn't officially supported by Clang and
> +		  may sooner or later go away or get renamed.
>  
>  endchoice
>  
> -- 
> 2.27.0.290.gba653c62da-goog
> 

-- 
Kees Cook

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

[Greg KH]

On Tue, Jun 16, 2020 at 10:34:35AM +0200, glider@google.com wrote:
> 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.
> 
> This patch renames CONFIG_INIT_STACK_ALL to
> CONFIG_INIT_STACK_ALL_PATTERN and introduces another config option,
> CONFIG_INIT_STACK_ALL_ZERO, that enables zero initialization for locals
> if the corresponding flags are supported by Clang.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Nit, your From: line of your email does not match your signed-off-by:
line :(

In the future, you should fix that up so that maintainers don't have to
do it for you...

> +# Initialize all stack variables with a zero pattern.
> +ifdef CONFIG_INIT_STACK_ALL_ZERO
> +# 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

Gotta love the name...

Anyway, if this is enabled, and clang changes the flag or drops it, does
the build suddenly break?

And does gcc have something like this as well, or does that have to come
in a compiler plugin?

thanks,

greg k-h

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

[Maciej Żenczykowski]

> Nit, your From: line of your email does not match your signed-off-by:
> line :(

This is *probably* a matter of configuring git:
git config --global user.name "Alexander Potapenko"
git config --global user.email "glider@google.com"
git config --global sendemail.from "Alexander Potapenko <glider@google.com>"

> Gotta love the name...

I've just read through a long discussion thread on clang dev (got
there from this cl's llvm link)...
There's a lot of interesting info in there.  But ultimately it seems
to point out tons of projects already use this or want to use it.
And there's security and stability benefits for production builds,
while pattern mode can be used for debug builds.

> Anyway, if this is enabled, and clang changes the flag or drops it, does
> the build suddenly break?

(my understanding of the patch is that the option does compiler
testing before it becomes available...
in at least some of our build systems built around kbuild the option
going away would then cause a build error due to its lack in the final
.config)

> And does gcc have something like this as well, or does that have to come
> in a compiler plugin?

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

[Alexander Potapenko]

On Tue, Jun 16, 2020 at 12:19 PM Maciej Żenczykowski <maze@google.com> wrote:
>
> > Nit, your From: line of your email does not match your signed-off-by:
> > line :(

That's hard to notice while being on the sending end, thanks for
pointing that out!
I'll look into this.

> This is *probably* a matter of configuring git:
> git config --global user.name "Alexander Potapenko"
> git config --global user.email "glider@google.com"
> git config --global sendemail.from "Alexander Potapenko <glider@google.com>"

I do have all these options set, although I don't have them quoted,
not sure if it's necessary.
Moreover, when I do `git send-email --dry-run` the From: line looks correct.

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

[Alexander Potapenko]

> > +KBUILD_CFLAGS        += -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> > +endif
>
> Gotta love the name...

This is basically the reason why we've been hesitating to add it to
the kernel from the very beginning.

> Anyway, if this is enabled, and clang changes the flag or drops it, does
> the build suddenly break?

My original intention (see v1 of this patch) was to make
zero-initialization a secondary option of INIT_STACK_ALL, so that
nothing changes for the existing users.
But I agree with Kees that these options should be made distinct, as
people may want to use them for different purposes (think debug vs.
release builds).

We could make INIT_STACK_ALL_ZERO fall back to INIT_STACK_ALL_PATTERN
if the compiler flag goes away - does this make sense?

> And does gcc have something like this as well, or does that have to come
> in a compiler plugin?

Kees mentioned someone's plans to implement that in GCC, but I don't
think they have done it already.

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

[Maciej Żenczykowski]

> We could make INIT_STACK_ALL_ZERO fall back to INIT_STACK_ALL_PATTERN
> if the compiler flag goes away - does this make sense?

No, I'm pretty sure failing to build, or at least not setting anything
is better.
AFAIK pattern actually introduces new bugs that aren't visible at all
with neither of these flags set.
(because in practice the default no flag behaviour seems to zero some
stuff [probably padding] that it doesn't with pattern)

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

[Kees Cook]

On Tue, Jun 16, 2020 at 02:15:52PM +0200, Alexander Potapenko wrote:
> > > +KBUILD_CFLAGS        += -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> > > +endif
> >
> > Gotta love the name...
> 
> This is basically the reason why we've been hesitating to add it to
> the kernel from the very beginning.
> 
> > Anyway, if this is enabled, and clang changes the flag or drops it, does
> > the build suddenly break?
> 
> My original intention (see v1 of this patch) was to make
> zero-initialization a secondary option of INIT_STACK_ALL, so that
> nothing changes for the existing users.
> But I agree with Kees that these options should be made distinct, as
> people may want to use them for different purposes (think debug vs.
> release builds).

Yeah, and if the flag changes again, we can adapt. But at this point,
it's getting used downstream, so we need to land the config in the
kernel.

> We could make INIT_STACK_ALL_ZERO fall back to INIT_STACK_ALL_PATTERN
> if the compiler flag goes away - does this make sense?

I don't like this idea -- I'm very hesitant to make security options do
something different than what they document. It means the end user can't
reason about how their kernel is built when looking at their CONFIGs.

> > And does gcc have something like this as well, or does that have to come
> > in a compiler plugin?
> 
> Kees mentioned someone's plans to implement that in GCC, but I don't
> think they have done it already.

I've had some GCC folks reach about about these features, but I haven't
seen any public discussion yet.

-- 
Kees Cook