Re: [PATCH v5 00/15] x86: Add support for Clang CFI

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Oct 27, 2021 at 10:11:28AM -0700, Kees Cook wrote:
> On Wed, Oct 27, 2021 at 03:04:55PM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 27, 2021 at 02:48:52PM +0200, Peter Zijlstra wrote:
> > > On Wed, Oct 27, 2021 at 02:22:27PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 27 Oct 2021 at 14:05, Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > > > > Should not this jump-table thingy get converted to an actual function
> > > > > > address somewhere around arch_static_call_transform() ? This also seems
> > > > > > relevant for arm64 (which already has CLANG_CFI supported) given:
> > > > > >
> > > > > >   https://lkml.kernel.org/r/20211025122102.46089-3-frederic@kernel.org
> > > > >
> > > > > Ugh, yeah, we'll need to do the function_nocfi() dance somewhere...
> > > > >
> > > > 
> > > > Sadly, that only works on symbol names, so we cannot use it to strip
> > > > CFI-ness from void *func arguments passed into the static call API,
> > > > unfortunately.
> > > 
> > > Right, and while mostly static_call_update() is used, whcih is a macro
> > > and could possibly be used to wrap this, we very much rely on
> > > __static_call_update() also working without that wrapper and then we're
> > > up a creek without no paddles.
> > 
> > Specifically, we support code like:
> > 
> > struct foo {
> > 	void (*func1)(args1);
> > 	void (*func2)(args2);
> > }
> > 
> > struct foo global_foo;
> 
> And global_foo is intentionally non-const?

Yep, since depending on the init function it can discover and stuff in
a wild variety of functions.

> > 
> > ...
> > 
> > DEFINE_STATIC_CALL_NULL(func1, *global_foo.func1);
> > 
> > ...
> > 
> > __init foo_init()
> > {
> > 	// whatever code that fills out foo
> > 
> > 	static_call_update(func1, global_foo.func1);
> > }
> > 
> > ...
> > 
> > hot_function()
> > {
> > 	...
> > 	static_cal(func1)(args1);
> > 	...
> > }
> > 
> > cold_function()
> > {
> > 	...
> > 	global_foo->func1(args1);
> > 	...
> > }
> 
> If global_foo is non-const, then the static call stuff is just an
> obfuscated indirect call.

It is not. The target is determined once, at boot time, depending on the
hardware, it then never changes. The static_call() results in a direct
call to that function.

> The attack CFI attempts to block is having
> a controlled write flaw turn into controlled execution. For example,
> in the above, an attacker would use a flaw that could aim a write to
> global_foo->func1, and then get the kernel to take an execution path
> that executes global_foo->func1 (i.e. through cold_function).

I know, and CFI works for cold_function.

> If static_call_update() accepts an arbitrary function argument, then it
> needs to perform the same validation that is currently being injected
> at indirect call sites to avoid having static calls weaken CFI.

static_call_update() is a macro and has compile time signature checks,
the actual function is __static_call_update() and someone can go add
extra validation in there if they so desire.

I did have this patch:

  https://lkml.kernel.org/r/20210904105529.GA5106@worktop.programming.kicks-ass.net

but I never did get around to finishing it. Although looking at it now,
I suppose static_call_seal() might be a better name.

And you're worried about __static_call_update() over text_poke_bp()
because?

> Getting a "jump table to actual function" primitive only avoids the added
> jump -- all the CFI checking remains bypassed.

Exactly, so the extra jump serves no purpose and needs to go. Doubly so
because people are looking at static_call() to undo some of the
performance damage introduced by CFI :-)

> If static call function
> address storage isn't const, for CFI to work as expected the update()
> routine will need to do the same checking that is done at indirect call
> sites when extracting the "real" function for writing into a direct call.

I've mentioned static_call like a hundred times in these CFI threads..
if you want to do CFI on them, go ahead. I'm just not sure the C type
system is up for that, you'll have to somehow frob the signature symbol
into __static_call_update(), something like __builtin_type_symbol().

> To avoid all of this, though, it'd be better if static calls only
> switched between one of a per-site const list of possible functions,
> which would make it a much tighter hand-rolled CFI system itself. :)
> (i.e. it would operate from a specific and short list of expected
> functions rather than the "best effort" approach of matching function
> prototypes as done by Clang CFI.)

That sounds like a ton of painful ugly.