[hwasan] print exact mismatch offset for short granules.

[eugenis]

The reason it was done this way was that what looks like a short granule might not be one - sometimes a low tag value is simply a low tag value, and the last byte of the granule is user memory that can match by chance.

On the other hand, the chance of that happening is very low and perhaps this is a good tradeoff for better report quality.

@pcc @hctim WDYT?

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c

15

why offset 10 and not 5?

[fmayer]

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c

15

The first 5 bytes, [5, 10) are the valid access, and the invalid one is [10, 26).

[fmayer]

In D104463#2825430, @eugenis wrote:

The reason it was done this way was that what looks like a short granule might not be one - sometimes a low tag value is simply a low tag value, and the last byte of the granule is user memory that can match by chance.

On the other hand, the chance of that happening is very low and perhaps this is a good tradeoff for better report quality.

@pcc @hctim WDYT?

isn't the chance of this lower than a normal tag matching by accident (also producing an incorrect report)? The chance of this should be (16 / 256) * (1 / 256).

[fmayer]

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c

15

Sorry, my mistake, replied too fast. Will look tomorrow.

[eugenis]

isn't the chance of this lower than a normal tag matching by accident (also producing an incorrect report)? The chance of this should be (16 / 256) * (1 / 256).

Right, we can find an unrelated alloc/dealloc stack trace because of an accidental tag match. That happens.

[fmayer]

Fixed logic.

[fmayer]

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c

15

Fixed. Sorry about that, added second test as well.

[eugenis]

compiler-rt/lib/hwasan/hwasan_report.cpp

635

Is this really different from ShadowToMem(tag_ptr) ?
If you prefer this form, please put (untagged_addr + offset) in brackets to make operator precedence clear.

But if I read __hwasan_test_shadow correctly, (untagged_addr + offset) must be granule-aligned already.

[fmayer]

Improve readability.

[fmayer]

Add test for aligned (untagged_addr + offset).

[fmayer]

compiler-rt/lib/hwasan/hwasan_report.cpp

635

Is this really different from ShadowToMem(tag_ptr) ?

I wrote it this way so it's easy to see I am separating the low from the high bits from this and the line below.

If you prefer this form, please put (untagged_addr + offset) in brackets to make operator precedence clear.

Done.

But if I read __hwasan_test_shadow correctly, (untagged_addr + offset) must be granule-aligned already.

There is the edge case where offset == 0, then untagged_addr + offset is not necessarily aligned.
I added more test cases to cover both the aligned and non-aligned scenarios.

[eugenis]

compiler-rt/lib/hwasan/hwasan_report.cpp

641

This line assumes that the first bad access has to be 1 byte past the end.

What if ex. the short granule is 5 byte in size, and the access is for offsets [10, 12) ?

[fmayer]

Correctly handle accesses out of range of granule.

[eugenis]

Please add a test where the bad access starts after the end of the allocation.

LGTM with that and the other comments.

compiler-rt/lib/hwasan/hwasan_report.cpp

634

call it "granule_ptr" or something like that.

638

"this is the offset of the leftmost accessed byte within the bad granule" or something more literate. I.e. it's not the bad address, it's the start of the accessed ranged clamped to the granule bounds.

643

"if the access starts after the end of the short granule, then the first bad byte is the first byte of the access; otherwise it is the first byte past the end of the short granule" or something like that in your own words. This is not really obvious from the code.

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c

22

I was really confused why the size is 10 until I realized that in "%t 5 2>&1" 2 is the file descriptor and not the argument. Please make all arguments explicit in the command lines and remove the argc checks (or rather turn them into asserts).

[fmayer]

Update comments and tests.

[fmayer]

Thanks!

[eugenis]

LGTM

[vitalybuka]

Probably caused by this patch https://lab.llvm.org/buildbot/#/builders/77/builds/7379

[fmayer]

Fix test on aarch64.

Tested on Android and x86.

[hctim]

(untested) maybe broke the x64 LAM under QEMU bot: https://lab.llvm.org/buildbot/#/builders/169/builds/1199/steps/25/logs/stdio

[fmayer]

In D104463#2845289, @hctim wrote:

(untested) maybe broke the x64 LAM under QEMU bot: https://lab.llvm.org/buildbot/#/builders/169/builds/1199/steps/25/logs/stdio

