From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Date: Wed, 04 Oct 2017 16:22:24 +0000 Subject: Re: [kernel-hardening] Re: [PATCH 2/3] Makefile: Move stackprotector availability out of Kconfig Message-Id: List-Id: References: <1506972007-80614-1-git-send-email-keescook@chromium.org> <1506972007-80614-3-git-send-email-keescook@chromium.org> <20171004151312.GA20938@kroah.com> In-Reply-To: <20171004151312.GA20938@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Greg KH Cc: Masahiro Yamada , Andrew Morton , Michal Marek , Ingo Molnar , Laura Abbott , Nicholas Piggin , Al Viro , Linux Kbuild mailing list , Yoshinori Sato , Rich Felker , "David S. Miller" , Linux Kernel Mailing List , linux-sh , "kernel-hardening@lists.openwall.com" , Mark Rutland On Wed, Oct 4, 2017 at 8:13 AM, Greg KH wrote: > On Wed, Oct 04, 2017 at 11:33:38PM +0900, Masahiro Yamada wrote: >> Hi Kees, >> >> >> 2017-10-03 4:20 GMT+09:00 Kees Cook : >> > Various portions of the kernel, especially per-architecture pieces, >> > need to know if the compiler is building it with the stack protector. >> > This was done in the arch/Kconfig with 'select', but this doesn't >> > allow a way to do auto-detected compiler support. In preparation for >> > creating an on-if-available default, move the logic for the definition of >> > CONFIG_CC_STACKPROTECTOR into the Makefile. >> > >> > Cc: Masahiro Yamada >> > Cc: Michal Marek >> > Cc: Andrew Morton >> > Cc: Ingo Molnar >> > Cc: Laura Abbott >> > Cc: Nicholas Piggin >> > Cc: Al Viro >> > Cc: linux-kbuild@vger.kernel.org >> > Signed-off-by: Kees Cook >> > --- >> > Makefile | 7 +++++-- >> > arch/Kconfig | 8 -------- >> > 2 files changed, 5 insertions(+), 10 deletions(-) >> > >> > diff --git a/Makefile b/Makefile >> > index d1119941261c..e122a9cf0399 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -688,8 +688,11 @@ else >> > stackp-flag := $(call cc-option, -fno-stack-protector) >> > endif >> > endif >> > -# Find arch-specific stack protector compiler sanity-checking script. >> > -ifdef CONFIG_CC_STACKPROTECTOR >> > +ifdef stackp-name >> > + # If the stack protector has been selected, inform the rest of the build. >> > + KBUILD_CFLAGS += -DCONFIG_CC_STACKPROTECTOR >> > + KBUILD_AFLAGS += -DCONFIG_CC_STACKPROTECTOR >> > + # Find arch-specific stack protector compiler sanity-checking script. >> > stackp-path := $(srctree)/scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh >> > stackp-check := $(wildcard $(stackp-path)) >> > endif >> >> >> I have not tested this series, >> but I think this commit is bad (with the follow-up patch applied). >> >> >> I thought of this scenario: >> >> [1] Kernel is configured with CONFIG_CC_STACKPROTECTOR_AUTO >> >> [2] Kernel is built with a compiler without stack protector support. >> >> [3] CONFIG_CC_STACKPROTECTOR is not defined, >> so __stack_chk_fail() is not compiled. >> >> [4] Out-of-tree modules are compiled with a compiler with >> stack protector support. >> __stack_chk_fail() is inserted to functions of the modules. > > We don't ever support the system of loading a module built with anything > other than the _exact_ same compiler than the kernel was. So this will > not happen (well, if someone tries it, they get to keep the pieces their > kernel image is now in...) > >> [5] insmod fails because reference to __stack_chk_fail() >> can not be resolved. > > Even nicer, we failed "cleanly" :) > > This isn't a real-world issue, sorry. If we wanted a slightly different failure, we could simply add the stack protection feature to the VERMAGIC_STRING define: diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h index af6c03f7f986..300163aba666 100644 --- a/include/linux/vermagic.h +++ b/include/linux/vermagic.h @@ -30,11 +30,19 @@ #else #define MODULE_RANDSTRUCT_PLUGIN #endif +#if defined(__SSP__) +#define MODULE_STACKPROTECTOR "stack-protector " +#elif define (__SSP_STRONG__) +#define MODULE_STACKPROTECTOR "stack-protector-strong " +#else +#define MODULE_STACKPROTECTOR "" +#endif #define VERMAGIC_STRING \ UTS_RELEASE " " \ MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT \ MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS \ MODULE_ARCH_VERMAGIC \ + MODULE_STACKPROTECTOR \ MODULE_RANDSTRUCT_PLUGIN Do you want me to send this patch, or should we allow it to fail with the "missing reference" (which may actually be more expressive...) I think the way it is right now is better, but I'm open to either. -Kees -- Kees Cook Pixel Security From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751322AbdJDQW3 (ORCPT ); Wed, 4 Oct 2017 12:22:29 -0400 Received: from mail-it0-f47.google.com ([209.85.214.47]:56775 "EHLO mail-it0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750937AbdJDQW1 (ORCPT ); Wed, 4 Oct 2017 12:22:27 -0400 X-Google-Smtp-Source: AOwi7QDpfPG6cMY9pbkyVpWFGEvPB0oTB6WozfQ2EbscS829xmCdAmFO3TU2PBgb1xU8CUt8CMw+BApTXeKMCQEJkg4= MIME-Version: 1.0 In-Reply-To: <20171004151312.GA20938@kroah.com> References: <1506972007-80614-1-git-send-email-keescook@chromium.org> <1506972007-80614-3-git-send-email-keescook@chromium.org> <20171004151312.GA20938@kroah.com> From: Kees Cook Date: Wed, 4 Oct 2017 09:22:24 -0700 X-Google-Sender-Auth: KrIVrIkbWtyK3xzdfKOpU3pQy2c Message-ID: Subject: Re: [kernel-hardening] Re: [PATCH 2/3] Makefile: Move stackprotector availability out of Kconfig To: Greg KH Cc: Masahiro Yamada , Andrew Morton , Michal Marek , Ingo Molnar , Laura Abbott , Nicholas Piggin , Al Viro , Linux Kbuild mailing list , Yoshinori Sato , Rich Felker , "David S. Miller" , Linux Kernel Mailing List , linux-sh , "kernel-hardening@lists.openwall.com" , Mark Rutland Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 4, 2017 at 8:13 AM, Greg KH wrote: > On Wed, Oct 04, 2017 at 11:33:38PM +0900, Masahiro Yamada wrote: >> Hi Kees, >> >> >> 2017-10-03 4:20 GMT+09:00 Kees Cook : >> > Various portions of the kernel, especially per-architecture pieces, >> > need to know if the compiler is building it with the stack protector. >> > This was done in the arch/Kconfig with 'select', but this doesn't >> > allow a way to do auto-detected compiler support. In preparation for >> > creating an on-if-available default, move the logic for the definition of >> > CONFIG_CC_STACKPROTECTOR into the Makefile. >> > >> > Cc: Masahiro Yamada >> > Cc: Michal Marek >> > Cc: Andrew Morton >> > Cc: Ingo Molnar >> > Cc: Laura Abbott >> > Cc: Nicholas Piggin >> > Cc: Al Viro >> > Cc: linux-kbuild@vger.kernel.org >> > Signed-off-by: Kees Cook >> > --- >> > Makefile | 7 +++++-- >> > arch/Kconfig | 8 -------- >> > 2 files changed, 5 insertions(+), 10 deletions(-) >> > >> > diff --git a/Makefile b/Makefile >> > index d1119941261c..e122a9cf0399 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -688,8 +688,11 @@ else >> > stackp-flag := $(call cc-option, -fno-stack-protector) >> > endif >> > endif >> > -# Find arch-specific stack protector compiler sanity-checking script. >> > -ifdef CONFIG_CC_STACKPROTECTOR >> > +ifdef stackp-name >> > + # If the stack protector has been selected, inform the rest of the build. >> > + KBUILD_CFLAGS += -DCONFIG_CC_STACKPROTECTOR >> > + KBUILD_AFLAGS += -DCONFIG_CC_STACKPROTECTOR >> > + # Find arch-specific stack protector compiler sanity-checking script. >> > stackp-path := $(srctree)/scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh >> > stackp-check := $(wildcard $(stackp-path)) >> > endif >> >> >> I have not tested this series, >> but I think this commit is bad (with the follow-up patch applied). >> >> >> I thought of this scenario: >> >> [1] Kernel is configured with CONFIG_CC_STACKPROTECTOR_AUTO >> >> [2] Kernel is built with a compiler without stack protector support. >> >> [3] CONFIG_CC_STACKPROTECTOR is not defined, >> so __stack_chk_fail() is not compiled. >> >> [4] Out-of-tree modules are compiled with a compiler with >> stack protector support. >> __stack_chk_fail() is inserted to functions of the modules. > > We don't ever support the system of loading a module built with anything > other than the _exact_ same compiler than the kernel was. So this will > not happen (well, if someone tries it, they get to keep the pieces their > kernel image is now in...) > >> [5] insmod fails because reference to __stack_chk_fail() >> can not be resolved. > > Even nicer, we failed "cleanly" :) > > This isn't a real-world issue, sorry. If we wanted a slightly different failure, we could simply add the stack protection feature to the VERMAGIC_STRING define: diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h index af6c03f7f986..300163aba666 100644 --- a/include/linux/vermagic.h +++ b/include/linux/vermagic.h @@ -30,11 +30,19 @@ #else #define MODULE_RANDSTRUCT_PLUGIN #endif +#if defined(__SSP__) +#define MODULE_STACKPROTECTOR "stack-protector " +#elif define (__SSP_STRONG__) +#define MODULE_STACKPROTECTOR "stack-protector-strong " +#else +#define MODULE_STACKPROTECTOR "" +#endif #define VERMAGIC_STRING \ UTS_RELEASE " " \ MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT \ MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS \ MODULE_ARCH_VERMAGIC \ + MODULE_STACKPROTECTOR \ MODULE_RANDSTRUCT_PLUGIN Do you want me to send this patch, or should we allow it to fail with the "missing reference" (which may actually be more expressive...) I think the way it is right now is better, but I'm open to either. -Kees -- Kees Cook Pixel Security From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f43.google.com ([209.85.214.43]:55817 "EHLO mail-it0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751066AbdJDQW1 (ORCPT ); Wed, 4 Oct 2017 12:22:27 -0400 Received: by mail-it0-f43.google.com with SMTP id p185so5834280itc.4 for ; Wed, 04 Oct 2017 09:22:27 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20171004151312.GA20938@kroah.com> References: <1506972007-80614-1-git-send-email-keescook@chromium.org> <1506972007-80614-3-git-send-email-keescook@chromium.org> <20171004151312.GA20938@kroah.com> From: Kees Cook Date: Wed, 4 Oct 2017 09:22:24 -0700 Message-ID: Subject: Re: [kernel-hardening] Re: [PATCH 2/3] Makefile: Move stackprotector availability out of Kconfig Content-Type: text/plain; charset="UTF-8" Sender: linux-kbuild-owner@vger.kernel.org List-ID: To: Greg KH Cc: Masahiro Yamada , Andrew Morton , Michal Marek , Ingo Molnar , Laura Abbott , Nicholas Piggin , Al Viro , Linux Kbuild mailing list , Yoshinori Sato , Rich Felker , "David S. Miller" , Linux Kernel Mailing List , linux-sh , "kernel-hardening@lists.openwall.com" , Mark Rutland On Wed, Oct 4, 2017 at 8:13 AM, Greg KH wrote: > On Wed, Oct 04, 2017 at 11:33:38PM +0900, Masahiro Yamada wrote: >> Hi Kees, >> >> >> 2017-10-03 4:20 GMT+09:00 Kees Cook : >> > Various portions of the kernel, especially per-architecture pieces, >> > need to know if the compiler is building it with the stack protector. >> > This was done in the arch/Kconfig with 'select', but this doesn't >> > allow a way to do auto-detected compiler support. In preparation for >> > creating an on-if-available default, move the logic for the definition of >> > CONFIG_CC_STACKPROTECTOR into the Makefile. >> > >> > Cc: Masahiro Yamada >> > Cc: Michal Marek >> > Cc: Andrew Morton >> > Cc: Ingo Molnar >> > Cc: Laura Abbott >> > Cc: Nicholas Piggin >> > Cc: Al Viro >> > Cc: linux-kbuild@vger.kernel.org >> > Signed-off-by: Kees Cook >> > --- >> > Makefile | 7 +++++-- >> > arch/Kconfig | 8 -------- >> > 2 files changed, 5 insertions(+), 10 deletions(-) >> > >> > diff --git a/Makefile b/Makefile >> > index d1119941261c..e122a9cf0399 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -688,8 +688,11 @@ else >> > stackp-flag := $(call cc-option, -fno-stack-protector) >> > endif >> > endif >> > -# Find arch-specific stack protector compiler sanity-checking script. >> > -ifdef CONFIG_CC_STACKPROTECTOR >> > +ifdef stackp-name >> > + # If the stack protector has been selected, inform the rest of the build. >> > + KBUILD_CFLAGS += -DCONFIG_CC_STACKPROTECTOR >> > + KBUILD_AFLAGS += -DCONFIG_CC_STACKPROTECTOR >> > + # Find arch-specific stack protector compiler sanity-checking script. >> > stackp-path := $(srctree)/scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh >> > stackp-check := $(wildcard $(stackp-path)) >> > endif >> >> >> I have not tested this series, >> but I think this commit is bad (with the follow-up patch applied). >> >> >> I thought of this scenario: >> >> [1] Kernel is configured with CONFIG_CC_STACKPROTECTOR_AUTO >> >> [2] Kernel is built with a compiler without stack protector support. >> >> [3] CONFIG_CC_STACKPROTECTOR is not defined, >> so __stack_chk_fail() is not compiled. >> >> [4] Out-of-tree modules are compiled with a compiler with >> stack protector support. >> __stack_chk_fail() is inserted to functions of the modules. > > We don't ever support the system of loading a module built with anything > other than the _exact_ same compiler than the kernel was. So this will > not happen (well, if someone tries it, they get to keep the pieces their > kernel image is now in...) > >> [5] insmod fails because reference to __stack_chk_fail() >> can not be resolved. > > Even nicer, we failed "cleanly" :) > > This isn't a real-world issue, sorry. If we wanted a slightly different failure, we could simply add the stack protection feature to the VERMAGIC_STRING define: diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h index af6c03f7f986..300163aba666 100644 --- a/include/linux/vermagic.h +++ b/include/linux/vermagic.h @@ -30,11 +30,19 @@ #else #define MODULE_RANDSTRUCT_PLUGIN #endif +#if defined(__SSP__) +#define MODULE_STACKPROTECTOR "stack-protector " +#elif define (__SSP_STRONG__) +#define MODULE_STACKPROTECTOR "stack-protector-strong " +#else +#define MODULE_STACKPROTECTOR "" +#endif #define VERMAGIC_STRING \ UTS_RELEASE " " \ MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT \ MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS \ MODULE_ARCH_VERMAGIC \ + MODULE_STACKPROTECTOR \ MODULE_RANDSTRUCT_PLUGIN Do you want me to send this patch, or should we allow it to fail with the "missing reference" (which may actually be more expressive...) I think the way it is right now is better, but I'm open to either. -Kees -- Kees Cook Pixel Security