From 381b89c8db97346e2276dd56b458d4531ef54c94 Mon Sep 17 00:00:00 2001 From: liyong Date: Tue, 24 May 2022 16:37:10 +0800 Subject: [PATCH] Fix scudo fault address processing. The code doesn't properly check if data is not read properly, so make it fail if reads fail. Also, change the algorithm so that first try and read the faulting page then 16 pages before and 16 pages after. Rather than trying to read every one of these pages, stop as soon as one is unreadable. This means that the total memory passed to the scudo error function is all valid data, rather than potentially being some uninitialized memory. Added new unit tests to cover scudo address processing. Bug: 233720136 Test: All unit tests pass. Test: atest CtsIncidentHostTestCases Change-Id: I18a97bdee9a0c44075c1c31ccd1b546d10895be9 --- debuggerd/Android.bp | 7 + .../libdebuggerd/include/libdebuggerd/scudo.h | 5 +- debuggerd/libdebuggerd/scudo.cpp | 117 ++++++--- debuggerd/libdebuggerd/test/scudo_test.cpp | 233 ++++++++++++++++++ debuggerd/libdebuggerd/tombstone_proto.cpp | 5 +- 5 files changed, 326 insertions(+), 41 deletions(-) create mode 100644 debuggerd/libdebuggerd/test/scudo_test.cpp diff --git a/debuggerd/Android.bp b/debuggerd/Android.bp index ad0231d8d..5c7d84790 100644 --- a/debuggerd/Android.bp +++ b/debuggerd/Android.bp @@ -293,6 +293,13 @@ cc_test { "libdebuggerd/test/utility_test.cpp", ], + product_variables: { + malloc_not_svelte: { + srcs: ["libdebuggerd/test/scudo_test.cpp"], + header_libs: ["scudo_headers"], + }, + }, + target: { android: { srcs: [ diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/scudo.h b/debuggerd/libdebuggerd/include/libdebuggerd/scudo.h index a506859a4..68bfd5b47 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/scudo.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/scudo.h @@ -34,9 +34,10 @@ class Memory; class ScudoCrashData { public: - ScudoCrashData() = delete; + ScudoCrashData() = default; ~ScudoCrashData() = default; - ScudoCrashData(unwindstack::Memory* process_memory, const ProcessInfo& process_info); + + bool SetErrorInfo(unwindstack::Memory* process_memory, const ProcessInfo& process_info); bool CrashIsMine() const; diff --git a/debuggerd/libdebuggerd/scudo.cpp b/debuggerd/libdebuggerd/scudo.cpp index 27fae25a1..9483e59e4 100644 --- a/debuggerd/libdebuggerd/scudo.cpp +++ b/debuggerd/libdebuggerd/scudo.cpp @@ -14,6 +14,11 @@ * limitations under the License. */ +#include +#include + +#include + #include "libdebuggerd/scudo.h" #include "libdebuggerd/tombstone.h" @@ -25,54 +30,92 @@ #include "tombstone.pb.h" -std::unique_ptr AllocAndReadFully(unwindstack::Memory* process_memory, uint64_t addr, - size_t size) { - auto buf = std::make_unique(size); - if (!process_memory->ReadFully(addr, buf.get(), size)) { - return std::unique_ptr(); - } - return buf; -} - -ScudoCrashData::ScudoCrashData(unwindstack::Memory* process_memory, - const ProcessInfo& process_info) { +bool ScudoCrashData::SetErrorInfo(unwindstack::Memory* process_memory, + const ProcessInfo& process_info) { if (!process_info.has_fault_address) { - return; + return false; } - auto stack_depot = AllocAndReadFully(process_memory, process_info.scudo_stack_depot, - __scudo_get_stack_depot_size()); - auto region_info = AllocAndReadFully(process_memory, process_info.scudo_region_info, - __scudo_get_region_info_size()); - auto ring_buffer = AllocAndReadFully(process_memory, process_info.scudo_ring_buffer, - __scudo_get_ring_buffer_size()); + std::vector stack_depot(__scudo_get_stack_depot_size()); + if (!process_memory->ReadFully(process_info.scudo_stack_depot, stack_depot.data(), + stack_depot.size())) { + return false; + } + std::vector region_info(__scudo_get_region_info_size()); + if (!process_memory->ReadFully(process_info.scudo_region_info, region_info.data(), + region_info.size())) { + return false; + } + std::vector ring_buffer(__scudo_get_ring_buffer_size()); + if (!process_memory->ReadFully(process_info.scudo_ring_buffer, ring_buffer.data(), + ring_buffer.size())) { + return false; + } + + uintptr_t page_size = getpagesize(); untagged_fault_addr_ = process_info.untagged_fault_address; - uintptr_t fault_page = untagged_fault_addr_ & ~(PAGE_SIZE - 1); + uintptr_t fault_page = untagged_fault_addr_ & ~(page_size - 1); - uintptr_t memory_begin = fault_page - PAGE_SIZE * 16; - if (memory_begin > fault_page) { - return; + // Attempt to get 16 pages before the fault page and 16 pages after. + constexpr size_t kExtraPages = 16; + std::vector memory(page_size * (kExtraPages * 2 + 1)); + + // Read faulting page first. + size_t memory_index = kExtraPages; + if (!process_memory->ReadFully(fault_page, &memory[memory_index * page_size], page_size)) { + return false; } - uintptr_t memory_end = fault_page + PAGE_SIZE * 16; - if (memory_end < fault_page) { - return; + // Attempt to read the pages after the fault page, stop as soon as we + // fail to read. + uintptr_t read_addr = fault_page; + if (!__builtin_add_overflow(fault_page, page_size, &read_addr)) { + memory_index++; + for (size_t i = 0; i < kExtraPages; i++, memory_index++) { + if (!process_memory->ReadFully(read_addr, &memory[memory_index * page_size], page_size)) { + break; + } + if (__builtin_add_overflow(read_addr, page_size, &read_addr)) { + break; + } + } + } + uintptr_t memory_end = read_addr; + + // Attempt to read the pages before the fault page, stop as soon as we + // fail to read. + memory_index = kExtraPages; + if (fault_page > 0) { + read_addr = fault_page - page_size; + for (size_t i = 0; i < kExtraPages; i++, memory_index--) { + if (!process_memory->ReadFully(read_addr, &memory[(memory_index - 1) * page_size], + page_size)) { + break; + } + if (read_addr == 0) { + memory_index--; + break; + } + read_addr -= page_size; + } + } + size_t start_memory_index = memory_index; + uintptr_t memory_begin = fault_page - (kExtraPages - memory_index) * page_size; + + std::vector memory_tags((memory_end - memory_begin) / kTagGranuleSize); + read_addr = memory_begin; + for (size_t i = 0; i < memory_tags.size(); i++) { + memory_tags[i] = process_memory->ReadTag(read_addr); + read_addr += kTagGranuleSize; } - auto memory = std::make_unique(memory_end - memory_begin); - for (auto i = memory_begin; i != memory_end; i += PAGE_SIZE) { - process_memory->ReadFully(i, memory.get() + i - memory_begin, PAGE_SIZE); - } + __scudo_get_error_info( + &error_info_, process_info.maybe_tagged_fault_address, stack_depot.data(), region_info.data(), + ring_buffer.data(), &memory[start_memory_index * page_size], + reinterpret_cast(memory_tags.data()), memory_begin, memory_end - memory_begin); - auto memory_tags = std::make_unique((memory_end - memory_begin) / kTagGranuleSize); - for (auto i = memory_begin; i != memory_end; i += kTagGranuleSize) { - memory_tags[(i - memory_begin) / kTagGranuleSize] = process_memory->ReadTag(i); - } - - __scudo_get_error_info(&error_info_, process_info.maybe_tagged_fault_address, stack_depot.get(), - region_info.get(), ring_buffer.get(), memory.get(), memory_tags.get(), - memory_begin, memory_end - memory_begin); + return true; } bool ScudoCrashData::CrashIsMine() const { diff --git a/debuggerd/libdebuggerd/test/scudo_test.cpp b/debuggerd/libdebuggerd/test/scudo_test.cpp new file mode 100644 index 000000000..d8fc6a7f3 --- /dev/null +++ b/debuggerd/libdebuggerd/test/scudo_test.cpp @@ -0,0 +1,233 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#include +#include + +#include "libdebuggerd/scudo.h" +#include "libdebuggerd/types.h" +#include "unwindstack/Memory.h" + +#include "log_fake.h" + +#include + +// This needs to match the kExtraPages from ScudoCrashData::SetErrorInfo. +constexpr uint64_t kMaxPages = 16; + +class MemoryAlwaysZero : public unwindstack::Memory { + public: + MemoryAlwaysZero() = default; + virtual ~MemoryAlwaysZero() = default; + + size_t Read(uint64_t addr, void* buffer, size_t size) override { + if (test_unreadable_addrs_.count(addr) != 0) { + return 0; + } + test_read_addrs_.insert(addr); + memset(buffer, 0, size); + return size; + } + + void TestAddUnreadableAddress(uint64_t addr) { test_unreadable_addrs_.insert(addr); } + + void TestClearAddresses() { + test_read_addrs_.clear(); + test_unreadable_addrs_.clear(); + } + + std::set& test_read_addrs() { return test_read_addrs_; } + + private: + std::set test_unreadable_addrs_; + + std::set test_read_addrs_; +}; + +TEST(ScudoTest, no_fault_address) { + MemoryAlwaysZero process_memory; + ProcessInfo info; + info.has_fault_address = false; + info.untagged_fault_address = 0x5000; + info.scudo_stack_depot = 0x1000; + info.scudo_region_info = 0x2000; + info.scudo_ring_buffer = 0x3000; + + ScudoCrashData crash; + ASSERT_FALSE(crash.SetErrorInfo(&process_memory, info)); + + info.has_fault_address = true; + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); +} + +TEST(ScudoTest, scudo_data_read_check) { + MemoryAlwaysZero process_memory; + ProcessInfo info; + info.has_fault_address = true; + info.untagged_fault_address = 0x5000; + info.scudo_stack_depot = 0x1000; + info.scudo_region_info = 0x2000; + info.scudo_ring_buffer = 0x3000; + + ScudoCrashData crash; + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + + // Stack Depot unreadable + process_memory.TestClearAddresses(); + process_memory.TestAddUnreadableAddress(0x1000); + ASSERT_FALSE(crash.SetErrorInfo(&process_memory, info)); + + // The Region Info doesn't exist for 32 bit. +#if defined(__LP64__) + // Region Info unreadable + process_memory.TestClearAddresses(); + process_memory.TestAddUnreadableAddress(0x2000); + ASSERT_FALSE(crash.SetErrorInfo(&process_memory, info)); +#endif + + // Ring Buffer unreadable + process_memory.TestClearAddresses(); + process_memory.TestAddUnreadableAddress(0x3000); + ASSERT_FALSE(crash.SetErrorInfo(&process_memory, info)); + + // Verify that with all scudo data readable, the error info works. + process_memory.TestClearAddresses(); + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); +} + +TEST(ScudoTest, fault_page_unreadable) { + MemoryAlwaysZero process_memory; + ProcessInfo info; + info.has_fault_address = true; + info.untagged_fault_address = 0x5124; + info.scudo_stack_depot = 0x1000; + info.scudo_region_info = 0x2000; + info.scudo_ring_buffer = 0x3000; + + ScudoCrashData crash; + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + + uint64_t fault_page = info.untagged_fault_address & ~(getpagesize() - 1); + process_memory.TestAddUnreadableAddress(fault_page); + ASSERT_FALSE(crash.SetErrorInfo(&process_memory, info)); +} + +TEST(ScudoTest, pages_before_fault_unreadable) { + MemoryAlwaysZero process_memory; + ProcessInfo info; + info.has_fault_address = true; + info.untagged_fault_address = 0x15124; + info.scudo_stack_depot = 0x1000; + info.scudo_region_info = 0x2000; + info.scudo_ring_buffer = 0x3000; + + ScudoCrashData crash; + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + + uint64_t page_size = getpagesize(); + uint64_t fault_page = info.untagged_fault_address & ~(page_size - 1); + + std::vector expected_reads = {0x1000, 0x2000, 0x3000}; + for (size_t i = 0; i <= kMaxPages; i++) { + expected_reads.emplace_back(fault_page + i * page_size); + } + + // Loop through and make pages before the fault page unreadable. + for (size_t i = 1; i <= kMaxPages + 1; i++) { + process_memory.TestClearAddresses(); + uint64_t unreadable_addr = fault_page - i * page_size; + SCOPED_TRACE(testing::Message() + << "Failed at unreadable address 0x" << std::hex << unreadable_addr); + process_memory.TestAddUnreadableAddress(unreadable_addr); + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + ASSERT_THAT(process_memory.test_read_addrs(), + testing::UnorderedElementsAreArray(expected_reads)); + // Need to add the previous unreadable_addr to the list of expected addresses. + expected_reads.emplace_back(unreadable_addr); + } +} + +TEST(ScudoTest, pages_after_fault_unreadable) { + MemoryAlwaysZero process_memory; + ProcessInfo info; + info.has_fault_address = true; + info.untagged_fault_address = 0x15124; + info.scudo_stack_depot = 0x1000; + info.scudo_region_info = 0x2000; + info.scudo_ring_buffer = 0x3000; + + ScudoCrashData crash; + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + + uint64_t page_size = getpagesize(); + uint64_t fault_page = info.untagged_fault_address & ~(page_size - 1); + + std::vector expected_reads = {0x1000, 0x2000, 0x3000}; + for (size_t i = 0; i <= kMaxPages; i++) { + expected_reads.emplace_back(fault_page - i * page_size); + } + + // Loop through and make pages after the fault page unreadable. + for (size_t i = 1; i <= kMaxPages + 1; i++) { + process_memory.TestClearAddresses(); + uint64_t unreadable_addr = fault_page + i * page_size; + SCOPED_TRACE(testing::Message() + << "Failed at unreadable address 0x" << std::hex << unreadable_addr); + process_memory.TestAddUnreadableAddress(unreadable_addr); + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + ASSERT_THAT(process_memory.test_read_addrs(), + testing::UnorderedElementsAreArray(expected_reads)); + // Need to add the previous unreadable_addr to the list of expected addresses. + expected_reads.emplace_back(unreadable_addr); + } +} + +// Make sure that if the fault address is low, you won't underflow. +TEST(ScudoTest, fault_address_low) { + MemoryAlwaysZero process_memory; + ProcessInfo info; + info.has_fault_address = true; + info.scudo_stack_depot = 0x21000; + info.scudo_region_info = 0x22000; + info.scudo_ring_buffer = 0x23000; + + ScudoCrashData crash; + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + + uint64_t page_size = getpagesize(); + for (size_t i = 0; i < kMaxPages + 1; i++) { + process_memory.TestClearAddresses(); + info.untagged_fault_address = 0x124 + i * getpagesize(); + SCOPED_TRACE(testing::Message() + << "Failed with fault address 0x" << std::hex << info.untagged_fault_address); + ASSERT_TRUE(crash.SetErrorInfo(&process_memory, info)); + std::vector expected_reads = {0x21000, 0x22000, 0x23000}; + uint64_t fault_page = info.untagged_fault_address & ~(page_size - 1); + expected_reads.emplace_back(fault_page); + for (size_t j = 1; j <= kMaxPages; j++) { + expected_reads.emplace_back(fault_page + j * page_size); + } + while (fault_page != 0) { + fault_page -= page_size; + expected_reads.emplace_back(fault_page); + } + ASSERT_THAT(process_memory.test_read_addrs(), + testing::UnorderedElementsAreArray(expected_reads)); + } +} diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp index 159ebc85f..6e1ce8f60 100644 --- a/debuggerd/libdebuggerd/tombstone_proto.cpp +++ b/debuggerd/libdebuggerd/tombstone_proto.cpp @@ -193,8 +193,9 @@ void set_human_readable_cause(Cause* cause, uint64_t fault_addr) { static void dump_probable_cause(Tombstone* tombstone, unwindstack::AndroidUnwinder* unwinder, const ProcessInfo& process_info, const ThreadInfo& main_thread) { #if defined(USE_SCUDO) - ScudoCrashData scudo_crash_data(unwinder->GetProcessMemory().get(), process_info); - if (scudo_crash_data.CrashIsMine()) { + ScudoCrashData scudo_crash_data; + if (scudo_crash_data.SetErrorInfo(unwinder->GetProcessMemory().get(), process_info) && + scudo_crash_data.CrashIsMine()) { scudo_crash_data.AddCauseProtos(tombstone, unwinder); return; }