Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 10 Feb 2018 06:48:43 +0100
From:   Ulf Magnusson <ulfalizer@gmail.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     Masahiro Yamada <yamada.masahiro@socionext.com>,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Nicolas Pitre <nicolas.pitre@linaro.org>,
        "Luis R . Rodriguez" <mcgrof@suse.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        Sam Ravnborg <sam@ravnborg.org>,
        Michal Marek <michal.lkml@markovi.net>,
        Martin Schwidefsky <schwidefsky@de.ibm.com>,
        Pavel Machek <pavel@ucw.cz>,
        linux-s390 <linux-s390@vger.kernel.org>,
        Jiri Kosina <jkosina@suse.cz>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 4/7] kconfig: support new special property shell=
Message-ID: <20180210054843.z3g7wvcmlccvww3h@huvuddator>
References: <1518106752-29228-1-git-send-email-yamada.masahiro@socionext.com>
 <1518106752-29228-5-git-send-email-yamada.masahiro@socionext.com>
 <20180209053038.pscoijvowmyudyzf@huvuddator>
 <CAK7LNATRxYCx9sE1haw815votyA5K8GQrMPPh0Q+n3kYa=JsgA@mail.gmail.com>
 <20180209124607.akjhncb5sempjqcn@huvuddator>
 <CAGXu5j+hv6G3m+fgr7ibXAnoE-2GsxtXoqr0pd5AaaNdFVQxYg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAGXu5j+hv6G3m+fgr7ibXAnoE-2GsxtXoqr0pd5AaaNdFVQxYg@mail.gmail.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@gmail.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
	
	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?

Cheers,
Ulf