Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 3 May 2022 15:02:44 -0700
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Joao Moreira <joao@overdrivepizza.com>
Cc:     linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
        peterz@infradead.org, andrew.cooper3@citrix.com,
        keescook@chromium.org, samitolvanen@google.com,
        mark.rutland@arm.com, hjl.tools@gmail.com,
        alyssa.milburn@linux.intel.com, ndesaulniers@google.com,
        gabriel.gomes@linux.intel.com, rick.p.edgecombe@intel.com
Subject: Re: [RFC PATCH 01/11] x86: kernel FineIBT
Message-ID: <20220503220244.vyz5flk3gg3y6rbw@treble>
References: <20220420004241.2093-1-joao@overdrivepizza.com>
 <20220420004241.2093-2-joao@overdrivepizza.com>
 <20220429013704.4n4lmadpstdioe7a@treble>
 <d82459b887bcaf9181ad836051e2d16b@overdrivepizza.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <d82459b887bcaf9181ad836051e2d16b@overdrivepizza.com>
Precedence: bulk

On Mon, May 02, 2022 at 10:17:42AM -0700, Joao Moreira wrote:
> > > +	asm("\t movq 0x90(%%rsp),%0" : "=r"(ret));
> > > +	asm("\t movq 0x98(%%rsp),%0" : "=r"(caller));
> > 
> > This is making some questionable assumptions about the stack layout.
> > 
> > I assume this function is still in the prototype stage ;-)
> 
> Yeah, this is just a messy instrumentation to get reports about mismatching
> prototypes (as the ones reported further down the series).
> 
> The issue with having the call is that it bloats the binary, so the ud2 is
> 3-bytes-per-function better. Yet, we may consider a FINEIBT_DEBUG config,
> which can then enable a handler. This would be useful together with a fuzzer
> or a stress tool to cover possible control-flow paths within the kernel and
> map mismatching prototypes more properly I guess.

It should be possible to have a non-fatal #UD2 handler.

See for example how WARN() is implemented with __WARN_FLAGS in
arch/x86/include/asm/bug.h .

So hopefully we can just get rid of the need for the "call handler"
thing altogether.

> > Not sure what would happen for "ibt=off"?  Maybe apply_ibt_endbr() could
> > NOP out all the FineIBT stuff.
> 
> Either that, or...
> 
> I'm thinking about a way to have FineIBT interchangeable with KCFI.
> Currently KCFI adds a 4 byte hash + 2 byte nops before function entry, to
> allow for proper prototype checking. After that, there should be an ENDBR of
> 4 bytes. This gives us 10 bytes in total. Then, my yet to be properly
> thought idea would be patch these 10 bytes with:
> 
> endbr
> call fineibt_handler_<$HASH>
> nop
> 
> and then, on the caller side, patch the "cmp <$HASH>, -0x6(%r11); je; ud2;
> call" sequence with a "sub 0x6, r11; mov $HASH, %r10; call %r11, add 0x6
> %r11". This would then allow the kernel to verify if the CPU is IBT capable
> on boot time and only then setting the proper scheme.
> 
> The downsides of having something like this would be that this sub r11/add
> r11 sequence is kinda meh. We can avoid that by having two padding nops
> after the original ENDBR, which will be skipped when the function is reached
> directly by the linker optimization I'm working on, and that we can convert
> into a JMP -offset that makes control flow reach the padding area before the
> prologue and from where we can call the fineibt_handler function. The
> resulting instrumentation would be something like:
> 
> 1:
> call fineibt_handler_<$HASH>
> jmp 2f
> <foo>
> endbr
> jmp 1b
> 2:
> 
> Also, it would prevent a paranoid user to have both schemes simultaneously
> (there are reasons why people could want that).
> 
> Any thoughts?

I'm not really qualified to comment on this too directly since I haven't
looked very much at the variations on FineIBT/CFI/KCFI, and what the
protections and drawbacks are for each approach, and when it might even
make sense to combine them for a "paranoid user".

Since we have multiple similar and possibly competing technologies being
discussed, one thing I do want to warn against is that we as kernel
developers tend to err on the side of giving people too many choices and
combinations which *never* get used.

All those unused options can confuse the user and significantly add to
complexity and maintenance overhead for us.  Especially for invasive
features like these.

(Just to be clear, I'm not saying that's happening here, but it's
something we need to be careful about.)

Here, documentation is going to be crucial, for both reviewers and
users.  Something that describes when/why I should use X or Y or X+Y.

If we truly want to add more options/combos for different use cases then
we'll also need clear and concise documentation about which
options/combos would be used under what circumstances.

-- 
Josh