[X86][KCFI] Do not emit a type prefix for nocf_check functions

[samitolvanen]

With indirect branch protection, the nocf_check attribute prevents a function from being called indirectly by omitting the ENDBR instruction from the beginning of the function body. As KCFI type prefixes are only needed for indirectly callable functions, don't emit the unnecessary prefix for nocf_check functions.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: Sami Tolvanen (samitolvanen)

Changes

With indirect branch protection, the nocf_check attribute prevents a function from being called indirectly by omitting the ENDBR instruction from the beginning of the function body. As KCFI type prefixes are only needed for indirectly callable functions, don't emit the unnecessary prefix for nocf_check functions.

---

Full diff: https://github.com/llvm/llvm-project/pull/158133.diff

2 Files Affected:

• (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (+3-2)
• (added) llvm/test/CodeGen/X86/kcfi-nocf-check.ll (+39)

diff --git a/llvm/lib/Target/X86/X86AsmPrinter.cpp b/llvm/lib/Target/X86/X86AsmPrinter.cpp
index ff22ee8c86fac..486bf3986cf5a 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ b/llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -170,8 +170,9 @@ void X86AsmPrinter::emitKCFITypeId(const MachineFunction &MF) {
     Type = mdconst::extract<ConstantInt>(MD->getOperand(0));
 
   // If we don't have a type to emit, just emit padding if needed to maintain
-  // the same alignment for all functions.
-  if (!Type) {
+  // the same alignment for all functions. Also skip `nocf_check` functions as
+  // they are not indirectly callable due to a missing ENDBR.
+  if (!Type || F.doesNoCfCheck()) {
     EmitKCFITypePadding(MF, /*HasType=*/false);
     return;
   }
diff --git a/llvm/test/CodeGen/X86/kcfi-nocf-check.ll b/llvm/test/CodeGen/X86/kcfi-nocf-check.ll
new file mode 100644
index 0000000000000..1ce886c1587f8
--- /dev/null
+++ b/llvm/test/CodeGen/X86/kcfi-nocf-check.ll
@@ -0,0 +1,39 @@
+; RUN: llc -mtriple=x86_64-unknown-unknown -x86-indirect-branch-tracking < %s | FileCheck %s
+
+; CHECK-LABEL: __cfi_cf_check_func:
+; CHECK:       movl	$12345678, %eax
+define void @cf_check_func() !kcfi_type !2 {
+; CHECK-LABEL: cf_check_func:
+; CHECK:       endbr64
+; CHECK:       retq
+entry:
+  ret void
+}
+
+; CHECK-NOT:   __cfi_notype_cf_check_func:
+; CHECK-NOT:   movl
+define void @notype_cf_check_func() {
+; CHECK-LABEL: notype_cf_check_func:
+; CHECK:       endbr64
+; CHECK:       retq
+entry:
+  ret void
+}
+
+; CHECK-NOT:   __cfi_nocf_check_func:
+; CHECK-NOT:   movl
+define void @nocf_check_func() #0 !kcfi_type !2 {
+; CHECK-LABEL: nocf_check_func:
+; CHECK-NOT:   endbr64
+; CHECK:       retq
+entry:
+  ret void
+}
+
+attributes #0 = { nocf_check }
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 8, !"cf-protection-branch", i32 1}
+!1 = !{i32 4, !"kcfi", i32 1}
+!2 = !{i32 12345678}

[MaskRay]

We just need one CHECK-NOT: __cfi_ and the next ; CHECK-NOT: __cfi_nocf_check_func: can be removed.

[samitolvanen]

We just need one CHECK-NOT: __cfi_ and the next ; CHECK-NOT: __cfi_nocf_check_func: can be removed.

Do you mean something like this?

; CHECK-NOT:   __cfi_
define void @notype_cf_check_func() {
; CHECK-LABEL: notype_cf_check_func:
; CHECK:       endbr64
; CHECK:       retq
entry:
  ret void
}

define void @nocf_check_func() #0 !kcfi_type !2 {
; CHECK-LABEL: nocf_check_func:
; CHECK-NOT:   endbr64
; CHECK:       retq
entry:
  ret void
}

Perhaps I misunderstood your comment, because if we now end up emitting a __cfi_ type prefix before nocf_check_func, FileCheck is not going to detect that. We're still going to need a second CHECK-NOT: __cfi_ before nocf_check_func to catch this case.

Am I missing some clever FileCheck trick that would make this simpler?

[kees]

