Message-ID: <20180210074924.3nhxsza5zdbaahxx@huvuddator>
Date:   Sat, 10 Feb 2018 08:49:24 +0100
From:   Ulf Magnusson <ulfalizer@...il.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Linux Kbuild mailing list <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>,
        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>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 4/7] kconfig: support new special property shell=

On Sat, Feb 10, 2018 at 04:12:13PM +0900, Masahiro Yamada wrote:
> 2018-02-10 14:48 GMT+09:00 Ulf Magnusson <ulfalizer@...il.com>:
> > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
> >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@...il.com> wrote:
> >> > One thing that makes Kconfig confusing (though it works well enough in
> >> > practice) is that .config files both record user selections (the saved
> >> > configuration) and serve as a configuration output format for make.
> >> >
> >> > It becomes easier to think about .config files once you realize that
> >> > assignments to promptless symbols never have an effect on Kconfig
> >> > itself: They're just configuration output, intermixed with the saved
> >> > user selections.
> >> >
> >> > Assume 'option env' symbols got written out for example:
> >> >
> >> >         - For a non-user-assignable symbol, the entry in the .config
> >> >           file is just configuration output and ignored by Kconfig,
> >> >           which will fetch the value from the environment instead.
> >> >
> >> >         - For an assignable 'option env' symbol, the entry in the
> >> >           .config file is a saved user selection (as well as
> >> >           configuration output), and will be respected by Kconfig.
> >>
> >> In the stack-protector case, this becomes quite important, since the
> >> goal is to record the user's selection regardless of compiler
> >> capability. For example, if someone selects _REGULAR, it shouldn't
> >> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
> >> way to pick "best possible for this compiler", though. If a user had
> >> previously selected _STRONG but they're doing builds with an older
> >> compiler (or a misconfigured newer compiler) without support, the goal
> >> is to _fail_ to build, not silently select _REGULAR.
> >>
> >> So, in this case, what's gained is the logic for _AUTO, and the logic
> >> to not show, say, _STRONG when it's not available in the compiler. But
> >> we must still fail to build if _STRONG was in the .config. It can't
> >> silently rewrite it to _REGULAR because the compiler support for
> >> _STRONG regressed.
> >>
> >> -Kees
> >>
> >> --
> >> Kees Cook
> >> Pixel Security
> >
> > Provided that would be the desired behavior:
> >
> > What about changing the meaning of the choice symbols from e.g. "select
> > -fstack-protector-strong" to "want -fstack-protector-strong"? Then the
> > user preference would always be remembered, regardless of what's
> > available.
> >
> > Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
> > fits pretty well here, since it works like a dependency-respecting
> > select.
> >
> >         config CC_HAS_STACKPROTECTOR_STRONG
> >                 bool
> >                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> >
> >         config CC_HAS_STACKPROTECTOR
> >                 bool
> >                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> >
> >
> >         choice
> >                 prompt "Stack Protector buffer overflow detection"
> >                 default WANT_CC_STACKPROTECTOR_STRONG
> >
> >         config WANT_CC_STACKPROTECTOR_STRONG
> >                 bool "Strong"
> >                 imply CC_STACKPROTECTOR_STRONG
> >
> >         config WANT_CC_STACKPROTECTOR_REGULAR
> >                 bool "Regular"
> >                 imply CC_STACKPROTECTOR_REGULAR
> >
> >         config WANT_CC_STACKPROTECTOR_NONE
> >                 bool "None"
> >                 imply CC_STACKPROTECTOR_NONE
> >
> >         endchoice
> >
> >
> >         config CC_STACKPROTECTOR_STRONG
> >                 bool
> >                 depends on CC_HAS_STACKPROTECTOR_STRONG
> 
> 
> Do you mean
> 
>          config CC_STACKPROTECTOR_STRONG
>                  bool
>                  depends on CC_HAS_STACKPROTECTOR_STRONG && \
>                             WANT_CC_STACKPROTECTOR_STRONG
> 
> or, maybe
> 
> 
>          config CC_STACKPROTECTOR_STRONG
>                  bool
>                  depends on CC_HAS_STACKPROTECTOR_STRONG
>                  default WANT_CC_STACKPROTECTOR_STRONG
> 
> ?

With the 'imply', it should work with just the 'depends on'. I had your
last version earlier though, and it works too.

'imply' kinda makes sense, as in "turn on the strong stack protector if
its dependencies are satisfied".

