diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp @@ -317,7 +317,7 @@ QualType ElementType) override; llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF, Address allocPtr, - CharUnits cookieSize) override; + CharUnits paddedCookieSize) override; void EmitGuardedInit(CodeGenFunction &CGF, const VarDecl &D, llvm::GlobalVariable *DeclPtr, @@ -448,8 +448,6 @@ llvm::Value *NumElements, const CXXNewExpr *expr, QualType ElementType) override; - llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF, Address allocPtr, - CharUnits cookieSize) override; }; class AppleARM64CXXABI : public ARMCXXABI { @@ -2163,69 +2161,71 @@ QualType ElementType) { assert(requiresArrayCookie(expr)); - unsigned AS = NewPtr.getAddressSpace(); + // getArrayCookieSizeImpl() returns the size of a padded cookie + CharUnits paddedCookieSize = getArrayCookieSizeImpl(ElementType); - ASTContext &Ctx = getContext(); - CharUnits SizeSize = CGF.getSizeSize(); + // The cookie itself is right-justified in the padded cookie + Address cookiePtr = NewPtr; + CharUnits padSize = paddedCookieSize - CGF.getSizeSize(); + if (!padSize.isZero()) + cookiePtr = + CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, padSize); + cookiePtr = CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy); - // The size of the cookie. - CharUnits CookieSize = - std::max(SizeSize, Ctx.getPreferredTypeAlignInChars(ElementType)); - assert(CookieSize == getArrayCookieSizeImpl(ElementType)); + // Store the number of elements in the cookie + llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, cookiePtr); - // Compute an offset to the cookie. - Address CookiePtr = NewPtr; - CharUnits CookieOffset = CookieSize - SizeSize; - if (!CookieOffset.isZero()) - CookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(CookiePtr, CookieOffset); - - // Write the number of elements into the appropriate slot. - Address NumElementsPtr = - CGF.Builder.CreateElementBitCast(CookiePtr, CGF.SizeTy); - llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, NumElementsPtr); - - // Handle the array cookie specially in ASan. - if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && AS == 0 && + // Handle ASan array cookie poisoning + if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && + NewPtr.getAddressSpace() == 0 && (expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() || CGM.getCodeGenOpts().SanitizeAddressPoisonCustomArrayCookie)) { - // The store to the CookiePtr does not need to be instrumented. + + // The store to the cookiePtr does not need to be instrumented. CGM.getSanitizerMetadata()->disableSanitizerForInstruction(SI); llvm::FunctionType *FTy = - llvm::FunctionType::get(CGM.VoidTy, NumElementsPtr.getType(), false); + llvm::FunctionType::get(CGM.VoidTy, cookiePtr.getType(), false); llvm::FunctionCallee F = CGM.CreateRuntimeFunction(FTy, "__asan_poison_cxx_array_cookie"); - CGF.Builder.CreateCall(F, NumElementsPtr.getPointer()); +// CGF.Builder.CreateCall(F, cookiePtr.getRawPointer(CGF)); + CGF.Builder.CreateCall(F, cookiePtr.getPointer()); } - // Finally, compute a pointer to the actual data buffer by skipping - // over the cookie completely. - return CGF.Builder.CreateConstInBoundsByteGEP(NewPtr, CookieSize); + // Return the usable userdata immediately after the cookie + return CGF.Builder.CreateConstInBoundsByteGEP(NewPtr, paddedCookieSize); } llvm::Value *ItaniumCXXABI::readArrayCookieImpl(CodeGenFunction &CGF, Address allocPtr, - CharUnits cookieSize) { - // The element size is right-justified in the cookie. - Address numElementsPtr = allocPtr; - CharUnits numElementsOffset = cookieSize - CGF.getSizeSize(); - if (!numElementsOffset.isZero()) - numElementsPtr = - CGF.Builder.CreateConstInBoundsByteGEP(numElementsPtr, numElementsOffset); - - unsigned AS = allocPtr.getAddressSpace(); - numElementsPtr = CGF.Builder.CreateElementBitCast(numElementsPtr, CGF.SizeTy); - if (!CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) || AS != 0) - return CGF.Builder.CreateLoad(numElementsPtr); - // In asan mode emit a function call instead of a regular load and let the - // run-time deal with it: if the shadow is properly poisoned return the - // cookie, otherwise return 0 to avoid an infinite loop calling DTORs. - // We can't simply ignore this load using nosanitize metadata because - // the metadata may be lost. + CharUnits paddedCookieSize) { + // The cookie is right-justified in the padded cookie + Address cookiePtr = allocPtr; + CharUnits padSize = paddedCookieSize - CGF.getSizeSize(); + if (!padSize.isZero()) + cookiePtr = + CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, padSize); + cookiePtr = CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy); + + // Return the cookie unless ASan will handle it + if (!CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) || + allocPtr.getAddressSpace() != 0) + return CGF.Builder.CreateLoad(cookiePtr); + + // In ASan mode we emit a function call to __asan_load_cxx_array_cookie() + // rather than doing a regular load. The ASan runtime will check the + // validity of the cookie using the shadow memory. If ASan determines that + // the cookie is invalid, it will log the invalid load of the the cookie + // and then either abort or return 0 depending on the ASan setting. If the + // cookie is indeed invalid, it is critical to NOT load it because it will + // then be used by delete[] to call the DTORs for each element of the array. + // This would result in DTORs being called for invalid elements and create + // a severe security issue. Returning 0 effectively avoids invalid DTORs. llvm::FunctionType *FTy = llvm::FunctionType::get(CGF.SizeTy, CGF.SizeTy->getPointerTo(0), false); llvm::FunctionCallee F = CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie"); - return CGF.Builder.CreateCall(F, numElementsPtr.getPointer()); +//return CGF.Builder.CreateCall(F, cookiePtr.getRawPointer(CGF)); + return CGF.Builder.CreateCall(F, cookiePtr.getPointer()); } CharUnits ARMCXXABI::getArrayCookieSizeImpl(QualType elementType) { @@ -2248,35 +2248,31 @@ QualType elementType) { assert(requiresArrayCookie(expr)); - // The cookie is always at the start of the buffer. - Address cookie = newPtr; - - // The first element is the element size. - cookie = CGF.Builder.CreateElementBitCast(cookie, CGF.SizeTy); - llvm::Value *elementSize = llvm::ConstantInt::get(CGF.SizeTy, - getContext().getTypeSizeInChars(elementType).getQuantity()); - CGF.Builder.CreateStore(elementSize, cookie); - - // The second element is the element count. - cookie = CGF.Builder.CreateConstInBoundsGEP(cookie, 1); - CGF.Builder.CreateStore(numElements, cookie); - - // Finally, compute a pointer to the actual data buffer by skipping - // over the cookie completely. - CharUnits cookieSize = ARMCXXABI::getArrayCookieSizeImpl(elementType); - return CGF.Builder.CreateConstInBoundsByteGEP(newPtr, cookieSize); -} - -llvm::Value *ARMCXXABI::readArrayCookieImpl(CodeGenFunction &CGF, - Address allocPtr, - CharUnits cookieSize) { - // The number of elements is at offset sizeof(size_t) relative to - // the allocated pointer. - Address numElementsPtr - = CGF.Builder.CreateConstInBoundsByteGEP(allocPtr, CGF.getSizeSize()); - - numElementsPtr = CGF.Builder.CreateElementBitCast(numElementsPtr, CGF.SizeTy); - return CGF.Builder.CreateLoad(numElementsPtr); + // Do everything Itanium first + Address cookiePtr = ItaniumCXXABI::InitializeArrayCookie(CGF, newPtr, + numElements, + expr, elementType); + + // ARM32 stores a second value *before* numElements which is sizeof(element) + // ARM64 doesn't seem to support this extra information but we've been doing + // it since ARMCXXABI. We've also maintained a minimum alignment of 16 bytes + // from the allocator so there is room for it. (NB: x86)64 doesn't return + // 16 byte aligned allocations when there are array cookies like ARM32 does) + + // The padded cookie had better be at least 2*size_t in size + assert(ARMCXXABI::getArrayCookieSizeImpl(elementType) >= + CharUnits::fromQuantity(2*CGM.SizeSizeInBytes)); + + // Add the "extra" sizeof(element) information behind the regular cookie. + Address xCookiePtr = CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy); + xCookiePtr = CGF.Builder.CreateConstGEP(xCookiePtr, -2); + llvm::Value *elementSize = + llvm::ConstantInt::get(CGF.SizeTy, + getContext().getTypeSizeInChars(elementType).getQuantity()); + CGF.Builder.CreateStore(elementSize, xCookiePtr); + + // Return just the numElements portion of the cookie as expected. + return cookiePtr; } /*********************** Static local initialization **************************/ diff --git a/clang/test/CodeGenCXX/arm.cpp b/clang/test/CodeGenCXX/arm.cpp --- a/clang/test/CodeGenCXX/arm.cpp +++ b/clang/test/CodeGenCXX/arm.cpp @@ -111,8 +111,8 @@ void a() { // CHECK-LABEL: define{{.*}} void @_ZN5test31aEv() // CHECK: call noalias nonnull i8* @_Znam(i32 48) - // CHECK: store i32 4 // CHECK: store i32 10 + // CHECK: store i32 4 A *x = new A[10]; } @@ -124,16 +124,16 @@ // CHECK: [[OR:%.*]] = or i1 // CHECK: [[SZ:%.*]] = select i1 [[OR]] // CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]]) - // CHECK: store i32 4 // CHECK: store i32 [[N]] + // CHECK: store i32 4 A *x = new A[n]; } void c() { // CHECK-LABEL: define{{.*}} void @_ZN5test31cEv() // CHECK: call noalias nonnull i8* @_Znam(i32 808) - // CHECK: store i32 4 // CHECK: store i32 200 + // CHECK: store i32 4 A (*x)[20] = new A[10][20]; } @@ -145,8 +145,8 @@ // CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8) // CHECK: [[SZ:%.*]] = select // CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]]) - // CHECK: store i32 4 // CHECK: store i32 [[NE]] + // CHECK: store i32 4 A (*x)[20] = new A[n][20]; } @@ -186,8 +186,8 @@ void a() { // CHECK-LABEL: define{{.*}} void @_ZN5test41aEv() // CHECK: call noalias nonnull i8* @_Znam(i32 48) - // CHECK: store i32 4 // CHECK: store i32 10 + // CHECK: store i32 4 A *x = new A[10]; } @@ -198,16 +198,16 @@ // CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8) // CHECK: [[SZ:%.*]] = select // CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]]) - // CHECK: store i32 4 // CHECK: store i32 [[N]] + // CHECK: store i32 4 A *x = new A[n]; } void c() { // CHECK-LABEL: define{{.*}} void @_ZN5test41cEv() // CHECK: call noalias nonnull i8* @_Znam(i32 808) - // CHECK: store i32 4 // CHECK: store i32 200 + // CHECK: store i32 4 A (*x)[20] = new A[10][20]; } @@ -219,8 +219,8 @@ // CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8) // CHECK: [[SZ:%.*]] = select // CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]]) - // CHECK: store i32 4 // CHECK: store i32 [[NE]] + // CHECK: store i32 4 A (*x)[20] = new A[n][20]; } @@ -387,12 +387,14 @@ // CHECK-NEXT: [[T3:%.*]] = extractvalue { i32, i1 } [[T2]], 0 // CHECK-NEXT: [[T4:%.*]] = select i1 [[OVERFLOW]], i32 -1, i32 [[T3]] // CHECK-NEXT: [[ALLOC:%.*]] = call noalias nonnull i8* @_Znam(i32 [[T4]]) -// CHECK-NEXT: [[T0:%.*]] = bitcast i8* [[ALLOC]] to i32* -// CHECK-NEXT: store i32 16, i32* [[T0]] -// CHECK-NEXT: [[T1:%.*]] = getelementptr inbounds i32, i32* [[T0]], i32 1 -// CHECK-NEXT: store i32 [[N]], i32* [[T1]] -// CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 16 -// CHECK-NEXT: bitcast i8* [[T0]] to [[TEST9]]* +// CHECK-NEXT: [[BPTR:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 12 +// CHECK-NEXT: [[NPTR:%.*]] = bitcast i8* [[BPTR]] to i32* +// CHECK-NEXT: store i32 [[N]], i32* [[NPTR]] +// CHECK-NEXT: [[BPTR:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 16 +// CHECK-NEXT: [[WPTR:%.*]] = bitcast i8* [[BPTR]] to i32* +// CHECK-NEXT: [[SPTR:%.*]] = getelementptr i32, i32* [[WPTR]], i32 -2 +// CHECK-NEXT: store i32 16, i32* [[SPTR]] +// CHECK-NEXT: bitcast i8* [[BPTR]] to [[TEST9]]* // Array allocation follows. void testDelete(A *array) { @@ -404,7 +406,7 @@ // CHECK-NEXT: br i1 [[T0]], // CHECK: [[T0:%.*]] = bitcast [[TEST9]]* [[BEGIN]] to i8* // CHECK-NEXT: [[ALLOC:%.*]] = getelementptr inbounds i8, i8* [[T0]], i32 -16 -// CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 4 +// CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 12 // CHECK-NEXT: [[T1:%.*]] = bitcast i8* [[T0]] to i32* // CHECK-NEXT: [[N:%.*]] = load i32, i32* [[T1]] // CHECK-NEXT: [[END:%.*]] = getelementptr inbounds [[TEST9]], [[TEST9]]* [[BEGIN]], i32 [[N]] diff --git a/compiler-rt/lib/asan/asan_poisoning.cpp b/compiler-rt/lib/asan/asan_poisoning.cpp --- a/compiler-rt/lib/asan/asan_poisoning.cpp +++ b/compiler-rt/lib/asan/asan_poisoning.cpp @@ -259,6 +259,10 @@ if (!flags()->poison_array_cookie) return; uptr s = MEM_TO_SHADOW(p); *reinterpret_cast(s) = kAsanArrayCookieMagic; +#if SANITIZER_ARM64 + // The ARM64 cookie has a second "size_t" entry so poison it as well + *(reinterpret_cast(s)-1) = kAsanArrayCookieMagic; +#endif } extern "C" SANITIZER_INTERFACE_ATTRIBUTE diff --git a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp --- a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp @@ -1,13 +1,12 @@ // REQUIRES: asan-64-bits // RUN: %clangxx_asan -O3 %s -o %t -// RUN: not %run %t 2>&1 | FileCheck %s +// RUN: not %run %t 2>&1 | FileCheck %s // RUN: %env_asan_opts=poison_array_cookie=1 not %run %t 2>&1 | FileCheck %s // RUN: %env_asan_opts=poison_array_cookie=0 not %run %t 2>&1 | FileCheck %s --check-prefix=NO_COOKIE -// UNSUPPORTED: ios - #include #include + struct C { int x; ~C() { @@ -17,11 +16,11 @@ }; int main(int argc, char **argv) { - C *buffer = new C[argc]; - buffer[-2].x = 10; + C *buffer = new C[1]; + reinterpret_cast(buffer)[-1] = 42; // CHECK: AddressSanitizer: heap-buffer-overflow // CHECK: in main {{.*}}new_array_cookie_test.cpp:[[@LINE-2]] -// CHECK: is located 0 bytes inside of 12-byte region +// CHECK: is located {{7 bytes inside of 12|15 bytes inside of 20}}-byte region // NO_COOKIE: ZZZZZZZZ delete [] buffer; } diff --git a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp --- a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp @@ -3,12 +3,10 @@ // RUN: %env_asan_opts=poison_array_cookie=1 not %run %t 2>&1 | FileCheck %s --check-prefix=COOKIE // RUN: %env_asan_opts=poison_array_cookie=0 not %run %t 2>&1 | FileCheck %s --check-prefix=NO_COOKIE -// UNSUPPORTED: ios - -#include -#include #include -int dtor_counter; +#include + +int dtor_counter=0; struct C { int x; ~C() { @@ -19,12 +17,11 @@ __attribute__((noinline)) void Delete(C *c) { delete[] c; } __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) { - long *p = reinterpret_cast(c); - p[-1] = 42; + reinterpret_cast(c)[-1] = 42; } int main(int argc, char **argv) { - C *buffer = new C[argc]; + C *buffer = new C[1]; delete [] buffer; Write42ToCookie(buffer); delete [] buffer; diff --git a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp --- a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp @@ -1,16 +1,22 @@ -// Test that we do not poison the array cookie if the operator new is defined -// inside the class. -// RUN: %clangxx_asan %s -o %t && %run %t +// For arrays allocated by classes that define their own custom "new", ASAN +// should NOT poison the array cookie unless the compilation unit is compiled +// with "-fsanitize-address-poison-custom-array-cookie". +// Here we test that: +// 1) w/o ASan, the array cookie location IS writable +// 2) w/ASan, the array cookie location IS writeable by default +// 3) w/ASan, the flag "-fno-sanitize-address-poison-custom-array-cookie" +// is a NOP (because this is the default) and finally, +// 4) w/ASan AND "-fsanitize-address-poison-custom-array-cookie", the +// array cookie location is NOT writable (ASAN successfully stopped it) // -// XFAIL: arm +// RUN: %clangxx %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clangxx_asan %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clangxx_asan -fno-sanitize-address-poison-custom-array-cookie %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clangxx_asan -fsanitize-address-poison-custom-array-cookie %s -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=COOKIE -// UNSUPPORTED: ios - -#include -#include -#include -#include #include +#include + struct Foo { void *operator new(size_t s) { return Allocate(s); } void *operator new[] (size_t s) { return Allocate(s); } @@ -22,7 +28,6 @@ } }; -Foo::~Foo() {} void *Foo::allocated; Foo *getFoo(size_t n) { @@ -33,8 +38,12 @@ Foo *foo = getFoo(10); fprintf(stderr, "foo : %p\n", foo); fprintf(stderr, "alloc: %p\n", Foo::allocated); - assert(reinterpret_cast(foo) == - reinterpret_cast(Foo::allocated) + sizeof(void*)); - *reinterpret_cast(Foo::allocated) = 42; + reinterpret_cast(foo)[-1] = 42; +// COOKIE: AddressSanitizer: heap-buffer-overflow +// COOKIE: in main {{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]] +// COOKIE: is located {{7 bytes inside of 18|15 bytes inside of 26}}-byte region + printf("Unsurprisingly, we were able to write to the array cookie\n"); +// CHECK: Unsurprisingly, we were able to write to the array cookie + return 0; }