[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

In D125195#3979478 , @vitalybuka 
wrote:

> Is this still relevant?
> If so, I would recommend to split ItaniumCXXABI from asan changes.

I haven't proceeded with this because this would be ABI breaking. I intend to 
support the (I believe) wrong ARM64 array cookie implementation and add the 
poisoning and tests re-enabled separately. Thanks for your input @vitalybuka.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka requested changes to this revision.
vitalybuka added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: Enna1.

Is this still relevant?
If so, I would recommend to split ItaniumCXXABI from asan changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 430461.
rsundahl added a comment.

Fix (and cleanup) for failure of CodeGen arm.c check in ci/cd pipeline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/arm.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ 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;
 }
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ 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;
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ 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 

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2191
 CGM.CreateRuntimeFunction(FTy, "__asan_poison_cxx_array_cookie");
+//  CGF.Builder.CreateCall(F, NumElementsPtr.getRawPointer(CGF));
 CGF.Builder.CreateCall(F, NumElementsPtr.getPointer());

NumElementsPtr.getRawPointer(CGF) had to be changed to 
CGF.Builder.CreateCall(F, NumElementsPtr.getPointer() but likely will have to 
be switched back when it appears downstream.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2223
   CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie");
+//return CGF.Builder.CreateCall(F, numElementsPtr.getRawPointer(CGF));
   return CGF.Builder.CreateCall(F, numElementsPtr.getPointer());

NumElementsPtr.getRawPointer(CGF) had to be changed to 
CGF.Builder.CreateCall(F, NumElementsPtr.getPointer() but likely will have to 
be switched back when it appears downstream.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 429352.
rsundahl added a comment.

This update corrects merge conflicts in Build #104091


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ 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;
 }
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ 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;
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ 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 
 

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2349
+  if (!cookieOffset.isZero())
+cookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, 
cookieOffset);
 

Variable names should start with uppercase:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Please also be conservative with "rename cleanups" in reviews.  The smaller 
your patch is, the easier it is to review.  You can do these in a follow-up NFC 
commit.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:266
+#endif
 }
 

I just realized that we are poisoning the cookie, but we are not doing anything 
on the detection side.
Is this actually required to make detection work/the tests pass?

If this bit isn't needed to make the existing tests pass, then I would like to 
suggest 1 of 2 things:
* Remove the additional poisoning (recommended), -or-
* Add another (ARM-specific?) test that requires poisoning of the second cookie 
word.  I think this would require adaptation of `__asan_load_cxx_array_cookie` 
as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl marked 8 inline comments as done.
rsundahl added a comment.

The update completes the suggested changes. The generated code is slightly 
different around initialization of the array cookie due to choosing one 
implementation over another when I "folded" ARMCXXABI::InitializeArrayCookie() 
into ItaniumCXXABI::InitializeArrayCookie(). The generated code LGTM but I 
would appreciate added scrutiny for the ItaniumCXXABI.cpp changes. 
check-sanitizer and check-asan completed successfully on x86_64, arm64 and 
arm64e.




Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2478
+  // 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

rsundahl wrote:
> delcypher wrote:
> > This comment is confusing. What's returning `0`? 
> > `__asan_load_cxx_array_cookie` doesn't do that and AFAICT neither does this 
> > code.
> (Same answer) It's only there because it's also there in 
> ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think 
> that the ARMCXXABI version would be different. That said, this is the 
> revision that introduced the notion originally: https://reviews.llvm.org/D5111
I'm not sure where to find a definitive spec for __asan_load_cxx_array_cookie() 
so I'm conservatively leaving it as it currently is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 429135.
rsundahl added a comment.

- Refactor InitializeArrayCookie and readArrayCookieImpl.

This update to the differential implements the final suggestions
for refactoring ItaniumCXXABI to remove duplicated code in the
function InitializeArrayCookie() and readArrayCookieImpl().


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ 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;
 }
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
@@ -3,15 +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
-
-// Added to allow enabling of tests but needs investigation (rdar://91448627)
-// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch))
-
-#include 
-#include 
 #include 
