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

[hwasan] Add fixed_shadow_base flag #73980

Merged
merged 6 commits into from
Dec 7, 2023
Merged

Conversation

thurstond
Copy link
Contributor

When set to non-zero, the HWASan runtime will map the shadow base at the
specified constant address.

This is particularly useful in conjunction with the existing compiler option
'hwasan-mapping-offset', which bakes a hardcoded constant address into
the instrumentation.

When set to non-zero, the HWASan runtime will map the shadow base at the
specified constant address.

This is particularly useful in conjunction with the existing compiler option
'hwasan-mapping-offset', which bakes a hardcoded constant address into
the instrumentation.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 30, 2023

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

Author: Thurston Dang (thurstond)

Changes

When set to non-zero, the HWASan runtime will map the shadow base at the
specified constant address.

This is particularly useful in conjunction with the existing compiler option
'hwasan-mapping-offset', which bakes a hardcoded constant address into
the instrumentation.


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

2 Files Affected:

  • (modified) compiler-rt/lib/hwasan/hwasan_flags.inc (+7)
  • (modified) compiler-rt/lib/hwasan/hwasan_linux.cpp (+8-2)
diff --git a/compiler-rt/lib/hwasan/hwasan_flags.inc b/compiler-rt/lib/hwasan/hwasan_flags.inc
index 978fa46b705cb9e..bb224fbb8eaa8b9 100644
--- a/compiler-rt/lib/hwasan/hwasan_flags.inc
+++ b/compiler-rt/lib/hwasan/hwasan_flags.inc
@@ -84,3 +84,10 @@ HWASAN_FLAG(bool, malloc_bisect_dump, false,
 // are untagged before the call.
 HWASAN_FLAG(bool, fail_without_syscall_abi, true,
             "Exit if fail to request relaxed syscall ABI.")
+
+HWASAN_FLAG(
+    uptr, fixed_shadow_base, 0,
+    "If non-zero, HWASan will attempt to allocate the shadow at this address, "
+    "instead of choosing one dynamically."
+    "Tip: this can be combined with the compiler option, "
+    "-hwasan-mapping-offset, to optimize the instrumentation.")
diff --git a/compiler-rt/lib/hwasan/hwasan_linux.cpp b/compiler-rt/lib/hwasan/hwasan_linux.cpp
index 81226da976d1161..e7cf36ef3161cbf 100644
--- a/compiler-rt/lib/hwasan/hwasan_linux.cpp
+++ b/compiler-rt/lib/hwasan/hwasan_linux.cpp
@@ -106,8 +106,14 @@ static uptr GetHighMemEnd() {
 }
 
 static void InitializeShadowBaseAddress(uptr shadow_size_bytes) {
-  __hwasan_shadow_memory_dynamic_address =
-      FindDynamicShadowStart(shadow_size_bytes);
+  // NULL is generally address zero, so it is not a valid location for the
+  // shadow.
+  if (flags()->fixed_shadow_base != 0) {
+    __hwasan_shadow_memory_dynamic_address = flags()->fixed_shadow_base;
+  } else {
+    __hwasan_shadow_memory_dynamic_address =
+        FindDynamicShadowStart(shadow_size_bytes);
+  }
 }
 
 static void MaybeDieIfNoTaggingAbi(const char *message) {

@@ -84,3 +84,10 @@ HWASAN_FLAG(bool, malloc_bisect_dump, false,
// are untagged before the call.
HWASAN_FLAG(bool, fail_without_syscall_abi, true,
"Exit if fail to request relaxed syscall ABI.")

HWASAN_FLAG(
uptr, fixed_shadow_base, 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

-1, not sure that none will need NULL as a base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// NULL is generally address zero, so it is not a valid location for the
// shadow.
if (flags()->fixed_shadow_base != 0) {
__hwasan_shadow_memory_dynamic_address = flags()->fixed_shadow_base;
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh,and maybe some test with use of the file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably linux not android only test

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 compiler-rt/test/hwasan/TestCases/Linux/fixed-shadow.c

Copy link

github-actions bot commented Dec 5, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

for (int i = 0; i < 256; i++) {
unsigned long long addr = (i * (1ULL << 40));

void *p = mmap((void *)addr, 4096, PROT_READ | PROT_WRITE,
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 possibility to re-map critical pages with FIXED and crash the process.
Would in't be enough to do the same without FIXED and use addr is a hint?

Alternative trivial approach?

print `extern uptr __hwasan_shadow_memory_dynamic_address;`
//CHECK: expected value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also there is DumpProcessMap(), maybe it's easy to see shadow there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is possibility to re-map critical pages with FIXED and crash the process. Would in't be enough to do the same without FIXED and use addr is a hint?

I've removed MAP_FIXED.

(My concern was that mmap might return addresses that are consecutive pages. In that case, this test will be useless at verifying that the entire address space can be correctly mapped to shadow memory.)

Alternative trivial approach?

print `extern uptr __hwasan_shadow_memory_dynamic_address;`
//CHECK: expected value

This will show that compiler-rt has the correct shadow address, but it doesn't prove that the compiler instrumentation is using the specified shadow base.
i.e., we want to test that -hwasan-mapping-offset and HWASAN_OPTIONS=fixed_shadow_base work together

Copy link
Contributor Author

@thurstond thurstond Dec 6, 2023

Choose a reason for hiding this comment

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

Likewise, this doesn't test the compiler instrumentation.

For example, if -hwasan-mapping-offset was not implemented properly, and the compiler instrumentation was still using the lookup in DTLS, it would defeat the purpose of a fixed shadow base, but it would still pass the DumpProcessMap() test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise, this doesn't test the compiler instrumentation.

That's fine to focus on runtime only. E.g. GCC also runs them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(My concern was that mmap might return addresses that are consecutive pages. In that case, this test will be useless at verifying that the entire address space can be correctly mapped to shadow memory.)

if ((unsigned long long)p != addr) { should solve that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(My concern was that mmap might return addresses that are consecutive pages. In that case, this test will be useless at verifying that the entire address space can be correctly mapped to shadow memory.)

if ((unsigned long long)p != addr) { should solve that?

This condition ensures that a passing test implies it the shadow mapping is highly likely to be correct, and will fail if it is unsure. The condition doesn't prevent the "fail if it is unsure" case (e.g., suppose mmap keeps returning addresses in the lower 1GB of memory).

@thurstond thurstond merged commit ea991a1 into llvm:main Dec 7, 2023
3 of 4 checks passed
@@ -84,3 +84,10 @@ HWASAN_FLAG(bool, malloc_bisect_dump, false,
// are untagged before the call.
HWASAN_FLAG(bool, fail_without_syscall_abi, true,
"Exit if fail to request relaxed syscall ABI.")

HWASAN_FLAG(
uptr, fixed_shadow_base, -1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@thurstond uptr -> uint64_t
we can compile for 64bit on 32bit platform

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.

None yet

3 participants