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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202109142257.7BAE8074@keescook>
Date:   Tue, 14 Sep 2021 23:24:51 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Sami Tolvanen <samitolvanen@...gle.com>, X86 ML <x86@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Sedat Dilek <sedat.dilek@...il.com>,
        linux-hardening@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
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@...gle.com> wrote:
> >
> > On Tue, Sep 14, 2021 at 1:02 PM Nick Desaulniers
> > <ndesaulniers@...gle.com> wrote:
> > >
> > > On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@...gle.com> wrote:
> > > >
> > > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro.
> > > >
> > > > Signed-off-by: Sami Tolvanen <samitolvanen@...gle.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