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

[Andy Lutomirski <luto@kernel.org>]

On 10/27/21 15:27, Kees Cook wrote:
> On Wed, Oct 27, 2021 at 11:21:26PM +0200, Peter Zijlstra wrote:
>> 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:
>>>> [...]
>>>> 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.
> 
> Right, I mean it is _effectively_ an indirect call in the sense that the
> call destination is under the control of a writable memory location.
> Yes, static_call_update() must be called to make the change, hence why I
> didn't wave my arms around when static_call was originally added. It's a
> net benefit for the kernel: actual indirect calls now become updatable
> direct calls. This means reachability becomes much harder for attackers;
> I'm totally a fan. :)

I think this means that function_nocfi() is the wrong abstraction.  To 
solve this properly, we want (sorry for fake C++ concept-ish syntax, but 
this would be a builtin):

template<function_pointer T> uintptr_t cfi_check_get_raw_address(T ptr);

You pass in a function pointer and you get out the address of the actual 
function body.  And this *also* emits the same type checking code that 
an actual call would emit.  So:

void (*ptr)(void);

ptr();

and:

void (*ptr)(void);

asm volatile ("call *%0" :: "rmi" (cfi_check_get_raw_address(ptr)));

would emit comparably safe code, except that the latter would actually 
read the code bytes and the latter is missing clobbers and such.  And 
yes, this means that, without special support, the jump table can't live 
in XO memory.

void foo(void);
int (*ptr)(void) = (void *)foo;
cfi_check_get_raw_address(ptr);

would crash due to a CFI violation.

For what it's worth, I feel fairly strongly that we should not merge 
half-baked CFI support.  Years ago, we merged half-baked stack protector 
support in which we shoehorned in a totally inappropriate x86_32 gcc 
implementation into the kernel, and the result was a mess.  It's 
*finally* cleaned up, and that only happened because we got gcc to add 
the actual required support, then we waited a while, and then we dropped 
stack protector support for old gcc versions.  Yuck.