Skip to content

[MTE] Disable all MTE protection of globals in sections #78443

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
Jan 22, 2024

Conversation

hctim
Copy link
Collaborator

@hctim hctim commented Jan 17, 2024

Previous work in this area (#70186) disabled MTE in constructor
sections. Looks like I missed one, ".preinit_array".

Also, in the meantime, I found an exciting feature in the linker where
globals placed into an explicit section, where the section name is a
valid C identifer, gets an implicit '_start' and
'_stop' symbol as well. This is convenient for iterating
over some globals, but of course iteration over differently-tagged
globals in MTE explodes.

Thus, disable MTE globals for anything that has a section.

Previous work in this area (llvm#70186) disabled MTE in constructor
sections. Looks like I missed one, ".preinit_array".

Also, in the meantime, I found an exciting feature in the linker where
globals placed into an explicit section, where the section name is a
valid C identifer, gets an implicit '__start_<sectionname>' and
'__stop_<sectionname>' symbol as well. This is convenient for iterating
over some globals, but of course iteration over differently-tagged
globals in MTE explodes.

Thus, disable MTE globals for anything that has a section.
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 labels Jan 17, 2024
@hctim hctim requested review from eugenis and vitalybuka January 17, 2024 13:12
@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2024

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang

Author: Mitch Phillips (hctim)

Changes

Previous work in this area (#70186) disabled MTE in constructor
sections. Looks like I missed one, ".preinit_array".

Also, in the meantime, I found an exciting feature in the linker where
globals placed into an explicit section, where the section name is a
valid C identifer, gets an implicit '_start<sectionname>' and
'_stop<sectionname>' symbol as well. This is convenient for iterating
over some globals, but of course iteration over differently-tagged
globals in MTE explodes.

Thus, disable MTE globals for anything that has a section.


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

2 Files Affected:

  • (modified) clang/test/CodeGen/memtag-globals-asm.cpp (+8)
  • (modified) llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp (+19-7)
diff --git a/clang/test/CodeGen/memtag-globals-asm.cpp b/clang/test/CodeGen/memtag-globals-asm.cpp
index 4b76b394e0c1dc..186045f8f2fb5b 100644
--- a/clang/test/CodeGen/memtag-globals-asm.cpp
+++ b/clang/test/CodeGen/memtag-globals-asm.cpp
@@ -271,6 +271,10 @@ CONSTRUCTOR(".ctors") func_t func_ctors = func_constructor;
 CONSTRUCTOR(".dtors") func_t func_dtors = func_constructor;
 CONSTRUCTOR(".init_array") func_t func_init_array = func_constructor;
 CONSTRUCTOR(".fini_array") func_t func_fini_array = func_constructor;
+CONSTRUCTOR(".preinit_array") func_t preinit_array = func_constructor;
+CONSTRUCTOR("array_of_globals") int global1;
+CONSTRUCTOR("array_of_globals") int global2;
+CONSTRUCTOR("array_of_globals") int global_string;
 
 // CHECK-NOT: .memtag func_constructor
 // CHECK-NOT: .memtag func_init
@@ -279,3 +283,7 @@ CONSTRUCTOR(".fini_array") func_t func_fini_array = func_constructor;
 // CHECK-NOT: .memtag func_dtors
 // CHECK-NOT: .memtag func_init_array
 // CHECK-NOT: .memtag func_fini_array
+// CHECK-NOT: .memtag preinit_array
+// CHECK-NOT: .memtag global1
+// CHECK-NOT: .memtag global2
+// CHECK-NOT: .memtag global_string
diff --git a/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp b/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
index 8ce6f94e7341de..27959489e7dfa4 100644
--- a/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
+++ b/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
@@ -43,13 +43,25 @@ static bool shouldTagGlobal(GlobalVariable &G) {
     return false;
   }
 
-  // Don't instrument function pointers that are going into various init arrays
-  // via `__attribute__((section(<foo>)))`:
-  // https://github.com/llvm/llvm-project/issues/69939
-  if (G.hasSection() &&
-      (G.getSection() == ".init" || G.getSection() == ".fini" ||
-       G.getSection() == ".init_array" || G.getSection() == ".fini_array" ||
-       G.getSection() == ".ctors" || G.getSection() == ".dtors")) {
+  // Globals can be placed implicitly or explicitly in sections. There's two
+  // different types of globals that meet this criteria that cause problems:
+  //  1. Function pointers that are going into various init arrays (either
+  //     explicitly through `__attribute__((section(<foo>)))` or implicitly
+  //     through `__attribute__((constructor)))`, such as ".(pre)init(_array)",
+  //     ".fini(_array)", ".ctors", and ".dtors". These function pointers end up
+  //     overaligned and overpadded, making iterating over them problematic, and
+  //     each function pointer is individually tagged (so the iteration over
+  //     them causes SIGSEGV/MTE[AS]ERR).
+  //  2. Global variables put into an explicit section, where the section's name
+  //     is a valid C-style identifier. The linker emits a `__start_<name>` and
+  //     `__stop_<na,e>` symbol for the section, so that you can iterate over
+  //     globals within this section. Unfortunately, again, these globals would
+  //     be tagged and so iteration causes SIGSEGV/MTE[AS]ERR.
+  //
+  // To mitigate both these cases, and because specifying a section is rare
+  // outside of these two cases, disable MTE protection for globals in any
+  // section.
+  if (G.hasSection()) {
     Meta.Memtag = false;
     G.setSanitizerMetadata(Meta);
     return false;

@hctim hctim merged commit c9f5b5c into llvm:main Jan 22, 2024
@hctim hctim deleted the mte/no-globals-in-sections branch January 22, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants