From 87ef2718bc375171c06e72b54b9d94dfc5a1b812 Mon Sep 17 00:00:00 2001 From: Daniel Bertalan Date: Sat, 14 Aug 2021 16:15:32 +0200 Subject: [PATCH] UserspaceEmulator+LibC: Use sys$emuctl() to disable auditing in malloc It was fragile to use the address of the body of the memory management functions to disable memory auditing within them. Functions called from these did not get exempted from the audits, so in some cases UserspaceEmulator reported bogus heap buffer overflows. Memory auditing did not work at all on Clang because when querying the addresses, their offset was taken relative to the base of `.text` which is not the first segment in the `R/RX/RW(RELRO)/RW(non-RELRO)` layout produced by LLD. Similarly to when setting metadata about the allocations, we now use the `emuctl` system call to selectively suppress auditing when we reach these functions. This ensures that functions called from `malloc` are affected too, and no issues occur because of the inconsistency between Clang and GCC memory layouts. --- .../DevTools/UserspaceEmulator/Emulator.cpp | 28 --------------- .../DevTools/UserspaceEmulator/Emulator.h | 35 ++----------------- .../UserspaceEmulator/Emulator_syscalls.cpp | 13 +++---- .../UserspaceEmulator/MallocTracer.cpp | 9 +++-- Userland/Libraries/LibC/malloc.cpp | 18 ++++++++++ 5 files changed, 34 insertions(+), 69 deletions(-) diff --git a/Userland/DevTools/UserspaceEmulator/Emulator.cpp b/Userland/DevTools/UserspaceEmulator/Emulator.cpp index 255ddf2ded..64bb35c54e 100644 --- a/Userland/DevTools/UserspaceEmulator/Emulator.cpp +++ b/Userland/DevTools/UserspaceEmulator/Emulator.cpp @@ -671,34 +671,6 @@ void Emulator::setup_signal_trampoline() mmu().add_region(move(trampoline_region)); } -bool Emulator::find_malloc_symbols(MmapRegion const& libc_text) -{ - auto file_or_error = MappedFile::map("/usr/lib/libc.so"); - if (file_or_error.is_error()) - return false; - - ELF::Image image(file_or_error.value()->bytes()); - auto malloc_symbol = image.find_demangled_function("malloc"); - auto free_symbol = image.find_demangled_function("free"); - auto realloc_symbol = image.find_demangled_function("realloc"); - auto calloc_symbol = image.find_demangled_function("calloc"); - auto malloc_size_symbol = image.find_demangled_function("malloc_size"); - if (!malloc_symbol.has_value() || !free_symbol.has_value() || !realloc_symbol.has_value() || !malloc_size_symbol.has_value()) - return false; - - m_malloc_symbol_start = malloc_symbol.value().value() + libc_text.base(); - m_malloc_symbol_end = m_malloc_symbol_start + malloc_symbol.value().size(); - m_free_symbol_start = free_symbol.value().value() + libc_text.base(); - m_free_symbol_end = m_free_symbol_start + free_symbol.value().size(); - m_realloc_symbol_start = realloc_symbol.value().value() + libc_text.base(); - m_realloc_symbol_end = m_realloc_symbol_start + realloc_symbol.value().size(); - m_calloc_symbol_start = calloc_symbol.value().value() + libc_text.base(); - m_calloc_symbol_end = m_calloc_symbol_start + calloc_symbol.value().size(); - m_malloc_size_symbol_start = malloc_size_symbol.value().value() + libc_text.base(); - m_malloc_size_symbol_end = m_malloc_size_symbol_start + malloc_size_symbol.value().size(); - return true; -} - void Emulator::dump_regions() const { const_cast(m_mmu).for_each_region([&](Region const& region) { diff --git a/Userland/DevTools/UserspaceEmulator/Emulator.h b/Userland/DevTools/UserspaceEmulator/Emulator.h index c1d18ac525..c3cecbbdd9 100644 --- a/Userland/DevTools/UserspaceEmulator/Emulator.h +++ b/Userland/DevTools/UserspaceEmulator/Emulator.h @@ -49,6 +49,7 @@ public: bool is_profiling() const { return m_is_profiling; } bool is_in_region_of_interest() const { return m_is_in_region_of_interest; } size_t profile_instruction_interval() const { return m_profile_instruction_interval; } + bool is_memory_auditing_suppressed() const { return m_is_memory_auditing_suppressed; } bool load_elf(); void dump_backtrace(); @@ -63,10 +64,8 @@ public: MallocTracer* malloc_tracer() { return m_malloc_tracer; } - bool is_in_malloc_or_free() const; bool is_in_loader_code() const; bool is_in_libsystem() const; - bool is_in_libc() const; void pause() { @@ -231,8 +230,6 @@ private: int virt$msyscall(FlatPtr); int virt$futex(FlatPtr); - bool find_malloc_symbols(MmapRegion const& libc_text); - void dispatch_one_pending_signal(); MmapRegion const* find_text_region(FlatPtr address); MmapRegion const* load_library_from_address(FlatPtr address); @@ -249,19 +246,6 @@ private: FlatPtr m_watched_addr { 0 }; RefPtr m_editor; - FlatPtr m_malloc_symbol_start { 0 }; - FlatPtr m_malloc_symbol_end { 0 }; - FlatPtr m_realloc_symbol_start { 0 }; - FlatPtr m_realloc_symbol_end { 0 }; - FlatPtr m_calloc_symbol_start { 0 }; - FlatPtr m_calloc_symbol_end { 0 }; - FlatPtr m_free_symbol_start { 0 }; - FlatPtr m_free_symbol_end { 0 }; - FlatPtr m_malloc_size_symbol_start { 0 }; - FlatPtr m_malloc_size_symbol_end { 0 }; - - FlatPtr m_libc_start { 0 }; - FlatPtr m_libc_end { 0 }; FlatPtr m_libsystem_start { 0 }; FlatPtr m_libsystem_end { 0 }; @@ -293,29 +277,14 @@ private: bool m_is_profiling { false }; size_t m_profile_instruction_interval { 0 }; bool m_is_in_region_of_interest { false }; + bool m_is_memory_auditing_suppressed { false }; }; -ALWAYS_INLINE bool Emulator::is_in_libc() const -{ - return m_cpu.base_eip() >= m_libc_start && m_cpu.base_eip() < m_libc_end; -} - ALWAYS_INLINE bool Emulator::is_in_libsystem() const { return m_cpu.base_eip() >= m_libsystem_start && m_cpu.base_eip() < m_libsystem_end; } -ALWAYS_INLINE bool Emulator::is_in_malloc_or_free() const -{ - if (!is_in_libc()) - return false; - return (m_cpu.base_eip() >= m_malloc_symbol_start && m_cpu.base_eip() < m_malloc_symbol_end) - || (m_cpu.base_eip() >= m_free_symbol_start && m_cpu.base_eip() < m_free_symbol_end) - || (m_cpu.base_eip() >= m_realloc_symbol_start && m_cpu.base_eip() < m_realloc_symbol_end) - || (m_cpu.base_eip() >= m_calloc_symbol_start && m_cpu.base_eip() < m_calloc_symbol_end) - || (m_cpu.base_eip() >= m_malloc_size_symbol_start && m_cpu.base_eip() < m_malloc_size_symbol_end); -} - ALWAYS_INLINE bool Emulator::is_in_loader_code() const { if (!m_loader_text_base.has_value() || !m_loader_text_size.has_value()) diff --git a/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp b/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp index 94ef1c1fb8..56a452bbb1 100644 --- a/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp +++ b/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp @@ -870,12 +870,7 @@ u32 Emulator::virt$mmap(u32 params_addr) mmu().add_region(MmapRegion::create_anonymous(final_address, final_size, params.prot, move(name_str))); } else { auto region = MmapRegion::create_file_backed(final_address, final_size, params.prot, params.flags, params.fd, params.offset, move(name_str)); - if (region->name() == "libc.so: .text" && !m_libc_start) { - m_libc_start = final_address; - m_libc_end = final_address + final_size; - bool rc = find_malloc_symbols(*region); - VERIFY(rc); - } else if (region->name() == "libsystem.so: .text" && !m_libsystem_start) { + if (region->name() == "libsystem.so: .text" && !m_libsystem_start) { m_libsystem_start = final_address; m_libsystem_end = final_address + final_size; } @@ -1128,6 +1123,12 @@ int Emulator::virt$emuctl(FlatPtr arg1, FlatPtr arg2, FlatPtr arg3) case 6: // mark ROI end m_is_in_region_of_interest = false; return 0; + case 7: + m_is_memory_auditing_suppressed = true; + return 0; + case 8: + m_is_memory_auditing_suppressed = false; + return 0; default: return -EINVAL; } diff --git a/Userland/DevTools/UserspaceEmulator/MallocTracer.cpp b/Userland/DevTools/UserspaceEmulator/MallocTracer.cpp index a870a7c30e..291d8f8d48 100644 --- a/Userland/DevTools/UserspaceEmulator/MallocTracer.cpp +++ b/Userland/DevTools/UserspaceEmulator/MallocTracer.cpp @@ -213,7 +213,11 @@ void MallocTracer::audit_read(const Region& region, FlatPtr address, size_t size if (!m_auditing_enabled) return; - if (m_emulator.is_in_malloc_or_free() || m_emulator.is_in_libsystem()) { + if (m_emulator.is_memory_auditing_suppressed()) { + return; + } + + if (m_emulator.is_in_libsystem()) { return; } @@ -260,8 +264,9 @@ void MallocTracer::audit_write(const Region& region, FlatPtr address, size_t siz if (!m_auditing_enabled) return; - if (m_emulator.is_in_malloc_or_free()) + if (m_emulator.is_memory_auditing_suppressed()) { return; + } if (m_emulator.is_in_loader_code()) { return; diff --git a/Userland/Libraries/LibC/malloc.cpp b/Userland/Libraries/LibC/malloc.cpp index c3f3f190b5..b41a704c94 100644 --- a/Userland/Libraries/LibC/malloc.cpp +++ b/Userland/Libraries/LibC/malloc.cpp @@ -73,6 +73,19 @@ ALWAYS_INLINE static void ue_notify_chunk_size_changed(const void* block, size_t syscall(SC_emuctl, 4, chunk_size, (FlatPtr)block); } +struct MemoryAuditingSuppressor { + ALWAYS_INLINE MemoryAuditingSuppressor() + { + if (s_in_userspace_emulator) + syscall(SC_emuctl, 7); + } + ALWAYS_INLINE ~MemoryAuditingSuppressor() + { + if (s_in_userspace_emulator) + syscall(SC_emuctl, 8); + } +}; + struct MallocStats { size_t number_of_malloc_calls; @@ -399,6 +412,7 @@ static void free_impl(void* ptr) [[gnu::flatten]] void* malloc(size_t size) { + MemoryAuditingSuppressor suppressor; void* ptr = malloc_impl(size, CallerWillInitializeMemory::No); if (s_profiling) perf_event(PERF_EVENT_MALLOC, size, reinterpret_cast(ptr)); @@ -407,6 +421,7 @@ static void free_impl(void* ptr) [[gnu::flatten]] void free(void* ptr) { + MemoryAuditingSuppressor suppressor; if (s_profiling) perf_event(PERF_EVENT_FREE, reinterpret_cast(ptr), 0); ue_notify_free(ptr); @@ -415,6 +430,7 @@ static void free_impl(void* ptr) void* calloc(size_t count, size_t size) { + MemoryAuditingSuppressor suppressor; if (Checked::multiplication_would_overflow(count, size)) { errno = ENOMEM; return nullptr; @@ -428,6 +444,7 @@ void* calloc(size_t count, size_t size) size_t malloc_size(void* ptr) { + MemoryAuditingSuppressor suppressor; if (!ptr) return 0; void* page_base = (void*)((FlatPtr)ptr & ChunkedBlock::block_mask); @@ -449,6 +466,7 @@ size_t malloc_good_size(size_t size) void* realloc(void* ptr, size_t size) { + MemoryAuditingSuppressor suppressor; if (!ptr) return malloc(size); if (!size) {