Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ARM64EC] Avoid emitting unnecessary symbol references with /guard:cf. #123235

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

efriedma-quic
Copy link
Collaborator

.gfids$y contains a list of indirect calls for Control Flow Guard. This wasn't working properly for ARM64EC: direct calls were being treated as indirect calls. Make sure we correctly filter out direct calls.

This improves the protection from Control Flow Guard, and also fixes a link error when using certain functions from oldnames.lib.

.gfids$y contains a list of indirect calls for Control Flow Guard. This
wasn't working properly for ARM64EC: direct calls were being treated as
indiret calls.  Make sure we correctly filter out direct calls.

This improves the protection from Control Flow Guard, and also fixes a
link error when using certain functions from oldnames.lib.
@llvmbot
Copy link
Member

llvmbot commented Jan 16, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Eli Friedman (efriedma-quic)

Changes

.gfids$y contains a list of indirect calls for Control Flow Guard. This wasn't working properly for ARM64EC: direct calls were being treated as indirect calls. Make sure we correctly filter out direct calls.

This improves the protection from Control Flow Guard, and also fixes a link error when using certain functions from oldnames.lib.


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp (+18-12)
  • (added) llvm/test/CodeGen/AArch64/cfguard-arm64ec.ll (+16)
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp
index 1a1e6f0117e2b8..09e4408c9bf234 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp
@@ -49,26 +49,32 @@ static bool isPossibleIndirectCallTarget(const Function *F) {
     const Value *FnOrCast = Users.pop_back_val();
     for (const Use &U : FnOrCast->uses()) {
       const User *FnUser = U.getUser();
-      if (isa<BlockAddress>(FnUser))
+      if (isa<BlockAddress>(FnUser)) {
+        // Block addresses are illegal to call.
         continue;
+      }
       if (const auto *Call = dyn_cast<CallBase>(FnUser)) {
-        if (!Call->isCallee(&U))
+        if ((!Call->isCallee(&U) || U != F) &&
+            !Call->getFunction()->getName().ends_with("$exit_thunk")) {
+          // Passing a function pointer to a call may lead to an indirect
+          // call. As an exception, ignore ARM64EC exit thunks.
           return true;
+        }
       } else if (isa<Instruction>(FnUser)) {
         // Consider any other instruction to be an escape. This has some weird
         // consequences like no-op intrinsics being an escape or a store *to* a
         // function address being an escape.
         return true;
-      } else if (const auto *C = dyn_cast<Constant>(FnUser)) {
-        // If this is a constant pointer cast of the function, don't consider
-        // this escape. Analyze the uses of the cast as well. This ensures that
-        // direct calls with mismatched prototypes don't end up in the CFG
-        // table. Consider other constants, such as vtable initializers, to
-        // escape the function.
-        if (C->stripPointerCasts() == F)
-          Users.push_back(FnUser);
-        else
-          return true;
+      } else if (const auto *G = dyn_cast<GlobalValue>(FnUser)) {
+        // Ignore llvm.arm64ec.symbolmap; it doesn't lower to an actual address.
+        if (G->getName() == "llvm.arm64ec.symbolmap")
+          continue;
+        // Globals (for example, vtables) are escapes.
+        return true;
+      } else if (isa<Constant>(FnUser)) {
+        // Constants which aren't a global are intermediate values; recursively
+        // analyze the users to see if they actually escape.
+        Users.push_back(FnUser);
       }
     }
   }
diff --git a/llvm/test/CodeGen/AArch64/cfguard-arm64ec.ll b/llvm/test/CodeGen/AArch64/cfguard-arm64ec.ll
new file mode 100644
index 00000000000000..bdbc99e2d98b0a
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/cfguard-arm64ec.ll
@@ -0,0 +1,16 @@
+; RUN: llc < %s -mtriple=arm64ec-pc-windows-msvc | FileCheck %s
+
+declare void @called()
+declare void @escaped()
+define void @f(ptr %dst) {
+  call void @called()
+  store ptr @escaped, ptr %dst
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 2, !"cfguard", i32 1}
+
+; CHECK-LABEL: .section .gfids$y,"dr"
+; CHECK-NEXT:  .symidx escaped
+; CHECK-NOT:   .symidx

Copy link
Contributor

@dpaoliello dpaoliello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@efriedma-quic efriedma-quic merged commit d540ebf into llvm:main Jan 22, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants