[MTE] decide whether to tag global in AsmPrinter #135891
8000
Jump to bottom
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-clang-codegen Author: Florian Mayer (fmayer) Changesthere are llvm passes that would invalidate the decision we make in Full diff: https://github.com/llvm/llvm-project/pull/135891.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/SanitizerMetadata.cpp b/clang/lib/CodeGen/SanitizerMetadata.cpp
index b7b212ba46efd..288f7f6415f44 100644
--- a/clang/lib/CodeGen/SanitizerMetadata.cpp
+++ b/clang/lib/CodeGen/SanitizerMetadata.cpp
@@ -32,37 +32,6 @@ static SanitizerMask expandKernelSanitizerMasks(SanitizerMask Mask) {
return Mask;
}
-static bool shouldTagGlobal(const llvm::GlobalVariable &G) {
- // For now, don't instrument constant data, as it'll be in .rodata anyway. It
- // may be worth instrumenting these in future to stop them from being used as
- // gadgets.
- if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
- return false;
-
- // 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_<name>` 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())
- return false;
-
- return true;
-}
-
void SanitizerMetadata::reportGlobal(llvm::GlobalVariable *GV,
SourceLocation Loc, StringRef Name,
QualType Ty,
@@ -89,15 +58,11 @@ void SanitizerMetadata::reportGlobal(llvm::GlobalVariable *GV,
Meta.NoHWAddress |= CGM.isInNoSanitizeList(
FsanitizeArgument.Mask & SanitizerKind::HWAddress, GV, Loc, Ty);
- if (shouldTagGlobal(*GV)) {
- Meta.Memtag |= static_cast<bool>(FsanitizeArgument.Mask &
- SanitizerKind::MemtagGlobals);
- Meta.Memtag &= !NoSanitizeAttrSet.hasOneOf(SanitizerKind::MemTag);
- Meta.Memtag &= !CGM.isInNoSanitizeList(
- FsanitizeArgument.Mask & SanitizerKind::MemTag, GV, Loc, Ty);
- } else {
- Meta.Memtag = false;
- }
+ Meta.Memtag |=
+ static_cast<bool>(FsanitizeArgument.Mask & SanitizerKind::MemtagGlobals);
+ Meta.Memtag &= !NoSanitizeAttrSet.hasOneOf(SanitizerKind::MemTag);
+ Meta.Memtag &= !CGM.isInNoSanitizeList(
+ FsanitizeArgument.Mask & SanitizerKind::MemTag, GV, Loc, Ty);
Meta.IsDynInit = IsDynInit && !Meta.NoAddress &&
FsanitizeArgument.has(SanitizerKind::Address) &&
diff --git a/clang/test/CodeGen/memtag-globals.cpp b/clang/test/CodeGen/memtag-globals.cpp
index 0d7fe22a7b0f0..407eea1c37cfa 100644
--- a/clang/test/CodeGen/memtag-globals.cpp
+++ b/clang/test/CodeGen/memtag-globals.cpp
@@ -25,8 +25,9 @@ void func() {
// CHECK: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
// CHECK: @{{.*}}global{{.*}} ={{.*}} sanitize_memtag
-// CHECK: @{{.*}}section_global{{.*}} =
-// CHECK-NOT: sanitize_memtag
+// This DOES NOT mean we are instrumenting the section global,
+// but we are ignoring it in AsmPrinter rather than in clang.
+// CHECK: @{{.*}}section_global{{.*}} ={{.*}} sanitize_memtag
// CHECK: @{{.*}}attributed_global{{.*}} =
// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}disable_instrumentation_global{{.*}} =
@@ -35,7 +36,7 @@ void func() {
// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}static_var{{.*}} ={{.*}} sanitize_memtag
-// CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}}
+// CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}} sanitize_memtag
// CHECK: @{{.*}}external_global{{.*}} ={{.*}} sanitize_memtag
// IGNORELIST: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 0deaf94502b11..ad34465e2c606 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -2398,6 +2398,41 @@ void AsmPrinter::emitRemarksSection(remarks::RemarkStreamer &RS) {
OutStreamer->emitBinaryData(Buf);
}
+static bool shouldTagGlobal(const llvm::GlobalVariable &G) {
+ // We used to do this in clang, but there are optimization passes that turn
+ // non-constant globals into constants. So now, clang only tells us whether
+ // it would *like* a global to be tagged, but we still make the decision here.
+ //
+ // For now, don't instrument constant data, as it'll be in .rodata anyway. It
+ // may be worth instrumenting these in future to stop them from being used as
+ // gadgets.
+ if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
+ return false;
+
+ // 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_<name>` 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())
+ return false;
+
+ return true;
+}
+
static void tagGlobalDefinition(Module &M, GlobalVariable *G) {
Constant *Initializer = G->getInitializer();
uint64_t SizeInBytes =
@@ -2430,6 +2465,12 @@ static void tagGlobalDefinition(Module &M, GlobalVariable *G) {
G->setUnnamedAddr(GlobalValue::UnnamedAddr::None);
}
+static void removeMemtagFromGlobal(GlobalVariable &G) {
+ auto Meta = G.getSanitizerMetadata();
+ Meta.Memtag = false;
+ G.setSanitizerMetadata(Meta);
+}
+
bool AsmPrinter::doFinalization(Module &M) {
// Set the MachineFunction to nullptr so that we can catch attempted
// accesses to MF specific features at the module level and so that
@@ -2440,6 +2481,12 @@ bool AsmPrinter::doFinalization(Module &M) {
for (GlobalVariable &G : M.globals()) {
if (G.isDeclaration() || !G.isTagged())
continue;
+ if (!shouldTagGlobal(G)) {
+ assert(G.hasSanitizerMetadata()); // because isTagged.
+ removeMemtagFromGlobal(G);
+ assert(!G.isTagged());
+ continue;
+ }
GlobalsToTag.push_back(&G);
}
for (GlobalVariable *G : GlobalsToTag)
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 7423e746dfa9a..b5bf286a65ce5 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -808,10 +808,6 @@ void Verifier::visitGlobalValue(const GlobalValue &GV) {
"visibility must be dso_local!",
&GV);
- if (GV.isTagged()) {
- Check(!GV.hasSection(), "tagged GlobalValue must not be in section.", &GV);
- }
-
forEachUser(&GV, GlobalValueVisited, [&](const Value *V) -> bool {
if (const Instruction *I = dyn_cast<Instruction>(V)) {
if (!I->getParent() || !I->getParent()->getParent())
diff --git a/llvm/unittests/IR/VerifierTest.cpp b/llvm/unittests/IR/VerifierTest.cpp
index 2b51f3c4ea2c2..7a136e6a382a7 100644
--- a/llvm/unittests/IR/VerifierTest.cpp
+++ b/llvm/unittests/IR/VerifierTest.cpp
@@ -416,26 +416,5 @@ TEST(VerifierTest, GetElementPtrInst) {
<< Error;
}
-TEST(VerifierTest, DetectTaggedGlobalInSection) {
- LLVMContext C;
- Module M("M", C);
- GlobalVariable *GV = new GlobalVariable(
- Type::getInt64Ty(C), false, GlobalValue::InternalLinkage,
- ConstantInt::get(Type::getInt64Ty(C), 1));
- GV->setDSOLocal(true);
- GlobalValue::SanitizerMetadata MD{};
- MD.Memtag = true;
- GV->setSanitizerMetadata(MD);
- GV->setSection("foo");
- M.insertGlobalVariable(GV);
-
- std::string Error;
- raw_string_ostream ErrorOS(Error);
- EXPECT_TRUE(verifyModule(M, &ErrorOS));
- EXPECT_TRUE(
- StringRef(Error).starts_with("tagged GlobalValue must not be in section"))
- << Error;
-}
-
} // end anonymous namespace
} // end namespace llvm
|
|
@llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) Changesthere are llvm passes that would invalidate the decision we make in Full diff: https://github.com/llvm/llvm-project/pull/135891.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/SanitizerMetadata.cpp b/clang/lib/CodeGen/SanitizerMetadata.cpp
index b7b212ba46efd..288f7f6415f44 100644
--- a/clang/lib/CodeGen/SanitizerMetadata.cpp
+++ b/clang/lib/CodeGen/SanitizerMetadata.cpp
@@ -32,37 +32,6 @@ static SanitizerMask expandKernelSanitizerMasks(SanitizerMask Mask) {
return Mask;
}
-static bool shouldTagGlobal(const llvm::GlobalVariable &G) {
- // For now, don't instrument constant data, as it'll be in .rodata anyway. It
- // may be worth instrumenting these in future to stop them from being used as
- // gadgets.
- if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
- return false;
-
- // 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_<name>` symbol for the section, so that you can iterate over
|
|
Should we add AsmPrinter tests to replace the ones we're removing from Clang? |
We have memtag-globals-asm.cpp |
pcc
approved these changes
Apr 16, 2025
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
there are llvm passes that would invalidate the decision we make in clang.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
there are llvm passes that would invalidate the decision we make in clang.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
there are llvm passes that would invalidate the decision we make in
clang.