Sorry about that, reverted in rG400509238a0ff71f62fbf3a5d0cb576bc163b4b8.

[fmayer]

do not check short granule if mem tag is 0.

compiler-rt/lib/hwasan/hwasan_report.cpp

void ReportTagMismatch(StackTrace *stack, uptr tagged_addr, uptr access_size,
   tag_t *tag_ptr =
       reinterpret_cast<tag_t *>(MemToShadow(untagged_addr + offset));
   tag_t mem_tag = *tag_ptr;
 
   Printf("%s", d.Access());
   Printf("%s of size %zu at %p tags: %02x/%02x (ptr/mem) in thread T%zd\n",
          is_store ? "WRITE" : "READ", access_size, untagged_addr, ptr_tag,
          mem_tag, t->unique_id());
+  if (mem_tag < kShadowAlignment) {
+    tag_t *mem_ptr = reinterpret_cast<tag_t *>((untagged_addr + offset) &
+                                               ~(kShadowAlignment - 1));
+    // If offset is 0, (untagged_addr + offset) is not aligned to granules, so
+    // we need to correct for that.
+    u8 in_granule_offset = (untagged_addr + offset) & (kShadowAlignment - 1);
+    // The first mismatch was a short granule that matched the ptr_tag.
+    if (mem_ptr[kShadowAlignment - 1] == ptr_tag) {
+      // If the failing access was within the granule, we need to correct for
+      // that. If it is outside, the offset is zero.
+      if (mem_tag > in_granule_offset) {
+        offset += mem_tag - in_granule_offset;
+      }
+    }
+  }
   if (offset != 0)
-    Printf("Invalid access starting at offset [%zu, %zu)\n", offset,
-           Min(access_size, static_cast<uptr>(offset) + (1 << kShadowScale)));
+    Printf("Invalid access starting at offset %zu\n", offset);
   Printf("%s", d.Default());
 
   stack->Print();
 
   PrintAddressDescription(tagged_addr, access_size,
                           current_stack_allocations.get());
   t->Announce();
 

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c

 // RUN: %clang_hwasan  %s -o %t
-// RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK
+// RUN: not %run %t 5 2>&1 | FileCheck %s --check-prefix=CHECK5
+// RUN: not %run %t 7 2>&1 | FileCheck %s --check-prefix=CHECK7
+// RUN: not %run %t 8 20 2>&1 | FileCheck %s --check-prefix=CHECK8
 
 // REQUIRES: stable-runtime
 
 #include <sanitizer/hwasan_interface.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
 int main(int argc, char **argv) {
   __hwasan_enable_allocator_tagging();
-  char *volatile x = (char *)malloc(10);
-  memset(x + 5, 0, 26);
-  // CHECK: is located 5 bytes inside 10-byte region
+  int read_offset = argc < 2 ? 5 : atoi(argv[1]);
+  int size = argc < 3 ? 10 : atoi(argv[2]);
+  char *volatile x = (char *)malloc(size);
+  memset(x + read_offset, 0, 26);
+  // CHECK5: Invalid access starting at offset 5
+  // CHECK5: is located 5 bytes inside 10-byte region
+  // CHECK7: Invalid access starting at offset 3
+  // CHECK7: is located 7 bytes inside 10-byte region
+  // CHECK8: Invalid access starting at offset 12
+  // CHECK8: is located 8 bytes inside 20-byte region
   free(x);
 }

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow.c

 //
 // CHECKMm30: is a large allocated heap chunk; size: 1003520 offset: -30
 // CHECKMm30: is located 30 bytes to the left of 1000000-byte region
 //
 // CHECKM: is a large allocated heap chunk; size: 1003520 offset: 1000000
 // CHECKM: is located 0 bytes to the right of 1000000-byte region
 //
 // CHECK31: tags: [[TAG:..]]/0e (ptr/mem)
+// CHECK31-NOT: Invalid access starting at offset
 // CHECK31: is located 1 bytes to the right of 30-byte region
 // CHECK31: Memory tags around the buggy address
 // CHECK31: [0e]
 // CHECK31: Tags for short granules around the buggy address
 // CHECK31: {{\[}}[[TAG]]]
 //
+// CHECK20-NOT: Invalid access starting at offset
 // CHECK20: is located 10 bytes to the right of 20-byte region [0x{{.*}}0,0x{{.*}}4)
   free(x);
 }