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

[Instrumentation] Do not run sanitizers for naked functions #108552

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

antoniofrighetto
Copy link
Contributor

@antoniofrighetto antoniofrighetto commented Sep 13, 2024

Sanitizers instrumentation may be incompatible with naked functions, which lack of standard prologue/epilogue.

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Antonio Frighetto (antoniofrighetto)

Changes

Sanitizers instrumentation is incompatible with naked functions, which are expected to contain only inline asm.


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

6 Files Affected:

  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+4)
  • (modified) llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp (+2-1)
  • (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+4)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp (+3)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp (+3)
  • (added) llvm/test/Instrumentation/sanitizers-no-naked.ll (+49)
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 5de0a78d087968..1e9e2d03c6ce42 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -2960,6 +2960,10 @@ bool AddressSanitizer::instrumentFunction(Function &F,
 
   bool FunctionModified = false;
 
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return FunctionModified;
+
   // If needed, insert __asan_init before checking for SanitizeAddress attr.
   // This function needs to be called even if the function body is not
   // instrumented.
diff --git a/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
index b4b5f67d2e62da..1cb579309089e1 100644
--- a/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
@@ -1545,8 +1545,9 @@ bool DataFlowSanitizer::runImpl(
   SmallPtrSet<Function *, 2> FnsWithForceZeroLabel;
   SmallPtrSet<Constant *, 1> PersonalityFns;
   for (Function &F : M)
+    // Do not apply any instrumentation for naked functions or if disabled.
     if (!F.isIntrinsic() && !DFSanRuntimeFunctions.contains(&F) &&
-        !LibAtomicFunction(F) &&
+        !LibAtomicFunction(F) && !F.hasFnAttribute(Attribute::Naked) &&
         !F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation)) {
       FnsToInstrument.push_back(&F);
       if (F.hasPersonalityFn())
diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 17c5638cf8ced7..90de58143955c3 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -6078,6 +6078,10 @@ bool MemorySanitizer::sanitizeFunction(Function &F, TargetLibraryInfo &TLI) {
   if (!CompileKernel && F.getName() == kMsanModuleCtorName)
     return false;
 
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return false;
+
   if (F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation))
     return false;
 
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index e326f30ad88eeb..eb346254f30dd7 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -258,6 +258,9 @@ bool SanitizerBinaryMetadata::run() {
 void SanitizerBinaryMetadata::runOn(Function &F, MetadataInfoSet &MIS) {
   if (F.empty())
     return;
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return;
   if (F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation))
     return;
   if (Ignorelist && Ignorelist->inSection("metadata", "fun", F.getName()))
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 6a89cee9aaf6cc..db4bf709c9cc9c 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -629,6 +629,9 @@ void ModuleSanitizerCoverage::instrumentFunction(Function &F) {
     return;
   if (Blocklist && Blocklist->inSection("coverage", "fun", F.getName()))
     return;
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return;
   if (F.hasFnAttribute(Attribute::NoSanitizeCoverage))
     return;
   if (F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation))
diff --git a/llvm/test/Instrumentation/sanitizers-no-naked.ll b/llvm/test/Instrumentation/sanitizers-no-naked.ll
new file mode 100644
index 00000000000000..72a78d9f1a33e5
--- /dev/null
+++ b/llvm/test/Instrumentation/sanitizers-no-naked.ll
@@ -0,0 +1,49 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=asan -S | FileCheck %s
+; RUN: opt < %s -passes=tsan -S | FileCheck %s
+; RUN: opt < %s -passes=dfsan -dfsan-track-origins=1  -S | FileCheck %s
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-control-flow -S | FileCheck %s
+; RUN: opt < %s -passes='module(sanmd-module)' -sanitizer-metadata-atomics -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @naked_function() naked {
+; CHECK-LABEL: define void @naked_function(
+; CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}
+
+define void @naked_function_with_asan() sanitize_address naked {
+; CHECK-LABEL: define void @naked_function_with_asan(
+; CHECK-SAME: ) #[[ATTR1:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}
+
+define void @naked_function_with_tsan() sanitize_thread naked {
+; CHECK-LABEL: define void @naked_function_with_tsan(
+; CHECK-SAME: ) #[[ATTR2:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}
+
+define void @naked_function_with_msan() sanitize_memory naked {
+; CHECK-LABEL: define void @naked_function_with_msan(
+; CHECK-SAME: ) #[[ATTR3:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Antonio Frighetto (antoniofrighetto)

Changes

Sanitizers instrumentation is incompatible with naked functions, which are expected to contain only inline asm.


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

6 Files Affected:

  • (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+4)
  • (modified) llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp (+2-1)
  • (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+4)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp (+3)
  • (modified) llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp (+3)
  • (added) llvm/test/Instrumentation/sanitizers-no-naked.ll (+49)
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 5de0a78d087968..1e9e2d03c6ce42 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -2960,6 +2960,10 @@ bool AddressSanitizer::instrumentFunction(Function &F,
 
   bool FunctionModified = false;
 
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return FunctionModified;
+
   // If needed, insert __asan_init before checking for SanitizeAddress attr.
   // This function needs to be called even if the function body is not
   // instrumented.
diff --git a/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
index b4b5f67d2e62da..1cb579309089e1 100644
--- a/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
@@ -1545,8 +1545,9 @@ bool DataFlowSanitizer::runImpl(
   SmallPtrSet<Function *, 2> FnsWithForceZeroLabel;
   SmallPtrSet<Constant *, 1> PersonalityFns;
   for (Function &F : M)
+    // Do not apply any instrumentation for naked functions or if disabled.
     if (!F.isIntrinsic() && !DFSanRuntimeFunctions.contains(&F) &&
-        !LibAtomicFunction(F) &&
+        !LibAtomicFunction(F) && !F.hasFnAttribute(Attribute::Naked) &&
         !F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation)) {
       FnsToInstrument.push_back(&F);
       if (F.hasPersonalityFn())
diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 17c5638cf8ced7..90de58143955c3 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -6078,6 +6078,10 @@ bool MemorySanitizer::sanitizeFunction(Function &F, TargetLibraryInfo &TLI) {
   if (!CompileKernel && F.getName() == kMsanModuleCtorName)
     return false;
 
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return false;
+
   if (F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation))
     return false;
 
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index e326f30ad88eeb..eb346254f30dd7 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -258,6 +258,9 @@ bool SanitizerBinaryMetadata::run() {
 void SanitizerBinaryMetadata::runOn(Function &F, MetadataInfoSet &MIS) {
   if (F.empty())
     return;
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return;
   if (F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation))
     return;
   if (Ignorelist && Ignorelist->inSection("metadata", "fun", F.getName()))
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 6a89cee9aaf6cc..db4bf709c9cc9c 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -629,6 +629,9 @@ void ModuleSanitizerCoverage::instrumentFunction(Function &F) {
     return;
   if (Blocklist && Blocklist->inSection("coverage", "fun", F.getName()))
     return;
+  // Do not apply any instrumentation for naked functions.
+  if (F.hasFnAttribute(Attribute::Naked))
+    return;
   if (F.hasFnAttribute(Attribute::NoSanitizeCoverage))
     return;
   if (F.hasFnAttribute(Attribute::DisableSanitizerInstrumentation))
diff --git a/llvm/test/Instrumentation/sanitizers-no-naked.ll b/llvm/test/Instrumentation/sanitizers-no-naked.ll
new file mode 100644
index 00000000000000..72a78d9f1a33e5
--- /dev/null
+++ b/llvm/test/Instrumentation/sanitizers-no-naked.ll
@@ -0,0 +1,49 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=asan -S | FileCheck %s
+; RUN: opt < %s -passes=tsan -S | FileCheck %s
+; RUN: opt < %s -passes=dfsan -dfsan-track-origins=1  -S | FileCheck %s
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-control-flow -S | FileCheck %s
+; RUN: opt < %s -passes='module(sanmd-module)' -sanitizer-metadata-atomics -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @naked_function() naked {
+; CHECK-LABEL: define void @naked_function(
+; CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}
+
+define void @naked_function_with_asan() sanitize_address naked {
+; CHECK-LABEL: define void @naked_function_with_asan(
+; CHECK-SAME: ) #[[ATTR1:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}
+
+define void @naked_function_with_tsan() sanitize_thread naked {
+; CHECK-LABEL: define void @naked_function_with_tsan(
+; CHECK-SAME: ) #[[ATTR2:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}
+
+define void @naked_function_with_msan() sanitize_memory naked {
+; CHECK-LABEL: define void @naked_function_with_msan(
+; CHECK-SAME: ) #[[ATTR3:[0-9]+]] {
+; CHECK-NEXT:    call void asm sideeffect "nop", ""()
+; CHECK-NEXT:    unreachable
+;
+  call void asm sideeffect "nop", ""()
+  unreachable
+}

@dtcxzyw dtcxzyw requested a review from vitalybuka September 13, 2024 15:02
@fmayer
Copy link
Contributor

fmayer commented Sep 13, 2024

What about HWAddressSanitizer?

@antoniofrighetto
Copy link
Contributor Author

What about HWAddressSanitizer?

Added!

@fmayer
Copy link
Contributor

fmayer commented Sep 13, 2024

I am not sure the commit message is completely accurate though. IR docs say

This attribute disables prologue / epilogue emission for the function. This can have very system-specific consequences. The arguments of a naked function can not be referenced through IR values.

This means non arguments could still be referenced through IR.

It would probably help if you could point towards a specific problem that this is solving.

@antoniofrighetto
Copy link
Contributor Author

See downstream issue: rust-lang/rust#129224. Yeah, the commit message can be made more accurate. I was having in mind Rust Naked functions when I wrote it; indeed inline asm only is not a strict requirement in LLVM. That said, I'm not sure you'd be able to leverage Asan instrumentation properly (e.g., for stack accesses), if the function lacks of prologue/epilogue in the first place.

@fmayer
Copy link
Contributor

fmayer commented Sep 13, 2024

LGTM % a more accurate commit message

; RUN: opt < %s -passes=hwasan -S | FileCheck %s --check-prefixes=CHECK-HWASAN
; RUN: opt < %s -passes=dfsan -dfsan-track-origins=1 -S | FileCheck %s
; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-control-flow -S | FileCheck %s
; RUN: opt < %s -passes='module(sanmd-module)' -sanitizer-metadata-atomics -S | FileCheck %s
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you precommit the test with the current behavior so it would be easy to see changes made by this patch

@vitalybuka
Copy link
Collaborator

vitalybuka commented Sep 13, 2024

I suspect msan/dfsan still needs to set shadow for return value.
asan/hwasan probably has nothing to touch a naked function anyway.

@vitalybuka
Copy link
Collaborator

vitalybuka commented Sep 13, 2024

also msan instruments inline assembly, I think we still need it for naked function in general, see ClHandleAsmConservative

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt < %s -passes=asan -S | FileCheck %s
; RUN: opt < %s -passes=tsan -S | FileCheck %s
; RUN: opt < %s -passes=hwasan -S | FileCheck %s --check-prefixes=CHECK-HWASAN
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is no -passes=msan

@antoniofrighetto
Copy link
Contributor Author

Tests in a separate commit, rephrased commit, dropped msan/dfsan. Does this look better?

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define void @naked_function() naked {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a byval argument to these functions? I think that will make it more likely that something actually gets instrumented and results in a verifier error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be missing something, how is the argument supposed to lead that something gets instrumented if the argument cannot be used?

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://llvm.godbolt.org/z/K4WdqTdve. Without the byval argument there is nothing to instrument anyway, so you're not really testing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't your function miss naked attribute? The verifier would lead to an error in that case.

Copy link
Contributor

@nikic nikic Sep 15, 2024

Choose a reason for hiding this comment

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

Yes, that's the point. You want a test case that leads to a verifier error before your change and no longer causes one after your change. (You should of course add naked to the actual test. You just also need the byval argument.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I see it now, thanks. Should be better. I was slightly confused by the naked_function without being naked.

@@ -0,0 +1,89 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt < %s -passes=asan -S | FileCheck %s --check-prefixes=CHECK-ASAN
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; RUN: opt < %s -passes=asan -S | FileCheck %s --check-prefixes=CHECK-ASAN
; RUN: opt < %s -passes=asan -S | FileCheck %s --check-prefixes=CHECK,CHECK-ASAN

etc. to remove duplicate check lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, but they seem unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm okay, sounds like a bug in UTC.

@antoniofrighetto antoniofrighetto force-pushed the feature/naked-no-san branch 2 times, most recently from bd9d496 to 4896ae9 Compare September 17, 2024 07:11
@antoniofrighetto
Copy link
Contributor Author

@nikic Think this is OK to land for you too?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@antoniofrighetto
Copy link
Contributor Author

Thanks!

Sanitizers instrumentation may be incompatible with naked functions,
which lack of standard prologue/epilogue.
@antoniofrighetto antoniofrighetto merged commit 942e872 into llvm:main Sep 17, 2024
4 of 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.

5 participants