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

@@ -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: [

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;
 

debuggerd/libdebuggerd/scudo.cpp

@@ -14,6 +14,11 @@
  * limitations under the License.
  */
 
+#include <stdint.h>
+#include <unistd.h>
+
+#include <vector>
+
 #include "libdebuggerd/scudo.h"
 #include "libdebuggerd/tombstone.h"
 
@@ -25,54 +30,92 @@
 
 #include "tombstone.pb.h"
 
-std::unique_ptr<char[]> AllocAndReadFully(unwindstack::Memory* process_memory, uint64_t addr,
+bool ScudoCrashData::SetErrorInfo(unwindstack::Memory* process_memory,
-                                          size_t size) {
-  auto buf = std::make_unique<char[]>(size);
-  if (!process_memory->ReadFully(addr, buf.get(), size)) {
-    return std::unique_ptr<char[]>();
-  }
-  return buf;
-}
-
-ScudoCrashData::ScudoCrashData(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,
+  std::vector<char> stack_depot(__scudo_get_stack_depot_size());
-                                       __scudo_get_stack_depot_size());
+  if (!process_memory->ReadFully(process_info.scudo_stack_depot, stack_depot.data(),
-  auto region_info = AllocAndReadFully(process_memory, process_info.scudo_region_info,
+                                 stack_depot.size())) {
-                                       __scudo_get_region_info_size());
+    return false;
-  auto ring_buffer = AllocAndReadFully(process_memory, process_info.scudo_ring_buffer,
+  }
-                                       __scudo_get_ring_buffer_size());
+  std::vector<char> 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<char> 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;
+  // Attempt to get 16 pages before the fault page and 16 pages after.
-  if (memory_begin > fault_page) {
+  constexpr size_t kExtraPages = 16;
-    return;
+  std::vector<char> 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;
+  // Attempt to read the pages after the fault page, stop as soon as we
-  if (memory_end < fault_page) {
+  // fail to read.
-    return;
+  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<long> 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<char[]>(memory_end - memory_begin);
+  __scudo_get_error_info(
-  for (auto i = memory_begin; i != memory_end; i += PAGE_SIZE) {
+      &error_info_, process_info.maybe_tagged_fault_address, stack_depot.data(), region_info.data(),
-    process_memory->ReadFully(i, memory.get() + i - memory_begin, PAGE_SIZE);
+      ring_buffer.data(), &memory[start_memory_index * page_size],
-  }
+      reinterpret_cast<const char*>(memory_tags.data()), memory_begin, memory_end - memory_begin);
 
-  auto memory_tags = std::make_unique<char[]>((memory_end - memory_begin) / kTagGranuleSize);
+  return true;
-  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);
 }
 
 bool ScudoCrashData::CrashIsMine() const {

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 <stdlib.h>
+#include <unistd.h>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "libdebuggerd/scudo.h"
+#include "libdebuggerd/types.h"
+#include "unwindstack/Memory.h"
+
+#include "log_fake.h"
+
+#include <inttypes.h>
+
+// 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<uint64_t>& test_read_addrs() { return test_read_addrs_; }
+
+ private:
+  std::set<uint64_t> test_unreadable_addrs_;
+
+  std::set<uint64_t> 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<uint64_t> 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<uint64_t> 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<uint64_t> 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));
+  }
+}

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);
+  ScudoCrashData scudo_crash_data;
-  if (scudo_crash_data.CrashIsMine()) {
+  if (scudo_crash_data.SetErrorInfo(unwinder->GetProcessMemory().get(), process_info) &&
+      scudo_crash_data.CrashIsMine()) {
     scudo_crash_data.AddCauseProtos(tombstone, unwinder);
     return;
   }