[HWASan] Add sizeof(global) in report even if symbols missing.

[hctim]

Summary

Refactor the current global header iteration to be callback-based, and add a feature that reports the size of the global variable during reporting. This allows binaries without symbols to still report the size of the global variable, which is always available in the HWASan globals PT_NOTE metadata.

[pcc]

Instead of adding these callbacks would it be simpler to have a function that takes the pointer to phdrs and returns (global_begin, global_end) (both 0 if not present)?

compiler-rt/lib/hwasan/hwasan.cpp

197

Why add this? (same below)

246

*all the notes

compiler-rt/lib/hwasan/hwasan_report.cpp

252

Name (and comment above) implies that the size is stored in the phdr itself. Maybe s/phdr/descriptor/'?

262

Can you use dladdr to avoid iterating and the callback here? (i.e. find ELF headers via dli_fbase)

[hctim]

In D80599#2056315, @pcc wrote:

Instead of adding these callbacks would it be simpler to have a function that takes the pointer to phdrs and returns (global_begin, global_end) (both 0 if not present)?

That would require duplicating the global iteration logic across the reporting and initialisation paths, which I was trying to avoid. Having a callback that recieves a single global is easier for my brain, but please let me know if you'd prefer it the other way around.

compiler-rt/lib/hwasan/hwasan.cpp

197

Naively assumed these symbols were being exported. Removed.

[hctim]

• Addressed Peter's comments, added more comments to header describing how these functions operate.

[pcc]

In D80599#2058889, @hctim wrote:

In D80599#2056315, @pcc wrote:

Instead of adding these callbacks would it be simpler to have a function that takes the pointer to phdrs and returns (global_begin, global_end) (both 0 if not present)?

That would require duplicating the global iteration logic across the reporting and initialisation paths, which I was trying to avoid. Having a callback that recieves a single global is easier for my brain, but please let me know if you'd prefer it the other way around.

With the function that returns (global_begin, global_end) it's just a for loop really. You could have member functions in hwasan_global like

size_t size() const { return info & 0xffffff; }

to make the client code easier to read.

[pcc]

And instead of returning (global_begin, global_end) you could return something like an ArrayRef<hwasan_global> (I don't think we have ArrayRef in compiler-rt, but you could add one, it's pretty simple). Now the client code just looks like

for (hwasan_global &g : HwasanGlobalsFor(phdr, ...)) {
 ...
}

[hctim]

• Pull out hwasan globals into its own file, and rework callback based to return-range based.
• Also add ArrayRef<T> to sanitizer common.

[hctim]

• Renamed a function, so update the comment.
• Moved PT note descriptions out of the header.

[pcc]

compiler-rt/lib/hwasan/hwasan_globals.cpp

25 ↗

