From e4adff0721930dcb6969245bfa298b6abd48d2d4 Mon Sep 17 00:00:00 2001 From: Mitch Phillips Date: Thu, 21 Jan 2021 20:41:50 -0800 Subject: [PATCH] [MTE] Cleanup tagged si_addr refs to fix mappings OOB bug. Currently, all MTE failures end up displaying 'Fault address falls at 0x after any mapped regions'. Clearly when scanning, we should use the untagged address to figure out which ranges it's in. I've taken the liberty of removing all si_addr parsing and moving it into the common ProcessInfo, as well as making it really explicit whether you want the (possibly tagged) original si_addr, or whether you want the untagged variant (for scanning /proc/maps or whatever). This is not particularly easily testable, as ReadCrashInfo isn't easily injectable and `dump_all_maps` should already be passed the untagged pointer to scan for. I've tested this locally on FVP under SYNC MTE with a simple UaF binary and noted the problem is fixed. Given that this is making the code more clear, I'm hoping the owners see no need for a regression test :). Bug: 135772972 Test: On FVP, run 'adb shell MEMTAG_OPTIONS=sync sanitizer-status' and check that the use-after-free test ends up with the /proc/maps desription in the right place. Change-Id: I220e4200c75a72474a95a67e5bbc36173a438dd2 --- debuggerd/crash_dump.cpp | 5 ++++- debuggerd/libdebuggerd/gwp_asan.cpp | 4 ++-- debuggerd/libdebuggerd/include/libdebuggerd/types.h | 3 ++- debuggerd/libdebuggerd/scudo.cpp | 4 ++-- debuggerd/libdebuggerd/tombstone.cpp | 9 +++++---- 5 files changed, 15 insertions(+), 10 deletions(-) diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index 4f600055d..007a20fce 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -299,7 +300,9 @@ static void ReadCrashInfo(unique_fd& fd, siginfo_t* siginfo, *siginfo = crash_info->data.s.siginfo; if (signal_has_si_addr(siginfo)) { process_info->has_fault_address = true; - process_info->fault_address = reinterpret_cast(siginfo->si_addr); + process_info->maybe_tagged_fault_address = reinterpret_cast(siginfo->si_addr); + process_info->untagged_fault_address = + untag_address(reinterpret_cast(siginfo->si_addr)); } regs->reset(unwindstack::Regs::CreateFromUcontext(unwindstack::Regs::CurrentArch(), &crash_info->data.s.ucontext)); diff --git a/debuggerd/libdebuggerd/gwp_asan.cpp b/debuggerd/libdebuggerd/gwp_asan.cpp index f27136558..9750fc4b0 100644 --- a/debuggerd/libdebuggerd/gwp_asan.cpp +++ b/debuggerd/libdebuggerd/gwp_asan.cpp @@ -72,8 +72,8 @@ GwpAsanCrashData::GwpAsanCrashData(unwindstack::Memory* process_memory, // Get the external crash address from the thread info. crash_address_ = 0u; - if (signal_has_si_addr(thread_info.siginfo)) { - crash_address_ = reinterpret_cast(thread_info.siginfo->si_addr); + if (process_info.has_fault_address) { + crash_address_ = process_info.untagged_fault_address; } // Ensure the error belongs to GWP-ASan. diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/types.h b/debuggerd/libdebuggerd/include/libdebuggerd/types.h index 30e75e11d..86522ee36 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/types.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/types.h @@ -46,5 +46,6 @@ struct ProcessInfo { uintptr_t scudo_region_info = 0; bool has_fault_address = false; - uintptr_t fault_address = 0; + uintptr_t untagged_fault_address = 0; + uintptr_t maybe_tagged_fault_address = 0; }; diff --git a/debuggerd/libdebuggerd/scudo.cpp b/debuggerd/libdebuggerd/scudo.cpp index f8bfe07ce..141c3bd18 100644 --- a/debuggerd/libdebuggerd/scudo.cpp +++ b/debuggerd/libdebuggerd/scudo.cpp @@ -44,7 +44,7 @@ ScudoCrashData::ScudoCrashData(unwindstack::Memory* process_memory, auto region_info = AllocAndReadFully(process_memory, process_info.scudo_region_info, __scudo_get_region_info_size()); - untagged_fault_addr_ = untag_address(process_info.fault_address); + untagged_fault_addr_ = process_info.untagged_fault_address; uintptr_t fault_page = untagged_fault_addr_ & ~(PAGE_SIZE - 1); uintptr_t memory_begin = fault_page - PAGE_SIZE * 16; @@ -67,7 +67,7 @@ ScudoCrashData::ScudoCrashData(unwindstack::Memory* process_memory, memory_tags[(i - memory_begin) / kTagGranuleSize] = process_memory->ReadTag(i); } - __scudo_get_error_info(&error_info_, process_info.fault_address, stack_depot.get(), + __scudo_get_error_info(&error_info_, process_info.maybe_tagged_fault_address, stack_depot.get(), region_info.get(), memory.get(), memory_tags.get(), memory_begin, memory_end - memory_begin); } diff --git a/debuggerd/libdebuggerd/tombstone.cpp b/debuggerd/libdebuggerd/tombstone.cpp index d88c5a93d..4bd71926f 100644 --- a/debuggerd/libdebuggerd/tombstone.cpp +++ b/debuggerd/libdebuggerd/tombstone.cpp @@ -151,7 +151,9 @@ static void dump_signal_info(log_t* log, const ThreadInfo& thread_info, const ProcessInfo& process_info, unwindstack::Memory* process_memory) { char addr_desc[64]; // ", fault addr 0x1234" if (process_info.has_fault_address) { - size_t addr = process_info.fault_address; + // SIGILL faults will never have tagged addresses, so okay to + // indiscriminately use the tagged address here. + size_t addr = process_info.maybe_tagged_fault_address; if (thread_info.siginfo->si_signo == SIGILL) { uint32_t instruction = {}; process_memory->Read(addr, &instruction, sizeof(instruction)); @@ -433,9 +435,8 @@ static bool dump_thread(log_t* log, unwindstack::Unwinder* unwinder, const Threa thread_info.registers.get()); if (maps != nullptr) { uint64_t addr = 0; - siginfo_t* si = thread_info.siginfo; - if (signal_has_si_addr(si)) { - addr = reinterpret_cast(si->si_addr); + if (process_info.has_fault_address) { + addr = process_info.untagged_fault_address; } dump_all_maps(log, unwinder, addr); }