[HWASAN] Improve tag-mismatch diagnostics

[evgeny777]

Summary

This patch improves tag-mismatch report in the following ways:

• SigTrap explicitly sets X0 register so fault address and tags are correctly shown on AArch64
• Access sizes not equal to power of 2 are correctly shown on both AArch64 and X86_64
• ptr and mem tags are displayed correctly when SigTrap is invoked from CheckAddressSized

[eugenis]

lib/hwasan/hwasan_checks.h

98

This would report an error with the size of the memory access being equal to the distance from the first bad granule to the end of the memset() buffer. This sounds very confusing.

I suggest doing the sized version of SigTrap with the starting address and the full size of the buffer, and then adding extra analysis in reporting code. The result could be a note saying that the first bad byte is somewhere at offset [A, A+16] from the start of the accessed range.

[evgeny777]

Addressed review comments

[eugenis]

lib/hwasan/hwasan_checks.h

27

this can be done better with register asm:

register uptr x0 asm("x0") = p;

and then use "r"(x0) in asm constraints.

It would also tell the compiler that x0 is clobbered, which might matter in the -fsanitize-recover mode.

lib/hwasan/hwasan_report.cc

408

Reuse __hwasan_test_shadow.

[evgeny777]

lib/hwasan/hwasan_report.cc

408

This function looks a bit strange to me. For instance it can return -1 either in case of error or in case of a tag mismatch when address is equal to start of granule plus one. It would make much more sense if this function returned pointer instead of offset

[evgeny777]

Changed way X0 and X1 are assigned on AArch64

[eugenis]

lib/hwasan/hwasan_report.cc

408

Oh, that's a good catch! I'd prefer the interface to stay as it is for consistency with __msan_test_shadow, but fix the return offset to be within [0, sz] is case of an error.

Also, this function assumes that tag 0 is match-all, which is not the case. The ptr_tag==0 check could be removed.

[eugenis]

lib/hwasan/hwasan_checks.h

28

brk %1

[evgeny777]

Addressed

[eugenis]

LGTM

lib/hwasan/hwasan_report.cc

419

This phrase sounds awkward. There should probably be a "the" before "first", at least... @pcc

[eugenis]

And we should probably skip this note in the case where it is obvious - for example, for an 8 byte, 8-aligned access!

[evgeny777]

assume that case is not "obvious" when offset is not zero.

[eugenis]

LGTM with comments.

Please add a -NOT test line somewhere to check that the new note is not printed in "obvious" cases.

test/hwasan/TestCases/mem-intrinsics.c

27

I think this could be flaky - there is a chance that PTR_TAG will the be last one on its line, and MEM_TAG - the first one on the following line. Align the buffer to at least 256 byte to prevent this from happening. Perhaps simply mmap a page of memory?

lib/hwasan/hwasan_checks.h

 // This file is a part of HWAddressSanitizer.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef HWASAN_CHECKS_H
 #define HWASAN_CHECKS_H
 
 #include "hwasan_mapping.h"
+#include "sanitizer_common/sanitizer_common.h"
 
 namespace __hwasan {
 template <unsigned X>
 __attribute__((always_inline)) static void SigTrap(uptr p) {
 #if defined(__aarch64__)
   (void)p;
   // 0x900 is added to do not interfere with the kernel use of lower values of
   // brk immediate.
-  // FIXME: Add a constraint to put the pointer into x0, the same as x86 branch.
-  asm("brk %0\n\t" ::"n"(0x900 + X));
+  asm("ldr X0, %0\nbrk %1\n\t" ::"g"(p), "n"(0x900 + X));
 #elif defined(__x86_64__)
   // INT3 + NOP DWORD ptr [EAX + X] to pass X to our signal handler, 5 bytes
   // total. The pointer is passed via rdi.
   // 0x40 is added as a safeguard, to help distinguish our trap from others and
   // to avoid 0 offsets in the command (otherwise it'll be reduced to a
   // different nop command, the three bytes one).
   asm volatile(
       "int3\n"
       "nopl %c0(%%rax)\n" ::"n"(0x40 + X),
       "D"(p));
 #else
   // FIXME: not always sigill.
   __builtin_trap();
 #endif
   // __builtin_unreachable();
 }
 
+// Version with access size which is not power of 2
+template <unsigned X>
+__attribute__((always_inline)) static void SigTrap(uptr p, uptr size) {
+#if defined(__aarch64__)
+  asm("ldr X0, %0\n"
+      "ldr X1, %1\n"
+      "brk %2\n\t" ::"g"(p),
+      "g"(size), "n"(0x900 + X));
+#elif defined(__x86_64__)
+  // Size is stored in rsi.
+  asm volatile(
+      "int3\n"
+      "nopl %c0(%%rax)\n" ::"n"(0x40 + X),
+      "D"(p), "S"(size));
+#else
+  __builtin_trap();
+#endif
+  // __builtin_unreachable();
+}
+
 enum class ErrorAction { Abort, Recover };
 enum class AccessType { Load, Store };
 
 template <ErrorAction EA, AccessType AT, unsigned LogSize>
 __attribute__((always_inline, nodebug)) static void CheckAddress(uptr p) {
   tag_t ptr_tag = GetTagFromPointer(p);
   uptr ptr_raw = p & ~kAddressTagMask;
   tag_t mem_tag = *(tag_t *)MemToShadow(ptr_raw);
if (sz == 0)
     return;
   tag_t ptr_tag = GetTagFromPointer(p);
   uptr ptr_raw = p & ~kAddressTagMask;
   tag_t *shadow_first = (tag_t *)MemToShadow(ptr_raw);
   tag_t *shadow_last = (tag_t *)MemToShadow(ptr_raw + sz - 1);
   for (tag_t *t = shadow_first; t <= shadow_last; ++t)
     if (UNLIKELY(ptr_tag != *t)) {
       SigTrap<0x20 * (EA == ErrorAction::Recover) +
-              0x10 * (AT == AccessType::Store) + 0xf>(p);
-      if (EA == ErrorAction::Abort)
-        __builtin_unreachable();
+              0x10 * (AT == AccessType::Store) + 0xf>(p, sz);
     }
 }
 }  // end namespace __hwasan
 
 #endif  // HWASAN_CHECKS_H

