Messages in this thread | | | Date | Tue, 14 Sep 2021 23:24:51 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v3 11/16] x86/purgatory: Disable CFI |
| |
On Tue, Sep 14, 2021 at 03:31:14PM -0700, Nick Desaulniers wrote: > On Tue, Sep 14, 2021 at 1:30 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Tue, Sep 14, 2021 at 1:02 PM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > > > > On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. > > > > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > > > I kind of prefer the existing convention that has explicit guards on > > > specific configs (ie. CONFIG_FUNCTION_TRACER, CONFIG_STACKPROTECTOR, > > > CONFIG_STACKPROTECTOR_STRONG, CONFIG_RETPOLINE); it's more obvious > > > which configs may introduce which flags that are problematic. This > > > patch is ok as is, but it kind of makes this Makefile more > > > inconsistent. I would prefer we had the explicit checks.
Can you explain your reasoning a bit more? It seems like redundant open-coded logic to me, but I do see this idiom repeated in the kernel. And/or maybe I've misunderstood you?
It seems like it's better to have a single variable (like in the proposed patch: CC_FLAGS_CFI) that has all the details internal -- no tests needed.
i.e.: instead of this in many places:
ifdef CONFIG_FEATURE PURGATORY_CFLAGS_REMOVE += -feature-flag endif
do this once:
CC_FEATURE_CFLAGS := -feature-flag ... KBUILD_CFLAGS += $(CC_FEATURE_CFLAGS)
and only repeat a single line in for targets:
CFLAGS_REMOVE_target.o += $(CC_FEATURE_CFLAGS)
> > > > The Makefile does already use DISABLE_STACKLEAK_PLUGIN in a similar > > way, but I don't have a strong preference here. > > mmm...DISABLE_STACKLEAK_PLUGIN adds to PURGATORY_CFLAGS. This patch > adds to PURGATORY_CFLAGS_REMOVE.
CFI is "simple" in that regard; its options can just be left off. This isn't true for some weirder stuff. Stack protector is a good one, in that just removing the options may not disable it depending on distro patches (which may turn it on by default), so both target_CFLAGS and target_REMOVE are needed there.
(In the case of the plugins, yes, I think they could be rearranged to use the target_REMOVE method, but I have a memory of REMOVE not working there for some weird thing? Hmm.)
-- Kees Cook
| |