From cb510e50e2c8d2b8b54e0bd221e3ce0881827544 Mon Sep 17 00:00:00 2001 From: Kostya Serebryany Date: Fri, 28 Dec 2012 15:24:16 +0000 Subject: [PATCH] [asan] implement more strict checking for memset/etc parameters. Instead of checking the first and the last byte, we check the entire shadow region. This costs ~10 slowdown for the instrumented functions. Motivated by a nasty memset-buffer-overflow-by-140-bytes in chrome which was reported as a use-after-free or not at all llvm-svn: 171198 --- compiler-rt/lib/asan/asan_interceptors.cc | 34 +++---------- compiler-rt/lib/asan/asan_poisoning.cc | 28 ++++++++++ compiler-rt/lib/asan/asan_rtl.cc | 1 + .../lib/asan/tests/asan_noinst_test.cc | 39 ++++++++++++++ compiler-rt/lib/asan/tests/asan_test.cc | 51 +++++++++++++++++-- .../lib/sanitizer_common/sanitizer_libc.cc | 19 +++++++ .../lib/sanitizer_common/sanitizer_libc.h | 5 ++ .../tests/sanitizer_libc_test.cc | 20 ++++++++ 8 files changed, 168 insertions(+), 29 deletions(-) diff --git a/compiler-rt/lib/asan/asan_interceptors.cc b/compiler-rt/lib/asan/asan_interceptors.cc index d3969ed59769..9b71f805cf5a 100644 --- a/compiler-rt/lib/asan/asan_interceptors.cc +++ b/compiler-rt/lib/asan/asan_interceptors.cc @@ -27,38 +27,20 @@ namespace __asan { -// Instruments read/write access to a single byte in memory. -// On error calls __asan_report_error, which aborts the program. -#define ACCESS_ADDRESS(address, isWrite) do { \ - if (!AddrIsInMem(address) || AddressIsPoisoned(address)) { \ - GET_CURRENT_PC_BP_SP; \ - __asan_report_error(pc, bp, sp, address, isWrite, /* access_size */ 1); \ - } \ -} while (0) - // We implement ACCESS_MEMORY_RANGE, ASAN_READ_RANGE, // and ASAN_WRITE_RANGE as macro instead of function so // that no extra frames are created, and stack trace contains // relevant information only. - -// Instruments read/write access to a memory range. -// More complex implementation is possible, for now just -// checking the first and the last byte of a range. -#define ACCESS_MEMORY_RANGE(offset, size, isWrite) do { \ - if (size > 0) { \ - uptr _ptr = (uptr)(offset); \ - ACCESS_ADDRESS(_ptr, isWrite); \ - ACCESS_ADDRESS(_ptr + (size) - 1, isWrite); \ - } \ +// We check all shadow bytes. +#define ACCESS_MEMORY_RANGE(offset, size, isWrite) do { \ + if (uptr __ptr = __asan_region_is_poisoned((uptr)(offset), size)) { \ + GET_CURRENT_PC_BP_SP; \ + __asan_report_error(pc, bp, sp, __ptr, isWrite, /* access_size */1); \ + } \ } while (0) -#define ASAN_READ_RANGE(offset, size) do { \ - ACCESS_MEMORY_RANGE(offset, size, false); \ -} while (0) - -#define ASAN_WRITE_RANGE(offset, size) do { \ - ACCESS_MEMORY_RANGE(offset, size, true); \ -} while (0) +#define ASAN_READ_RANGE(offset, size) ACCESS_MEMORY_RANGE(offset, size, false) +#define ASAN_WRITE_RANGE(offset, size) ACCESS_MEMORY_RANGE(offset, size, true); // Behavior of functions like "memcpy" or "strcpy" is undefined // if memory intervals overlap. We report error in this case. diff --git a/compiler-rt/lib/asan/asan_poisoning.cc b/compiler-rt/lib/asan/asan_poisoning.cc index 11aa87af038f..295db5a66834 100644 --- a/compiler-rt/lib/asan/asan_poisoning.cc +++ b/compiler-rt/lib/asan/asan_poisoning.cc @@ -16,6 +16,7 @@ #include "asan_internal.h" #include "asan_mapping.h" #include "sanitizer/asan_interface.h" +#include "sanitizer_common/sanitizer_libc.h" namespace __asan { @@ -154,6 +155,33 @@ bool __asan_address_is_poisoned(void const volatile *addr) { return __asan::AddressIsPoisoned((uptr)addr); } +uptr __asan_region_is_poisoned(uptr beg, uptr size) { + if (!size) return 0; + uptr end = beg + size; + if (!AddrIsInMem(beg)) return beg; + if (!AddrIsInMem(end)) return end; + uptr aligned_b = RoundUpTo(beg, SHADOW_GRANULARITY); + uptr aligned_e = RoundDownTo(end, SHADOW_GRANULARITY); + uptr shadow_beg = MemToShadow(aligned_b); + uptr shadow_end = MemToShadow(aligned_e); + // First check the first and the last application bytes, + // then check the SHADOW_GRANULARITY-aligned region by calling + // mem_is_zero on the corresponding shadow. + if (!__asan::AddressIsPoisoned(beg) && + !__asan::AddressIsPoisoned(end - 1) && + (shadow_end <= shadow_beg || + __sanitizer::mem_is_zero((const char *)shadow_beg, + shadow_end - shadow_beg))) + return 0; + // The fast check failed, so we have a poisoned byte somewhere. + // Find it slowly. + for (; beg < end; beg++) + if (__asan::AddressIsPoisoned(beg)) + return beg; + UNREACHABLE("mem_is_zero returned false, but poisoned byte was not found"); + return 0; +} + // This is a simplified version of __asan_(un)poison_memory_region, which // assumes that left border of region to be poisoned is properly aligned. static void PoisonAlignedStackMemory(uptr addr, uptr size, bool do_poison) { diff --git a/compiler-rt/lib/asan/asan_rtl.cc b/compiler-rt/lib/asan/asan_rtl.cc index 15a34aa179e8..9ed320ab95d1 100644 --- a/compiler-rt/lib/asan/asan_rtl.cc +++ b/compiler-rt/lib/asan/asan_rtl.cc @@ -253,6 +253,7 @@ static NOINLINE void force_interface_symbols() { case 31: __asan_after_dynamic_init(); break; case 32: __asan_poison_stack_memory(0, 0); break; case 33: __asan_unpoison_stack_memory(0, 0); break; + case 34: __asan_region_is_poisoned(0, 0); break; } } diff --git a/compiler-rt/lib/asan/tests/asan_noinst_test.cc b/compiler-rt/lib/asan/tests/asan_noinst_test.cc index 44e1d9f1b995..99a7bb65c726 100644 --- a/compiler-rt/lib/asan/tests/asan_noinst_test.cc +++ b/compiler-rt/lib/asan/tests/asan_noinst_test.cc @@ -683,6 +683,45 @@ TEST(AddressSanitizerInterface, PoisoningStressTest) { } } +TEST(AddressSanitizerInterface, PoisonedRegion) { + size_t rz = 16; + for (size_t size = 1; size <= 64; size++) { + char *p = new char[size]; + uptr x = reinterpret_cast(p); + for (size_t beg = 0; beg < size + rz; beg++) { + for (size_t end = beg; end < size + rz; end++) { + uptr first_poisoned = __asan_region_is_poisoned(x + beg, end - beg); + if (beg == end) { + EXPECT_FALSE(first_poisoned); + } else if (beg < size && end <= size) { + EXPECT_FALSE(first_poisoned); + } else if (beg >= size) { + EXPECT_EQ(x + beg, first_poisoned); + } else { + EXPECT_GT(end, size); + EXPECT_EQ(x + size, first_poisoned); + } + } + } + delete [] p; + } +} + +// This is a performance benchmark for manual runs. +// asan's memset interceptor calls mem_is_zero for the entire shadow region. +// the profile should look like this: +// 89.10% [.] __memset_sse2 +// 10.50% [.] __sanitizer::mem_is_zero +// I.e. mem_is_zero should consume ~ SHADOW_GRANULARITY less CPU cycles +// than memset itself. +TEST(AddressSanitizerInterface, DISABLED_Stress_memset) { + size_t size = 1 << 20; + char *x = new char[size]; + for (int i = 0; i < 100000; i++) + Ident(memset)(x, 0, size); + delete [] x; +} + static const char *kInvalidPoisonMessage = "invalid-poison-memory-range"; static const char *kInvalidUnpoisonMessage = "invalid-unpoison-memory-range"; diff --git a/compiler-rt/lib/asan/tests/asan_test.cc b/compiler-rt/lib/asan/tests/asan_test.cc index f9cc66445063..e7e1075b9501 100644 --- a/compiler-rt/lib/asan/tests/asan_test.cc +++ b/compiler-rt/lib/asan/tests/asan_test.cc @@ -19,6 +19,7 @@ #include #include #include +#include #ifdef __linux__ # include @@ -921,6 +922,50 @@ TEST(AddressSanitizer, MemSetOOBTest) { // We can test arrays of structres/classes here, but what for? } +// Try to allocate two arrays of 'size' bytes that are near each other. +// Strictly speaking we are not guaranteed to find such two pointers, +// but given the structure of asan's allocator we will. +static bool AllocateTwoAjacentArrays(char **x1, char **x2, size_t size) { + vector v; + bool res = false; + for (size_t i = 0; i < 1000U && !res; i++) { + v.push_back(new char[size]); + if (i == 0) continue; + sort(v.begin(), v.end()); + for (size_t j = 1; j < v.size(); j++) { + assert(v[j] > v[j-1]); + if (v[j] - v[j-1] < size * 2) { + *x2 = v[j]; + *x1 = v[j-1]; + res = true; + break; + } + } + } + + for (size_t i = 0; i < v.size(); i++) { + if (res && v[i] == *x1) continue; + if (res && v[i] == *x2) continue; + delete [] v[i]; + } + return res; +} + +TEST(AddressSanitizer, LargeOOBInMemset) { + for (size_t size = 200; size < 100000; size += size / 2) { + char *x1, *x2; + if (!AllocateTwoAjacentArrays(&x1, &x2, size)) + continue; + // fprintf(stderr, " large oob memset: %p %p %zd\n", x1, x2, size); + // Do a memset on x1 with huge out-of-bound access that will end up in x2. + EXPECT_DEATH(memset(x1, 0, size * 2), "is located 0 bytes to the right"); + delete [] x1; + delete [] x2; + return; + } + assert(0 && "Did not find two adjacent malloc-ed pointers"); +} + // Same test for memcpy and memmove functions template void MemTransferOOBTestTemplate(size_t length) { @@ -1634,7 +1679,7 @@ TEST(AddressSanitizer, pread) { EXPECT_DEATH(pread(fd, x, 15, 0), ASAN_PCRE_DOTALL "AddressSanitizer: heap-buffer-overflow" - ".* is located 4 bytes to the right of 10-byte region"); + ".* is located 0 bytes to the right of 10-byte region"); close(fd); delete [] x; } @@ -1646,7 +1691,7 @@ TEST(AddressSanitizer, pread64) { EXPECT_DEATH(pread64(fd, x, 15, 0), ASAN_PCRE_DOTALL "AddressSanitizer: heap-buffer-overflow" - ".* is located 4 bytes to the right of 10-byte region"); + ".* is located 0 bytes to the right of 10-byte region"); close(fd); delete [] x; } @@ -1658,7 +1703,7 @@ TEST(AddressSanitizer, read) { EXPECT_DEATH(read(fd, x, 15), ASAN_PCRE_DOTALL "AddressSanitizer: heap-buffer-overflow" - ".* is located 4 bytes to the right of 10-byte region"); + ".* is located 0 bytes to the right of 10-byte region"); close(fd); delete [] x; } diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_libc.cc b/compiler-rt/lib/sanitizer_common/sanitizer_libc.cc index 01eaef34b4fc..349be35012dd 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_libc.cc +++ b/compiler-rt/lib/sanitizer_common/sanitizer_libc.cc @@ -205,4 +205,23 @@ s64 internal_simple_strtoll(const char *nptr, char **endptr, int base) { } } +bool mem_is_zero(const char *beg, uptr size) { + CHECK_LE(size, 1UL << FIRST_32_SECOND_64(30, 40)); // Sanity check. + const char *end = beg + size; + uptr *aligned_beg = (uptr *)RoundUpTo((uptr)beg, sizeof(uptr)); + uptr *aligned_end = (uptr *)RoundDownTo((uptr)end, sizeof(uptr)); + uptr all = 0; + // Prologue. + for (const char *mem = beg; mem < (char*)aligned_beg && mem < end; mem++) + all |= *mem; + // Aligned loop. + for (; aligned_beg < aligned_end; aligned_beg++) + all |= *aligned_beg; + // Epilogue. + if ((char*)aligned_end >= beg) + for (const char *mem = (char*)aligned_end; mem < end; mem++) + all |= *mem; + return all == 0; +} + } // namespace __sanitizer diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_libc.h b/compiler-rt/lib/sanitizer_common/sanitizer_libc.h index 79794839d28b..aa052c654d39 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_libc.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_libc.h @@ -47,6 +47,11 @@ char *internal_strstr(const char *haystack, const char *needle); // Works only for base=10 and doesn't set errno. s64 internal_simple_strtoll(const char *nptr, char **endptr, int base); +// Return true if all bytes in [mem, mem+size) are zero. +// Optimized for the case when the result is true. +bool mem_is_zero(const char *mem, uptr size); + + // Memory void *internal_mmap(void *addr, uptr length, int prot, int flags, int fd, u64 offset); diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc b/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc index ff38e16ae1e8..b9d8414e0cbf 100644 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc @@ -20,3 +20,23 @@ TEST(SanitizerCommon, InternalMemmoveRegression) { EXPECT_EQ(dest[0], src[0]); EXPECT_EQ(dest[4], src[4]); } + +TEST(SanitizerCommon, mem_is_zero) { + size_t size = 128; + char *x = new char[size]; + memset(x, 0, size); + for (size_t pos = 0; pos < size; pos++) { + x[pos] = 1; + for (size_t beg = 0; beg < size; beg++) { + for (size_t end = beg; end < size; end++) { + // fprintf(stderr, "pos %zd beg %zd end %zd \n", pos, beg, end); + if (beg <= pos && pos < end) + EXPECT_FALSE(__sanitizer::mem_is_zero(x + beg, end - beg)); + else + EXPECT_TRUE(__sanitizer::mem_is_zero(x + beg, end - beg)); + } + } + x[pos] = 0; + } + delete [] x; +}