Re: [PATCH 05/15] x86: Implement function_nocfi

[Kees Cook]

On Fri, Apr 16, 2021 at 03:52:44PM -0700, Andy Lutomirski wrote:
> Maybe ABI is the wrong word, or maybe I'm not fully clued in.  But, if I do:
> 
> extern void call_it(void (*ptr)(void));
> 
> and I define call_it in one translation unit and call it from another,
> the ABI effectively changed, right?  Because ptr is (depending on the
> "canonical" mode) no longer a regular function pointer.

Right, I was thinking maybe "calling convention", or really, "the
ability to still use 'ptr' as if it were a function". Which is true,
yes. It's just that 'ptr' is aimed at a jump table that jumps to the
"true" function body.

> 1. I defined a function in asm.  I want to tell clang that this
> function is defined in asm, and for clang to behave accordingly:
> 
> .globl func
> func:
>  ; do stuff
> 
> later:
> 
> extern void func(void) [something here];
> 
> There really should be a way to write this correctly such that it
> works regardless of the setting of
> -fsanitize-cfi-canonical-jump-tables.  This should not bypass CFI.  It
> should *work*, with CFI enforced.  If I read all the various things
> you linked correctly, this would be something like __cfi_noncanonical,
> and I reserve the right to think that this is a horrible name.

Yes, I find the name confusing too. Without noncanonical, we'd need
C call wrappers for every single .S function that had its address
taken. This is very common in crypto, for example. That level of extra
code seemed like a total non-starter. Instead, we just get a few places
we have to mark.

> 2. I need a raw function pointer, thank you very much.  I would like
> to be honest about it, and I don't really want to bypass CFI, but I
> need the actual bits in the actual symbol.
> 
> translation unit 1 defines func.  Maybe it's C with
> -fsanitize-cfi-canonical-jump-tables, maybe it's C with
> -fno-sanitize-cfi-canonical-jump-tables or however it's spelled, and
> maybe it's plain asm.  Now translation unit 2 does:
> 
> 2a. Uses a literal symbol, because it's going to modify function text
> or poke an MSR or whatever:
> 
> wrmsrl(MSR_WHATEVER, func);
> 
> clang needs to give us *some* way to have a correct declaration of
> func such that we can, without resorting to inline asm kludges, get
> the actual bit pattern of the actual symbol.

We don't want version of a global symbol alias of func that points to
the function body, though; it's only very specific cases where this
should be stripped (MSR, ftrace, etc).

So, if there were some Clang-specific syntax for this, it would still be
used on a case-by-case basis. It would still be something like:

wrmsrl(MSR_WAT, __builtin_gimme_body_p(func));

Which is basically what already exists, just with a different name.

> 2b. Maybe optional: convert from function pointer to bit pattern of
> actual symbol.
> 
> If someone gives me a real, correctly typed C pointer representing a
> function pointer, I want a way to find the address of the body of the
> function.  Then we can use it for things that aren't *calling* it per
> se, e.g. disassembling it.  This is not necessarily a fully formed
> thought right now, but I think something along these lines might be
> needed.
> 
> The reverse of 2b (converting from actual symbol to function pointer)
> might be equivalent to 1, or it might not.  I suppose there are some
> subtleties here.
> 
> Be that as it may, it sounds like right now clang has some issues
> interoperating with asm when CFI is enabled.  If so, clang needs to be
> improved.
> 
> (The unsigned long hack is not necessarily good enough.  I should be able to do:
> 
> .global func
> func:
>  ; C ABI compliant code here
> 
> extern void func(void) [attribute as in 1]
> 
> unsigned long actual_address = [something clang actually understands](func);
> 
> If this thing refuses to work when fed a nonconstant function pointer
> because of some genuinely good reason, fine.  But, if 'func' is an
> actual literal symbol name, this thing needs to be compile-time
> constant expression.

Okay, you're saying you want __builtin_gimme_body_p() to be a constant
expression for the compiler, not inline asm?

Given the very few places this is expected to be used, and the fact that
it works as-is already, why is this additional requirement useful?

> > So, instead of a cast, a wrapper is used to bypass instrumentation in
> > the very few cases its needed.
> 
> NAK.  The compiler needs to cooperate IMO.

It's trying very hard. ;)

> > But note that this shouldn't turn into a discussion of "maybe Clang could
> > do CFI differently"; this is what Clang has.
> >
> > https://clang.llvm.org/docs/ControlFlowIntegrity.html
> 
> If this is what Clang has, and Clang won't improve, then we can just
> not apply these patches...

I'm not saying Clang can't change -- I'm saying redesigning the entire
implementation of Clang's CFI isn't feasible, and I want to avoid having
that become the requirement because that's unreasonable. Clang's current
CFI works for many other projects, it's supported, it's what Android
has been using on its kernels 3 years now. The twist, obviously, is that
other projects don't use asm the way the kernel does, so that's where
things get weird, and where we've already been getting help from LLVM
folks to improve the situation.

If the solution is a new Clang builtin, okay, but I'd just like to
understand why that's justified compared to the existing solution
(especially since the resulting machine code is likely to be nearly
identical in the current uses).

-Kees

-- 
Kees Cook