Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On Wed, Apr 24, 2019 at 4:36 AM Kees Cook wrote: > > On Thu, Apr 11, 2019 at 6:39 PM Masahiro Yamada > wrote: > > > > On Fri, Apr 12, 2019 at 3:01 AM Kees Cook wrote: > > > > > > Right now kernel hardening options are scattered around various Kconfig > > > files. This can be a central place to collect these kinds of options > > > going forward. This is initially populated with the memory initialization > > > options from the gcc-plugins. > > > > > > Signed-off-by: Kees Cook > > > --- > > > scripts/gcc-plugins/Kconfig | 74 +++-- > > > security/Kconfig| 2 + > > > security/Kconfig.hardening | 93 + > > > 3 files changed, 102 insertions(+), 67 deletions(-) > > > create mode 100644 security/Kconfig.hardening > > > > > > diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig > > > index 74271dba4f94..84d471dea2b7 100644 > > > --- a/scripts/gcc-plugins/Kconfig > > > +++ b/scripts/gcc-plugins/Kconfig > > > @@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS > > > An arch should select this symbol if it supports building with > > > GCC plugins. > > > > > > -menuconfig GCC_PLUGINS > > > - bool "GCC plugins" > > > +config GCC_PLUGINS > > > + bool > > > depends on HAVE_GCC_PLUGINS > > > depends on PLUGIN_HOSTCC != "" > > > + default y > > > help > > > GCC plugins are loadable modules that provide extra features to > > > the > > > compiler. They are useful for runtime instrumentation and > > > static analysis. > > > @@ -25,6 +26,8 @@ menuconfig GCC_PLUGINS > > > > > > if GCC_PLUGINS > > > > > > +menu "GCC plugins" > > > + > > > > > > > > Just a tip to save "if" ... "endif" block. > > > > > > If you like, you can write like follows: > > > > > > menu "GCC plugins" > > depends on GCC_PLUGINS > > > > > > > > endmenu > > Ah yes, thanks! Adjusted. > > > > +menu "Memory initialization" > > > + > > > +choice > > > + prompt "Initialize kernel stack variables at function entry" > > > + depends on GCC_PLUGINS > > > > On second thought, > > this 'depends on' is unnecessary > > because INIT_STACK_NONE should be always visible. > > Oh yes, excellent point. Adjusted. > > > Another behavior change is > > GCC_PLUGIN_STRUCTLEAK was previously enabled by all{yes,mod}config, > > and in the compile-test coverage. > > I could set the defaults based on CONFIG_COMPILE_TEST, though? I.e.: > > 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_NONE Looks a good idea to me. Thanks. -- Best Regards Masahiro Yamada
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On Thu, Apr 11, 2019 at 6:39 PM Masahiro Yamada wrote: > > On Fri, Apr 12, 2019 at 3:01 AM Kees Cook wrote: > > > > Right now kernel hardening options are scattered around various Kconfig > > files. This can be a central place to collect these kinds of options > > going forward. This is initially populated with the memory initialization > > options from the gcc-plugins. > > > > Signed-off-by: Kees Cook > > --- > > scripts/gcc-plugins/Kconfig | 74 +++-- > > security/Kconfig| 2 + > > security/Kconfig.hardening | 93 + > > 3 files changed, 102 insertions(+), 67 deletions(-) > > create mode 100644 security/Kconfig.hardening > > > > diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig > > index 74271dba4f94..84d471dea2b7 100644 > > --- a/scripts/gcc-plugins/Kconfig > > +++ b/scripts/gcc-plugins/Kconfig > > @@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS > > An arch should select this symbol if it supports building with > > GCC plugins. > > > > -menuconfig GCC_PLUGINS > > - bool "GCC plugins" > > +config GCC_PLUGINS > > + bool > > depends on HAVE_GCC_PLUGINS > > depends on PLUGIN_HOSTCC != "" > > + default y > > help > > GCC plugins are loadable modules that provide extra features to > > the > > compiler. They are useful for runtime instrumentation and static > > analysis. > > @@ -25,6 +26,8 @@ menuconfig GCC_PLUGINS > > > > if GCC_PLUGINS > > > > +menu "GCC plugins" > > + > > > > Just a tip to save "if" ... "endif" block. > > > If you like, you can write like follows: > > > menu "GCC plugins" > depends on GCC_PLUGINS > > > > endmenu Ah yes, thanks! Adjusted. > > +menu "Memory initialization" > > + > > +choice > > + prompt "Initialize kernel stack variables at function entry" > > + depends on GCC_PLUGINS > > On second thought, > this 'depends on' is unnecessary > because INIT_STACK_NONE should be always visible. Oh yes, excellent point. Adjusted. > Another behavior change is > GCC_PLUGIN_STRUCTLEAK was previously enabled by all{yes,mod}config, > and in the compile-test coverage. I could set the defaults based on CONFIG_COMPILE_TEST, though? I.e.: 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_NONE > > It will be disabled for all{yes,mod}config with this patch. > > This is up to you. Just FYI. Thanks for looking this over! -- Kees Cook
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On 16.04.2019 16:56, Kees Cook wrote: > On Tue, Apr 16, 2019 at 8:55 AM Alexander Popov wrote: >> >> On 16.04.2019 7:02, Kees Cook wrote: >>> On Mon, Apr 15, 2019 at 11:44 AM Alexander Popov >>> wrote: What do you think about some separator between memory initialization options and CONFIG_CRYPTO? >>> >>> This was true before too >> >> Hm, yes, it's a generic behavior - there is no any separator at 'endmenu' and >> config options stick together. >> >> I've created a patch to fix that. What do you think about it? >> I can send it to LKML separately. >> >> >> From 50bf59d30fafcdebb3393fb742e1bd51e7d2f2da Mon Sep 17 00:00:00 2001 >> From: Alexander Popov >> Date: Tue, 16 Apr 2019 16:09:40 +0300 >> Subject: [PATCH 1/1] kconfig: Terminate menu blocks with a newline in the >> generated config >> >> Currently menu blocks start with a pretty header but end with nothing in >> the generated config. So next config options stick together with the >> options from the menu block. >> >> Let's terminate menu blocks with a newline in the generated config. >> >> Signed-off-by: Alexander Popov >> --- >> scripts/kconfig/confdata.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c >> index 08ba146..1459153 100644 >> --- a/scripts/kconfig/confdata.c >> +++ b/scripts/kconfig/confdata.c >> @@ -888,6 +888,8 @@ int conf_write(const char *name) >> if (menu->next) >> menu = menu->next; >> else while ((menu = menu->parent)) { >> + if (!menu->sym && menu_is_visible(menu)) >> + fprintf(out, "\n"); >> if (menu->next) { >> menu = menu->next; >> break; > > Seems fine to me. I defer to Masahiro, though. :) Hi! I've sent this patch separately to LKML: https://lore.kernel.org/lkml/1555669773-9766-1-git-send-email-alex.po...@linux.com/T/#u Best regards, Alexander
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On Tue, Apr 16, 2019 at 8:55 AM Alexander Popov wrote: > > On 16.04.2019 7:02, Kees Cook wrote: > > On Mon, Apr 15, 2019 at 11:44 AM Alexander Popov > > wrote: > >> > >> What do you think about some separator between memory initialization > >> options and > >> CONFIG_CRYPTO? > > > > This was true before too > > Hm, yes, it's a generic behavior - there is no any separator at 'endmenu' and > config options stick together. > > I've created a patch to fix that. What do you think about it? > I can send it to LKML separately. > > > From 50bf59d30fafcdebb3393fb742e1bd51e7d2f2da Mon Sep 17 00:00:00 2001 > From: Alexander Popov > Date: Tue, 16 Apr 2019 16:09:40 +0300 > Subject: [PATCH 1/1] kconfig: Terminate menu blocks with a newline in the > generated config > > Currently menu blocks start with a pretty header but end with nothing in > the generated config. So next config options stick together with the > options from the menu block. > > Let's terminate menu blocks with a newline in the generated config. > > Signed-off-by: Alexander Popov > --- > scripts/kconfig/confdata.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > index 08ba146..1459153 100644 > --- a/scripts/kconfig/confdata.c > +++ b/scripts/kconfig/confdata.c > @@ -888,6 +888,8 @@ int conf_write(const char *name) > if (menu->next) > menu = menu->next; > else while ((menu = menu->parent)) { > + if (!menu->sym && menu_is_visible(menu)) > + fprintf(out, "\n"); > if (menu->next) { > menu = menu->next; > break; Seems fine to me. I defer to Masahiro, though. :) -- Kees Cook
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On 16.04.2019 7:02, Kees Cook wrote: > On Mon, Apr 15, 2019 at 11:44 AM Alexander Popov wrote: >> >> What do you think about some separator between memory initialization options >> and >> CONFIG_CRYPTO? > > This was true before too Hm, yes, it's a generic behavior - there is no any separator at 'endmenu' and config options stick together. I've created a patch to fix that. What do you think about it? I can send it to LKML separately. >From 50bf59d30fafcdebb3393fb742e1bd51e7d2f2da Mon Sep 17 00:00:00 2001 From: Alexander Popov Date: Tue, 16 Apr 2019 16:09:40 +0300 Subject: [PATCH 1/1] kconfig: Terminate menu blocks with a newline in the generated config Currently menu blocks start with a pretty header but end with nothing in the generated config. So next config options stick together with the options from the menu block. Let's terminate menu blocks with a newline in the generated config. Signed-off-by: Alexander Popov --- scripts/kconfig/confdata.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 08ba146..1459153 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -888,6 +888,8 @@ int conf_write(const char *name) if (menu->next) menu = menu->next; else while ((menu = menu->parent)) { + if (!menu->sym && menu_is_visible(menu)) + fprintf(out, "\n"); if (menu->next) { menu = menu->next; break; -- 2.7.4
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On Mon, Apr 15, 2019 at 11:44 AM Alexander Popov wrote: > > On 11.04.2019 21:01, Kees Cook wrote: > > Right now kernel hardening options are scattered around various Kconfig > > files. This can be a central place to collect these kinds of options > > going forward. This is initially populated with the memory initialization > > options from the gcc-plugins. > > > > Signed-off-by: Kees Cook > > Hello Kees, hello everyone! > > After applying this series the kernel config looks like that: > > ... > ... > CONFIG_LSM="yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor" > > # > # Kernel hardening options > # > > # > # Memory initialization > # > CONFIG_INIT_STACK_NONE=y > # CONFIG_GCC_PLUGIN_STRUCTLEAK_USER is not set > # CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF is not set > # CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not set > # CONFIG_GCC_PLUGIN_STACKLEAK is not set > CONFIG_CRYPTO=y > > # > # Crypto core or helper > # > CONFIG_CRYPTO_ALGAPI=y > ... > ... > > What do you think about some separator between memory initialization options > and > CONFIG_CRYPTO? This was true before too: ... # CONFIG_DEFAULT_SECURITY_DAC is not set CONFIG_LSM="yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor" CONFIG_XOR_BLOCKS=y CONFIG_ASYNC_CORE=y CONFIG_ASYNC_MEMCPY=y CONFIG_ASYNC_XOR=y CONFIG_ASYNC_PQ=y CONFIG_ASYNC_RAID6_RECOV=y CONFIG_CRYPTO=y ... Perhaps crypto/Kconfig's comment line could move to the top of the file? comment "Crypto core or helper" is what generates the separator... -- Kees Cook
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On 11.04.2019 21:01, Kees Cook wrote: > Right now kernel hardening options are scattered around various Kconfig > files. This can be a central place to collect these kinds of options > going forward. This is initially populated with the memory initialization > options from the gcc-plugins. > > Signed-off-by: Kees Cook Hello Kees, hello everyone! After applying this series the kernel config looks like that: ... ... CONFIG_LSM="yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor" # # Kernel hardening options # # # Memory initialization # CONFIG_INIT_STACK_NONE=y # CONFIG_GCC_PLUGIN_STRUCTLEAK_USER is not set # CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF is not set # CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not set # CONFIG_GCC_PLUGIN_STACKLEAK is not set CONFIG_CRYPTO=y # # Crypto core or helper # CONFIG_CRYPTO_ALGAPI=y ... ... What do you think about some separator between memory initialization options and CONFIG_CRYPTO? Best regards, Alexander
Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
On Fri, Apr 12, 2019 at 3:01 AM Kees Cook wrote: > > Right now kernel hardening options are scattered around various Kconfig > files. This can be a central place to collect these kinds of options > going forward. This is initially populated with the memory initialization > options from the gcc-plugins. > > Signed-off-by: Kees Cook > --- > scripts/gcc-plugins/Kconfig | 74 +++-- > security/Kconfig| 2 + > security/Kconfig.hardening | 93 + > 3 files changed, 102 insertions(+), 67 deletions(-) > create mode 100644 security/Kconfig.hardening > > diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig > index 74271dba4f94..84d471dea2b7 100644 > --- a/scripts/gcc-plugins/Kconfig > +++ b/scripts/gcc-plugins/Kconfig > @@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS > An arch should select this symbol if it supports building with > GCC plugins. > > -menuconfig GCC_PLUGINS > - bool "GCC plugins" > +config GCC_PLUGINS > + bool > depends on HAVE_GCC_PLUGINS > depends on PLUGIN_HOSTCC != "" > + default y > help > GCC plugins are loadable modules that provide extra features to the > compiler. They are useful for runtime instrumentation and static > analysis. > @@ -25,6 +26,8 @@ menuconfig GCC_PLUGINS > > if GCC_PLUGINS > > +menu "GCC plugins" > + Just a tip to save "if" ... "endif" block. If you like, you can write like follows: menu "GCC plugins" depends on GCC_PLUGINS endmenu instead of if GCC_PLUGINS menu "GCC plugins" endmenu fi > +menu "Memory initialization" > + > +choice > + prompt "Initialize kernel stack variables at function entry" > + depends on GCC_PLUGINS On second thought, this 'depends on' is unnecessary because INIT_STACK_NONE should be always visible. > + default INIT_STACK_NONE > + help > + This option enables initialization of stack variables at > + function entry time. This has the possibility to have the > + greatest coverage (since all functions can have their > + variables initialized), but the performance impact depends > + on the function calling complexity of a given workload's > + syscalls. > + > + This chooses the level of coverage over classes of potentially > + uninitialized variables. The selected class will be > + initialized before use in a function. > + > + config INIT_STACK_NONE > + bool "no automatic initialization (weakest)" > + help > + Disable automatic stack variable initialization. > + This leaves the kernel vulnerable to the standard > + classes of uninitialized stack variable exploits > + and information exposures. > + > + config GCC_PLUGIN_STRUCTLEAK_USER > + bool "zero-init structs marked for userspace (weak)" > + depends on GCC_PLUGINS > + select GCC_PLUGIN_STRUCTLEAK > + help > + Zero-initialize any structures on the stack containing > + a __user attribute. This can prevent some classes of > + uninitialized stack variable exploits and information > + exposures, like CVE-2013-2141: > + https://git.kernel.org/linus/b9e146d8eb3b9eca > + > + config GCC_PLUGIN_STRUCTLEAK_BYREF > + bool "zero-init structs passed by reference (strong)" > + depends on GCC_PLUGINS > + select GCC_PLUGIN_STRUCTLEAK > + help > + Zero-initialize any structures on the stack that may > + be passed by reference and had not already been > + explicitly initialized. This can prevent most classes > + of uninitialized stack variable exploits and information > + exposures, like CVE-2017-1000410: > + https://git.kernel.org/linus/06e7e776ca4d3654 > + > + config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL > + bool "zero-init anything passed by reference (very strong)" > + depends on GCC_PLUGINS > + select GCC_PLUGIN_STRUCTLEAK > + help > + Zero-initialize any stack variables that may be passed > + by reference and had not already been explicitly > + initialized. This is intended to eliminate all classes > + of uninitialized stack variable exploits and information > + exposures. > + > +endchoice Another behavior change is GCC_PLUGIN_STRUCTLEAK was previously enabled by all{yes,mod}config, and in the compile-test coverage. It will be disabled for all{yes,mod}config with this patch. This is up to you. Just FYI. -- Best Regards Masahiro Yamada