[PATCH] D119296: KCFI sanitizer

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

Please don't land the patch this state. There are several questions which should be sorted out first.

a) Naming

About the 'k' prefix: this is generic and does not need to be coupled with "kernel",
but perhaps an argument can be made that the 'k' does not need to refer to "kernel".

b) `-fsanitize=function` (D1338 <https://reviews.llvm.org/D1338>)

It is a very similar feature but uses prolog data (after the function label)
instead of prefix data (before the function label), with some limitation that
only available on x86 and restricted to C++.
I am thinking whether we can lift the C++ restriction (we'll need to move away from
`CGM.GetAddrOfRTTIDescriptor`) and switch to prefix data (easier to add more ports). @pcc

c) inliner

Add this change to enabling inlining

  --- i/llvm/lib/Transforms/Utils/InlineFunction.cpp
  +++ w/llvm/lib/Transforms/Utils/InlineFunction.cpp
  @@ -1776,6 +1776,8 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
           continue;
         if (Tag == LLVMContext::OB_clang_arc_attachedcall)
           continue;
  +      if (Tag == LLVMContext::OB_kcfi)
  +        continue;
   
         return InlineResult::failure("unsupported operand bundle");
       }

d) x86-64 scheme

Why is the following scheme picked? How are the double-int3 before and after movl useful?

          .type   test, at function
          .globl  __cfi_test                      # @test
          .type   __cfi_test, at function
  __cfi_test:
          int3
          int3
          movl    $917620134, %eax                # imm = 0x36B1C5A6
          int3
          int3

e) `__kcfi_typeid_*`

This deserves a better description in the summary.

Is it useful to suppress the typeid symbol for `_Z` (if some C++ projects want to use this feature)? A mangled name has encoded the type information.

IIUC the current scheme only works when two TUs have address-taken declarations of the same name but with different signatures,
and done by a linker warning.

ELF linkers don't error for two `SHN_ABS` `STB_GLOBAL` symbols of the same `st_value`.
When the `st_value` fields differ, there will be a diagnostic. If needed, the specialized diagnostic can be added there.
But I'd prefer the lld patch to be separate and be properly tested (I add myself as a reviewer for lld/ELF changes to catch possibly unintended usage, sorry!)

  % cat a.s
  .globl foo
  .set foo, 1
  % cat b.s
  .globl foo
  .set foo, 2
  % ld.lld a.o b.o
  ld.lld: error: duplicate symbol: foo
  >>> defined in a.o
  >>> defined in b.o



================
Comment at: clang/docs/ControlFlowIntegrity.rst:314
+
+KCFI, enabled by ``-fsanitize=kcfi``, is an alternative indirect call
+control-flow integrity scheme designed for low-level system software, such
----------------
Don't repeat the heading.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2269
+  for (const char &C : Name) {
+    if (llvm::isAlnum(C) || C == '_' || C == '.')
+      continue;
----------------
```
if (!(llvm::isAlnum(C) || C == '_' || C == '.'))
  return false;
```


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6688
+                                            StringRef Suffix,
+                                            bool OnlyExternal /*=true*/) {
   if (auto *FnType = T->getAs<FunctionProtoType>())
----------------
T


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1421
+  /// Set type hash as prefix data to the given function
+  void SetKCFITypePrefix(const FunctionDecl *FD, llvm::Function *F);
+
----------------
Unlikely `Emit*`, there are many `set*` functions which use the correct case, so there is no excuse using the wrong case.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:707
+      // With -fpatchable-function-entry=N,M, where M > 0,
+      // llvm::AsmPrinter::emitFunctionHeader injects nops before before the
+      // KCFI type identifier, which is currently unsupported.
----------------



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1339
+
+  if (Section) {
+    OutStreamer->PushSection();
----------------
early return


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1345
+    OutStreamer->emitLabel(Loc);
+    OutStreamer->emitAbsoluteSymbolDiff(Symbol, Loc, 4);
+
----------------
Use getCodePointerSIze().

A 4-byte PC-relative relocation is insufficient if text and data are more than 31-bit away.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1130
+  const MCSectionELF &ElfSec = static_cast<const MCSectionELF &>(TextSec);
+  unsigned Flags = ELF::SHF_LINK_ORDER | ELF::SHF_ALLOC | ELF::SHF_WRITE;
+  StringRef GroupName;
----------------
Remove SHF_WRITE. The section doesn't need a dynamic relocation.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:135
+  if (MAI->hasDotTypeDotSizeDirective()) {
+    MCSymbol *EndSym = OutContext.createTempSymbol("__cfi_func_end");
+    OutStreamer->emitLabel(EndSym);
----------------
For a temporary symbol (STB_LOCAL), no need to add `__` prefix.


================
Comment at: llvm/test/CodeGen/AArch64/kcfi-bti.ll:4
+; RUN: llc -mtriple=aarch64-- -stop-after=finalize-isel < %s | FileCheck %s --check-prefixes=MIR,FINAL
+
+; ASM:       .word 12345678
----------------
Add a test with `"patchable-function-entry"="2"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296