[PATCH] D119296: KCFI sanitizer

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Please add a test where calls to `@llvm.kcfi.check` produce `KCFI_CHECK` during instruction selection. (You can use `-stop-before=finalize-isel` to dump the IR prior to isel; this can stay a .ll test, I think, rather than .mir).



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2273-2275
+    std::string Name = "__kcfi_typeid_" + F.getName().str();
+    if (!allowKCFIIdentifier(Name))
+      continue;
----------------
You could probably avoid re-checking `"__kcfi_typeid_"` repeatedly?


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:157
+      {"dfsan_abilist.txt", SanitizerKind::DataFlow},
+      {"cfi_ignorelist.txt", SanitizerKind::CFI | SanitizerKind::KCFI},
+      {"ubsan_ignorelist.txt",
----------------
none of the other kernel variants of `SanitizerKind` support these ignore lists. Do we plan to use them for the Linux kernel?


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:700
+      D.Diag(diag::err_drv_argument_not_allowed_with) << "-fsanitize=kcfi"
+                                                      << "-fsanitize=cfi*";
+
----------------
Can we use `lastArgumentForMask` here to be slightly more precise?


================
Comment at: clang/test/CodeGen/kcfi.c:31
+
+// CHECK-DAG: define internal i32 @f3() #[[#ATTR]] prefix i32 [[#HASH]]
+static int f3(void) { return 1; }
----------------
TIL about CHECK-DAG!
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive


================
Comment at: clang/test/CodeGen/kcfi.c:43
+         call(f3) +
+         call(f4);
+}
----------------
How about a direct call to `f5`? That should have no hash, right? Perhaps test that `test` has no  hash as well?


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:359
+  MCSection *getKCFISection(const MCSection &TextSec,
+                            const std::string &Name) const;
+
----------------
can we take a StringRef instead (and include the right header)?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:774
+    // Emit an .alt_entry directive for the actual function symbol.
+    if (MAI->hasSubsectionsViaSymbols())
       OutStreamer->emitSymbolAttribute(CurrentFnSym, MCSA_AltEntry);
----------------
consider saving this to a `bool` scoped to the `if (F.hasPrefixData()) {` block.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1111
+
+  const MCSectionELF &ElfSec = static_cast<const MCSectionELF &>(TextSec);
+  unsigned Flags = ELF::SHF_LINK_ORDER;
----------------
or just `cast`.
https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1120
+  return Ctx->getELFSection(Name, ELF::SHT_PROGBITS,
+                            Flags | ELF::SHF_ALLOC | ELF::SHF_WRITE, 0,
+                            GroupName, true, ElfSec.getUniqueID(),
----------------
Why don't you initialize `Flags` to these values, rather than set them here?


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1121
+                            Flags | ELF::SHF_ALLOC | ELF::SHF_WRITE, 0,
+                            GroupName, true, ElfSec.getUniqueID(),
+                            cast<MCSymbolELF>(TextSec.getBeginSymbol()));
----------------
`/*IsComdat=*/true`


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:355
+  OutStreamer->emitLabel(Trap);
+  EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::BRK).addImm(0x801));
+  emitKCFITrapEntry(MF, Trap);
----------------
Is this constant something the kernel will recognize?


================
Comment at: llvm/test/CodeGen/AArch64/kcfi-check.ll:3
+
+target triple = "aarch64--"
+
----------------
put this in the RUN line?


================
Comment at: llvm/test/CodeGen/AArch64/kcfi-check.ll:10
+; CHECK-NEXT:   .word 12345678
+define void @f1(i8* %x) #0 prefix i32 12345678 {
+; CHECK-LABEL: f1:
----------------
why does `@f1` have the hash? We're not calling it indirectly.


================
Comment at: llvm/test/CodeGen/AArch64/kcfi-check.ll:25
+; CHECK-NEXT:    ret
+  call void @llvm.kcfi.check(i8* %x, i32 12345678)
+  ret void
----------------
perhaps add a `call` to `%x` itself so we can see how the generated control flow looks?


================
Comment at: llvm/test/CodeGen/X86/kcfi-check.ll:3
+
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
put this in the RUN line?


================
Comment at: llvm/test/CodeGen/X86/kcfi-check.ll:11
+; CHECK-NEXT:   .zero 2
+define void @f1(i8* %x) #0 prefix i32 12345678 {
+; CHECK-LABEL: f1:
----------------
why does `@f1` have the hash? We're not calling it indirectly.


================
Comment at: llvm/test/CodeGen/X86/kcfi-check.ll:23
+; CHECK-NEXT:    retq
+  call void @llvm.kcfi.check(i8* %x, i32 12345678)
+  ret void
----------------
perhaps add a `call` to `%x` itself so we can see how the generated control flow looks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296