Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 16 Jun 2020 02:08:08 -0700
From:   Kees Cook <keescook@chromium.org>
To:     glider@google.com
Cc:     yamada.masahiro@socionext.com, jmorris@namei.org, maze@google.com,
        ndesaulniers@google.com, gregkh@linuxfoundation.org,
        linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] [RFC] security: allow using Clang's zero
 initialization for stack variables
Message-ID: <202006160207.E9C6FDDB7@keescook>
References: <20200616083435.223038-1-glider@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200616083435.223038-1-glider@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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