From a453c2df744f46f87813e3c3dface12d570d3dc6 Mon Sep 17 00:00:00 2001 From: Florian Mayer Date: Fri, 9 Feb 2024 00:40:45 +0000 Subject: [PATCH] Reland "[MTE] remap stacks with PROT_MTE when requested by dlopened library" This reverts commit c20e1c2bdfc00b3fb7931da5e88ceac3fa4df4b2. Reason for revert: Was not the root-cause of test failure. Change-Id: I7dcd9fc3cbac47703fa8ecd5aafd7e1c3ed87301 --- libc/bionic/heap_tagging.cpp | 2 + libc/bionic/libc_init_dynamic.cpp | 13 ++- libc/bionic/pthread_attr.cpp | 57 +---------- libc/bionic/pthread_internal.cpp | 84 ++++++++++++++++ libc/bionic/pthread_internal.h | 5 + libc/private/bionic_globals.h | 3 + linker/linker.cpp | 8 ++ tests/Android.bp | 25 +++++ tests/libs/Android.bp | 11 +++ tests/memtag_stack_dlopen_test.cpp | 148 +++++++++++++++++++++++++++++ 10 files changed, 302 insertions(+), 54 deletions(-) create mode 100644 tests/memtag_stack_dlopen_test.cpp diff --git a/libc/bionic/heap_tagging.cpp b/libc/bionic/heap_tagging.cpp index 230899a4c..6fdd1e01d 100644 --- a/libc/bionic/heap_tagging.cpp +++ b/libc/bionic/heap_tagging.cpp @@ -57,6 +57,7 @@ void SetDefaultHeapTaggingLevel() { break; case M_HEAP_TAGGING_LEVEL_SYNC: case M_HEAP_TAGGING_LEVEL_ASYNC: + atomic_store(&globals->memtag, true); atomic_store(&globals->memtag_stack, __libc_shared_globals()->initial_memtag_stack); break; default: @@ -116,6 +117,7 @@ bool SetHeapTaggingLevel(HeapTaggingLevel tag_level) { globals->heap_pointer_tag = static_cast(0xffull << UNTAG_SHIFT); } atomic_store(&globals->memtag_stack, false); + atomic_store(&globals->memtag, false); }); if (heap_tagging_level != M_HEAP_TAGGING_LEVEL_TBI) { diff --git a/libc/bionic/libc_init_dynamic.cpp b/libc/bionic/libc_init_dynamic.cpp index c61810e34..295484b82 100644 --- a/libc/bionic/libc_init_dynamic.cpp +++ b/libc/bionic/libc_init_dynamic.cpp @@ -39,11 +39,12 @@ * all dynamic linking has been performed. */ +#include #include +#include #include #include -#include -#include +#include "bionic/pthread_internal.h" #include "libc_init_common.h" #include "private/bionic_defs.h" @@ -59,6 +60,10 @@ extern "C" { extern int __cxa_atexit(void (*)(void *), void *, void *); }; +void memtag_stack_dlopen_callback() { + __pthread_internal_remap_stack_with_mte(); +} + // Use an initializer so __libc_sysinfo will have a fallback implementation // while .preinit_array constructors run. #if defined(__i386__) @@ -156,6 +161,10 @@ __noreturn void __libc_init(void* raw_args, __libc_init_mte_late(); + // This roundabout way is needed so we don't use the static libc linked into the linker, which + // will not affect the process. + __libc_shared_globals()->memtag_stack_dlopen_callback = memtag_stack_dlopen_callback; + exit(slingshot(args.argc - __libc_shared_globals()->initial_linker_arg_count, args.argv + __libc_shared_globals()->initial_linker_arg_count, args.envp)); diff --git a/libc/bionic/pthread_attr.cpp b/libc/bionic/pthread_attr.cpp index de4cc9e8d..f6c0401bb 100644 --- a/libc/bionic/pthread_attr.cpp +++ b/libc/bionic/pthread_attr.cpp @@ -155,36 +155,6 @@ int pthread_attr_setstack(pthread_attr_t* attr, void* stack_base, size_t stack_s return 0; } -static uintptr_t __get_main_stack_startstack() { - FILE* fp = fopen("/proc/self/stat", "re"); - if (fp == nullptr) { - async_safe_fatal("couldn't open /proc/self/stat: %m"); - } - - char line[BUFSIZ]; - if (fgets(line, sizeof(line), fp) == nullptr) { - async_safe_fatal("couldn't read /proc/self/stat: %m"); - } - - fclose(fp); - - // See man 5 proc. There's no reason comm can't contain ' ' or ')', - // so we search backwards for the end of it. We're looking for this field: - // - // startstack %lu (28) The address of the start (i.e., bottom) of the stack. - uintptr_t startstack = 0; - const char* end_of_comm = strrchr(line, ')'); - if (sscanf(end_of_comm + 1, " %*c " - "%*d %*d %*d %*d %*d " - "%*u %*u %*u %*u %*u %*u %*u " - "%*d %*d %*d %*d %*d %*d " - "%*u %*u %*d %*u %*u %*u %" SCNuPTR, &startstack) != 1) { - async_safe_fatal("couldn't parse /proc/self/stat"); - } - - return startstack; -} - static int __pthread_attr_getstack_main_thread(void** stack_base, size_t* stack_size) { ErrnoRestorer errno_restorer; @@ -198,28 +168,11 @@ static int __pthread_attr_getstack_main_thread(void** stack_base, size_t* stack_ if (stack_limit.rlim_cur == RLIM_INFINITY) { stack_limit.rlim_cur = 8 * 1024 * 1024; } - - // Ask the kernel where our main thread's stack started. - uintptr_t startstack = __get_main_stack_startstack(); - - // Hunt for the region that contains that address. - FILE* fp = fopen("/proc/self/maps", "re"); - if (fp == nullptr) { - async_safe_fatal("couldn't open /proc/self/maps: %m"); - } - char line[BUFSIZ]; - while (fgets(line, sizeof(line), fp) != nullptr) { - uintptr_t lo, hi; - if (sscanf(line, "%" SCNxPTR "-%" SCNxPTR, &lo, &hi) == 2) { - if (lo <= startstack && startstack <= hi) { - *stack_size = stack_limit.rlim_cur; - *stack_base = reinterpret_cast(hi - *stack_size); - fclose(fp); - return 0; - } - } - } - async_safe_fatal("stack not found in /proc/self/maps"); + uintptr_t lo, hi; + __find_main_stack_limits(&lo, &hi); + *stack_size = stack_limit.rlim_cur; + *stack_base = reinterpret_cast(hi - *stack_size); + return 0; } __BIONIC_WEAK_FOR_NATIVE_BRIDGE diff --git a/libc/bionic/pthread_internal.cpp b/libc/bionic/pthread_internal.cpp index 6a7ee2f5b..bfe2f982e 100644 --- a/libc/bionic/pthread_internal.cpp +++ b/libc/bionic/pthread_internal.cpp @@ -40,6 +40,7 @@ #include "private/ErrnoRestorer.h" #include "private/ScopedRWLock.h" #include "private/bionic_futex.h" +#include "private/bionic_globals.h" #include "private/bionic_tls.h" static pthread_internal_t* g_thread_list = nullptr; @@ -119,6 +120,89 @@ pthread_internal_t* __pthread_internal_find(pthread_t thread_id, const char* cal return nullptr; } +static uintptr_t __get_main_stack_startstack() { + FILE* fp = fopen("/proc/self/stat", "re"); + if (fp == nullptr) { + async_safe_fatal("couldn't open /proc/self/stat: %m"); + } + + char line[BUFSIZ]; + if (fgets(line, sizeof(line), fp) == nullptr) { + async_safe_fatal("couldn't read /proc/self/stat: %m"); + } + + fclose(fp); + + // See man 5 proc. There's no reason comm can't contain ' ' or ')', + // so we search backwards for the end of it. We're looking for this field: + // + // startstack %lu (28) The address of the start (i.e., bottom) of the stack. + uintptr_t startstack = 0; + const char* end_of_comm = strrchr(line, ')'); + if (sscanf(end_of_comm + 1, + " %*c " + "%*d %*d %*d %*d %*d " + "%*u %*u %*u %*u %*u %*u %*u " + "%*d %*d %*d %*d %*d %*d " + "%*u %*u %*d %*u %*u %*u %" SCNuPTR, + &startstack) != 1) { + async_safe_fatal("couldn't parse /proc/self/stat"); + } + + return startstack; +} + +void __find_main_stack_limits(uintptr_t* low, uintptr_t* high) { + // Ask the kernel where our main thread's stack started. + uintptr_t startstack = __get_main_stack_startstack(); + + // Hunt for the region that contains that address. + FILE* fp = fopen("/proc/self/maps", "re"); + if (fp == nullptr) { + async_safe_fatal("couldn't open /proc/self/maps: %m"); + } + char line[BUFSIZ]; + while (fgets(line, sizeof(line), fp) != nullptr) { + uintptr_t lo, hi; + if (sscanf(line, "%" SCNxPTR "-%" SCNxPTR, &lo, &hi) == 2) { + if (lo <= startstack && startstack <= hi) { + *low = lo; + *high = hi; + fclose(fp); + return; + } + } + } + async_safe_fatal("stack not found in /proc/self/maps"); +} + +void __pthread_internal_remap_stack_with_mte() { +#if defined(__aarch64__) + // If process doesn't have MTE enabled, we don't need to do anything. + if (!__libc_globals->memtag) return; + bool prev = true; + __libc_globals.mutate( + [&prev](libc_globals* globals) { prev = atomic_exchange(&globals->memtag_stack, true); }); + if (prev) return; + uintptr_t lo, hi; + __find_main_stack_limits(&lo, &hi); + + if (mprotect(reinterpret_cast(lo), hi - lo, + PROT_READ | PROT_WRITE | PROT_MTE | PROT_GROWSDOWN)) { + async_safe_fatal("error: failed to set PROT_MTE on main thread"); + } + ScopedWriteLock creation_locker(&g_thread_creation_lock); + ScopedReadLock list_locker(&g_thread_list_lock); + for (pthread_internal_t* t = g_thread_list; t != nullptr; t = t->next) { + if (t->terminating || t->is_main()) continue; + if (mprotect(t->mmap_base_unguarded, t->mmap_size_unguarded, + PROT_READ | PROT_WRITE | PROT_MTE)) { + async_safe_fatal("error: failed to set PROT_MTE on thread: %d", t->tid); + } + } +#endif +} + bool android_run_on_all_threads(bool (*func)(void*), void* arg) { // Take the locks in this order to avoid inversion (pthread_create -> // __pthread_internal_add). diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h index 3b9e6a481..091f711eb 100644 --- a/libc/bionic/pthread_internal.h +++ b/libc/bionic/pthread_internal.h @@ -178,6 +178,7 @@ class pthread_internal_t { bionic_tls* bionic_tls; int errno_value; + bool is_main() { return start_routine == nullptr; } }; struct ThreadMapping { @@ -207,6 +208,7 @@ __LIBC_HIDDEN__ pthread_internal_t* __pthread_internal_find(pthread_t pthread_id __LIBC_HIDDEN__ pid_t __pthread_internal_gettid(pthread_t pthread_id, const char* caller); __LIBC_HIDDEN__ void __pthread_internal_remove(pthread_internal_t* thread); __LIBC_HIDDEN__ void __pthread_internal_remove_and_free(pthread_internal_t* thread); +__LIBC_HIDDEN__ void __find_main_stack_limits(uintptr_t* low, uintptr_t* high); static inline __always_inline bionic_tcb* __get_bionic_tcb() { return reinterpret_cast(&__get_tls()[MIN_TLS_SLOT]); @@ -266,6 +268,9 @@ __LIBC_HIDDEN__ extern void __bionic_atfork_run_prepare(); __LIBC_HIDDEN__ extern void __bionic_atfork_run_child(); __LIBC_HIDDEN__ extern void __bionic_atfork_run_parent(); +// Re-map all threads and successively launched threads with PROT_MTE. +__LIBC_HIDDEN__ void __pthread_internal_remap_stack_with_mte(); + extern "C" bool android_run_on_all_threads(bool (*func)(void*), void* arg); extern pthread_rwlock_t g_thread_creation_lock; diff --git a/libc/private/bionic_globals.h b/libc/private/bionic_globals.h index 8ea7d4d66..08a61f056 100644 --- a/libc/private/bionic_globals.h +++ b/libc/private/bionic_globals.h @@ -50,6 +50,7 @@ struct libc_globals { uintptr_t heap_pointer_tag; _Atomic(bool) memtag_stack; _Atomic(bool) decay_time_enabled; + _Atomic(bool) memtag; // In order to allow a complete switch between dispatch tables without // the need for copying each function by function in the structure, @@ -135,6 +136,8 @@ struct libc_shared_globals { HeapTaggingLevel initial_heap_tagging_level = M_HEAP_TAGGING_LEVEL_NONE; bool initial_memtag_stack = false; int64_t heap_tagging_upgrade_timer_sec = 0; + + void (*memtag_stack_dlopen_callback)() = nullptr; }; __LIBC_HIDDEN__ libc_shared_globals* __libc_shared_globals(); diff --git a/linker/linker.cpp b/linker/linker.cpp index 135eaa380..ffbf136b0 100644 --- a/linker/linker.cpp +++ b/linker/linker.cpp @@ -2212,6 +2212,14 @@ void* do_dlopen(const char* name, int flags, loading_trace.End(); if (si != nullptr) { + if (si->has_min_version(7) && si->memtag_stack()) { + LD_LOG(kLogDlopen, "... dlopen enabling MTE for: realpath=\"%s\", soname=\"%s\"", + si->get_realpath(), si->get_soname()); + if (auto* cb = __libc_shared_globals()->memtag_stack_dlopen_callback) { + cb(); + } + } + void* handle = si->to_handle(); LD_LOG(kLogDlopen, "... dlopen calling constructors: realpath=\"%s\", soname=\"%s\", handle=%p", diff --git a/tests/Android.bp b/tests/Android.bp index a53418aa0..c09ddadb6 100644 --- a/tests/Android.bp +++ b/tests/Android.bp @@ -1106,6 +1106,31 @@ cc_test { test_suites: ["device-tests"], } +cc_test { + name: "memtag_stack_dlopen_test", + enabled: false, + // This does not use bionic_tests_defaults because it is not supported on + // host. + arch: { + arm64: { + enabled: true, + }, + }, + sanitize: { + memtag_heap: true, + memtag_stack: false, + }, + srcs: [ + "memtag_stack_dlopen_test.cpp", + ], + shared_libs: [ + "libbase", + ], + data_libs: ["libtest_simple_memtag_stack"], + header_libs: ["bionic_libc_platform_headers"], + test_suites: ["device-tests"], +} + cc_test { name: "bionic-stress-tests", defaults: [ diff --git a/tests/libs/Android.bp b/tests/libs/Android.bp index 0bef469da..b497d2ec0 100644 --- a/tests/libs/Android.bp +++ b/tests/libs/Android.bp @@ -233,6 +233,17 @@ cc_test_library { srcs: ["dlopen_testlib_simple.cpp"], } +// ----------------------------------------------------------------------------- +// Library used by memtag_stack_dlopen_test tests +// ----------------------------------------------------------------------------- +cc_test_library { + name: "libtest_simple_memtag_stack", + sanitize: { + memtag_stack: true, + }, + srcs: ["dlopen_testlib_simple.cpp"], +} + // ----------------------------------------------------------------------------- // Libraries used by hwasan_test // ----------------------------------------------------------------------------- diff --git a/tests/memtag_stack_dlopen_test.cpp b/tests/memtag_stack_dlopen_test.cpp new file mode 100644 index 000000000..308af1eb9 --- /dev/null +++ b/tests/memtag_stack_dlopen_test.cpp @@ -0,0 +1,148 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include + +#include +#include + +#include + +#include +#include +#include "utils.h" + +#if defined(__BIONIC__) && defined(__aarch64__) +__attribute__((target("mte"))) bool is_stack_mte_on() { + alignas(16) int x = 0; + void* p = reinterpret_cast(reinterpret_cast(&x) + (1UL << 57)); + void* p_cpy = p; + __builtin_arm_stg(p); + p = __builtin_arm_ldg(p); + __builtin_arm_stg(&x); + return p == p_cpy; +} + +// We can't use pthread_getattr_np because that uses the rlimit rather than the actual mapping +// bounds. +static void find_main_stack_limits(uintptr_t* low, uintptr_t* high) { + uintptr_t startstack = reinterpret_cast(__builtin_frame_address(0)); + + // Hunt for the region that contains that address. + FILE* fp = fopen("/proc/self/maps", "re"); + if (fp == nullptr) { + abort(); + } + char line[BUFSIZ]; + while (fgets(line, sizeof(line), fp) != nullptr) { + uintptr_t lo, hi; + if (sscanf(line, "%" SCNxPTR "-%" SCNxPTR, &lo, &hi) == 2) { + if (lo <= startstack && startstack <= hi) { + *low = lo; + *high = hi; + fclose(fp); + return; + } + } + } + abort(); +} + +template +unsigned int fault_new_stack_page(uintptr_t low, Fn f) { + uintptr_t new_low; + uintptr_t new_high; + volatile char buf[4096]; + buf[4095] = 1; + find_main_stack_limits(&new_low, &new_high); + if (new_low < low) { + f(); + return new_high; + } + // Useless, but should defeat TCO. + return new_low + fault_new_stack_page(low, f); +} + +#endif + +enum State { kInit, kThreadStarted, kStackRemapped }; + +TEST(MemtagStackDlopenTest, DlopenRemapsStack) { +#if defined(__BIONIC__) && defined(__aarch64__) + if (!running_with_mte()) GTEST_SKIP() << "Test requires MTE."; + + std::string path = android::base::GetExecutableDirectory() + "/libtest_simple_memtag_stack.so"; + ASSERT_EQ(0, access(path.c_str(), F_OK)); // Verify test setup. + EXPECT_FALSE(is_stack_mte_on()); + std::mutex m; + std::condition_variable cv; + State state = kInit; + + bool is_early_thread_mte_on = false; + std::thread early_th([&] { + { + std::lock_guard lk(m); + state = kThreadStarted; + } + cv.notify_one(); + { + std::unique_lock lk(m); + cv.wait(lk, [&] { return state == kStackRemapped; }); + } + is_early_thread_mte_on = is_stack_mte_on(); + }); + { + std::unique_lock lk(m); + cv.wait(lk, [&] { return state == kThreadStarted; }); + } + void* handle = dlopen(path.c_str(), RTLD_NOW); + { + std::lock_guard lk(m); + state = kStackRemapped; + } + cv.notify_one(); + ASSERT_NE(handle, nullptr); + EXPECT_TRUE(is_stack_mte_on()); + + bool new_stack_page_mte_on = false; + uintptr_t low; + uintptr_t high; + find_main_stack_limits(&low, &high); + fault_new_stack_page(low, [&] { new_stack_page_mte_on = is_stack_mte_on(); }); + EXPECT_TRUE(new_stack_page_mte_on); + + bool is_late_thread_mte_on = false; + std::thread late_th([&] { is_late_thread_mte_on = is_stack_mte_on(); }); + late_th.join(); + early_th.join(); + EXPECT_TRUE(is_early_thread_mte_on); + EXPECT_TRUE(is_late_thread_mte_on); +#else + GTEST_SKIP() << "requires bionic arm64"; +#endif +}