From bb4b49c63c64a622d1b4bc6df00c53a93b3da97d Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Wed, 6 Jan 2021 21:02:19 -0800 Subject: Teach debuggerd to pass the secondary ring buffer to __scudo_get_error_info(). With this change we can report memory errors involving secondary allocations. Update the existing crasher tests to also test UAF/overflow/underflow on allocations with sizes sufficient to trigger the secondary allocator. Bug: 135772972 Change-Id: Ic8925c1f18621a8f272e26d5630e5d11d6d34d38 --- debuggerd/debuggerd_test.cpp | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) (limited to 'debuggerd/debuggerd_test.cpp') diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index b9d66063e..abce0569f 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -391,7 +391,11 @@ static void SetTagCheckingLevelSync() { } #endif -TEST_F(CrasherTest, mte_uaf) { +struct SizeParamCrasherTest : CrasherTest, testing::WithParamInterface {}; + +INSTANTIATE_TEST_SUITE_P(Sizes, SizeParamCrasherTest, testing::Values(16, 131072)); + +TEST_P(SizeParamCrasherTest, mte_uaf) { #if defined(__aarch64__) if (!mte_supported()) { GTEST_SKIP() << "Requires MTE"; @@ -399,9 +403,9 @@ TEST_F(CrasherTest, mte_uaf) { int intercept_result; unique_fd output_fd; - StartProcess([]() { + StartProcess([&]() { SetTagCheckingLevelSync(); - volatile int* p = (volatile int*)malloc(16); + volatile int* p = (volatile int*)malloc(GetParam()); free((void *)p); p[0] = 42; }); @@ -416,8 +420,9 @@ TEST_F(CrasherTest, mte_uaf) { std::string result; ConsumeFd(std::move(output_fd), &result); - ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\), code 9 \(SEGV_MTESERR\))"); - ASSERT_MATCH(result, R"(Cause: \[MTE\]: Use After Free, 0 bytes into a 16-byte allocation.* + ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\))"); + ASSERT_MATCH(result, R"(Cause: \[MTE\]: Use After Free, 0 bytes into a )" + + std::to_string(GetParam()) + R"(-byte allocation.* allocated by thread .* #00 pc)"); @@ -428,7 +433,7 @@ allocated by thread .* #endif } -TEST_F(CrasherTest, mte_overflow) { +TEST_P(SizeParamCrasherTest, mte_overflow) { #if defined(__aarch64__) if (!mte_supported()) { GTEST_SKIP() << "Requires MTE"; @@ -436,10 +441,10 @@ TEST_F(CrasherTest, mte_overflow) { int intercept_result; unique_fd output_fd; - StartProcess([]() { + StartProcess([&]() { SetTagCheckingLevelSync(); - volatile int* p = (volatile int*)malloc(16); - p[4] = 42; + volatile char* p = (volatile char*)malloc(GetParam()); + p[GetParam()] = 42; }); StartIntercept(&output_fd); @@ -453,7 +458,8 @@ TEST_F(CrasherTest, mte_overflow) { ConsumeFd(std::move(output_fd), &result); ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\))"); - ASSERT_MATCH(result, R"(Cause: \[MTE\]: Buffer Overflow, 0 bytes right of a 16-byte allocation.* + ASSERT_MATCH(result, R"(Cause: \[MTE\]: Buffer Overflow, 0 bytes right of a )" + + std::to_string(GetParam()) + R"(-byte allocation.* allocated by thread .* #00 pc)"); @@ -462,7 +468,7 @@ allocated by thread .* #endif } -TEST_F(CrasherTest, mte_underflow) { +TEST_P(SizeParamCrasherTest, mte_underflow) { #if defined(__aarch64__) if (!mte_supported()) { GTEST_SKIP() << "Requires MTE"; @@ -470,9 +476,9 @@ TEST_F(CrasherTest, mte_underflow) { int intercept_result; unique_fd output_fd; - StartProcess([]() { + StartProcess([&]() { SetTagCheckingLevelSync(); - volatile int* p = (volatile int*)malloc(16); + volatile int* p = (volatile int*)malloc(GetParam()); p[-1] = 42; }); @@ -487,7 +493,8 @@ TEST_F(CrasherTest, mte_underflow) { ConsumeFd(std::move(output_fd), &result); ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\), code 9 \(SEGV_MTESERR\))"); - ASSERT_MATCH(result, R"(Cause: \[MTE\]: Buffer Underflow, 4 bytes left of a 16-byte allocation.* + ASSERT_MATCH(result, R"(Cause: \[MTE\]: Buffer Underflow, 4 bytes left of a )" + + std::to_string(GetParam()) + R"(-byte allocation.* allocated by thread .* #00 pc)"); -- cgit v1.2.3 From 7168a217b9cea298feb8b1917c5457672cd309ee Mon Sep 17 00:00:00 2001 From: Mitch Phillips Date: Tue, 9 Mar 2021 16:53:23 -0800 Subject: [GWP-ASan] Add debuggerd end-to-end tests and remove unique wording. Looks like we unintentionally had a breakage after aosp/1595302, where both GWP-ASan and MTE tests started failing because the extra information wasn't plumbed through the tombstones. MTE has end-to-end tests but aren't run continuously, and GWP-ASan was missing the e2e tests. Also remove some unique wording for GWP-ASan, a UaF on the free'd pointer is now "0 bytes into a 16-byte allocation" instead of "on a 16-byte allocation". The former is more descriptive and is more ubiquitously used in our tooling. This patch adds the E2E tests, but the underlying problem needs to be fixed as well, before this patch can land. Bug: 182489365 Test: atest debuggerd_test Change-Id: I0fe8aba7ea443b3071724987f46b19a6525cda3c --- debuggerd/debuggerd_test.cpp | 74 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) (limited to 'debuggerd/debuggerd_test.cpp') diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index ab95768de..665fcd893 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -34,6 +34,7 @@ #include #include +#include #include #include @@ -98,6 +99,13 @@ constexpr char kWaitForGdbKey[] = "debug.debuggerd.wait_for_gdb"; ASSERT_MATCH(result, \ R"(#\d\d pc [0-9a-f]+\s+ \S+ (\(offset 0x[0-9a-f]+\) )?\()" frame_name R"(\+)"); +// Enable GWP-ASan at the start of this process. GWP-ASan is enabled using +// process sampling, so we need to ensure we force GWP-ASan on. +__attribute__((constructor)) static void enable_gwp_asan() { + bool force = true; + android_mallopt(M_INITIALIZE_GWP_ASAN, &force, sizeof(force)); +} + static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, unique_fd* output_fd, InterceptStatus* status, DebuggerdDumpType intercept_type) { intercept_fd->reset(socket_local_client(kTombstonedInterceptSocketName, @@ -392,6 +400,72 @@ static void SetTagCheckingLevelSync() { } #endif +// Number of iterations required to reliably guarantee a GWP-ASan crash. +// GWP-ASan's sample rate is not truly nondeterministic, it initialises a +// thread-local counter at 2*SampleRate, and decrements on each malloc(). Once +// the counter reaches zero, we provide a sampled allocation. Then, double that +// figure to allow for left/right allocation alignment, as this is done randomly +// without bias. +#define GWP_ASAN_ITERATIONS_TO_ENSURE_CRASH (0x20000) + +struct GwpAsanTestParameters { + size_t alloc_size; + bool free_before_access; + int access_offset; + std::string cause_needle; // Needle to be found in the "Cause: [GWP-ASan]" line. +}; + +struct GwpAsanCrasherTest : CrasherTest, testing::WithParamInterface {}; + +GwpAsanTestParameters gwp_asan_tests[] = { + {/* alloc_size */ 7, /* free_before_access */ true, /* access_offset */ 0, "Use After Free, 0 bytes into a 7-byte allocation"}, + {/* alloc_size */ 7, /* free_before_access */ true, /* access_offset */ 1, "Use After Free, 1 byte into a 7-byte allocation"}, + {/* alloc_size */ 7, /* free_before_access */ false, /* access_offset */ 16, "Buffer Overflow, 9 bytes right of a 7-byte allocation"}, + {/* alloc_size */ 16, /* free_before_access */ false, /* access_offset */ -1, "Buffer Underflow, 1 byte left of a 16-byte allocation"}, +}; + +INSTANTIATE_TEST_SUITE_P(GwpAsanTests, GwpAsanCrasherTest, testing::ValuesIn(gwp_asan_tests)); + +TEST_P(GwpAsanCrasherTest, gwp_asan_uaf) { + if (mte_supported()) { + // Skip this test on MTE hardware, as MTE will reliably catch these errors + // instead of GWP-ASan. + GTEST_SKIP() << "Skipped on MTE."; + } + + GwpAsanTestParameters params = GetParam(); + + int intercept_result; + unique_fd output_fd; + StartProcess([¶ms]() { + for (unsigned i = 0; i < GWP_ASAN_ITERATIONS_TO_ENSURE_CRASH; ++i) { + volatile char* p = reinterpret_cast(malloc(params.alloc_size)); + if (params.free_before_access) free(static_cast(const_cast(p))); + p[params.access_offset] = 42; + if (!params.free_before_access) free(static_cast(const_cast(p))); + } + }); + + StartIntercept(&output_fd); + FinishCrasher(); + AssertDeath(SIGSEGV); + FinishIntercept(&intercept_result); + + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + + ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\), code 2 \(SEGV_ACCERR\))"); + ASSERT_MATCH(result, R"(Cause: \[GWP-ASan\]: )" + params.cause_needle); + if (params.free_before_access) { + ASSERT_MATCH(result, R"(deallocated by thread .* + #00 pc)"); + } + ASSERT_MATCH(result, R"(allocated by thread .* + #00 pc)"); +} + struct SizeParamCrasherTest : CrasherTest, testing::WithParamInterface {}; INSTANTIATE_TEST_SUITE_P(Sizes, SizeParamCrasherTest, testing::Values(16, 131072)); -- cgit v1.2.3 From 1a1f7d79a4489671c705e6c5f20bb19dc35e8ba6 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Mon, 8 Mar 2021 16:53:54 -0800 Subject: Support MTE and GWP-ASan features in proto tombstones. Proto tombstones were missing tagged fault addresses, tagged_addr_ctrl, tags in memory dumps and Scudo and GWP-ASan error reports. Since text tombstones now go via protos, all of these features broke when we switched to text tombstones generated from protos by default. Fix the features by adding support for them to the proto format, tombstone_proto and tombstone_proto_to_text. Bug: 135772972 Bug: 182489365 Change-Id: I3ca854546c38755b1f6410a1f6198a44d25ed1c5 --- debuggerd/debuggerd_test.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) (limited to 'debuggerd/debuggerd_test.cpp') diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index ab95768de..dcb5395d4 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -423,12 +423,11 @@ TEST_P(SizeParamCrasherTest, mte_uaf) { ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\))"); ASSERT_MATCH(result, R"(Cause: \[MTE\]: Use After Free, 0 bytes into a )" + - std::to_string(GetParam()) + R"(-byte allocation.* - -allocated by thread .* - #00 pc)"); + std::to_string(GetParam()) + R"(-byte allocation)"); ASSERT_MATCH(result, R"(deallocated by thread .* #00 pc)"); + ASSERT_MATCH(result, R"(allocated by thread .* + #00 pc)"); #else GTEST_SKIP() << "Requires aarch64"; #endif @@ -460,9 +459,8 @@ TEST_P(SizeParamCrasherTest, mte_overflow) { ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\))"); ASSERT_MATCH(result, R"(Cause: \[MTE\]: Buffer Overflow, 0 bytes right of a )" + - std::to_string(GetParam()) + R"(-byte allocation.* - -allocated by thread .* + std::to_string(GetParam()) + R"(-byte allocation)"); + ASSERT_MATCH(result, R"(allocated by thread .* #00 pc)"); #else GTEST_SKIP() << "Requires aarch64"; @@ -495,9 +493,8 @@ TEST_P(SizeParamCrasherTest, mte_underflow) { ASSERT_MATCH(result, R"(signal 11 \(SIGSEGV\), code 9 \(SEGV_MTESERR\))"); ASSERT_MATCH(result, R"(Cause: \[MTE\]: Buffer Underflow, 4 bytes left of a )" + - std::to_string(GetParam()) + R"(-byte allocation.* - -allocated by thread .* + std::to_string(GetParam()) + R"(-byte allocation)"); + ASSERT_MATCH(result, R"(allocated by thread .* #00 pc)"); #else GTEST_SKIP() << "Requires aarch64"; -- cgit v1.2.3 From e4781d54a5d7fcabb420f0966259d9ac15d4c111 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Wed, 17 Mar 2021 09:15:15 -0700 Subject: debuggerd: prepare to abandon ship^Wgdb. Talk of "gdb" when we currently mean "gdb or lldb" and will soon mean "lldb" is starting to confuse people. Let's use the more neutral "debugger" in places where it really doesn't matter. The switch from gdbclient.py to lldbclient.py is a change for another day... Test: treehugger Change-Id: If39ca7e1cdf4c8bb9475f1791cdaf201fbea50e0 --- debuggerd/debuggerd_test.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'debuggerd/debuggerd_test.cpp') diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index ab95768de..de37a5b5a 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -69,7 +69,7 @@ using android::base::unique_fd; #define ARCH_SUFFIX "" #endif -constexpr char kWaitForGdbKey[] = "debug.debuggerd.wait_for_gdb"; +constexpr char kWaitForDebuggerKey[] = "debug.debuggerd.wait_for_debugger"; #define TIMEOUT(seconds, expr) \ [&]() { \ @@ -157,7 +157,7 @@ static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, uniq class CrasherTest : public ::testing::Test { public: pid_t crasher_pid = -1; - bool previous_wait_for_gdb; + bool previous_wait_for_debugger; unique_fd crasher_pipe; unique_fd intercept_fd; @@ -178,8 +178,13 @@ class CrasherTest : public ::testing::Test { }; CrasherTest::CrasherTest() { - previous_wait_for_gdb = android::base::GetBoolProperty(kWaitForGdbKey, false); - android::base::SetProperty(kWaitForGdbKey, "0"); + previous_wait_for_debugger = android::base::GetBoolProperty(kWaitForDebuggerKey, false); + android::base::SetProperty(kWaitForDebuggerKey, "0"); + + // Clear the old property too, just in case someone's been using it + // on this device. (We only document the new name, but we still support + // the old name so we don't break anyone's existing setups.) + android::base::SetProperty("debug.debuggerd.wait_for_gdb", "0"); } CrasherTest::~CrasherTest() { @@ -189,7 +194,7 @@ CrasherTest::~CrasherTest() { TEMP_FAILURE_RETRY(waitpid(crasher_pid, &status, WUNTRACED)); } - android::base::SetProperty(kWaitForGdbKey, previous_wait_for_gdb ? "1" : "0"); + android::base::SetProperty(kWaitForDebuggerKey, previous_wait_for_debugger ? "1" : "0"); } void CrasherTest::StartIntercept(unique_fd* output_fd, DebuggerdDumpType intercept_type) { @@ -734,9 +739,9 @@ TEST_F(CrasherTest, intercept_timeout) { AssertDeath(SIGABRT); } -TEST_F(CrasherTest, wait_for_gdb) { - if (!android::base::SetProperty(kWaitForGdbKey, "1")) { - FAIL() << "failed to enable wait_for_gdb"; +TEST_F(CrasherTest, wait_for_debugger) { + if (!android::base::SetProperty(kWaitForDebuggerKey, "1")) { + FAIL() << "failed to enable wait_for_debugger"; } sleep(1); -- cgit v1.2.3