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

Andy Lutomirski
2021-11-01T04:13:13Z

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.