tequilaOS/platform_system_core
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

[MTE] Cleanup tagged si_addr refs to fix mappings OOB bug.

Browse source 
Currently, all MTE failures end up displaying 'Fault address falls at
0x<addr> 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
This commit is contained in:
Mitch Phillips 2021-01-21 20:41:50 -08:00
parent 79f7e3f1fc
commit e4adff0721
5 changed files with 15 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
debuggerd/crash_dump.cpp
View file

@ -40,6 +40,7 @@
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
#include <android-base/unique_fd.h>
#include <bionic/macros.h>
#include <bionic/reserved_signals.h>
#include <cutils/sockets.h>
#include <log/log.h>
@ -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<uintptr_t>(siginfo->si_addr);
process_info->maybe_tagged_fault_address = reinterpret_cast<uintptr_t>(siginfo->si_addr);
process_info->untagged_fault_address =
untag_address(reinterpret_cast<uintptr_t>(siginfo->si_addr));
}
regs->reset(unwindstack::Regs::CreateFromUcontext(unwindstack::Regs::CurrentArch(),
&crash_info->data.s.ucontext));

4
debuggerd/libdebuggerd/gwp_asan.cpp
View file

@ -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<uintptr_t>(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.

3
debuggerd/libdebuggerd/include/libdebuggerd/types.h
View file

@ -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;
};

4
debuggerd/libdebuggerd/scudo.cpp
View file

@ -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);
}

9
debuggerd/libdebuggerd/tombstone.cpp
View file

@ -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<uint64_t>(si->si_addr);
if (process_info.has_fault_address) {
addr = process_info.untagged_fault_address;
}
dump_all_maps(log, unwinder, addr);
}