[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added a comment.

In D119296#3457740 <https://reviews.llvm.org/D119296#3457740>, @nickdesaulniers wrote:

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

I added a check for this in the existing tests. Although if isel didn't produce a `KCFI_CHECK` here for some reason, I'm fairly sure the tests would have blown up either way.



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


================
Comment at: clang/test/CodeGen/kcfi.c:43
+         call(f3) +
+         call(f4);
+}
----------------
nickdesaulniers wrote:
> How about a direct call to `f5`? That should have no hash, right? Perhaps test that `test` has no  hash as well?
`test` is a global function, which needs to have a hash as it could be indirectly called from another module.

Note that we don't need hashes in non-address-taken local functions, but the initial patch emitted them anyway. I changed the code to drop those hashes and added a directly called `static f5(void)` to the test.


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


================
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:
----------------
nickdesaulniers wrote:
> why does `@f1` have the hash? We're not calling it indirectly.
Because I want to include a test for the prefix data being emitted correctly. Also, a global function needs a hash even if we don't call it indirectly in this module. I'll rename these tests to `kcfi.ll` to not make it sound like this only tests `llvm.kcfi.check` output.


================
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:
----------------
nickdesaulniers wrote:
> why does `@f1` have the hash? We're not calling it indirectly.
See above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296