Re: [PATCH v3 11/16] x86/purgatory: Disable CFI

[Kees Cook]

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@xxxxxxxxxx> wrote:
> >
> > On Tue, Sep 14, 2021 at 1:02 PM Nick Desaulniers
> > <ndesaulniers@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@xxxxxxxxxx> wrote:
> > > >
> > > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro.
> > > >
> > > > Signed-off-by: Sami Tolvanen <samitolvanen@xxxxxxxxxx>
> > >
> > > 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