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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions compiler-rt/lib/hwasan/hwasan_flags.inc
Original file line number Diff line number Diff line change
Expand Up @@ -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

"If not -1, 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.")
8 changes: 6 additions & 2 deletions compiler-rt/lib/hwasan/hwasan_linux.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,12 @@ static uptr GetHighMemEnd() {
}

static void InitializeShadowBaseAddress(uptr shadow_size_bytes) {
__hwasan_shadow_memory_dynamic_address =
FindDynamicShadowStart(shadow_size_bytes);
if (flags()->fixed_shadow_base != (uptr)-1) {
__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
Contributor

Choose a reason for hiding this comment

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

For some reason, for static Android binaries, this branch gets taken

} else {
__hwasan_shadow_memory_dynamic_address =
FindDynamicShadowStart(shadow_size_bytes);
}
}

static void MaybeDieIfNoTaggingAbi(const char *message) {
Expand Down
76 changes: 76 additions & 0 deletions compiler-rt/test/hwasan/TestCases/Linux/fixed-shadow.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Test fixed shadow base functionality.
//
// Default compiler instrumentation works with any shadow base (dynamic or fixed).
// RUN: %clang_hwasan %s -o %t && %run %t
// RUN: %clang_hwasan %s -o %t && HWASAN_OPTIONS=fixed_shadow_base=263878495698944 %run %t
// RUN: %clang_hwasan %s -o %t && HWASAN_OPTIONS=fixed_shadow_base=4398046511104 %run %t
//
// If -hwasan-mapping-offset is set, then the fixed_shadow_base needs to match.
// RUN: %clang_hwasan %s -mllvm -hwasan-mapping-offset=263878495698944 -o %t && HWASAN_OPTIONS=fixed_shadow_base=263878495698944 %run %t
// RUN: %clang_hwasan %s -mllvm -hwasan-mapping-offset=4398046511104 -o %t && HWASAN_OPTIONS=fixed_shadow_base=4398046511104 %run %t
// RUN: %clang_hwasan %s -mllvm -hwasan-mapping-offset=263878495698944 -o %t && HWASAN_OPTIONS=fixed_shadow_base=4398046511104 not %run %t
// RUN: %clang_hwasan %s -mllvm -hwasan-mapping-offset=4398046511104 -o %t && HWASAN_OPTIONS=fixed_shadow_base=263878495698944 not %run %t
//
// Note: if fixed_shadow_base is not set, compiler-rt will dynamically choose a
// shadow base, which has a tiny but non-zero probability of matching the
// compiler instrumentation. To avoid test flake, we do not test this case.
//
// Assume 48-bit VMA
// REQUIRES: aarch64-target-arch
//
// REQUIRES: Clang
//
// UNSUPPORTED: android

#include <assert.h>
#include <sanitizer/allocator_interface.h>
#include <sanitizer/hwasan_interface.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>

int main() {
__hwasan_enable_allocator_tagging();

// We test that the compiler instrumentation is able to access shadow memory
// for many different addresses. If we only test a small number of addresses,
// it might work by chance even if the shadow base does not match between the
// compiler instrumentation and compiler-rt.
void **mmaps[256];
// 48-bit VMA
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).

MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
// We don't use MAP_FIXED, to avoid overwriting critical memory.
// However, if we don't get allocated the requested address, it
// isn't a useful test.
if ((unsigned long long)p != addr) {
munmap(p, 4096);
mmaps[i] = MAP_FAILED;
} else {
mmaps[i] = p;
}
}

int failures = 0;
for (int i = 0; i < 256; i++) {
if (mmaps[i] == MAP_FAILED) {
failures++;
} else {
printf("%d %p\n", i, mmaps[i]);
munmap(mmaps[i], 4096);
}
}

// We expect roughly 17 failures:
// - the page at address zero
// - 16 failures because the shadow memory takes up 1/16th of the address space
// We could also get unlucky e.g., if libraries or binaries are loaded into the
// exact addresses where we tried to map.
// To avoid test flake, we allow some margin of error.
printf("Failed: %d\n", failures);
assert(failures < 48);
return 0;
}