[PATCH] D147975: [StackProtector] don't check stack protector before calling nounwind functions

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers created this revision.
nickdesaulniers added reviewers: xiangzhangllvm, LuoYuanke, pengfei, andrew.w.kaylor, hjl.tools.
Herald added a subscriber: hiraditya.
Herald added a project: All.
nickdesaulniers requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

https://reviews.llvm.org/rGd656ae28095726830f9beb8dbd4d69f5144ef821
introduced a additional checks before calling noreturn functions in
response to this security paper related to Catch Handler Oriented
Programming (CHOP):
https://download.vusec.net/papers/chop_ndss23.pdf
See also:
https://bugs.chromium.org/p/llvm/issues/detail?id=30

This causes stack canaries to be inserted in C code which was
unexpected; we noticed certain Linux kernel trees stopped booting after
this (in functions trying to initialize the stack canary itself).
https://github.com/ClangBuiltLinux/linux/issues/1815

There is no point checking the stack canary like this when exceptions
are disabled (-fno-exceptions or function is marked noexcept) or for C
code.  The GCC patch for this issue does something similar:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=a25982ada523689c8745d7fb4b1b93c8f5dab2e7


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147975

Files:
  llvm/lib/CodeGen/StackProtector.cpp
  llvm/test/CodeGen/X86/stack-protector-2.ll
  llvm/test/CodeGen/X86/stack-protector-recursively.ll


Index: llvm/test/CodeGen/X86/stack-protector-recursively.ll
===================================================================
--- llvm/test/CodeGen/X86/stack-protector-recursively.ll
+++ llvm/test/CodeGen/X86/stack-protector-recursively.ll
@@ -12,15 +12,14 @@
 ; CHECK-NEXT:    cmpq (%rsp), %rax
 ; CHECK-NEXT:    jne .LBB0_2
 ; CHECK-NEXT:  # %bb.1: # %SP_return
-; CHECK-NEXT:    ud2
+; CHECK-NEXT:    callq foo at PLT
 ; CHECK-NEXT:  .LBB0_2: # %CallStackCheckFailBlk
 ; CHECK-NEXT:    callq __stack_chk_fail
 entry:
-  tail call void @llvm.trap()
+  tail call void @foo() noreturn
   unreachable
 }
 
-declare void @llvm.trap() #1
+declare void @foo() noreturn
 
 attributes #0 = { noreturn nounwind sspreq }
-attributes #1 = { noreturn nounwind }
Index: llvm/test/CodeGen/X86/stack-protector-2.ll
===================================================================
--- llvm/test/CodeGen/X86/stack-protector-2.ll
+++ llvm/test/CodeGen/X86/stack-protector-2.ll
@@ -1,4 +1,5 @@
-; RUN: llc -mtriple=x86_64-pc-linux-gnu -start-before=stack-protector -stop-after=stack-protector -o - < %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-pc-linux-gnu -start-before=stack-protector \
+; RUN:   -stop-after=stack-protector -o - < %s | FileCheck %s
 ; Bugs 42238/43308: Test some additional situations not caught previously.
 
 define void @store_captures() #0 {
@@ -219,5 +220,14 @@
   ret i32 0
 }
 
+declare void @callee() noreturn nounwind
+define void @caller() sspstrong {
+; Test that a stack protector is NOT inserted when we call nounwind functions.
+; CHECK-LABEL: @caller
+; CHECK-NEXT: call void @callee
+  call void @callee() noreturn nounwind
+  ret void
+}
+
 attributes #0 = { sspstrong }
 attributes #1 = { noreturn sspreq}
Index: llvm/lib/CodeGen/StackProtector.cpp
===================================================================
--- llvm/lib/CodeGen/StackProtector.cpp
+++ llvm/lib/CodeGen/StackProtector.cpp
@@ -482,18 +482,15 @@
     if (&BB == FailBB)
       continue;
     Instruction *CheckLoc = dyn_cast<ReturnInst>(BB.getTerminator());
-    if (!CheckLoc && !DisableCheckNoReturn) {
-      for (auto &Inst : BB) {
-        auto *CB = dyn_cast<CallBase>(&Inst);
-        if (!CB)
-          continue;
-        if (!CB->doesNotReturn())
-          continue;
-        // Do stack check before non-return calls (e.g: __cxa_throw)
-        CheckLoc = CB;
-        break;
-      }
-    }
+    if (!CheckLoc && !DisableCheckNoReturn)
+      for (auto &Inst : BB)
+        if (auto *CB = dyn_cast<CallBase>(&Inst))
+          // Do stack check before noreturn calls that aren't nounwind (e.g:
+          // __cxa_throw).
+          if (CB->doesNotReturn() && !CB->doesNotThrow()) {
+            CheckLoc = CB;
+            break;
+          }
 
     if (!CheckLoc)
       continue;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147975.512274.patch
Type: text/x-patch
Size: 2794 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230410/541f2456/attachment.bin>