Messages in this thread | | | Date | Wed, 27 Oct 2021 23:21:26 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v5 00/15] x86: Add support for Clang CFI |
| |
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.
| |