Message-ID: <CAGXu5jJ02qRCcqTK_M9sFtekizz+0WoHBCBKF_mh-A870eg6eg@mail.gmail.com>
Date:   Fri, 9 Feb 2018 05:30:46 +1100
From:   Kees Cook <keescook@...omium.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     linux-kbuild <linux-kbuild@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nicolas Pitre <nicolas.pitre@...aro.org>,
        "Luis R . Rodriguez" <mcgrof@...e.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Ulf Magnusson <ulfalizer@...il.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Pavel Machek <pavel@....cz>,
        linux-s390 <linux-s390@...r.kernel.org>,
        Jiri Kosina <jkosina@...e.cz>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 7/7] Test stackprotector options in Kconfig to kill CC_STACKPROTECTOR_AUTO

On Fri, Feb 9, 2018 at 3:19 AM, Masahiro Yamada
<yamada.masahiro@...ionext.com> wrote:
> Add CC_HAS_STACKPROTECTOR(_STRONG) and proper dependency.
>
> I re-arranged the choice values, _STRONG, _REGULAR, _NONE in this order
> because the default of choice is the first visible symbol.
> [...]
> +# is this necessary?
> +#ifeq ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
> +#KBUILD_CFLAGS += -fno-stack-protector
> +#endif

Yes, and also in the case of a broken stack protector, because some
compilers enable stack protector by default, so if we've selected it
to be NONE or detected it as broken, we need to force it off in the
compiler.

> +# TODO: run scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh from Kconfig

FWIW, this is the part that I got stuck on.
gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh depends on the KBUILD
flags that got built up and detected up to this point in the Makefile,
so I couldn't find a way to run it out of Kconfig since it didn't know
what the KBUILD flags were yet.

> +
>  ifeq ($(cc-name),clang)
>  KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
>  KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 76c0b54..50723d8 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -538,10 +538,20 @@ config HAVE_CC_STACKPROTECTOR
>           - its compiler supports the -fstack-protector option
>           - it has implemented a stack canary (e.g. __stack_chk_guard)
>
> +config CC_HAS_STACKPROTECTOR
> +       bool
> +       option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> +
> +config CC_HAS_STACKPROTECTOR_STRONG
> +       bool
> +       option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"

I'm nervous we'll get tripped up here, since $CC may not include the
right $(KBUILD_CPPFLAGS) and $(CC_OPTION_CFLAGS) as in cc-option, both
of which are calculated during the Makefile run. But maybe it won't be
a problem in actual use.

> +
> +config CC_STACKPROTECTOR
> +       bool
> +
>  choice
>         prompt "Stack Protector buffer overflow detection"
>         depends on HAVE_CC_STACKPROTECTOR
> -       default CC_STACKPROTECTOR_AUTO
>         help
>           This option turns on the "stack-protector" GCC feature. This
>           feature puts, at the beginning of functions, a canary value on
> @@ -551,26 +561,10 @@ choice
>           overwrite the canary, which gets detected and the attack is then
>           neutralized via a kernel panic.
>
> -config CC_STACKPROTECTOR_NONE
> -       bool "None"
> -       help
> -         Disable "stack-protector" GCC feature.
> -
> -config CC_STACKPROTECTOR_REGULAR
> -       bool "Regular"
> -       help
> -         Functions will have the stack-protector canary logic added if they
> -         have an 8-byte or larger character array on the stack.
> -
> -         This feature requires gcc version 4.2 or above, or a distribution
> -         gcc with the feature backported ("-fstack-protector").
> -
> -         On an x86 "defconfig" build, this feature adds canary checks to
> -         about 3% of all kernel functions, which increases kernel code size
> -         by about 0.3%.
> -
>  config CC_STACKPROTECTOR_STRONG
>         bool "Strong"
> +       depends on CC_HAS_STACKPROTECTOR_STRONG
> +       select CC_STACKPROTECTOR
>         help
>           Functions will have the stack-protector canary logic added in any
>           of the following conditions:
> @@ -588,11 +582,25 @@ config CC_STACKPROTECTOR_STRONG
>           about 20% of all kernel functions, which increases the kernel code
>           size by about 2%.
>
> -config CC_STACKPROTECTOR_AUTO
> -       bool "Automatic"
> +config CC_STACKPROTECTOR_REGULAR
> +       bool "Regular"
> +       depends on CC_HAS_STACKPROTECTOR
> +       select CC_STACKPROTECTOR
> +       help
> +         Functions will have the stack-protector canary logic added if they
> +         have an 8-byte or larger character array on the stack.
> +
> +         This feature requires gcc version 4.2 or above, or a distribution
> +         gcc with the feature backported ("-fstack-protector").
> +
> +         On an x86 "defconfig" build, this feature adds canary checks to
> +         about 3% of all kernel functions, which increases kernel code size
> +         by about 0.3%.
> +
> +config CC_STACKPROTECTOR_NONE
> +       bool "None"
>         help
> -         If the compiler supports it, the best available stack-protector
> -         option will be chosen.
> +         Disable "stack-protector" GCC feature.
>
>  endchoice

I continue to love the idea, but we can't know a given ssp option is
_working_ until we run the test script, which may depend on compiler
flags. Regardless, I'll give this series a try and see if I can fix
anything I trip over. I've got a lot of notes on testing after getting
..._AUTO working. Whatever happens, I hugely prefer having the
automatic selection possible in the Kconfig! Thanks for working on
this! :)

-Kees

-- 
Kees Cook
Pixel Security

Powered by blists - more mailing lists