[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D119296#3467905 <https://reviews.llvm.org/D119296#3467905>, @samitolvanen wrote:

> In D119296#3466027 <https://reviews.llvm.org/D119296#3466027>, @joaomoreira wrote:
>
>> I played a little bit with kcfi and here are some thoughts:
>>
>> - under -Os I saw functions being inlined, regardless of the source code calling them indirectly. In these scenarios, the KCFI check was still in place, even though there was not a pointer involved in the call. Although not semantically incorrect, it would be great to prevent the unnecessary overhead (see attached source/compile it with -Os and -fsanitize=kcfi).
>
> Yes, I suspect this might be an issue with Clang's existing CFI schemes too, and would probably require an additional pass to drop checks before calls that were either inlined or optimized into direct calls.

IIRC, this wasn't really an issue for -fsanitize=cfi because 99% of the time indirect calls that were going to resolve to direct calls had already done so by the time we got to the LowerTypeTests pass (which was only run during LTO).

It would probably be enough to have a pass at the end of the target-independent pipeline like you're doing here. You can probably check how effective it is by experimentally adding an assertion or something to the backend code so it complains if a direct call is found.

I probably wouldn't call the pass "KCFI" though as that implies that the pass itself implements KCFI. Maybe something like KCFIRemoveChecks would be better. Or maybe this is simple enough to add to another pass like InstCombine instead of adding a new one.

(That's if you don't go for the operand bundle approach described below.)

>> - I noticed that KCFI checks are placed much before the indirect call arguments are properly placed in the passing registers. This makes the position of the check unpredictable. I assume this is a bad thing in case the kernel eventually decides to come up with an approach that uses alternatives CFI schemes through text-patching.
>
> It shouldn't matter for text patching as we'll still know the exact location of the instruction sequence that we want to patch by looking at the `.kcfi_traps` section.
>
>> Because of things like the above, in the past I decided to implement these things in the very backend of the compiler, so other optimizations would not break the layout nor leave dummy checks around. I find it nice to have this implemented as a more architecture general feature, but maybe it would be cool to have a finalization pass in the X86 backend just to tie things.
>
> @pcc, any thoughts about these issues?

This seems like a reasonable approach, and was also the approach taken for the PAuth ABI. The PAuth ABI attaches an operand bundle to the call instruction and arranges for the code for the check to be generated together with the call. This helps with avoiding spills of the verified function pointer between the check and the call. The code isn't upstream but is available on this branch: https://github.com/pcc/llvm-project/tree/apple-pac4

Grep for something like `undle.*ptrauth` and you should find the relevant code.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2287
+    const Twine &Asm = ".weak __kcfi_typeid_" + Name + "\n" +
+                       ".set __kcfi_typeid_" + Name + ", " +
+                       Twine(Id->getZExtValue()) + "\n";
----------------
Won't this bloat symbol tables with mostly unused entries? I was thinking you could make them hidden, but that wouldn't have an effect on kernel modules. Maybe you should have this be opt-in with an attribute and have the kernel's asmlinkage expand to the attribute.


================
Comment at: llvm/lib/Transforms/IPO/KCFI.cpp:62
+
+    auto *Target = dyn_cast_or_null<Function>(CI->getArgOperand(0));
+    if (!Target || !Target->hasFnAttribute("kcfi") || !Target->hasPrefixData())
----------------
Should it look through bitcasts? (Probably moot with the imminent switch to opaque pointers though.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119296/new/

https://reviews.llvm.org/D119296