| Message ID | 20200616083435.223038-1-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,RFC] security: allow using Clang's zero initialization for stack variables | expand |
> 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>
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 >
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
> 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?
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.
> > +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.
> 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)
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.
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
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(-)