Skip to content

[LLD][COFF] Set __guard_flags to CF_INSTRUMENTED if any object is instrumented #115374

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

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Nov 7, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-platform-windows

Author: Jacek Caban (cjacek)

Changes

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

2 Files Affected:

  • (modified) lld/COFF/Writer.cpp (+15-2)
  • (added) lld/test/COFF/cfguard-off-instrumented.s (+22)
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 71ee5ce4685553..58d0700c52aaf4 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -1217,8 +1217,7 @@ void Writer::createMiscChunks() {
     createSEHTable();
 
   // Create /guard:cf tables if requested.
-  if (config->guardCF != GuardCFLevel::Off)
-    createGuardCFTables();
+  createGuardCFTables();
 
   if (isArm64EC(config->machine))
     createECChunks();
@@ -1979,6 +1978,20 @@ void Writer::markSymbolsWithRelocations(ObjFile *file,
 void Writer::createGuardCFTables() {
   Configuration *config = &ctx.config;
 
+  if (config->guardCF == GuardCFLevel::Off) {
+    // MSVC marks the entire image as instrumented if any input object was built
+    // with /guard:cf.
+    for (ObjFile *file : ctx.objFileInstances) {
+      if (file->hasGuardCF()) {
+        Symbol *flagSym = ctx.symtab.findUnderscore("__guard_flags");
+        cast<DefinedAbsolute>(flagSym)->setVA(
+            uint32_t(GuardFlags::CF_INSTRUMENTED));
+        break;
+      }
+    }
+    return;
+  }
+
   SymbolRVASet addressTakenSyms;
   SymbolRVASet giatsRVASet;
   std::vector<Symbol *> giatsSymbols;
diff --git a/lld/test/COFF/cfguard-off-instrumented.s b/lld/test/COFF/cfguard-off-instrumented.s
new file mode 100644
index 00000000000000..4bd81d99568927
--- /dev/null
+++ b/lld/test/COFF/cfguard-off-instrumented.s
@@ -0,0 +1,22 @@
+// Verify that __guard_flags is set to CF_INSTRUMENTED if CF guard is disabled,
+// but the input object was built with CF guard.
+
+// REQUIRES: x86
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %s -o %t.obj
+// RUN: lld-link -out:%t1.dll %t.obj -dll -noentry
+// RUN: lld-link -out:%t2.dll %t.obj -dll -noentry -guard:no
+
+// RUN: llvm-readobj --hex-dump=.test %t1.dll | FileCheck %s
+// RUN: llvm-readobj --hex-dump=.test %t2.dll | FileCheck %s
+// CHECK: 0x180001000 00010000
+
+        .def     @feat.00;
+        .scl    3;
+        .type   0;
+        .endef
+        .globl  @feat.00
+@feat.00 = 0x800
+
+        .section .test, "r"
+        .long __guard_flags

@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

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

2 Files Affected:

  • (modified) lld/COFF/Writer.cpp (+15-2)
  • (added) lld/test/COFF/cfguard-off-instrumented.s (+22)
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 71ee5ce4685553..58d0700c52aaf4 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -1217,8 +1217,7 @@ void Writer::createMiscChunks() {
     createSEHTable();
 
   // Create /guard:cf tables if requested.
-  if (config->guardCF != GuardCFLevel::Off)
-    createGuardCFTables();
+  createGuardCFTables();
 
   if (isArm64EC(config->machine))
     createECChunks();
@@ -1979,6 +1978,20 @@ void Writer::markSymbolsWithRelocations(ObjFile *file,
 void Writer::createGuardCFTables() {
   Configuration *config = &ctx.config;
 
+  if (config->guardCF == GuardCFLevel::Off) {
+    // MSVC marks the entire image as instrumented if any input object was built
+    // with /guard:cf.
+    for (ObjFile *file : ctx.objFileInstances) {
+      if (file->hasGuardCF()) {
+        Symbol *flagSym = ctx.symtab.findUnderscore("__guard_flags");
+        cast<DefinedAbsolute>(flagSym)->setVA(
+            uint32_t(GuardFlags::CF_INSTRUMENTED));
+        break;
+      }
+    }
+    return;
+  }
+
   SymbolRVASet addressTakenSyms;
   SymbolRVASet giatsRVASet;
   std::vector<Symbol *> giatsSymbols;
diff --git a/lld/test/COFF/cfguard-off-instrumented.s b/lld/test/COFF/cfguard-off-instrumented.s
new file mode 100644
index 00000000000000..4bd81d99568927
--- /dev/null
+++ b/lld/test/COFF/cfguard-off-instrumented.s
@@ -0,0 +1,22 @@
+// Verify that __guard_flags is set to CF_INSTRUMENTED if CF guard is disabled,
+// but the input object was built with CF guard.
+
+// REQUIRES: x86
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %s -o %t.obj
+// RUN: lld-link -out:%t1.dll %t.obj -dll -noentry
+// RUN: lld-link -out:%t2.dll %t.obj -dll -noentry -guard:no
+
+// RUN: llvm-readobj --hex-dump=.test %t1.dll | FileCheck %s
+// RUN: llvm-readobj --hex-dump=.test %t2.dll | FileCheck %s
+// CHECK: 0x180001000 00010000
+
+        .def     @feat.00;
+        .scl    3;
+        .type   0;
+        .endef
+        .globl  @feat.00
+@feat.00 = 0x800
+
+        .section .test, "r"
+        .long __guard_flags

@cjacek
Copy link
Contributor Author

cjacek commented Nov 7, 2024

I noticed this issue while examining load config differences between LLD and MSVC for ARM64EC. After some experimentation, I found that this behavior isn’t unique to ARM64EC: __guard_flags is marked as CF_INSTRUMENTED if any input object is built with /guard:cf. On ARM64EC, the load config from msvcrt pulls in additional code, which is enough to trigger this flag.

Since LLD has functioned well without this behavior so far, it’s unlikely to matter in practice. However, implementing it for compatibility seems straightforward enough.

@mstorsjo mstorsjo requested a review from alvinhochun November 7, 2024 21:50
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

LGTM, seems reasonable.

@alvinhochun
Copy link
Contributor

Just speculating here without having tested the patch: I'm not sure if this is fine for llvm-mingw, which builds mingw-w64 with CFG. It looks like this patch would effectively set CF_INSTRUMENTED on all binaries built with llvm-mingw even if the user hasn't asked for it. In mingw-w64 CRT (and by extension ucrt/msvcrt) there may be functions that calls function pointers to user code (.ctors/.dtors are probably some of them, maybe atexit handlers too), which may trip CFG if calling functions from object files not built with -mguard=cf.

Though LLD may have some fallbacks to heuristically add functions to the list of valid call targets. I don't know, maybe it is possible to craft some object files in assembly that would trip CFG? I recall there being a problem with OpenSSL that enabling CFG with llvm-mingw will crash when calling into some objects built with NASM.

@cjacek
Copy link
Contributor Author

cjacek commented Nov 8, 2024

Note that this patch only concerns the CF_INSTRUMENTED flag. It does not make LLD emit function tables or set the CF_FUNCTION_TABLE_PRESENT flag, so as far as I understand, it won’t have any functional impact. I’m not sure what might be special about mingw-w64 here; if this were an issue, it would likely affect MSVC as well.

Copy link
Contributor

@alvinhochun alvinhochun left a comment

Choose a reason for hiding this comment

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

Ah okay. If it doesn't set CF_FUNCTION_TABLE_PRESENT, then it should be alright.

@cjacek cjacek merged commit 5fbe9b9 into llvm:main Nov 8, 2024
12 checks passed
@cjacek cjacek deleted the cf-instrumented branch November 8, 2024 13:01
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
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