[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 ↗

(On Diff #181283)

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 ↗

(On Diff #181508)

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 ↗

(On Diff #181508)

Reuse __hwasan_test_shadow.

[evgeny777]

lib/hwasan/hwasan_report.cc

408 ↗

(On Diff #181508)

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 ↗

(On Diff #181508)

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 ↗

(On Diff #181784)

brk %1

[evgeny777]

Addressed

[eugenis]

LGTM

lib/hwasan/hwasan_report.cc

415 ↗

(On Diff #182004)

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 ↗

(On Diff #182466)

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?

compiler-rt/trunk/lib/hwasan/hwasan.cc

void __hwasan_print_shadow(const void *p, uptr sz) {
   for (uptr s = shadow_first; s <= shadow_last; ++s)
     Printf("  %zx: %x\n", ShadowToMem(s), *(tag_t *)s);
 }
 
 sptr __hwasan_test_shadow(const void *p, uptr sz) {
   if (sz == 0)
     return -1;
   tag_t ptr_tag = GetTagFromPointer((uptr)p);
-  if (ptr_tag == 0)
-    return -1;
   uptr ptr_raw = UntagAddr(reinterpret_cast<uptr>(p));
   uptr shadow_first = MemToShadow(ptr_raw);
   uptr shadow_last = MemToShadow(ptr_raw + sz - 1);
   for (uptr s = shadow_first; s <= shadow_last; ++s)
-    if (*(tag_t*)s != ptr_tag)
-      return ShadowToMem(s) - ptr_raw;
+    if (*(tag_t *)s != ptr_tag) {
+      sptr offset = ShadowToMem(s) - ptr_raw;
+      return offset < 0 ? 0 : offset;
+    }
   return -1;
 }
 
 u16 __sanitizer_unaligned_load16(const uu16 *p) {
   return *p;
 }
 u32 __sanitizer_unaligned_load32(const uu32 *p) {
   return *p;

compiler-rt/trunk/lib/hwasan/hwasan_checks.h

 //===-- hwasan_checks.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // 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));
+  register uptr x0 asm("x0") = p;
+  asm("brk %1\n\t" ::"r"(x0), "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__)
+  register uptr x0 asm("x0") = p;
+  register uptr x1 asm("x1") = size;
+  asm("brk %2\n\t" ::"r"(x0), "r"(x1), "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);
+              0x10 * (AT == AccessType::Store) + 0xf>(p, sz);
       if (EA == ErrorAction::Abort)
         __builtin_unreachable();
     }
 }
 }  // end namespace __hwasan
 
 #endif  // HWASAN_CHECKS_H

compiler-rt/trunk/lib/hwasan/hwasan_report.cc

void ReportTagMismatch(StackTrace *stack, uptr tagged_addr, uptr access_size,
   // TODO: when possible, try to print heap-use-after-free, etc.
   const char *bug_type = "tag-mismatch";
   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();
 
+  sptr offset =
+      __hwasan_test_shadow(reinterpret_cast<void *>(tagged_addr), access_size);
+  CHECK(offset >= 0 && offset < static_cast<sptr>(access_size));
   tag_t ptr_tag = GetTagFromPointer(tagged_addr);
-  tag_t *tag_ptr = reinterpret_cast<tag_t*>(MemToShadow(untagged_addr));
+  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 (offset != 0)
+    Printf("Invalid access starting at offset [%zu, %zu)\n", offset,
+           Min(access_size, static_cast<uptr>(offset) + (1 << kShadowScale)));
   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

compiler-rt/trunk/test/hwasan/TestCases/mem-intrinsics.c

 // RUN: %clang_hwasan %s -DTEST_NO=1 -mllvm -hwasan-instrument-mem-intrinsics -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=WRITE
 // RUN: %clang_hwasan %s -DTEST_NO=2 -mllvm -hwasan-instrument-mem-intrinsics -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=READ
 // RUN: %clang_hwasan %s -DTEST_NO=3 -mllvm -hwasan-instrument-mem-intrinsics -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=WRITE
 // RUN: %clang_hwasan %s -DTEST_NO=2 -mllvm -hwasan-instrument-mem-intrinsics -o %t && not %env_hwasan_opts=halt_on_error=0 %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
 
 // REQUIRES: stable-runtime
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
 
 int main() {
-  char Q[16];
-  char P[16];
+  char Q[16] __attribute__((aligned(256)));
+  char P[16] __attribute__((aligned(256)));
 #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: Invalid access starting at offset [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-NOT: Invalid access starting at offset
   // READ: READ {{.*}} tags: [[PTR_TAG:..]]/[[MEM_TAG:..]] (ptr/mem)
   // 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;
 }