[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added a comment.

In D119296#3480793 <https://reviews.llvm.org/D119296#3480793>, @pcc wrote:

>> 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).

I also confirmed this, LowerTypeTestsPass drops unneeded checks in a similar way.

> 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.

You're right, InstCombine looks like a good place for this. In my tests, this dropped all the checks from direct calls, including the ones that were inlined.

> 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.

Thanks for the link, I'll take a look.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2287
+    const Twine &Asm = ".weak __kcfi_typeid_" + Name + "\n" +
+                       ".set __kcfi_typeid_" + Name + ", " +
+                       Twine(Id->getZExtValue()) + "\n";
----------------
pcc wrote:
> 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.
This adds ~2k symbols to an arm64 defconfig vmlinux, which is a fair amount, but quite minimal compared to the ~38k jump table symbols we emit with the existing CFI scheme. I did consider using an attribute to reduce the bloat, but since the extra symbols aren't causing any actual issues at the moment, I figured this is something we can optimize later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296