> 
> 
> 
> 
> 
> >         config CC_STACKPROTECTOR_REGULAR
> >                 bool
> >                 depends on CC_HAS_STACKPROTECTOR_REGULAR
> >
> >         config CC_STACKPROTECTOR_NONE
> >                 bool
> >
> > This version has the drawback of always showing all the options, even if
> > some they wouldn't be available. Kconfig comments could be added to warn
> > if an option isn't available at least:
> >
> >         comment "Warning: Your compiler does not support -fstack-protector-strong"
> >                 depends on !CC_HAS_STACKPROTECTOR_STRONG
> >
> >         config WANT_CC_STACKPROTECTOR_STRONG
> >                 ...
> >
> >
> >         comment "Warning: Your compiler does not support -fstack-protector"
> >                 depends on !CC_HAS_STACKPROTECTOR_REGULAR
> >
> >         config WANT_CC_STACKPROTECTOR_REGULAR
> >                 ...
> >
> > This final comment might be nice to have too:
> >
> >         comment "Warning: Selected stack protector not available"
> >                 depends on !(CC_STACKPROTECTOR_STRONG ||
> >                              CC_STACKPROTECTOR_REGULAR ||
> >                              CC_STACKPROTECTOR_NONE)
> >
> > Should probably introduce a clear warning that tells the user what they
> > need to change in Kconfig if they build with a broken selection too.
> >
> >
> > CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
> > way too. Maybe there's something neater.
> >
> >         config CC_STACKPROTECTOR_AUTO
> >                 bool "Automatic"
> >                 imply CC_STACKPROTECTOR_STRONG
> >                 imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
> >                 imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
> >                                                    !CC_HAS_STACKPROTECTOR_REGULAR
> >
> >
> > Another drawback of this approach is that it breaks existing .config
> > files (the CC_STACKPROTECTOR_* settings are ignored, since they just
> > look like "configuration output" to Kconfig now). If that'd be a
> > problem, the old names could be used instead of
> > WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
> > though it'd look a bit cryptic.
> >
> > Ideas?
> >
> 
> 
> 
> FWIW, the following is what I was playing with.
> (The idea for emitting warnings is Ulf's idea)
> 
> 
> ------------------>8-------------------
> config CC
>         string
>         option env="CC"
> 
> 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"
> 
> config CC_HAS_STACKPROTECTOR_NONE
>         bool
>         option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> 
> config CC_STACKPROTECTOR
>         bool
> 
> choice
>         prompt "Stack Protector buffer overflow detection"
> 
> config CC_STACKPROTECTOR_AUTO
>         bool "Auto"
>         select CC_STACKPROTECTOR if (CC_HAS_STACKPROTECTOR || \
>                                      CC_HAS_STACKPROTECTOR_STRONG)

With this approach, I guess you would still need to handle the
CC_STACKPROTECTOR_AUTO logic outside of Kconfig, since e.g.
CC_STACKPROTECTOR_STRONG won't get enabled automatically if supported.

The idea above was to make it "internal" to the Kconfig files (though it
still gets written out), with the
CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} variables automatically getting
set as appropriate.

The build could then the detect if none of
CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} are set and do what's
appropriate (error out in some semi-helpful way or whatever... not
deeply familiar with kernel policy here :).

> 
> config CC_STACKPROTECTOR_REGULAR
>         bool "Regular"
>         select CC_STACKPROTECTOR
> 
> config CC_STACKPROTECTOR_STRONG
>         bool "Strong"
>         select CC_STACKPROTECTOR
> 
> config CC_STACKPROTECTOR_NONE
>         bool "None"
> 
> endchoice
> 
> 
> comment "(WARNING) stackprotecter was chosen, but your compile does
> not support it.  Build will fail"
>         depends on CC_STACKPROTECTOR_REGULAR && \
>                    !CC_HAS_STACKPROTECTOR
> 
> comment "(WARNING) stackprotecter-strong was chosen, but your compile
> does not support it.  Build will fail"
>         depends on CC_STACKPROTECTOR_STRONG && \
>                    !CC_HAS_STACKPROTECTOR_STRONG
> ------------------------->8---------------------------------
> 
> 
> 
> 
> 
> BTW, setting option flags in Makefile is dirty, like follows:
> 
> 
> ccflags-$(CONFIG_CC_STACKPROTECTOR_STRONG)  += -fstack-protector-strong
> ccflags-$(CONFIG_CC_STACKPROTECTOR_REGULAR) += -fstack-protector
> 
> if ($(CONFIG_CC_STACKPROTECTOR_AUTO),y)
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR)        += -fstack-protector
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_STRONG) += -fstack-protector-strong
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> endif
> 
> if ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> endif
> 
> 
> 
> 
> One idea could be to calculate the compiler option in Kconfig.
> 
> config CC_OPT_STACKPROTECTOR
>         string
>         default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG || \
>                                              (CC_STACKPROTECTOR_AUTO && \
>                                               CC_HAS_STACKPROTECTOR_STRONG)
>         default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR || \
>                                               (CC_STACKPROTECTOR_AUTO && \
>                                                CC_HAS_STACKPROTECTOR)
>         default "-fno-stack-protector"        if CC_HAS_STACKPROTECTOR_NONE

If CC_STACKPROTECTOR_AUTO is made "internal", this could be simplified
to something like

	config CC_OPT_STACKPROTECTOR
		string
		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
		# If the compiler doesn't even support
		# -fno-stack-protector
		default ""

(Last default is just to make the empty string explicit. That's the
value it would get anyway.)

> 
> 
> 
> Makefile will become clean.
> Of course, this is at the cost of ugliness in Kconfig.
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Please tell me if I've misunderstood some aspect of the old behavior.

Cheers,
Ulf

Powered by blists - more mailing lists