[hwasan] Fixup mmap tagging regions

[vitalybuka]

update

[thurston]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

233–236

if (IsAligned(beg, GetPageSize()))
  __hwasan::TagMemoryAligned(beg, RoundUpTo(length, GetPageSize()), 0);
int res = real_munmap(addr, length);
CHECK(IsAligned(beg, GetPageSize()) || (res != 0));

Why not CHECK(IsAligned) at the beginning, to avoid duplicating the check?

CHECK(IsAligned(beg, GetPageSize());
__hwasan::TagMemoryAligned(beg, RoundUpTo(length, GetPageSize()), 0);
int res = real_munmap(addr, length);
CHECK(res != 0);

[thurston]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

233–236

Ignore my comment, just noticed it was 'CHECK(IsAligned(beg, GetPageSize()) || (res != 0));' not '&&'

[kstoimenov]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

233

So if the pointer is not aligned we will not untag? Could you pease explain the logic?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

233

So if the pointer is not aligned we will not untag? Could you pease explain the logic?

This part is confusing.

So if pointer misaligned, real_munmap should fail. So if we untag the shadow, memory will still be there.
Ideally for this issue it would be nice to tag after real_munmap, but it will cause a data race with other threads.

Maybe it's fine just round the region and and untag always.
WDYT?

[kstoimenov]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

233

We should verify that real_munmap fails when the pointer is miss

233

I've read the description of munmap and I think you you did makes sense. You should probably also add a check for valid memory similar to mmap. So the logic will be:

bool untagged = false;
if (Aligned(...) && MemIsApp(...) && length > 0) {
  Untag();
  untagged = true;
}

res = munmap(...)

// This might trigger under legitimate situation. For example the 
// address is owned by the app, aligned and length > 0, but it points 
// to memory which is not mapped. Should we fail at this point? 
if (untagged) CHECK(res == 0);

[vitalybuka]

update

[vitalybuka]

check length

[vitalybuka]

format

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

static void *mmap_interceptor(Mmap real_mmap, void *addr, SIZE_T length,
                               int prot, int flags, int fd, OFF64_T offset) {
   if (addr) {
     if (flags & map_fixed) CHECK_EQ(addr, UntagPtr(addr));
 
     addr = UntagPtr(addr);
   }
   SIZE_T rounded_length = RoundUpTo(length, GetPageSize());
   void *end_addr = (char *)addr + (rounded_length - 1);
-  if (addr && (!MemIsApp(reinterpret_cast<uptr>(addr)) ||
-               !MemIsApp(reinterpret_cast<uptr>(end_addr)))) {
+  if (addr && length &&
+      (!MemIsApp(reinterpret_cast<uptr>(addr)) ||
     // User requested an address that is incompatible with HWASan's
     // memory layout. Use a different address if allowed, else fail.
     if (flags & map_fixed) {
       errno = errno_EINVAL;
       return (void *)-1;
     } else {
       addr = nullptr;
     }
   }
   void *res = real_mmap(addr, length, prot, flags, fd, offset);
-  if (res != (void *)-1) {
-    void *end_res = (char *)res + (rounded_length - 1);
-    if (!MemIsApp(reinterpret_cast<uptr>(res)) ||
-        !MemIsApp(reinterpret_cast<uptr>(end_res))) {
+  if (length && res != (void *)-1) {
+    uptr beg = reinterpret_cast<uptr>(res);
+    DCHECK(IsAligned(beg, GetPageSize()));
+    if (!MemIsApp(beg) || !MemIsApp(beg + rounded_length - 1)) {
       // Application has attempted to map more memory than is supported by
       // HWASan. Act as if we ran out of memory.
       internal_munmap(res, length);
       errno = errno_ENOMEM;
       return (void *)-1;
     }
-    __hwasan::TagMemory(reinterpret_cast<uptr>(addr), length, 0);
+    __hwasan::TagMemoryAligned(beg, rounded_length, 0);
   }
 
   return res;
 }
 
 template <class Munmap>
 static int munmap_interceptor(Munmap real_munmap, void *addr, SIZE_T length) {
-  __hwasan::TagMemory(reinterpret_cast<uptr>(addr), length, 0);
+  // We should not tag if munmap fail, but it's to late to tag after
+  // real_munmap, as the pages could be mmaped by another thread.
+  uptr beg = reinterpret_cast<uptr>(addr);
+  if (length && IsAligned(beg, GetPageSize())) {
+    SIZE_T rounded_length = RoundUpTo(length, GetPageSize());
+    // Protect from unmapping the shadow.
+    if (!MemIsApp(beg) || !MemIsApp(beg + rounded_length - 1)) {
+      errno = errno_EINVAL;
+      return -1;
+    }
+    __hwasan::TagMemoryAligned(beg, rounded_length, 0);
+  }
   return real_munmap(addr, length);
 }
 
 #    define COMMON_INTERCEPTOR_MMAP_IMPL(ctx, mmap, addr, length, prot, flags, \
                                          fd, offset)                           \
       do {                                                                     \
         (void)(ctx);                                                           \
         return mmap_interceptor(REAL(mmap), addr, sz, prot, flags, fd, off);   \
       } while (false)
 
 #    define COMMON_INTERCEPTOR_MUNMAP_IMPL(ctx, munmap, addr, length)          \
       do {                                                                     \
         (void)(ctx);                                                           \
         return munmap_interceptor(REAL(munmap), addr, sz);                     \
       } while (false)

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

 // RUN: %clang_hwasan  %s -o %t
-// RUN: %run %t 2>&1
+// RUN: %run %t 1 2>&1
+// RUN: %run %t 2 2>&1
 
 // REQUIRES: pointer-tagging
 
 #include <sanitizer/hwasan_interface.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/mman.h>
+#include <unistd.h>
 
 int main(int argc, char **argv) {
-  const int kSize = 4096;
+  const size_t kPS = sysconf(_SC_PAGESIZE);
+  const int kSize = kPS / atoi(argv[1]);
   const int kTag = 47;
 
   // Get any mmaped pointer.
   void *r =
       mmap(0, kSize, PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
   if (r == MAP_FAILED) {
     perror("Failed to mmap: ");
     abort();
   }
 
   // Check that the pointer is untagged.
   if (r != __hwasan_tag_pointer(r, 0)) {
     fprintf(stderr, "Pointer returned by mmap should be untagged.\n");
     abort();
   }
 
   // Manually tag the pointer and the memory.
-  __hwasan_tag_memory(r, kTag, kSize);
+  __hwasan_tag_memory(r, kTag, kPS);
   int *p1 = __hwasan_tag_pointer(r, kTag);
 
   // Check that the pointer and the tag match.
-  if (__hwasan_test_shadow(p1, kSize) != -1) {
+  if (__hwasan_test_shadow(p1, kPS) != -1) {
     fprintf(stderr, "Failed to tag.\n");
     abort();
   }
 
+  if (munmap((char *)r + 1, kSize) == 0) {
+    perror("munmap should fail: ");
+    abort();
+  }
+
+  if (__hwasan_test_shadow(p1, kPS) != -1) {
+    fprintf(stderr, "Still must be tagged.\n");
+    abort();
+  }
+
   // First munmmap and then mmap the same pointer using MAP_FIXED.
   if (munmap((char *)r, kSize) != 0) {
     perror("Failed to unmap: ");
     abort();
   }
 
-  if (__hwasan_test_shadow(r, kSize) != -1) {
+  if (__hwasan_test_shadow(r, kPS) != -1) {
     fprintf(stderr, "Shadow memory was not cleaned by munmap.\n");
     abort();
   }
-  __hwasan_tag_memory(r, kTag, kSize);
+  __hwasan_tag_memory(r, kTag, kPS);
   int *p2 = (int *)mmap(r, kSize, PROT_READ | PROT_WRITE,
                         MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
 
   // Check that the pointer has no tag in it.
   if (p2 != r) {
     fprintf(stderr, "The mmap pointer has a non-zero tag in it.\n");
     abort();
   }
 
   // Make sure we can access the shadow with an untagged pointer.
-  if (__hwasan_test_shadow(p2, kSize) != -1) {
+  if (__hwasan_test_shadow(p2, kPS) != -1) {
     fprintf(stderr, "Shadow memory was not cleaned by mmap.\n");
     abort();
   }
   return 0;
 }