lkml.org 
[lkml]   [2021]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2021-10-27 23:29    [W:4.327 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site