* [PATCH 0/1] Zero initialise kernel stack variables @ 2018-02-27 11:15 P J P 2018-02-27 11:15 ` [PATCH 1/1] Add an option to build kernel with -finit-local-vars P J P 2018-02-27 19:28 ` [PATCH 0/1] Zero initialise kernel stack variables Kees Cook 0 siblings, 2 replies; 14+ messages in thread From: P J P @ 2018-02-27 11:15 UTC (permalink / raw) To: kernel-hardening; +Cc: Florian Weimer, P J P From: P J P <pjp@fedoraproject.org> Hello, Please see: -> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html This experimental patch by Florian Weimer(CC'd) adds an option '-finit-local-vars' to gcc(1) compiler. When a program(or kernel) is built using this option, its automatic(local) variables are initialised with zero(0). This could significantly reduce the kernel information leakage issues. A dnf(8) repository of the latest gcc-7.3.1 package built with the above patch and kernel-4.15.5 package built using '-finit-local-vars' option on Fedora-27 is available below -> https://pjp.fedorapeople.org/init-vars/ This same kernel is running on my F27 test machine as I write this. There is no slowness or notice-able performance impact as such. The patch here adds a kbuild menu option to enable/disable '-finit-local-vars' compiler flag while building the Linux kernel. I'd appreciate your review and/or inputs to test this option further. Thank you. -- Prasad J Pandit / Red Hat Product Security Team (1): Add an option to build kernel with -finit-local-vars Makefile | 4 ++++ lib/Kconfig.debug | 8 ++++++++ 2 files changed, 12 insertions(+) -- 2.14.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/1] Add an option to build kernel with -finit-local-vars 2018-02-27 11:15 [PATCH 0/1] Zero initialise kernel stack variables P J P @ 2018-02-27 11:15 ` P J P 2018-02-27 19:22 ` Laura Abbott 2018-02-27 19:28 ` [PATCH 0/1] Zero initialise kernel stack variables Kees Cook 1 sibling, 1 reply; 14+ messages in thread From: P J P @ 2018-02-27 11:15 UTC (permalink / raw) To: kernel-hardening; +Cc: Florian Weimer, P J P From: P J P <pjp@fedoraproject.org> Add a configuration option to build kernel with -finit-local-vars compiler option.[*] It'll zero initialize the automatic kernel function variables, thus helping to reduce kernel information leakage issues. [*] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html Signed-off-by: P J P <pjp@fedoraproject.org> --- Makefile | 4 ++++ lib/Kconfig.debug | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/Makefile b/Makefile index 659a7780aeb3..7b25a77470ca 100644 --- a/Makefile +++ b/Makefile @@ -781,6 +781,10 @@ KBUILD_CFLAGS += $(call cc-option, -femit-struct-debug-baseonly) \ $(call cc-option,-fno-var-tracking) endif +ifdef CONFIG_FINIT_LOCAL_VARS +KBUILD_CFLAGS += $(call cc-option, -finit-local-vars) +endif + ifdef CONFIG_FUNCTION_TRACER ifndef CC_FLAGS_FTRACE CC_FLAGS_FTRACE := -pg diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 64155e310a9f..8da18d145c5b 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -206,6 +206,14 @@ config ENABLE_WARN_DEPRECATED Disable this to suppress the "warning: 'foo' is deprecated (declared at kernel/power/somefile.c:1234)" messages. +config FINIT_LOCAL_VARS + bool "Enable -finit-local-vars" + default n + help + Enable the -finit-local-vars compiler option during the kernel build. + It'll zero initialise the automatic kernel function variables, thus + helping to reduce kernel information leakage issues. + config ENABLE_MUST_CHECK bool "Enable __must_check logic" default y -- 2.14.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] Add an option to build kernel with -finit-local-vars 2018-02-27 11:15 ` [PATCH 1/1] Add an option to build kernel with -finit-local-vars P J P @ 2018-02-27 19:22 ` Laura Abbott 2018-02-27 19:33 ` Kees Cook 2018-02-28 5:49 ` P J P 0 siblings, 2 replies; 14+ messages in thread From: Laura Abbott @ 2018-02-27 19:22 UTC (permalink / raw) To: P J P, kernel-hardening; +Cc: Florian Weimer, P J P On 02/27/2018 03:15 AM, P J P wrote: > From: P J P <pjp@fedoraproject.org> > > Add a configuration option to build kernel with -finit-local-vars > compiler option.[*] It'll zero initialize the automatic kernel > function variables, thus helping to reduce kernel information > leakage issues. > I think this would make the existing structleak plugin (scripts/gcc-plugins/structleak_plugin.c) obsolete. This isn't a bad thing but we'd need to figure out a deprecation strategy. > [*] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html > > Signed-off-by: P J P <pjp@fedoraproject.org> > --- > Makefile | 4 ++++ > lib/Kconfig.debug | 8 ++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/Makefile b/Makefile > index 659a7780aeb3..7b25a77470ca 100644 > --- a/Makefile > +++ b/Makefile > @@ -781,6 +781,10 @@ KBUILD_CFLAGS += $(call cc-option, -femit-struct-debug-baseonly) \ > $(call cc-option,-fno-var-tracking) > endif > > +ifdef CONFIG_FINIT_LOCAL_VARS > +KBUILD_CFLAGS += $(call cc-option, -finit-local-vars) > +endif > + > ifdef CONFIG_FUNCTION_TRACER > ifndef CC_FLAGS_FTRACE > CC_FLAGS_FTRACE := -pg > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 64155e310a9f..8da18d145c5b 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -206,6 +206,14 @@ config ENABLE_WARN_DEPRECATED > Disable this to suppress the "warning: 'foo' is deprecated > (declared at kernel/power/somefile.c:1234)" messages. > > +config FINIT_LOCAL_VARS > + bool "Enable -finit-local-vars" > + default n > + help > + Enable the -finit-local-vars compiler option during the kernel build. > + It'll zero initialise the automatic kernel function variables, thus > + helping to reduce kernel information leakage issues. > + A few words about the expected runtime/kernel size impact would be helpful. Thanks, Laura > config ENABLE_MUST_CHECK > bool "Enable __must_check logic" > default y > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] Add an option to build kernel with -finit-local-vars 2018-02-27 19:22 ` Laura Abbott @ 2018-02-27 19:33 ` Kees Cook 2018-02-28 5:49 ` P J P 1 sibling, 0 replies; 14+ messages in thread From: Kees Cook @ 2018-02-27 19:33 UTC (permalink / raw) To: Laura Abbott; +Cc: P J P, Kernel Hardening, Florian Weimer, P J P On Tue, Feb 27, 2018 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote: > On 02/27/2018 03:15 AM, P J P wrote: >> Add a configuration option to build kernel with -finit-local-vars >> compiler option.[*] It'll zero initialize the automatic kernel >> function variables, thus helping to reduce kernel information >> leakage issues. > > I think this would make the existing structleak plugin > (scripts/gcc-plugins/structleak_plugin.c) obsolete. This isn't > a bad thing but we'd need to figure out a deprecation strategy. It would be nice to make it obsolete, but I don't think that'll happen right away. We still have issues with structure padding, passed-by-reference init, and possibly performance. I wouldn't want to rule anything out until we can have both more complete coverage and better benchmarks (e.g. this is wipe-before, not wipe-after, so there may be cache effects, etc). -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] Add an option to build kernel with -finit-local-vars 2018-02-27 19:22 ` Laura Abbott 2018-02-27 19:33 ` Kees Cook @ 2018-02-28 5:49 ` P J P 1 sibling, 0 replies; 14+ messages in thread From: P J P @ 2018-02-28 5:49 UTC (permalink / raw) To: Laura Abbott; +Cc: kernel-hardening, Florian Weimer, P J P, Kees Cook +-- On Tue, 27 Feb 2018, Laura Abbott wrote --+ | A few words about the expected runtime/kernel size impact would be helpful. Okay, will add. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 11:15 [PATCH 0/1] Zero initialise kernel stack variables P J P 2018-02-27 11:15 ` [PATCH 1/1] Add an option to build kernel with -finit-local-vars P J P @ 2018-02-27 19:28 ` Kees Cook 2018-02-27 23:26 ` Laura Abbott ` (2 more replies) 1 sibling, 3 replies; 14+ messages in thread From: Kees Cook @ 2018-02-27 19:28 UTC (permalink / raw) To: P J P; +Cc: Kernel Hardening, Florian Weimer, P J P On Tue, Feb 27, 2018 at 3:15 AM, P J P <ppandit@redhat.com> wrote: > Hello, Hi! > > Please see: > -> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html > > This experimental patch by Florian Weimer(CC'd) adds an option > '-finit-local-vars' to gcc(1) compiler. When a program(or kernel) > is built using this option, its automatic(local) variables are > initialised with zero(0). This could significantly reduce the kernel > information leakage issues. > > A dnf(8) repository of the latest gcc-7.3.1 package built with the above > patch and kernel-4.15.5 package built using '-finit-local-vars' option > on Fedora-27 is available below > > -> https://pjp.fedorapeople.org/init-vars/ > > This same kernel is running on my F27 test machine as I write this. > There is no slowness or notice-able performance impact as such. Unfortunately "noticeable" isn't going to be a viable metric. You'll need to do some real-world benchmarks (i.e. kernel builds, hackbench, etc), and compare the results. Even just initializing passed-by-reference variables (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had measurable performance impact. It would be nice to have four options/features available from the compiler, from least to most performance impact: - initialize padding to zero when static initializers are used (this would make foo = { .field = something }; identical to memset(&foo, 0, sizeof(foo)); foo.field = something for all structures, but now, any structures with padding _must_ use the latter to be safe, which is highly error-prone). - initialize all uninitialized variables that contain a structure marked with a special attribute (e.g. __attribute__((force_initialize)) ). - initialize all uninitialized variables that are passed by reference (see GCC_PLUGIN_STRUCTLEAK_BYREF_ALL). - initialize all uninitialized variables (-finit-local-vars seems to do this) > The patch here adds a kbuild menu option to enable/disable '-finit-local-vars' > compiler flag while building the Linux kernel. Since this is a single patch, I think it'd be better to fold this entire cover letter into patch 1. > I'd appreciate your review and/or inputs to test this option further. Thanks for sending this! -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 19:28 ` [PATCH 0/1] Zero initialise kernel stack variables Kees Cook @ 2018-02-27 23:26 ` Laura Abbott 2018-02-27 23:38 ` Kees Cook 2018-02-28 6:04 ` P J P 2018-03-02 19:52 ` Nick Kralevich 2 siblings, 1 reply; 14+ messages in thread From: Laura Abbott @ 2018-02-27 23:26 UTC (permalink / raw) To: Kees Cook, P J P; +Cc: Kernel Hardening, Florian Weimer, P J P On 02/27/2018 11:28 AM, Kees Cook wrote: > On Tue, Feb 27, 2018 at 3:15 AM, P J P <ppandit@redhat.com> wrote: >> Hello, > > Hi! > >> >> Please see: >> -> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html >> >> This experimental patch by Florian Weimer(CC'd) adds an option >> '-finit-local-vars' to gcc(1) compiler. When a program(or kernel) >> is built using this option, its automatic(local) variables are >> initialised with zero(0). This could significantly reduce the kernel >> information leakage issues. >> >> A dnf(8) repository of the latest gcc-7.3.1 package built with the above >> patch and kernel-4.15.5 package built using '-finit-local-vars' option >> on Fedora-27 is available below >> >> -> https://pjp.fedorapeople.org/init-vars/ >> >> This same kernel is running on my F27 test machine as I write this. >> There is no slowness or notice-able performance impact as such. > > Unfortunately "noticeable" isn't going to be a viable metric. You'll > need to do some real-world benchmarks (i.e. kernel builds, hackbench, > etc), and compare the results. Even just initializing > passed-by-reference variables (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had > measurable performance impact. > For comparison (-300 is official Fedora, -301 is from the repo): text data bss dec hex filename 16948437 6771094 1777872 25497403 1850f3b /lib/debug/lib/modules/4.15.5-300.fc27.x86_64/vmlinux 16970359 6776078 1777872 25524309 1857855 /lib/debug/lib/modules/4.15.5-301.fc27.x86_64/vmlinux Thanks, Laura ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 23:26 ` Laura Abbott @ 2018-02-27 23:38 ` Kees Cook 2018-02-28 6:22 ` P J P 2018-02-28 8:25 ` Florian Weimer 0 siblings, 2 replies; 14+ messages in thread From: Kees Cook @ 2018-02-27 23:38 UTC (permalink / raw) To: Laura Abbott; +Cc: P J P, Kernel Hardening, Florian Weimer, P J P On Tue, Feb 27, 2018 at 3:26 PM, Laura Abbott <labbott@redhat.com> wrote: > On 02/27/2018 11:28 AM, Kees Cook wrote: >> >> On Tue, Feb 27, 2018 at 3:15 AM, P J P <ppandit@redhat.com> wrote: >>> >>> Hello, >> >> >> Hi! >> >>> >>> Please see: >>> -> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html >>> >>> This experimental patch by Florian Weimer(CC'd) adds an option >>> '-finit-local-vars' to gcc(1) compiler. When a program(or kernel) >>> is built using this option, its automatic(local) variables are >>> initialised with zero(0). This could significantly reduce the kernel >>> information leakage issues. >>> >>> A dnf(8) repository of the latest gcc-7.3.1 package built with the above >>> patch and kernel-4.15.5 package built using '-finit-local-vars' option >>> on Fedora-27 is available below >>> >>> -> https://pjp.fedorapeople.org/init-vars/ >>> >>> This same kernel is running on my F27 test machine as I write this. >>> There is no slowness or notice-able performance impact as such. >> >> >> Unfortunately "noticeable" isn't going to be a viable metric. You'll >> need to do some real-world benchmarks (i.e. kernel builds, hackbench, >> etc), and compare the results. Even just initializing >> passed-by-reference variables (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had >> measurable performance impact. >> > > > For comparison (-300 is official Fedora, -301 is from the repo): > > text data bss dec hex filename > 16948437 6771094 1777872 25497403 1850f3b > /lib/debug/lib/modules/4.15.5-300.fc27.x86_64/vmlinux > 16970359 6776078 1777872 25524309 1857855 > /lib/debug/lib/modules/4.15.5-301.fc27.x86_64/vmlinux That's a surprisingly small text change! I'd love to see benchmarks too. Are you able to verify this is initializing the passed-by-reference variables too? Hmm, I suspect it's time for another LKDTM test. ;) -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 23:38 ` Kees Cook @ 2018-02-28 6:22 ` P J P 2018-02-28 8:25 ` Florian Weimer 1 sibling, 0 replies; 14+ messages in thread From: P J P @ 2018-02-28 6:22 UTC (permalink / raw) To: Kees Cook; +Cc: Laura Abbott, Kernel Hardening, Florian Weimer +-- On Tue, 27 Feb 2018, Kees Cook wrote --+ | On Tue, Feb 27, 2018 at 3:26 PM, Laura Abbott <labbott@redhat.com> wrote: | > For comparison (-300 is official Fedora, -301 is from the repo): | > | > text data bss dec hex filename | > 16948437 6771094 1777872 25497403 1850f3b /lib/debug/lib/modules/4.15.5-300.fc27.x86_64/vmlinux | > 16970359 6776078 1777872 25524309 1857855 /lib/debug/lib/modules/4.15.5-301.fc27.x86_64/vmlinux | | That's a surprisingly small text change! I'd love to see benchmarks | too. Are you able to verify this is initializing the | passed-by-reference variables too? Hmm, I suspect it's time for | another LKDTM test. ;) It could be because I had to build Crypt target support(CONFIG_DM_CRYPT=y) into kernel. It would not boot without it, not sure why. F27 -300 kernel does boot with DM_CRYPT as module. === $ diff -Naurp config-4.15.5-300.fc27.x86_64 config-4.15.5-301.fc27.x86_64 --- config-4.15.5-300.fc27.x86_64 2018-02-28 11:49:52.248210900 +0530 +++ config-4.15.5-301.fc27.x86_64 2018-02-28 11:50:35.448957090 +0530 @@ -1,6 +1,6 @@ # # Automatically generated file; DO NOT EDIT. -# Linux/x86_64 4.15.5-300.fc27.x86_64 Kernel Configuration +# Linux/x86_64 4.15.5-301.fc27.x86_64 Kernel Configuration # CONFIG_64BIT=y CONFIG_X86_64=y @@ -2394,7 +2394,7 @@ CONFIG_DM_DEBUG_BLOCK_MANAGER_LOCKING=y # CONFIG_DM_DEBUG_BLOCK_STACK_TRACING is not set CONFIG_DM_BIO_PRISON=m CONFIG_DM_PERSISTENT_DATA=m -CONFIG_DM_CRYPT=m +CONFIG_DM_CRYPT=y CONFIG_DM_SNAPSHOT=y CONFIG_DM_THIN_PROVISIONING=m CONFIG_DM_CACHE=m @@ -7612,6 +7612,7 @@ CONFIG_DEBUG_INFO=y CONFIG_DEBUG_INFO_VTA=y # CONFIG_GDB_SCRIPTS is not set # CONFIG_ENABLE_WARN_DEPRECATED is not set +CONFIG_FINIT_LOCAL_VARS=y CONFIG_ENABLE_MUST_CHECK=y CONFIG_FRAME_WARN=2048 CONFIG_STRIP_ASM_SYMS=y === Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 23:38 ` Kees Cook 2018-02-28 6:22 ` P J P @ 2018-02-28 8:25 ` Florian Weimer 1 sibling, 0 replies; 14+ messages in thread From: Florian Weimer @ 2018-02-28 8:25 UTC (permalink / raw) To: Kees Cook, Laura Abbott; +Cc: P J P, Kernel Hardening, P J P On 02/28/2018 12:38 AM, Kees Cook wrote: > That's a surprisingly small text change! I'd love to see benchmarks > too. When we benchmarked this a while back, we saw a measurable performance hit for processing small packet processing (both UDP and TCP). I assumed this was due to the initialization of the socket address structure. Unfortunately, this was a place where past leaks happened. My feeling at the time was that the return path for the socket address would have to be overhauled, such that returning a partially initialized result would be prevent by the API, without having to clear the entire return buffer. Apart from that, we didn't see any changes in performance. > Are you able to verify this is initializing the > passed-by-reference variables too? Shouldn't the initialization happen in the caller? Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 19:28 ` [PATCH 0/1] Zero initialise kernel stack variables Kees Cook 2018-02-27 23:26 ` Laura Abbott @ 2018-02-28 6:04 ` P J P 2018-03-02 19:52 ` Nick Kralevich 2 siblings, 0 replies; 14+ messages in thread From: P J P @ 2018-02-28 6:04 UTC (permalink / raw) To: Kees Cook; +Cc: Kernel Hardening, Florian Weimer, P J P, Laura Abbott Hello Kees, Laura, Thank you so much for the kind review, I appreciate it. +-- On Tue, 27 Feb 2018, Kees Cook wrote --+ | Unfortunately "noticeable" isn't going to be a viable metric. You'll need to | do some real-world benchmarks (i.e. kernel builds, hackbench, etc), and | compare the results. Yes, okay. I'll do this exercise and get back with the results. | Even just initializing passed-by-reference variables | (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had measurable performance impact. I see. | It would be nice to have four options/features available from the | compiler, from least to most performance impact: | | - initialize padding to zero when static initializers are used (this would | make foo = { .field = something }; identical to memset(&foo, 0, | sizeof(foo)); foo.field = something for all structures, but now, any | structures with padding _must_ use the latter to be safe, which is highly | error-prone). | | - initialize all uninitialized variables that contain a structure marked | with a special attribute (e.g. __attribute__((force_initialize)) ). | | - initialize all uninitialized variables that are passed by reference (see | GCC_PLUGIN_STRUCTLEAK_BYREF_ALL). | | - initialize all uninitialized variables (-finit-local-vars seems to do | this) I see, will check about these. | > The patch here adds a kbuild menu option to enable/disable '-finit-local-vars' | > compiler flag while building the Linux kernel. | | Since this is a single patch, I think it'd be better to fold this | entire cover letter into patch 1. Right, okay. Thank you so much. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-02-27 19:28 ` [PATCH 0/1] Zero initialise kernel stack variables Kees Cook 2018-02-27 23:26 ` Laura Abbott 2018-02-28 6:04 ` P J P @ 2018-03-02 19:52 ` Nick Kralevich 2018-03-02 21:01 ` Casey Schaufler 2 siblings, 1 reply; 14+ messages in thread From: Nick Kralevich @ 2018-03-02 19:52 UTC (permalink / raw) To: Kees Cook; +Cc: P J P, Kernel Hardening, Florian Weimer, P J P On Tue, Feb 27, 2018 at 11:28 AM, Kees Cook <keescook@chromium.org> wrote: >> This same kernel is running on my F27 test machine as I write this. >> There is no slowness or notice-able performance impact as such. > > Unfortunately "noticeable" isn't going to be a viable metric. You'll > need to do some real-world benchmarks (i.e. kernel builds, hackbench, > etc), and compare the results. Even just initializing > passed-by-reference variables (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had > measurable performance impact. > > It would be nice to have four options/features available from the > compiler, from least to most performance impact: One of the recurring themes I've noticed from the kernel-hardening mailing list is that someone will propose a valuable compile time feature (such as this one), people will ask for performance measurements, some specific use case will be found which has adverse performance impact, and the entire change will be rejected. It's silly. The beautiful thing about compile time features is that they can be enabled / disabled per compilation unit. If there's a performance problem in a certain chunk of code, figure out a way to opt that code out of the protections. 95% of the performance of the kernel is likely only dependent on 5% of the code, and even if we opt out that code, we still protect the remaining 95% of the code. If you assume that bugs are evenly distributed throughout the code base, a 95% reduction is HUGE. Strategically, we shouldn't let the perfect be the enemy of the good. Let's move away from fixating on benchmarks, and just enable easy opt-out for the folks who demonstrate performance bottlenecks. I suspect that many kernel-hardening features would have an easier time being accepted if it wasn't presented as an all or nothing feature. My $0.02. -- Nick ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-03-02 19:52 ` Nick Kralevich @ 2018-03-02 21:01 ` Casey Schaufler 2018-03-05 20:42 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Casey Schaufler @ 2018-03-02 21:01 UTC (permalink / raw) To: Nick Kralevich, Kees Cook; +Cc: P J P, Kernel Hardening, Florian Weimer, P J P On 3/2/2018 11:52 AM, Nick Kralevich wrote: > On Tue, Feb 27, 2018 at 11:28 AM, Kees Cook <keescook@chromium.org> wrote: >>> This same kernel is running on my F27 test machine as I write this. >>> There is no slowness or notice-able performance impact as such. >> Unfortunately "noticeable" isn't going to be a viable metric. You'll >> need to do some real-world benchmarks (i.e. kernel builds, hackbench, >> etc), and compare the results. Even just initializing >> passed-by-reference variables (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had >> measurable performance impact. >> >> It would be nice to have four options/features available from the >> compiler, from least to most performance impact: > One of the recurring themes I've noticed from the kernel-hardening > mailing list is that someone will propose a valuable compile time > feature (such as this one), people will ask for performance > measurements, some specific use case will be found which has adverse > performance impact, and the entire change will be rejected. It's > silly. A security developers we *must* consider performance to be critical at all times. There is no easier way to get something rejected out of hand than for it to have serious performance impact. On the other hand, if I had a nickel for every time I saw something really stoopid done to improve performance I'd have a big pile of nickels. One of the performance issues your changed caused was in the common case of network packet delivery. The people who work on that code are fanatical about performance. Just as the security people might flame an obvious buffer overflow, the networking people will stomp on an unnecessary performance hit. > The beautiful thing about compile time features is that they can be > enabled / disabled per compilation unit. If there's a performance > problem in a certain chunk of code, figure out a way to opt that code > out of the protections. 95% of the performance of the kernel is likely > only dependent on 5% of the code, and even if we opt out that code, we > still protect the remaining 95% of the code. If you assume that bugs > are evenly distributed throughout the code base, a 95% reduction is > HUGE. For good or ill there is a strong tradition that a 95% security solution is broken. You don't need all the code to be penetrated, just a teeny bit. And what about the code that couldn't be penetrated anyway, but now has to be slower to protect that little bit? > Strategically, we shouldn't let the perfect be the enemy of the good. > Let's move away from fixating on benchmarks, and just enable easy > opt-out for the folks who demonstrate performance bottlenecks. The problem is not that people will choose performance over security, or the other way around. It's that they don't consider both to be important in the first place. > > I suspect that many kernel-hardening features would have an easier > time being accepted if it wasn't presented as an all or nothing > feature. You're up against the notion that you can ignore one aspect of the system to enhance another. > > My $0.02. > > -- Nick > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/1] Zero initialise kernel stack variables 2018-03-02 21:01 ` Casey Schaufler @ 2018-03-05 20:42 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2018-03-05 20:42 UTC (permalink / raw) To: Casey Schaufler Cc: Nick Kralevich, P J P, Kernel Hardening, Florian Weimer, P J P On Fri, Mar 2, 2018 at 1:01 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/2/2018 11:52 AM, Nick Kralevich wrote: >> On Tue, Feb 27, 2018 at 11:28 AM, Kees Cook <keescook@chromium.org> wrote: >>>> This same kernel is running on my F27 test machine as I write this. >>>> There is no slowness or notice-able performance impact as such. >>> Unfortunately "noticeable" isn't going to be a viable metric. You'll >>> need to do some real-world benchmarks (i.e. kernel builds, hackbench, >>> etc), and compare the results. Even just initializing >>> passed-by-reference variables (GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) had >>> measurable performance impact. >>> >>> It would be nice to have four options/features available from the >>> compiler, from least to most performance impact: >> >> One of the recurring themes I've noticed from the kernel-hardening >> mailing list is that someone will propose a valuable compile time >> feature (such as this one), people will ask for performance >> measurements, some specific use case will be found which has adverse >> performance impact, and the entire change will be rejected. It's >> silly. The way I think about adding security hardening to upstream is that we have to consider a number of conflicting interests. I would say the primary principle is "secure by default", which comes with lots of caveats, namely in the form of performance impact, maintainability impact, and false positive rate. If something has zero performance and maintenance impact and zero false positive rate, then this is a slam-dunk and there's no problem. This is rarely the case, of course. Frequently the maintainability and false positive rate can be seen or inferred from the content of the patches, so the explicit question, when not answered in a commit log or documentation, becomes "what is the performance impact?" When something can't be always-on, then we have to look at asking "can this be a _runtime_ toggle?" rather than a compile-time option, since many of the end-users of the kernel are getting it through distros, and distros are allergic to making many different kernel packages with different compile-time options. They just want _one_ kernel binary. Now, speaking to "the entire change will be rejected", this usually happens in two cases. Either the added complexity is seen as not worth the hardening (in which case, better evidence is needed to support the hardening, or refactoring to reduce maintenance burdens), or we bump up against what I'll call "security absolutism" ways of thinking from some maintainers. Dealing with the latter is much more difficult, since they don't see pragmatic security improvements as having value compared to "perfect protection". Obviously perfect is preferred, but it can't always be done. (e.g. see "an unpowered computer is safest", etc.) I want hardening features to _actually_ get used by the widest possible audience of end users. That means trying to balance all the things above, and get the best possible approach for upstream and downstream. In this specific case, we can't have a runtime toggle, which means we need to get the performance impact as low as possible if we want to see distros using it. We may not get down to a level were a distro wants to enable it, then that's fine, other people who do their own kernel builds will still have it available as a compile-time option, but then at least we've tried, and we'll have evidence to show for what kinds of improvements would be needed in the future to gain wider roll-out. > A security developers we *must* consider performance to be > critical at all times. There is no easier way to get something > rejected out of hand than for it to have serious performance > impact. On the other hand, if I had a nickel for every time I > saw something really stoopid done to improve performance I'd > have a big pile of nickels. One of the performance issues your > changed caused was in the common case of network packet delivery. > The people who work on that code are fanatical about performance. > Just as the security people might flame an obvious buffer overflow, > the networking people will stomp on an unnecessary performance hit. The networking folks are especially unhappy about doing (what they see as) redundant memory initialization, even when it kills classes of bugs. I personally find this rather frustrating, but I understand that performance is critical to them. In these cases, having a generalized approach (such as a compiler flag or plugin) provides a way for the general public to benefit from killing bug classes and for the perf-critical users to disable the protection. I don't like having to face these trade-offs, but as we're seeing, the corner-cutting by CPUs and kernels continue to haunt us, and we need to find efficient ways to initialize memory, avoid side-channels, etc. >> The beautiful thing about compile time features is that they can be >> enabled / disabled per compilation unit. If there's a performance >> problem in a certain chunk of code, figure out a way to opt that code >> out of the protections. 95% of the performance of the kernel is likely >> only dependent on 5% of the code, and even if we opt out that code, we >> still protect the remaining 95% of the code. If you assume that bugs >> are evenly distributed throughout the code base, a 95% reduction is >> HUGE. > > For good or ill there is a strong tradition that a 95% security > solution is broken. You don't need all the code to be penetrated, > just a teeny bit. And what about the code that couldn't be > penetrated anyway, but now has to be slower to protect that little > bit? Yeah, and it's specifically the networking code that has had many of the flaws in leaking memory contents due to drivers not fully initializing some structure here or there. So we'd leave ourselves open to a known source of regular bugs. I think the best use of per-compilation-unit protections is for things that have high false-positive rates, and it can be used as a way to start building up coverage over time across the entire kernel with the goal of 100% coverage (e.g. runtime integer overflow detection like Clang's -fsanitize=integer). >> Strategically, we shouldn't let the perfect be the enemy of the good. >> Let's move away from fixating on benchmarks, and just enable easy >> opt-out for the folks who demonstrate performance bottlenecks. I agree that opt-out is desired, but upstream conservatism tends to dictate opt-in initially. Having performance numbers (like I talk about above) is just part of the larger piece for trying to figure out a strategy for upstreaming. Once it's in, then distros enable it by default, then the kernel does too. This can span years, though. > The problem is not that people will choose performance over security, > or the other way around. It's that they don't consider both to be > important in the first place. > >> >> I suspect that many kernel-hardening features would have an easier >> time being accepted if it wasn't presented as an all or nothing >> feature. The largest constraint, IMO, is the maintainability one. Having complex code with lots of #ifdefs for the compile-time toggles creates code that bit-rots, gets less testing, etc. Perf is part of the picture, but one that gets discussed more explicitly since it can impact the other areas of concern. > You're up against the notion that you can ignore one aspect of > the system to enhance another. > >> >> My $0.02. Thanks for bringing it up! I wish things were easier, but this is what we have. :) I'd also like to think that if we can hammer out all the concerns for a patch series on this list, then we can avoid flame-wars and auto-ignore when presenting things for merging. -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-05 20:42 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-27 11:15 [PATCH 0/1] Zero initialise kernel stack variables P J P 2018-02-27 11:15 ` [PATCH 1/1] Add an option to build kernel with -finit-local-vars P J P 2018-02-27 19:22 ` Laura Abbott 2018-02-27 19:33 ` Kees Cook 2018-02-28 5:49 ` P J P 2018-02-27 19:28 ` [PATCH 0/1] Zero initialise kernel stack variables Kees Cook 2018-02-27 23:26 ` Laura Abbott 2018-02-27 23:38 ` Kees Cook 2018-02-28 6:22 ` P J P 2018-02-28 8:25 ` Florian Weimer 2018-02-28 6:04 ` P J P 2018-03-02 19:52 ` Nick Kralevich 2018-03-02 21:01 ` Casey Schaufler 2018-03-05 20:42 ` Kees Cook
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.