I see this is only testing the IR level, but not the front-end. Do you want -fsanitize=kcfi to work with nocf_check regardless of -fcf-protection? I think it should -- there may actually be some value in being able to remove preambles for a function regardless of -fcf-protection. (This is the logic I'm implementing in the GCC KCFI.)

That way we won't get these kinds of errors if someone tries to use nocf_check:

kcfi-runtime.c:17:16: warning: 'nocf_check' attribute ignored; use -fcf-protection to enable the
      attribute [-Wignored-attributes]
   17 | __attribute__((nocf_check))
      |                ^

[samitolvanen]

I see this is only testing the IR level, but not the front-end.

Correct, this doesn't change the behavior of nocf_check in Clang.

Do you want -fsanitize=kcfi to work with nocf_check regardless of -fcf-protection? I think it should -- there may actually be some value in being able to remove preambles for a function regardless of -fcf-protection. (This is the logic I'm implementing in the GCC KCFI.)

That sounds reasonable to me. Out of curiosity, nocf_check is currently limited to X86 only in Clang. Does your GCC KCFI implementation support nocf_check on other architectures too?

That way we won't get these kinds of errors if someone tries to use nocf_check:

kcfi-runtime.c:17:16: warning: 'nocf_check' attribute ignored; use -fcf-protection to enable the
      attribute [-Wignored-attributes]
   17 | __attribute__((nocf_check))
      |                ^

I think this isn't actually a problem in the kernel though, as nocf_check is only used with CONFIG_X86_KERNEL_IBT.

[kees]

I see this is only testing the IR level, but not the front-end.

Correct, this doesn't change the behavior of nocf_check in Clang.

Do you want -fsanitize=kcfi to work with nocf_check regardless of -fcf-protection? I think it should -- there may actually be some value in being able to remove preambles for a function regardless of -fcf-protection. (This is the logic I'm implementing in the GCC KCFI.)

That sounds reasonable to me. Out of curiosity, nocf_check is currently limited to X86 only in Clang. Does your GCC KCFI implementation support nocf_check on other architectures too?

I modified the nocf_check attribute handler to check for either -cf-protection or -fsanitize=kcfi being enabled.

That way we won't get these kinds of errors if someone tries to use nocf_check:

kcfi-runtime.c:17:16: warning: 'nocf_check' attribute ignored; use -fcf-protection to enable the
      attribute [-Wignored-attributes]
   17 | __attribute__((nocf_check))
      |                ^

I think this isn't actually a problem in the kernel though, as nocf_check is only used with CONFIG_X86_KERNEL_IBT.

Correct. In the future we could modify the nocf_check Linux macro to become enabled under either config.

[samitolvanen]

I modified the nocf_check attribute handler to check for either -cf-protection or -fsanitize=kcfi being enabled.

Looking at this a bit closer, the nocf_check attribute also applies to function pointers with -fcf-protection, and disables tracking for indirect branches. Since KCFI already has the no_sanitize("kcfi") attribute for this purpose, we would presumably just ignore the nocf_check attribute on pointers used for indirect calls when only KCFI is enabled, but preserve the semantics for assigning and casting the attributed pointers. How does the GCC KCFI implementation handle this? I would like to make sure both compilers behave the same way here.

Edit: RISC-V also supports -fcf-protection, but not nocf_check, so presumably we would still want to produce a warning for nocf_check usage when used with -fcf-protection alone on non-X86 architectures if we want to support the attribute on other architectures with KCFI.

[kees]

Looking at this a bit closer, the nocf_check attribute also applies to function pointers with -fcf-protection, and disables tracking for indirect branches.

Wait, like, as variable/struct-member attribute?

Since KCFI already has the no_sanitize("kcfi") attribute for this purpose, we would presumably just ignore the nocf_check attribute on pointers used for indirect calls when only KCFI is enabled, but preserve the semantics for assigning and casting the attributed pointers. How does the GCC KCFI implementation handle this? I would like to make sure both compilers behave the same way here.

Yeah, I was only looking at disabling the KCFI preamble generation. I was following Clang's KCFI implementation so no_sanitize("kcfi") disables indirect call checking within the attributed function.

Edit: RISC-V also supports -fcf-protection, but not nocf_check, so presumably we would still want to produce a warning for nocf_check usage when used with -fcf-protection alone on non-X86 architectures if we want to support the attribute on other architectures with KCFI.

Hm, this is getting a bit weird. Perhaps we should instead leave nocf_check alone and create a KCFI-specific attribute for preamble disabling instead? Bike-shed: __attribute__((kcfi_invisible))? kcfi_uncallable? kcfi_no_preamble?

[samitolvanen]

Looking at this a bit closer, the nocf_check attribute also applies to function pointers with -fcf-protection, and disables tracking for indirect branches.

Wait, like, as variable/struct-member attribute?

For function pointer types, specifically. Performing an indirect call through a nocf_check attributed function pointer sets the NOTRACK prefix, which disables IBT for the branch. This is disabled in the kernel, but the compiler should still handle this gracefully.

Edit: RISC-V also supports -fcf-protection, but not nocf_check, so presumably we would still want to produce a warning for nocf_check usage when used with -fcf-protection alone on non-X86 architectures if we want to support the attribute on other architectures with KCFI.

Hm, this is getting a bit weird. Perhaps we should instead leave nocf_check alone and create a KCFI-specific attribute for preamble disabling instead? Bike-shed: __attribute__((kcfi_invisible))? kcfi_uncallable? kcfi_no_preamble?

Adding a new attribute would certainly avoid the corner cases with nocf_check. @AaronBallman any thoughts about this?