(On Diff #266956)

No trivial accessors please.

95 ↗

(On Diff #266956)

With the ArrayRef ctor changed this becomes return {}.

compiler-rt/lib/hwasan/hwasan_globals.h

24–32 ↗

(On Diff #266956)

I wouldn't add this stuff, this type of error is pretty unlikely and would be caught in code review.

37 ↗

(On Diff #266956)

I would just put the definitions inline so that readers don't need to go hunting for them, they're pretty small.

46 ↗

(On Diff #266956)

Remove obvious comment.

compiler-rt/lib/hwasan/hwasan_report.cpp

250

s/phdr/descriptor/

compiler-rt/lib/sanitizer_common/sanitizer_common.h

985 ↗

(On Diff #266956)

Shouldn't it create an empty ArrayRef?

[hctim]

compiler-rt/lib/hwasan/hwasan_globals.h

24–32 ↗

(On Diff #266956)

Done - but left the comment at the top of the class.

[hctim]

• Address pcc's comments.

[pcc]

compiler-rt/lib/hwasan/hwasan.cpp

311

Remove braces

357

Remove braces

411–412

Remove braces

compiler-rt/lib/hwasan/hwasan_globals.cpp

92 ↗

(On Diff #266977)

Newline

compiler-rt/lib/hwasan/hwasan_globals.h

27 ↗

(On Diff #266977)

2^24, "larger globals have multiple descriptors"

37 ↗

(On Diff #266977)

Rename this back to gv_relptr.

51 ↗

(On Diff #266977)

Newline

compiler-rt/lib/hwasan/hwasan_report.cpp

264

Remove braces

264

I'm not sure that this first argument can be ehdr -- it needs to be the load bias, which is normally the same as ehdr in position-independent binaries, but it can be different in e.g. non-PIE executables, binaries created using lld's partitioning feature [1] and possibly also binaries linked using a linker script.

To compute the load bias from the address of the ELF header, you can look for a PT_LOAD with p_offset=0. The load bias is found by subtracting that program header's p_vaddr from the address of the ELF header.

[1] https://lld.llvm.org/Partitions.html

364

Is size == 0 realistic? I don't think you can declare a zero-size global in C.

[hctim]

• Address some code style problems and ensure we get the correct load bias.

compiler-rt/lib/hwasan/hwasan_report.cpp

364

Sorry - I don't understand the comment. size == 0 here indicates that the descriptor lookup failed.

[hctim]

• Change sizeof(global) comment.

[hctim]

compiler-rt/lib/hwasan/hwasan_report.cpp

278

This is wrong. TODO(me).

[hctim]

compiler-rt/lib/hwasan/hwasan_report.cpp

268

And this is also wrong, not walking the headers here.

[hctim]

• Fix up phdr iteration and correctly find the load bias.

As it turns out, dli_fbase returned by dladdr() and dlpi_addr by dl_iterate_phdr both call themselves "base address", but are different things. Absolutely did my head in.

[hctim]

Friendly ping

[pcc]

LGTM

compiler-rt/lib/hwasan/hwasan_globals.h

27 ↗

(On Diff #266977)

nit: 2 << 24 == 2^25 != 2^24.

compiler-rt/lib/hwasan/hwasan_report.cpp

364

Ah okay. I'd probably leave a brief comment about that here then.

[hctim]

• Rebase and address some of pcc's nits.

[hctim]

Looks like my local commits didn't get squashed before upstream, but the net impact was [base .. a744142] (i.e. the last version before commit).

compiler-rt/lib/hwasan/hwasan.h

struct __hw_jmp_buf_struct {
   __hw_register_buf __jmpbuf; // Calling environment.
   int __mask_was_saved;       // Saved the signal mask?
   __hw_sigset_t __saved_mask; // Saved signal mask.
 };
 typedef struct __hw_jmp_buf_struct __hw_jmp_buf[1];
 typedef struct __hw_jmp_buf_struct __hw_sigjmp_buf[1];
 #endif // HWASAN_WITH_INTERCEPTORS && __aarch64__
 
+// Callback function type for the globals iteration functions below. Arguments
+// include the fully-relocated pointer to the global, the size and tag of said
+// global, and the data argument passed to the iteration function that
+// dispatches this callback. Returns true if the iteration should stop, false
+// otherwise.
+typedef bool (*hwasan_globals_cb)(uptr global_ptr, uptr global_size,
+                                  u8 global_tag, void *data);
+
+// Walk through all loaded DSO's, and call `cb()` for each HWASan global.
+// This function returns the return value of the last `cb()` invocation.
+bool IterateHwasanGlobals(hwasan_globals_cb cb, void *data);
+// Walk through the specific DSO (as specified by the base, phdr, and phnum),
+// calling `cb()` for each HWASan global. Returns the return value of the last
+// `cb()` invocation.
+bool IterateHwasanGlobalsInObject(ElfW(Addr) base, const ElfW(Phdr) * phdr,
+                                  ElfW(Half) phnum, hwasan_globals_cb cb,
+                                  void *data);
+
 #endif  // HWASAN_H

compiler-rt/lib/hwasan/hwasan.cpp

if (!t) {
     // The thread is still being created, or has already been destroyed.
     size = 0;
     return;
   }
   Unwind(max_depth, pc, bp, context, t->stack_top(), t->stack_bottom(),
          request_fast);
 }
 
 struct hwasan_global {
   s32 gv_relptr;
   u32 info;
 };
 
-static void InitGlobals(const hwasan_global *begin, const hwasan_global *end) {
-  for (auto *desc = begin; desc != end; ++desc) {
-    uptr gv = reinterpret_cast<uptr>(desc) + desc->gv_relptr;
-    uptr size = desc->info & 0xffffff;
-    uptr full_granule_size = RoundDownTo(size, 16);
-    u8 tag = desc->info >> 24;
-    TagMemoryAligned(gv, full_granule_size, tag);
-    if (size % 16)
-      TagMemoryAligned(gv + full_granule_size, 16, size % 16);
-  }
-}
-
 enum { NT_LLVM_HWASAN_GLOBALS = 3 };
 
 struct hwasan_global_note {
   s32 begin_relptr;
   s32 end_relptr;
 };
 
 // Check that the given library meets the code model requirements for tagged
if (max_addr - min_addr > 1ull << 32) {
     Die();
   }
   if (max_addr > 1ull << 48) {
     Report("FATAL: HWAddressSanitizer: library loaded above address 2^48\n");
     Die();
   }
 }
 
-static void InitGlobalsFromPhdrs(ElfW(Addr) base, const ElfW(Phdr) * phdr,
-                                 ElfW(Half) phnum) {
+bool IterateHwasanGlobalsInObject(ElfW(Addr) base, const ElfW(Phdr) * phdr,
+                                  ElfW(Half) phnum, hwasan_globals_cb cb,
+                                  void *data) {
+  // Read the phdrs from this DSO.
   for (unsigned i = 0; i != phnum; ++i) {
     if (phdr[i].p_type != PT_NOTE)
       continue;
+
     const char *note = reinterpret_cast<const char *>(base + phdr[i].p_vaddr);
     const char *nend = note + phdr[i].p_memsz;
+
+    // Traverse all the notes until we find a HWASan note.
     while (note < nend) {
       auto *nhdr = reinterpret_cast<const ElfW(Nhdr) *>(note);
       const char *name = note + sizeof(ElfW(Nhdr));
       const char *desc = name + RoundUpTo(nhdr->n_namesz, 4);
+
+      // Discard non-HWASan-Globals notes.
       if (nhdr->n_type != NT_LLVM_HWASAN_GLOBALS ||
           internal_strcmp(name, "LLVM") != 0) {
         note = desc + RoundUpTo(nhdr->n_descsz, 4);
         continue;
       }
 
       // Only libraries with instrumented globals need to be checked against the
       // code model since they use relocations that aren't checked at link time.
       CheckCodeModel(base, phdr, phnum);
 
       auto *global_note = reinterpret_cast<const hwasan_global_note *>(desc);
-      auto *global_begin = reinterpret_cast<const hwasan_global *>(
+      auto *globals_begin = reinterpret_cast<const hwasan_global *>(
           note + global_note->begin_relptr);
-      auto *global_end = reinterpret_cast<const hwasan_global *>(
+      auto *globals_end = reinterpret_cast<const hwasan_global *>(
           note + global_note->end_relptr);
-      InitGlobals(global_begin, global_end);
-      return;
+
+      for (auto *global = globals_begin; global != globals_end; ++global) {
+        uptr addr = reinterpret_cast<uptr>(global) + global->gv_relptr;
+        uptr size = global->info & 0xffffff;
+        u8 tag = global->info >> 24;
+        if (cb(addr, size, tag, data))
+          return true;
       }
+      break;
     }
   }
 
-static void InitLoadedGlobals() {
-  dl_iterate_phdr(
-      [](dl_phdr_info *info, size_t size, void *data) {
-        InitGlobalsFromPhdrs(info->dlpi_addr, info->dlpi_phdr,
-                             info->dlpi_phnum);
-        return 0;
+  return false;
+}
+
+bool IterateHwasanGlobals(hwasan_globals_cb cb, void *data) {
+  struct HwasanGlobalsCbArgumentWrapper {
+    hwasan_globals_cb cb;
+    void *data;
+  };
+  HwasanGlobalsCbArgumentWrapper args{.cb = cb, .data = data};
+  return dl_iterate_phdr(
+             [](dl_phdr_info *info, size_t /* size */, void *data) -> int {
+               auto *args =
+                   reinterpret_cast<HwasanGlobalsCbArgumentWrapper *>(data);
+               return IterateHwasanGlobalsInObject(
+                   info->dlpi_addr, info->dlpi_phdr, info->dlpi_phnum, args->cb,
+                   args->data);
              },
-      nullptr);
+             &args) == 0;
+}
+
+static bool InitializeSingleGlobal(uptr addr, uptr size, u8 tag,
+                                   void * /* data */) {
+  uptr full_granule_size = RoundDownTo(size, 16);
+  TagMemoryAligned(addr, full_granule_size, tag);
+  if (size % 16)
+    TagMemoryAligned(addr + full_granule_size, 16, size % 16);
+  return false;
+}
+
+static void InitLoadedGlobals() {
+  IterateHwasanGlobals(InitializeSingleGlobal, nullptr);
 }
 
 // Prepare to run instrumented code on the main thread.
 static void InitInstrumentation() {
   if (hwasan_instrumentation_inited) return;
 
   InitPrctl();
 
   if (!InitShadow()) {
void __hwasan_init_static() {
   InitShadowGOT();
   InitInstrumentation();
 
   // In the non-static code path we call dl_iterate_phdr here. But at this point
   // libc might not have been initialized enough for dl_iterate_phdr to work.
   // Fortunately, since this is a statically linked executable we can use the
   // linker-defined symbol __ehdr_start to find the only relevant set of phdrs.
   extern ElfW(Ehdr) __ehdr_start;
-  InitGlobalsFromPhdrs(
-      0,
+  IterateHwasanGlobalsInObject(
+      /* base */ 0,
       reinterpret_cast<const ElfW(Phdr) *>(
           reinterpret_cast<const char *>(&__ehdr_start) + __ehdr_start.e_phoff),
-      __ehdr_start.e_phnum);
+      __ehdr_start.e_phnum, InitializeSingleGlobal, /* data */ nullptr);
 }
 
 void __hwasan_init() {
   CHECK(!hwasan_init_is_running);
   if (hwasan_inited) return;
   hwasan_init_is_running = 1;
   SanitizerToolName = "HWAddressSanitizer";
 
   InitTlsSize();
 
   CacheBinaryName();
   InitializeFlags();
 
#endif
   VPrintf(1, "HWAddressSanitizer init done\n");
 
   hwasan_init_is_running = 0;
   hwasan_inited = 1;
 }
 
 void __hwasan_library_loaded(ElfW(Addr) base, const ElfW(Phdr) * phdr,
                              ElfW(Half) phnum) {
-  InitGlobalsFromPhdrs(base, phdr, phnum);
+  IterateHwasanGlobalsInObject(base, phdr, phnum, InitializeSingleGlobal,
+                               /* data */ nullptr);
 }
 
 void __hwasan_library_unloaded(ElfW(Addr) base, const ElfW(Phdr) * phdr,
                                ElfW(Half) phnum) {
   for (; phnum != 0; ++phdr, --phnum)
     if (phdr->p_type == PT_LOAD)
       TagMemory(base + phdr->p_vaddr, phdr->p_memsz, 0);
 }

compiler-rt/lib/hwasan/hwasan_report.cpp

 //===-- hwasan_report.cpp -------------------------------------------------===//
 //
 // 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.

 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_mutex.h"
 #include "sanitizer_common/sanitizer_report_decorator.h"
 #include "sanitizer_common/sanitizer_stackdepot.h"
 #include "sanitizer_common/sanitizer_stacktrace_printer.h"
 #include "sanitizer_common/sanitizer_symbolizer.h"
 
+#include <dlfcn.h>
+
 using namespace __sanitizer;
 
 namespace __hwasan {
 
 class ScopedReport {
  public:
   ScopedReport(bool fatal = false) : error_message_(1), fatal(fatal) {
     BlockingMutexLock lock(&error_message_lock_);
if (tag == *tag_ptr)
     return true;
   if (*tag_ptr == 0 || *tag_ptr > kShadowAlignment - 1)
     return false;
   uptr mem = ShadowToMem(reinterpret_cast<uptr>(tag_ptr));
   tag_t inline_tag = *reinterpret_cast<tag_t *>(mem + kShadowAlignment - 1);
   return tag == inline_tag;
 }
 
+// HWASan globals store the size of the global in the descriptor. In cases where
+// we don't have a binary with symbols, we can't grab the size of the global
+// from the debug info - but we might be able to retrieve it from the phdr.
+// Returns zero if the lookup failed.
+static uptr GetGlobalSizeFromDescriptor(uptr ptr) {
+  uptr global_size = 0u;
+
+  struct GlobalSizeWalkArgWrapper {
+    uptr ptr;
+    uptr *global_size;
+  };
+
+  GlobalSizeWalkArgWrapper args{.ptr = ptr, .global_size = &global_size};
+
+  // Find the ELF object that this global resides in.
+  Dl_info info;
+  dladdr(reinterpret_cast<void *>(ptr), &info);
+  auto *ehdr = reinterpret_cast<ElfW(Ehdr) *>(info.dli_fbase);
+  auto *phdr = reinterpret_cast<ElfW(Phdr) *>(reinterpret_cast<u8 *>(ehdr) +
+                                              ehdr->e_phoff);
+
+  // Walk all globals in this ELF object, looking for the one we're interested
+  // in. Once we find it, we can stop iterating and return the size of the
+  // global we're interested in.
+  IterateHwasanGlobalsInObject(
+      reinterpret_cast<ElfW(Addr)>(ehdr), phdr, ehdr->e_phnum,
+      [](uptr global_addr, uptr global_size, u8 /* tag */, void *data) -> bool {
+        auto *args = reinterpret_cast<GlobalSizeWalkArgWrapper *>(data);
+        if (global_addr <= args->ptr && args->ptr < global_addr + global_size) {
+          *args->global_size = global_size;
+          return true;
+        }
+        return false;
+      },
+      &args);
+
+  return global_size;
+}
+
 void PrintAddressDescription(
     uptr tagged_addr, uptr access_size,
     StackAllocationsRingBuffer *current_stack_allocations) {
   Decorator d;
   int num_descriptions_printed = 0;
   uptr untagged_addr = UntagAddr(tagged_addr);
 
   // Print some very basic information about the address, if it's a heap.
if (chunk.IsAllocated()) {
               "%p is located %zd bytes to the %s of %zd-byte global variable "
               "%s [%p,%p) in %s\n",
               untagged_addr,
               candidate == left ? untagged_addr - (info.start + info.size)
                                 : info.start - untagged_addr,
               candidate == left ? "right" : "left", info.size, info.name,
               info.start, info.start + info.size, module_name);
         } else {
-          Printf("%p is located to the %s of a global variable in (%s+0x%x)\n",
-                 untagged_addr, candidate == left ? "right" : "left",
-                 module_name, module_address);
+          uptr size = GetGlobalSizeFromDescriptor(mem);
+          if (size == 0)
+            Printf(
+                "%p is located to the %s of a global variable in (%s+0x%x)\n",
+          else
+            Printf(
+                "%p is located to the %s of a %zd-byte global variable in "
+                "(%s+0x%x)\n",
+                untagged_addr, candidate == left ? "right" : "left", size,
+                module_name, module_address);
         }
         num_descriptions_printed++;
       }
     }
   }
 
   hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
     // Scan all threads' ring buffers to find if it's a heap-use-after-free.

compiler-rt/test/hwasan/TestCases/global.c

 // RUN: %clang_hwasan %s -o %t
 // RUN: %run %t 0
 // RUN: not %run %t 1 2>&1 | FileCheck --check-prefixes=CHECK,RSYM %s
 // RUN: not %env_hwasan_opts=symbolize=0 %run %t 1 2>&1 | FileCheck --check-prefixes=CHECK,RNOSYM %s
 // RUN: not %run %t -1 2>&1 | FileCheck --check-prefixes=CHECK,LSYM %s
 // RUN: not %env_hwasan_opts=symbolize=0 %run %t -1 2>&1 | FileCheck --check-prefixes=CHECK,LNOSYM %s
 
 int x = 1;
 
 int main(int argc, char **argv) {
   // RSYM: is located 0 bytes to the right of 4-byte global variable x {{.*}} in {{.*}}global.c.tmp
-  // RNOSYM: is located to the right of a global variable in ({{.*}}global.c.tmp+{{.*}})
+  // RNOSYM: is located to the right of a 4-byte global variable in ({{.*}}global.c.tmp+{{.*}})
   // LSYM: is located 4 bytes to the left of 4-byte global variable x {{.*}} in {{.*}}global.c.tmp
-  // LNOSYM: is located to the left of a global variable in ({{.*}}global.c.tmp+{{.*}})
+  // LNOSYM: is located to the left of a 4-byte global variable in ({{.*}}global.c.tmp+{{.*}})
   // CHECK-NOT: can not describe
   (&x)[atoi(argv[1])] = 1;
 }