From cf06279931ba91b98fbbd7689353cb6068817497 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 6 Jul 2016 10:02:45 -0700 Subject: [PATCH] Implemented loose checking for potential widened loads BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374 R=kschimpf@google.com Review URL: https://codereview.chromium.org/2115693002 . --- docs/ASAN.rst | 9 ++++++ runtime/szrt_asan.c | 56 ++++++++++++++++++++++++--------- src/IceASanInstrumentation.cpp | 15 +++++---- src/IceASanInstrumentation.h | 3 +- tests_lit/asan_tests/errors.ll | 22 ++++++------- tests_lit/asan_tests/instrumentload.ll | 30 +++++++++--------- tests_lit/asan_tests/instrumentstore.ll | 20 ++++++------ tests_lit/asan_tests/wideloads.ll | 55 ++++++++++++++++++++++++++++++++ 8 files changed, 152 insertions(+), 58 deletions(-) create mode 100644 tests_lit/asan_tests/wideloads.ll diff --git a/docs/ASAN.rst b/docs/ASAN.rst index 5526c59b2..d815817c0 100644 --- a/docs/ASAN.rst +++ b/docs/ASAN.rst @@ -27,3 +27,12 @@ AddressSanitizer and properly linked into a final executable using subzero/pydir/szbuild.py with the --fsanitize-address flag, i.e.:: pydir/szbuild.py --fsanitize-address hello.pexe + +Handling Wide Loads +=================== + +Since AddressSanitizer is implemented only in Subzero, the target .pexe may +contain widened loads that would cause false positives. To avoid reporting such +loads as errors, we treat any word-aligned, four byte load as a potentially +widened load and only check the first byte of the loaded word against shadow +memory. diff --git a/runtime/szrt_asan.c b/runtime/szrt_asan.c index 797f1b99b..a406b9323 100644 --- a/runtime/szrt_asan.c +++ b/runtime/szrt_asan.c @@ -32,7 +32,8 @@ // Assuming 48 bit address space on 64 bit systems #define SHADOW_LENGTH_64 (1u << (48 - SHADOW_SCALE_LOG2)) #define SHADOW_LENGTH_32 (1u << (32 - SHADOW_SCALE_LOG2)) -#define IS_32_BIT (sizeof(void *) == 4) +#define WORD_SIZE (sizeof(uint32_t)) +#define IS_32_BIT (sizeof(void *) == WORD_SIZE) #define SHADOW_OFFSET(p) ((uintptr_t)(p) % SHADOW_SCALE) #define IS_SHADOW_ALIGNED(p) (SHADOW_OFFSET(p) == 0) @@ -45,13 +46,51 @@ static char *shadow_offset = NULL; +static void __asan_error(char *, int); +static void __asan_check(char *, int, bool); + void __asan_init(int, void **, int *); -void __asan_check(char *, int); +void __asan_check_load(char *, int); +void __asan_check_store(char *, int); void *__asan_malloc(size_t); void __asan_free(char *); void __asan_poison(char *, int); void __asan_unpoison(char *, int); +static void __asan_error(char *ptr, int size) { + fprintf(stderr, "Illegal access of %d bytes at %p\n", size, ptr); + abort(); +} + +// check only the first byte of each word unless strict +static void __asan_check(char *ptr, int size, bool strict) { + assert(strict || (uintptr_t)ptr % WORD_SIZE == 0); + printf("%s check %d bytes at %p\n", (strict) ? "strict" : "loose", size, ptr); + char *end = ptr + size; + int step = (strict) ? 1 : WORD_SIZE; + for (char *cur = ptr; cur < end; cur += step) { + char shadow = *(char *)MEM2SHADOW(cur); + printf("checking %p against %p with shadow %d\n", cur, MEM2SHADOW(cur), + shadow); + if (shadow != 0 && (shadow < 0 || SHADOW_OFFSET(cur) >= shadow)) { + __asan_error(ptr, size); + } + } +} + +void __asan_check_load(char *ptr, int size) { + // aligned single word accesses may be widened single byte accesses, but for + // all else use strict check + bool strict = !((uintptr_t)ptr % WORD_SIZE == 0 && size == WORD_SIZE); + __asan_check(ptr, size, strict); +} + +void __asan_check_store(char *ptr, int size) { + // stores may never be partially out of bounds so use strict check + bool strict = true; + __asan_check(ptr, size, strict); +} + void __asan_init(int n_rzs, void **rzs, int *rz_sizes) { // ensure the redzones are large enough to hold metadata assert(RZ_SIZE >= sizeof(void *) && RZ_SIZE >= sizeof(size_t)); @@ -80,19 +119,6 @@ void __asan_init(int n_rzs, void **rzs, int *rz_sizes) { } } -void __asan_check(char *ptr, int size) { - printf("check %d bytes at %p\n", size, ptr); - char *end = ptr + size; - for (; ptr < end; ++ptr) { - char shadow = *(char *)MEM2SHADOW(ptr); - printf("checking %p with shadow %d\n", ptr, shadow); - if (shadow != 0 && (shadow < 0 || SHADOW_OFFSET(ptr) > shadow)) { - fprintf(stderr, "Illegal access of %d bytes at %p\n", size, ptr); - abort(); - } - } -} - void *__asan_malloc(size_t size) { printf("malloc() called with size %d\n", size); size_t padding = diff --git a/src/IceASanInstrumentation.cpp b/src/IceASanInstrumentation.cpp index 8b54fba6b..bc519bfe5 100644 --- a/src/IceASanInstrumentation.cpp +++ b/src/IceASanInstrumentation.cpp @@ -261,26 +261,29 @@ void ASanInstrumentation::instrumentCall(LoweringContext &Context, void ASanInstrumentation::instrumentLoad(LoweringContext &Context, InstLoad *Instr) { + Constant *Func = + Ctx->getConstantExternSym(Ctx->getGlobalString("__asan_check_load")); instrumentAccess(Context, Instr->getSourceAddress(), - typeWidthInBytes(Instr->getDest()->getType())); + typeWidthInBytes(Instr->getDest()->getType()), Func); } void ASanInstrumentation::instrumentStore(LoweringContext &Context, InstStore *Instr) { + Constant *Func = + Ctx->getConstantExternSym(Ctx->getGlobalString("__asan_check_store")); instrumentAccess(Context, Instr->getAddr(), - typeWidthInBytes(Instr->getData()->getType())); + typeWidthInBytes(Instr->getData()->getType()), Func); } // TODO(tlively): Take size of access into account as well void ASanInstrumentation::instrumentAccess(LoweringContext &Context, - Operand *Op, SizeT Size) { - Constant *AccessCheck = - Ctx->getConstantExternSym(Ctx->getGlobalString("__asan_check")); + Operand *Op, SizeT Size, + Constant *CheckFunc) { constexpr SizeT NumArgs = 2; constexpr Variable *Void = nullptr; constexpr bool NoTailCall = false; auto *Call = InstCall::create(Context.getNode()->getCfg(), NumArgs, Void, - AccessCheck, NoTailCall); + CheckFunc, NoTailCall); Call->addArg(Op); Call->addArg(ConstantInteger32::create(Ctx, IceType_i32, Size)); // play games to insert the call before the access instruction diff --git a/src/IceASanInstrumentation.h b/src/IceASanInstrumentation.h index 4757f24a5..68d0bf3c0 100644 --- a/src/IceASanInstrumentation.h +++ b/src/IceASanInstrumentation.h @@ -47,7 +47,8 @@ private: void instrumentRet(LoweringContext &Context, InstRet *Instr) override; void instrumentLoad(LoweringContext &Context, InstLoad *Instr) override; void instrumentStore(LoweringContext &Context, InstStore *Instr) override; - void instrumentAccess(LoweringContext &Context, Operand *Op, SizeT Size); + void instrumentAccess(LoweringContext &Context, Operand *Op, SizeT Size, + Constant *AccessFunc); void instrumentStart(Cfg *Func) override; void finishFunc(Cfg *Func) override; ICE_TLS_DECLARE_FIELD(std::vector *, LocalDtors); diff --git a/tests_lit/asan_tests/errors.ll b/tests_lit/asan_tests/errors.ll index dd665cc28..05ec493db 100644 --- a/tests_lit/asan_tests/errors.ll +++ b/tests_lit/asan_tests/errors.ll @@ -38,7 +38,7 @@ declare external void @exit(i32) ; A global array @array = internal constant [12 x i8] zeroinitializer -define i32 @access(i32 %is_local_i, i32 %err) { +define void @access(i32 %is_local_i, i32 %err) { ; get the base pointer to either the local or global array %local = alloca i8, i32 12, align 1 %global = bitcast [12 x i8]* @array to i8* @@ -54,11 +54,11 @@ define i32 @access(i32 %is_local_i, i32 %err) { ; calculate the address to access %arraddr = ptrtoint i8* %arr to i32 %badaddr = add i32 %arraddr, %offset - %badptr = inttoptr i32 %badaddr to i32* + %badptr = inttoptr i32 %badaddr to i8* ; perform the bad access - %result = load i32, i32* %badptr, align 1 - ret i32 %result + %result = load i8, i8* %badptr, align 1 + ret void } ; use argc to determine which test routine to run @@ -74,31 +74,31 @@ define void @_start(i32 %arg) { i32 6, label %neg_global] one_local: ; Access one past the end of a local - call i32 @access(i32 1, i32 0) + call void @access(i32 1, i32 0) br label %error many_local: ; Access five past the end of a local - call i32 @access(i32 1, i32 4) + call void @access(i32 1, i32 4) br label %error neg_local: ; Access one before the beginning of a local - call i32 @access(i32 1, i32 -1) + call void @access(i32 1, i32 -1) br label %error one_global: ; Access one past the end of a global - call i32 @access(i32 0, i32 0) + call void @access(i32 0, i32 0) br label %error many_global: ; Access five past the end of a global - call i32 @access(i32 0, i32 4) + call void @access(i32 0, i32 4) br label %error neg_global: ; Access one before the beginning of a global - call i32 @access(i32 0, i32 -1) + call void @access(i32 0, i32 -1) br label %error error: call void @exit(i32 1) unreachable } -; CHECK: Illegal access of 4 bytes at +; CHECK: Illegal access of 1 bytes at diff --git a/tests_lit/asan_tests/instrumentload.ll b/tests_lit/asan_tests/instrumentload.ll index 037e71cfe..811e62327 100644 --- a/tests_lit/asan_tests/instrumentload.ll +++ b/tests_lit/asan_tests/instrumentload.ll @@ -69,35 +69,35 @@ define internal void @doLoads() { ; DUMP-LABEL: ================ Instrumented CFG ================ ; DUMP-NEXT: define internal void @doLoads() { ; DUMP-NEXT: __0: -; DUMP: call void @__asan_check(i32 @srcConst8, i32 1) +; DUMP: call void @__asan_check_load(i32 @srcConst8, i32 1) ; DUMP-NEXT: %dest1 = load i8, i8* @srcConst8, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcConst16, i32 2) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcConst16, i32 2) ; DUMP-NEXT: %dest2 = load i16, i16* @srcConst16, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcConst32, i32 4) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcConst32, i32 4) ; DUMP-NEXT: %dest3 = load i32, i32* @srcConst32, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcConst64, i32 8) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcConst64, i32 8) ; DUMP-NEXT: %dest4 = load i64, i64* @srcConst64, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcConst128, i32 16) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcConst128, i32 16) ; DUMP-NEXT: %dest5 = load <4 x i32>, <4 x i32>* @srcConst128, align 4 -; DUMP-NEXT: call void @__asan_check(i32 @srcGlobal8, i32 1) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcGlobal8, i32 1) ; DUMP-NEXT: %dest6 = load i8, i8* @srcGlobal8, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcGlobal16, i32 2) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcGlobal16, i32 2) ; DUMP-NEXT: %dest7 = load i16, i16* @srcGlobal16, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcGlobal32, i32 4) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcGlobal32, i32 4) ; DUMP-NEXT: %dest8 = load i32, i32* @srcGlobal32, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcGlobal64, i32 8) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcGlobal64, i32 8) ; DUMP-NEXT: %dest9 = load i64, i64* @srcGlobal64, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @srcGlobal128, i32 16) +; DUMP-NEXT: call void @__asan_check_load(i32 @srcGlobal128, i32 16) ; DUMP-NEXT: %dest10 = load <4 x i32>, <4 x i32>* @srcGlobal128, align 4 -; DUMP-NEXT: call void @__asan_check(i32 %srcLocal8, i32 1) +; DUMP-NEXT: call void @__asan_check_load(i32 %srcLocal8, i32 1) ; DUMP-NEXT: %dest11 = load i8, i8* %srcLocal8, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %srcLocal16, i32 2) +; DUMP-NEXT: call void @__asan_check_load(i32 %srcLocal16, i32 2) ; DUMP-NEXT: %dest12 = load i16, i16* %srcLocal16, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %srcLocal32, i32 4) +; DUMP-NEXT: call void @__asan_check_load(i32 %srcLocal32, i32 4) ; DUMP-NEXT: %dest13 = load i32, i32* %srcLocal32, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %srcLocal64, i32 8) +; DUMP-NEXT: call void @__asan_check_load(i32 %srcLocal64, i32 8) ; DUMP-NEXT: %dest14 = load i64, i64* %srcLocal64, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %srcLocal128, i32 16) +; DUMP-NEXT: call void @__asan_check_load(i32 %srcLocal128, i32 16) ; DUMP-NEXT: %dest15 = load <4 x i32>, <4 x i32>* %srcLocal128, align 4 ; DUMP: ret void ; DUMP-NEXT: } diff --git a/tests_lit/asan_tests/instrumentstore.ll b/tests_lit/asan_tests/instrumentstore.ll index 0a4c94db3..b9ac8e5a2 100644 --- a/tests_lit/asan_tests/instrumentstore.ll +++ b/tests_lit/asan_tests/instrumentstore.ll @@ -50,25 +50,25 @@ define internal void @doStores(<4 x i32> %vecSrc) { ; DUMP-LABEL: ================ Instrumented CFG ================ ; DUMP-NEXT: define internal void @doStores(<4 x i32> %vecSrc) { ; DUMP-NEXT: __0: -; DUMP: call void @__asan_check(i32 @destGlobal8, i32 1) +; DUMP: call void @__asan_check_store(i32 @destGlobal8, i32 1) ; DUMP-NEXT: store i8 42, i8* @destGlobal8, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @destGlobal16, i32 2) +; DUMP-NEXT: call void @__asan_check_store(i32 @destGlobal16, i32 2) ; DUMP-NEXT: store i16 42, i16* @destGlobal16, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @destGlobal32, i32 4) +; DUMP-NEXT: call void @__asan_check_store(i32 @destGlobal32, i32 4) ; DUMP-NEXT: store i32 42, i32* @destGlobal32, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @destGlobal64, i32 8) +; DUMP-NEXT: call void @__asan_check_store(i32 @destGlobal64, i32 8) ; DUMP-NEXT: store i64 42, i64* @destGlobal64, align 1 -; DUMP-NEXT: call void @__asan_check(i32 @destGlobal128, i32 16) +; DUMP-NEXT: call void @__asan_check_store(i32 @destGlobal128, i32 16) ; DUMP-NEXT: store <4 x i32> %vecSrc, <4 x i32>* @destGlobal128, align 4 -; DUMP-NEXT: call void @__asan_check(i32 %destLocal8, i32 1) +; DUMP-NEXT: call void @__asan_check_store(i32 %destLocal8, i32 1) ; DUMP-NEXT: store i8 42, i8* %destLocal8, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %destLocal16, i32 2) +; DUMP-NEXT: call void @__asan_check_store(i32 %destLocal16, i32 2) ; DUMP-NEXT: store i16 42, i16* %destLocal16, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %destLocal32, i32 4) +; DUMP-NEXT: call void @__asan_check_store(i32 %destLocal32, i32 4) ; DUMP-NEXT: store i32 42, i32* %destLocal32, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %destLocal64, i32 8) +; DUMP-NEXT: call void @__asan_check_store(i32 %destLocal64, i32 8) ; DUMP-NEXT: store i64 42, i64* %destLocal64, align 1 -; DUMP-NEXT: call void @__asan_check(i32 %destLocal128, i32 16) +; DUMP-NEXT: call void @__asan_check_store(i32 %destLocal128, i32 16) ; DUMP-NEXT: store <4 x i32> %vecSrc, <4 x i32>* %destLocal128, align 4 ; DUMP: ret void ; DUMP-NEXT: } diff --git a/tests_lit/asan_tests/wideloads.ll b/tests_lit/asan_tests/wideloads.ll new file mode 100644 index 000000000..626a757d9 --- /dev/null +++ b/tests_lit/asan_tests/wideloads.ll @@ -0,0 +1,55 @@ +; Test that potentially widened loads to not trigger an error report + +; REQUIRES: no_minimal_build + +; check for wide load exception +; RUN: llvm-as %s -o - | pnacl-freeze > %t.pexe && %S/../../pydir/szbuild.py \ +; RUN: --fsanitize-address --sz="-allow-externally-defined-symbols" \ +; RUN: %t.pexe -o %t && %t | FileCheck %s --check-prefix=WIDE + +; check for error reporting +; RUN: llvm-as %s -o - | pnacl-freeze > %t.pexe && %S/../../pydir/szbuild.py \ +; RUN: --fsanitize-address --sz="-allow-externally-defined-symbols" \ +; RUN: %t.pexe -o %t && %t 1 2>&1 | FileCheck %s --check-prefix=NOWIDE + + +declare external void @exit(i32) + +define internal void @wide_load() { + %str = alloca i8, i32 1, align 1 + %str4 = bitcast i8* %str to i32* + %contents = load i32, i32* %str4, align 1 + call void @exit(i32 0) + unreachable +} + +define internal void @no_wide_load() { + %str = alloca i8, i32 1, align 1 + %straddr = ptrtoint i8* %str to i32 + %off1addr = add i32 %straddr, 1 + %off1 = inttoptr i32 %off1addr to i8* + %contents = load i8, i8* %off1, align 1 + call void @exit(i32 1) + unreachable +} + +; WIDE-NOT: Illegal access +; NOWIDE: Illegal access of 1 bytes at + +; use argc to determine which test routine to run +define void @_start(i32 %arg) { + %argcaddr = add i32 %arg, 8 + %argcptr = inttoptr i32 %argcaddr to i32* + %argc = load i32, i32* %argcptr, align 1 + switch i32 %argc, label %error [i32 1, label %wide_load + i32 2, label %no_wide_load] +wide_load: + call void @wide_load() + br label %error +no_wide_load: + call void @no_wide_load() + br label %error +error: + call void @exit(i32 1) + unreachable +} \ No newline at end of file -- 2.11.0