Re: [RFC PATCH 7/7] Test stackprotector options in Kconfig to kill CC_STACKPROTECTOR_AUTO

[Masahiro Yamada <yamada.masahiro@socionext.com>]

2018-02-09 3:30 GMT+09:00 Kees Cook <keescook@chromium.org>:
> On Fri, Feb 9, 2018 at 3:19 AM, Masahiro Yamada
> <yamada.masahiro@socionext.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.


SRCARCH is fixed when loading Kconfig files.

BITS is derived from CONFIG_64BIT.

config 64BIT
        bool "64-bit kernel" if ARCH = "x86"
        default ARCH != "i386"
        ---help---
          Say yes to build a 64-bit kernel - formerly known as x86_64
          Say no to build a 32-bit kernel - formerly known as i386


This is a more difficult part because users can toggle this option
from menuconfig, etc.

If this option is changed, the compiler options must be re-computed,
i.e. system() must be called again.

This is missing in my first draft.

I have not checked how slow it is.




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


Right, I had noticed this is a problem, but not implemented yet.

At least, some basic compiler options must be imported into Kconfig.

Especially this is a problem for clang.
One clang executable is built with lots of
architecture back-ends.
So,
CLANG_TARGET    := --target=$(notdir $(CROSS_COMPILE:%-=%))
etc. is mandatory.


If I remember correctly, there existed some options
that depend on others.

I am not sure about the stackprotector case.



>> +
>> +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! :)

Yes, your help is appreciated.
We will find more TODO items during trial and error.  :)



-- 
Best Regards
Masahiro Yamada