[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<long*>(c);
+  size_t *p = reinterpret_cast<size_t*>(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