[PATCH 0/1] Zero initialise kernel stack variables

[PATCH 0/1] Zero initialise kernel stack variables

[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

[PATCH 1/1] Add an option to build kernel with -finit-local-vars

[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

Re: [PATCH 1/1] Add an option to build kernel with -finit-local-vars

[Laura Abbott]

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
> 

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Kees Cook]

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

Re: [PATCH 1/1] Add an option to build kernel with -finit-local-vars

[Kees Cook]

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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Laura Abbott]

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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Kees Cook]

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

Re: [PATCH 1/1] Add an option to build kernel with -finit-local-vars

[P J P]

+-- 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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[P J P]

  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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[P J P]

+-- 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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Florian Weimer]

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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Nick Kralevich]

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

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Casey Schaufler]

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
>

Re: [PATCH 0/1] Zero initialise kernel stack variables

[Kees Cook]

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