-int dtor_counter;
+#include 
+
+int dtor_counter=0;
 struct C {
   int x;
   ~C() {
@@ -22,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;
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
@@ -1,16 +1,12 @@
 // REQUIRES: asan-64-bits
 // RUN: %clangxx_asan -O3 %s -o %t
-// RUN:   

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:263
+#if SANITIZER_ARM64
+  // The ARM64 cookie has a second "size_t" entry so poison it as well
+  *(reinterpret_cast(s)-1) = kAsanArrayCookieMagic;





Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
 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;

`C *buffer = new C[argc];`

Although this looks weird, I think this is to prevent the compiler from 
reasoning about the array size.
I know that we had tests in the past that were "silently passing", because the 
compiler optimized away parts of the test.

I am not sure if it applies here, but let's keep it just in case ...



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:20
+  C *buffer = new C[1];
+  reinterpret_cast(buffer)[-1] = 42;
 // CHECK: AddressSanitizer: heap-buffer-overflow





Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:4-15
+// 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)

Thanks for improving the test like this!  This is now better than "just a 
test", it's essentially "executable documentation"! :)



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:36-38
-  assert(reinterpret_cast(foo) ==
- reinterpret_cast(Foo::allocated) + sizeof(void*));
-  *reinterpret_cast(Foo::allocated) = 42;

Is the reason that we had to remove this assert (and change how we overwrite 
the cookie) that on arm the cookie is 2 words?

Can we do the following?
```
int cooke_words =  : 2 : 1;
assert(reinterpret_cast(foo) ==
 reinterpret_cast(Foo::allocated) + cookie_words * 
sizeof(void*));
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 428731.
rsundahl marked 6 inline comments as done.
rsundahl added a comment.

Update diff back to dc7e02d4b4dc30d44ddebd832719a6e63396e718


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ 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;
 }
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
@@ -3,15 +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
-
-// Added to allow enabling of tests but needs investigation (rdar://91448627)
-// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch))
-
-#include 
-#include 
 #include 
-int dtor_counter;
+#include 
+
+int dtor_counter=0;
 struct C {
   int x;
   ~C() {
@@ -22,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;
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
@@ -1,16 +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: 

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 428718.
rsundahl added a comment.

Revert ItaniumCXXABI.cpp for now (unintentional push)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp


Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -2339,10 +2339,16 @@
  QualType ElementType) {
   assert(requiresArrayCookie(expr));
 
-  CharUnits SizeSize = CGF.getSizeSize();
-  CharUnits CookieSize = getArrayCookieSizeImpl(ElementType);
   unsigned AS = NewPtr.getAddressSpace();
 
+  ASTContext  = getContext();
+  CharUnits SizeSize = CGF.getSizeSize();
+
+  // The size of the cookie.
+  CharUnits CookieSize =
+  std::max(SizeSize, Ctx.getPreferredTypeAlignInChars(ElementType));
+  assert(CookieSize == getArrayCookieSizeImpl(ElementType));
+
   // Compute an offset to the cookie.
   Address CookiePtr = NewPtr;
   CharUnits CookieOffset = CookieSize - SizeSize;
@@ -2418,19 +2424,11 @@
  QualType elementType) {
   assert(requiresArrayCookie(expr));
 
-  CharUnits sizeSize = CGF.getSizeSize();
-  CharUnits cookieSize = getArrayCookieSizeImpl(elementType);
   unsigned AS = newPtr.getAddressSpace();
 
   // The cookie is always at the start of the buffer.
   Address cookie = newPtr;
 
-  // Compute an offset to the cookie.
-  CharUnits cookieOffset = cookieSize - sizeSize*2;
-  assert(cookieOffset.isZero());
-  if (!cookieOffset.isZero())
-cookie = CGF.Builder.CreateConstInBoundsByteGEP(cookie, cookieOffset);
-
   // The first element is the element size.
   cookie = CGF.Builder.CreateElementBitCast(cookie, CGF.SizeTy);
   llvm::Value *elementSize = llvm::ConstantInt::get(CGF.SizeTy,
@@ -2456,6 +2454,7 @@
 
   // 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);
 }
 


Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -2339,10 +2339,16 @@
  QualType ElementType) {
   assert(requiresArrayCookie(expr));
 
-  CharUnits SizeSize = CGF.getSizeSize();
-  CharUnits CookieSize = getArrayCookieSizeImpl(ElementType);
   unsigned AS = NewPtr.getAddressSpace();
 
+  ASTContext  = getContext();
+  CharUnits SizeSize = CGF.getSizeSize();
+
+  // The size of the cookie.
+  CharUnits CookieSize =
+  std::max(SizeSize, Ctx.getPreferredTypeAlignInChars(ElementType));
+  assert(CookieSize == getArrayCookieSizeImpl(ElementType));
+
   // Compute an offset to the cookie.
   Address CookiePtr = NewPtr;
   CharUnits CookieOffset = CookieSize - SizeSize;
@@ -2418,19 +2424,11 @@
  QualType elementType) {
   assert(requiresArrayCookie(expr));
 
-  CharUnits sizeSize = CGF.getSizeSize();
-  CharUnits cookieSize = getArrayCookieSizeImpl(elementType);
   unsigned AS = newPtr.getAddressSpace();
 
   // The cookie is always at the start of the buffer.
   Address cookie = newPtr;
 
-  // Compute an offset to the cookie.
-  CharUnits cookieOffset = cookieSize - sizeSize*2;
-  assert(cookieOffset.isZero());
-  if (!cookieOffset.isZero())
-cookie = CGF.Builder.CreateConstInBoundsByteGEP(cookie, cookieOffset);
-
   // The first element is the element size.
   cookie = CGF.Builder.CreateElementBitCast(cookie, CGF.SizeTy);
   llvm::Value *elementSize = llvm::ConstantInt::get(CGF.SizeTy,
@@ -2456,6 +2454,7 @@
 
   // 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);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 428716.
rsundahl added a comment.

Implement suggestions from reviews. (Incremental update.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
@@ -2,23 +2,21 @@
 // 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 sanitizer, the array cookie location IS writable
-//   2) w/sanitizer, the array cookie location IS writeable by default
-//   3) w/sanitizer, the flag "-fno-sanitize-address-poison-custom-array-cookie"
+//   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/sanitizer AND "-fsanitize-address-poison-custom-array-cookie", the
+//   4) w/ASan AND "-fsanitize-address-poison-custom-array-cookie", the
 //  array cookie location is NOT writable (ASAN successfully stopped it)
 //
-// RUN: %clangxx   %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
-// RUN: %clangxx_asan  %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
-// RUN: %clangxx_asan -fno-sanitize-address-poison-custom-array-cookie %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
-// RUN: %clangxx_asan -fsanitize-address-poison-custom-array-cookie%s -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_YES
+// 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
 
-#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); }
@@ -32,19 +30,20 @@
 
 void *Foo::allocated;
 
-Foo *getFoo(size_t s) {
-  return new Foo[s];
+Foo *getFoo(size_t n) {
+  return new Foo[n];
 }
 
 int main() {
   Foo *foo = getFoo(10);
-
-  *reinterpret_cast(Foo::allocated) = 42;
-// SANITIZE_YES: AddressSanitizer: heap-buffer-overflow
-// SANITIZE_YES: in main {{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]]
-// SANITIZE_YES: is located 0 bytes inside of {{18|26}}-byte region
+  fprintf(stderr, "foo  : %p\n", foo);
+  fprintf(stderr, "alloc: %p\n", Foo::allocated);
+  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");
-// SANITIZE_NOT: Unsurprisingly, we were able to write to the array cookie
+// CHECK: Unsurprisingly, we were able to write to the array cookie
 
   return 0;
 }
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
@@ -3,12 +3,12 @@
 // 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
 
-#include 
-#include 
 #include 
-int dtor_counter;
+#include 
+
+int dtor_counter=0;
 struct C {
-  size_t x;
+  int x;
   ~C() {
 dtor_counter++;
 fprintf(stderr, "DTOR %d\n", dtor_counter);
@@ -17,12 +17,11 @@
 
 __attribute__((noinline)) void Delete(C *c) { delete[] c; }
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  size_t *p = reinterpret_cast(c);
-  p[-1] = 42;
+  

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl marked an inline comment as not done.
rsundahl added inline comments.



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
   C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  buffer[-1].x = 10;
 // CHECK: AddressSanitizer: heap-buffer-overflow

rsundahl wrote:
> yln wrote:
> > Can we try to simulate a 1-byte buffer-underflow (and leave the definition 
> > of `struct C` as-is)?  This would show that ASan still reports the smallest 
> > possible infraction.
> That's a good idea, but it's not about leaving the struct the same because 
> it's definitely wrong in that type "int" has nothing to do with the cookie 
> and it only happens to be true that  (buffer[-2].x) gets to the beginning of 
> the cookie since two "int"s happen to fit inside of a "size_t". (This may not 
> always be true as in most 64-on-32 type arrangements) It's better to treat 
> the cookie as [-1] of an array of size_t elements so that when indexing [-1] 
> you get all of the cookie at once (see: 
> https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies). The reason 
> I write to the *whole* cookie at once is because of endian-ness. The existing 
> code which only writes to the first half of the cookie would reach a 
> different half of the cookie on a big-endian machine so this covers both. But 
> you have a good point, we should see if the minimum underflow is caught so 
> I'll think of a way to do that in an endian-agnostic way... 
I believe that because the existing code: 
```
buffer[-1].x = 10
```
uses a negative address from an array of int, I took that as something 
unchangeable and if it were, my arguments for making the size of the elements 
size_t have some context, //But...//now I see that unjustified and merely 
clever in some way and your approach explicitly separating the type of the 
elements of the allocation from the overflow is better in at least two ways. 
First, it removes any such "cleverness" from the test (which is really more 
distracting than it is useful) and second, it allows us to test the smallest 
possible ingress into the cookie area. Sorry that was a hard sell but 
appreciate the insight. 
```
buffer[-1].x = 10
```



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp:20
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  long *p = reinterpret_cast(c);
+  size_t *p = reinterpret_cast(c);
   p[-1] = 42;

rsundahl wrote:
> yln wrote:
> > Can we try to simulate a 1-byte buffer-underflow (and leave the definition 
> > of struct C as-is)? This would show that ASan still reports the smallest 
> > possible infraction.
> (Same answer as above)
In this situation the UAF isn't caught until the corrupted cookie has called 
delete on all 42 (supposed) elements of the array. The test depends on seeing 
all 42 members' being deleted so the write must go the low-byte. If you place 
the 42 (or anything non-zero) in the high byte, the cookie becomes > 2^54 with 
a corresponding number of calls to delete each member. If all the deletes are 
not allowed to execute, the actual UAF detection does not get executed. 
Therefore I'm going to leave this one as it is and leave the 
"minimum-underflow" to new_array_cookie_test. (The test explicitly disables 
sanitizing the underflow to the cookie but what is not clear to me is why 
access to the cookie by the second "delete" after the free doesn't trap the 
read of the poisoned cookie //before// deleting the (supposed) members. We 
might want to instrument ASan to check for a poisoned cookie earlier.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added a comment.

Adding dialog to comments made by reviewers.




Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2443
+  // Handle poisoning the array cookie in asan
+  if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && AS == 0 &&
+  (expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() ||

delcypher wrote:
> Why is there a restriction on the address space?
It's only there because it's also there in 
ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think 
that the ARMCXXABI version would be different. That said, this is the revision 
that introduced the notion originally: https://reviews.llvm.org/D5111



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2478
+  // 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

delcypher wrote:
> This comment is confusing. What's returning `0`? 
> `__asan_load_cxx_array_cookie` doesn't do that and AFAICT neither does this 
> code.
(Same answer) It's only there because it's also there in 
ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think 
that the ARMCXXABI version would be different. That said, this is the revision 
that introduced the notion originally: https://reviews.llvm.org/D5111



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2479
+  // 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.

delcypher wrote:
> I also don't understand what you mean by the comment. Could you elaborate?
(Same answer) It's only there because it's also there in 
ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think 
that the ARMCXXABI version would be different. That said, this is the revision 
that introduced the notion originally: https://reviews.llvm.org/D5111



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2485
+  CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie");
+  return CGF.Builder.CreateCall(F, numElementsPtr.getRawPointer(CGF));
 }

yln wrote:
> Same here: extract common helper for the ASan stuff to be shared by ARM and 
> ItaniumCXXABI.
Was limiting my impact but yes, I agree with you and thanks for expecting it. 
FYI and FWIW: the same concern that @delcypher has in the preceding comments 
will still exist because the factored code will come from the existing Itanium 
methods and those concerns come from that existing code that was introduced by: 
https://reviews.llvm.org/D5111.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262
   *reinterpret_cast(s) = kAsanArrayCookieMagic;
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64

yln wrote:
> yln wrote:
> > yln wrote:
> > > Nitpicking extreme:
> > > * I think the code inside `#if` shouldn't have extra indent (because 
> > > preprocessor sections don't establish code).  If in doubt, let 
> > > `clang-format` do it for you.
> > > * Let's move the comment inside the #if, just above before the line of 
> > > code.  If you ever read the pre-processed source-code, then the comment 
> > > "lives and dies" with the line of code it relates too, i.e, on x86, 
> > > currently there would be a comment without the code.
> > 
> I find this a bit confusing
> * x86_64: cookie is 1 word and passed `p` points to it
> * arm64: cookie is 2 words and passed `p` points to second half of it
> 
> Would it be worth to take the extra care in CodeGen to always pass the 
> "beginning of the cookie" to `__asan_poison_cxx_array_cookie()` and then have 
> something like that:
> ```
> size_t shadow_cookie_size = SANITIZER_ARM64 ? 2 : 1:
> internal_memset(s, kAsanArrayCookieMagic, shadow_cookie_size);
> ```
I don't think so for a collection of reasons. When the ItaniumCXXABI defines 
the "array cookie" it states: "The cookie will have size sizeof(size_t)" and it 
then describes how there may be padding preceding the cookie but makes no 
mention of a second element anywhere. 
(https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies). Now, while 
the ARMCXXABI(32) specification defines the cookie as two 4-byte (int) values 
(sizeof(element),numElements), the ARMCXXABI(64) 
(https://github.com/ARM-software/abi-aa/blob/320a56971fdcba282b7001cf4b84abb4fd993131/cppabi64/cppabi64.rst#the-generic-c-abi)
 does no such similar thing and defers to the Itanium specification of a 
single, type size_t (8 bytes) cookie which contains only (numElements). The 
initial commit creating the ARMCXXABI(64) mimicking the ARMCXXABI(32) "pair" of 
values occurred here: https://marc.info/?l=cfe-commits=135915714232578=2 
and is then (unfortunately) this 

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262
   *reinterpret_cast(s) = kAsanArrayCookieMagic;
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64

yln wrote:
> yln wrote:
> > Nitpicking extreme:
> > * I think the code inside `#if` shouldn't have extra indent (because 
> > preprocessor sections don't establish code).  If in doubt, let 
> > `clang-format` do it for you.
> > * Let's move the comment inside the #if, just above before the line of 
> > code.  If you ever read the pre-processed source-code, then the comment 
> > "lives and dies" with the line of code it relates too, i.e, on x86, 
> > currently there would be a comment without the code.
> 
I find this a bit confusing
* x86_64: cookie is 1 word and passed `p` points to it
* arm64: cookie is 2 words and passed `p` points to second half of it

Would it be worth to take the extra care in CodeGen to always pass the 
"beginning of the cookie" to `__asan_poison_cxx_array_cookie()` and then have 
something like that:
```
size_t shadow_cookie_size = SANITIZER_ARM64 ? 2 : 1:
internal_memset(s, kAsanArrayCookieMagic, shadow_cookie_size);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262
   *reinterpret_cast(s) = kAsanArrayCookieMagic;
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64

yln wrote:
> Nitpicking extreme:
> * I think the code inside `#if` shouldn't have extra indent (because 
> preprocessor sections don't establish code).  If in doubt, let `clang-format` 
> do it for you.
> * Let's move the comment inside the #if, just above before the line of code.  
> If you ever read the pre-processed source-code, then the comment "lives and 
> dies" with the line of code it relates too, i.e, on x86, currently there 
> would be a comment without the code.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2453
+CGF.Builder.CreateCall(F, cookie.getRawPointer(CGF));
+  }
 

This code is very similar to what we have in 
`ItaniumCXXABI::InitializeArrayCookie()`.  Can we try to extract a local 
`handleASanArrayCookiePoisoning()` helper?



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2485
+  CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie");
+  return CGF.Builder.CreateCall(F, numElementsPtr.getRawPointer(CGF));
 }

Same here: extract common helper for the ASan stuff to be shared by ARM and 
ItaniumCXXABI.



Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262-265
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64
+*(reinterpret_cast(s)-1) = kAsanArrayCookieMagic;
+  #endif

Nitpicking extreme:
* I think the code inside `#if` shouldn't have extra indent (because 
preprocessor sections don't establish code).  If in doubt, let `clang-format` 
do it for you.
* Let's move the comment inside the #if, just above before the line of code.  
If you ever read the pre-processed source-code, then the comment "lives and 
dies" with the line of code it relates too, i.e, on x86, currently there would 
be a comment without the code.



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:3
 // 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

❤  The boy scouts rule:
> Always leave the campground cleaner than you found it.



Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
   C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  buffer[-1].x = 10;
 // CHECK: AddressSanitizer: heap-buffer-overflow

Can we try to simulate a 1-byte buffer-underflow (and leave the definition of 
`struct C` as-is)?  This would show that ASan still reports the smallest 
possible infraction.



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp:20
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  long *p = reinterpret_cast(c);
+  size_t *p = reinterpret_cast(c);
   p[-1] = 42;

Can we try to simulate a 1-byte buffer-underflow (and leave the definition of 
struct C as-is)? This would show that ASan still reports the smallest possible 
infraction.



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:5
+// Here we test that:
+//   1) w/o sanitizer, the array cookie location IS writable
+//   2) w/sanitizer, the array cookie location IS writeable by default

sanitizer -> ASan



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:12
 //
-// XFAIL: arm
-
-// UNSUPPORTED: ios
+// RUN: %clangxx   %s -o 
%t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
+// RUN: %clangxx_asan  %s -o 
%t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT

Avoid `NOT` in FileCheck prefix names, because `-NOT` is the directive to do a 
"negative match", that is, "make sure this text does not appear".

How about:
`--check-prefix=COOKIE` for the cookie case and just skip `--check-prefix` for 
the "no cookie" (it's the default after all)



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:35
-  fprintf(stderr, "foo  : %p\n", foo);
-  fprintf(stderr, "alloc: %p\n", Foo::allocated);
-  assert(reinterpret_cast(foo) ==

Why remove this debug output?  It might be useful...



Comment at: 
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:43-47
+// SANITIZE_YES: AddressSanitizer: heap-buffer-overflow
+// SANITIZE_YES: in main 
{{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]]
+// SANITIZE_YES: is located 0 bytes inside of {{18|26}}-byte region
+  printf("Unsurprisingly, we were able to write to the array cookie\n");
+// SANITIZE_NOT: Unsurprisingly, we were able to write to the array cookie




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl added reviewers: vitalybuka, kcc.
rsundahl added a comment.

Adding Vitaly Buka and Kostya Serebryany (sanitizer maintainers).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2443
+  // Handle poisoning the array cookie in asan
+  if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && AS == 0 &&
+  (expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() ||

Why is there a restriction on the address space?



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2478
+  // 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

This comment is confusing. What's returning `0`? `__asan_load_cxx_array_cookie` 
doesn't do that and AFAICT neither does this code.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2479
+  // 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.

I also don't understand what you mean by the comment. Could you elaborate?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl updated this revision to Diff 428099.
rsundahl added a comment.

Adding ASAN test "new_array_cookie_with_new_from_class" (rdar://92884511)

This test "new_array_cookie_with_new_from_class " was dependent on
the cookie size on x86_64 and was either unsupported or expected to
fail on arm. Those restrictions can be removed with this revision.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125195/new/

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
@@ -1,10 +1,18 @@
-// 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 sanitizer, the array cookie location IS writable
+//   2) w/sanitizer, the array cookie location IS writeable by default
+//   3) w/sanitizer, the flag "-fno-sanitize-address-poison-custom-array-cookie"
+//  is a NOP (because this is the default) and finally,
+//   4) w/sanitizer AND "-fsanitize-address-poison-custom-array-cookie", the
+//  array cookie location is NOT writable (ASAN successfully stopped it)
 //
-// XFAIL: arm
-
-// UNSUPPORTED: ios
+// RUN: %clangxx   %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
+// RUN: %clangxx_asan  %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
+// RUN: %clangxx_asan -fno-sanitize-address-poison-custom-array-cookie %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
+// RUN: %clangxx_asan -fsanitize-address-poison-custom-array-cookie%s -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_YES
 
 #include 
 #include 
@@ -22,19 +30,21 @@
   }
 };
 
-Foo::~Foo() {}
 void *Foo::allocated;
 
-Foo *getFoo(size_t n) {
-  return new Foo[n];
+Foo *getFoo(size_t s) {
+  return new Foo[s];
 }
 
 int main() {
   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::allocated) = 42;
+// SANITIZE_YES: AddressSanitizer: heap-buffer-overflow
+// SANITIZE_YES: in main {{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]]
+// SANITIZE_YES: is located 0 bytes inside of {{18|26}}-byte region
+  printf("Unsurprisingly, we were able to write to the array cookie\n");
+// SANITIZE_NOT: Unsurprisingly, we were able to write to the array cookie
+
   return 0;
 }
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
@@ -3,17 +3,12 @@
 // 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
-
-// Added to allow enabling of tests but needs investigation (rdar://91448627)
-// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch))
-
 #include 
 #include 
 #include 
 int dtor_counter;
 struct C {
-  int x;
+  size_t x;
   ~C() {
 dtor_counter++;
 fprintf(stderr, "DTOR %d\n", dtor_counter);
@@ -22,7 +17,7 @@
 
 __attribute__((noinline)) void Delete(C *c) { delete[] c; }
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  long *p = reinterpret_cast(c);
+  size_t *p = reinterpret_cast(c);
   p[-1] = 42;
 }
 
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
@@ -1,18 +1,13 @@
 // REQUIRES: asan-64-bits
 // RUN: %clangxx_asan -O3 %s -o %t
-// RUN:not %run %t 2>&1  | FileCheck %s
+// RUN:  

[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl created this revision.
rsundahl added reviewers: yln, kubamracek, rjmccall, dcoughlin, delcypher, 
aralisza, thetruestblue, wrotki.
Herald added subscribers: pengfei, kristof.beyls.
Herald added a project: All.
rsundahl requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

Hooks into the address sanitizer that support array cookie poisoning and
validation were being generated for x86_64 but not for ARM. (amended)

In addition to the ItaniumCXXABI array cookie of a single size_t element
containing the number of elements in the allocated array, the ARMCXXABI adds
a second size_t element containing the sizeof(element). This difference in
cookie size created the need to override the methods ::InitializeArrayCookie()
and ::readArrayCookieImpl(). Later, in support of ASAN poison array cookies,
calls to __asan_poison_cxx_array_cookie() and __asan_load_cxx_array_cookie()
were added to each method respectively. However, these "hooks" were only
implemented for the ItaniumCXXABI. This commit adds the same functionality
to the overridden ARMCXXABI methods.

rdar://92765369


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125195

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  compiler-rt/lib/asan/asan_poisoning.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
  compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp

Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
@@ -3,17 +3,12 @@
 // 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
-
-// Added to allow enabling of tests but needs investigation (rdar://91448627)
-// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch))
-
 #include 
 #include 
 #include 
 int dtor_counter;
 struct C {
-  int x;
+  size_t x;
   ~C() {
 dtor_counter++;
 fprintf(stderr, "DTOR %d\n", dtor_counter);
@@ -22,7 +17,7 @@
 
 __attribute__((noinline)) void Delete(C *c) { delete[] c; }
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  long *p = reinterpret_cast(c);
+  size_t *p = reinterpret_cast(c);
   p[-1] = 42;
 }
 
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
@@ -1,18 +1,13 @@
 // 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
-
-// Added to allow enabling of tests but needs investigation (rdar://91448627)
-// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch))
-
 #include 
 #include 
 struct C {
-  int x;
+  size_t x;
   ~C() {
 fprintf(stderr, "\n");
 exit(1);
@@ -21,10 +16,10 @@
 
 int main(int argc, char **argv) {
   C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  buffer[-1].x = 10;
 // 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 {{0 bytes inside of 12|8 bytes inside of 24}}-byte region
 // NO_COOKIE: 
   delete [] buffer;
 }
Index: compiler-rt/lib/asan/asan_poisoning.cpp
===
--- compiler-rt/lib/asan/asan_poisoning.cpp
+++ 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;
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64
+*(reinterpret_cast(s)-1) = kAsanArrayCookieMagic;
+  #endif
 }
 
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -2424,6 +2424,8 @@
  QualType elementType) {
   assert(requiresArrayCookie(expr));
 
+  unsigned AS = newPtr.getAddressSpace();
+
   // The cookie is always at the start of the buffer.
   Address cookie = newPtr;
 
@@ -2435,7 +2437,20 @@
 
   //