[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added a comment.

In D119296#3625796 <https://reviews.llvm.org/D119296#3625796>, @MaskRay wrote:

>> It uses LLVM prefix data to store a type identifier for each function and injects verification code before indirect calls.
>
> Is "prefix data" stale now?

Yes, I forgot to update the commit message here. Fixed.

> There are 30+ comments not marked as "done". Having so many is distracting to reviewers as it's unclear whether this is in a ready-for-review state.

My bad, I wasn't familiar with the convention here. I've just been marking my replies as done.

> I personally definitely don't consider this in a ready-for-land state.

I left some questions about your comments below. PTAL.



================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:63
     SanitizerKind::Unreachable | SanitizerKind::Return;
-static const SanitizerMask AlwaysRecoverable =
-    SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
+static const SanitizerMask AlwaysRecoverable = SanitizerKind::KernelAddress |
+                                               SanitizerKind::KernelHWAddress |
----------------
MaskRay wrote:
> This is incorrect.
> 
> If a violation is found, ud2 is executed. ud2 is not followed by normal control flow so I don't think recovery from the error is supported.
> 
> This seems like `Unrecoverable`
This variable is only used to indicate whether `-fno-sanitize-recover` command line parameter can be used with the sanitizer. It makes no sense to allow this with KCFI as we always emit a recoverable instruction sequence, hence it's included here.

Also, ud2 absolutely is recoverable in the kernel, and Linux specifically uses ud2 to trigger warnings in assembly code.


================
Comment at: clang/test/CodeGen/kcfi.c:2
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck --check-prefixes=CHECK,O0 %s
+// RUN: %clang_cc1 -O2 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck --check-prefixes=CHECK,O2 %s
+#if !__has_feature(kcfi)
----------------
MaskRay wrote:
> MaskRay wrote:
> > If `-O2` has different behavior, having the test in clang/test/CodeGen is likely a layering violation.
> > Optimization tests should be placed in llvm/test/, in the relevant pass. Folks working on llvm optimizations generally don't want to test every frontend.
> If may be useful to have a `-x c++` test.
> 
> Add a not-address-taken external linkage function.
> If -O2 has different behavior, having the test in clang/test/CodeGen is likely a layering violation.

Good point, the `-O2` test isn't actually relevant anymore as we no longer rely on it. I'll drop it.

> Optimization tests should be placed in llvm/test/, in the relevant pass.

Yes, the `InstCombine` test below covers this already.



================
Comment at: clang/test/CodeGen/kcfi.c:2
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck --check-prefixes=CHECK,O0 %s
+// RUN: %clang_cc1 -O2 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck --check-prefixes=CHECK,O2 %s
+#if !__has_feature(kcfi)
----------------
samitolvanen wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > If `-O2` has different behavior, having the test in clang/test/CodeGen is likely a layering violation.
> > > Optimization tests should be placed in llvm/test/, in the relevant pass. Folks working on llvm optimizations generally don't want to test every frontend.
> > If may be useful to have a `-x c++` test.
> > 
> > Add a not-address-taken external linkage function.
> > If -O2 has different behavior, having the test in clang/test/CodeGen is likely a layering violation.
> 
> Good point, the `-O2` test isn't actually relevant anymore as we no longer rely on it. I'll drop it.
> 
> > Optimization tests should be placed in llvm/test/, in the relevant pass.
> 
> Yes, the `InstCombine` test below covers this already.
> 
> If may be useful to have a `-x c++` test.

Would you mind elaborating on this? The main difference with `-x c++` is the name mangling, I'm not sure if that adds much value in this case.

> Add a not-address-taken external linkage function.

Added.


================
Comment at: clang/test/Driver/fsanitize.c:652
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=kcfi -fsanitize=cfi -flto -fvisibility=hidden %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-KCFI-NOCFI
+// CHECK-KCFI-NOCFI: error: invalid argument '-fsanitize=kcfi' not allowed with '-fsanitize=cfi'
----------------
MaskRay wrote:
> Use modern spelling `--target=`. `-target ` is deprecated
> Use modern spelling `--target=`. `-target ` is deprecated

It doesn't look like `--target` works here:

`clang-15: error: unsupported option '--target'; did you mean '-target'?`

Also the rest of the file uses `-target`, so perhaps these can all be updated at once when the flag is deprecated?


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:125
+
+  // Emit int3 padding to allow runtime patching of the preamble.
+  EmitAndCountInstruction(MCInstBuilder(X86::INT3));
----------------
MaskRay wrote:
> A previous comment isn't done. This doesn't explain that the double-int3 before and after the identifier is an objtool requirement (or similar).
> A previous comment isn't done. This doesn't explain that the double-int3 before and after the identifier is an objtool requirement (or similar).

These are not objtool requirements, and I believe the comments do explain why the int3 padding is added. The first two are to reserve some space for runtime patching this code, and the latter two is to avoid call target gadgets in the checks. I'm obviously happy to improve the comments, is there something specific you'd like me to add?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296