lib/hwasan/hwasan_report.cc

void ReportTagMismatch(StackTrace *stack, uptr tagged_addr, uptr access_size,
   uptr pc = stack->size ? stack->trace[0] : 0;
   Report("ERROR: %s: %s on address %p at pc %p\n", SanitizerToolName, bug_type,
          untagged_addr, pc);
 
   Thread *t = GetCurrentThread();
 
   tag_t ptr_tag = GetTagFromPointer(tagged_addr);
   tag_t *tag_ptr = reinterpret_cast<tag_t*>(MemToShadow(untagged_addr));
+
+  // For accesses spanned over granule boundary tag_ptr may not point to
+  // correct tag
+  uptr granule = untagged_addr;
+  for (; *tag_ptr == ptr_tag;
+       ++tag_ptr, granule = RoundUpTo(granule + 1, 1 << kShadowScale)) {
+  }
+  CHECK(granule - untagged_addr < access_size);
   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());
+  Printf("Offset to first bad byte is in range [%zu, %zu)\n",
+         granule - untagged_addr,
+         Min(access_size,
+             RoundUpTo(granule + 1, 1 << kShadowScale) - untagged_addr));
   Printf("%s", d.Default());
 
   stack->Print();
 
   PrintAddressDescription(tagged_addr, access_size,
                           current_stack_allocations.get());
   t->Announce();
 
   PrintTagsAroundAddr(tag_ptr);
 
   ReportErrorSummary(bug_type, stack);
 }
 
 }  // namespace __hwasan

test/hwasan/TestCases/mem-intrinsics.c

#if TEST_NO == 1
   memset(Q, 0, 32);
 #elif TEST_NO == 2
   memmove(Q, Q + 16, 16);
 #elif TEST_NO == 3
   memcpy(Q, P, 32);
 #endif
   write(STDOUT_FILENO, "recovered\n", 10);
   // WRITE: ERROR: HWAddressSanitizer: tag-mismatch on address
-  // WRITE: WRITE {{.*}} tags: [[PTR_TAG:..]]/[[MEM_TAG:..]] (ptr/mem)
+  // WRITE: WRITE of size 32 at {{.*}} tags: [[PTR_TAG:..]]/[[MEM_TAG:..]] (ptr/mem)
+  // WRITE: Offset to first bad byte is in range [16, 32)
   // WRITE: Memory tags around the buggy address (one tag corresponds to 16 bytes):
-  // WRITE: =>{{.*}}[[MEM_TAG]]
+  // WRITE: =>{{.*}}[[PTR_TAG]]{{[[:space:]]\[}}[[MEM_TAG]]
   // WRITE-NOT: recovered
 
   // READ: ERROR: HWAddressSanitizer: tag-mismatch on address
-  // READ: READ {{.*}} tags: [[PTR_TAG:..]]/[[MEM_TAG:..]] (ptr/mem)
+  // READ: READ of size 16 at {{.*}} tags: [[PTR_TAG:..]]/[[MEM_TAG:..]] (ptr/mem)
+  // READ: Offset to first bad byte is in range [0, 16)
   // READ: Memory tags around the buggy address (one tag corresponds to 16 bytes):
-  // READ: =>{{.*}}[[MEM_TAG]]
+  // READ: =>{{.*}}[[PTR_TAG]]{{[[:space:]]\[}}[[MEM_TAG]]
   // READ-NOT: recovered
 
   // RECOVER: recovered
   return 0;
 }

test/hwasan/TestCases/rich-stack.c

int main(int argc, char **argv) {
   int err_depth = atoi(argv[2]);
   int offset = atoi(argv[3]);
   FOO(depth, err_depth, offset);
   return 0;
 }
 
 // R321: HWAddressSanitizer: tag-mismatch
 // R321-NEXT: WRITE of size 8
+// R321-NEXT: Offset to first bad byte is in range [0, 8)
 // R321-NEXT: in BAR
 // R321-NEXT: in FOO
 // R321-NEXT: in main
 // R321: is located in stack of thread T0