[libunwind] Use process_vm_readv to avoid potential segfaults

[smeenai]

Summary

We've observed segfaults in libunwind when attempting to check for the
Linux aarch64 sigreturn frame, presumably because of bad unwind info
leading to an incorrect PC that we attempt to read from. Use
process_vm_readv to read the memory safely instead.

The s390x code path should likely follow suit, but I don't have the
hardware to be able to test that, so I didn't modify it here either.

[smeenai]

Use sizeof instead of raw number

[smeenai]

XFAIL with MSAN, similar to other tests

[smeenai]

Better MSAN suppression

[MaskRay]

LGTM, but hope @rprichard can verify this.

libunwind/test/bad_unwind_info.pass.cpp

55

only supported on aarch64 and x86-64.

#error does not need a quoted argument.

70

Use __attribute__((naked)) with basic asm (https://gcc.gnu.org/onlinedocs/gcc/Basic-Asm.html), then get rid of the inline assembly language at file scope.

[MaskRay]

libunwind/test/bad_unwind_info.pass.cpp

24

move aarch64 before x86_64 for an alphabetical order.

[smeenai]

libunwind/test/bad_unwind_info.pass.cpp

70

I wanted to do this originally, but GCC doesn't support __attribute__((naked)) on aarch64: https://godbolt.org/z/15rbv6GEG. I'm not sure if we need to support gcc in the test suite, but I thought at least libc++ tried to.

[smeenai]

libunwind/test/bad_unwind_info.pass.cpp

70

That being said, it looks like GCC isn't performing any stack manipulation in this case anyway, just adding a nop and a ret, which should be harmless. Not sure if we can rely on that in general though.

[smeenai]

Address review comments

libunwind/test/bad_unwind_info.pass.cpp

70

I ended up just unsupporting the test on gcc, because __attribute__((naked)) makes it much cleaner. Thanks for the suggestion :)

[rprichard]

The fix and the test both look good to me.

[smeenai]

Rebase

[uweigand]

The s390x code path should likely follow suit, but I don't have the
hardware to be able to test that, so I didn't modify it here either.

Thanks for the heads-up! It look me a while to get around to it, but I've now fixed the bug on s390x here: https://reviews.llvm.org/D129856

[mpdenton]

The Chrome sandbox disallows process_vm_readv() (I think most seccomp sandboxes would) so this causes crashes when trying to collect backtraces at runtime.

What would happen if I caused process_vm_read() to return EPERM here? Would the unwinder still be able to unwind past the sigreturn trampoline with heuristics?

If not, is it possible to introduce a fallback that reads from the address directly, or possibly uses mincore() to check if the address is valid (a small race is possible)?

[smeenai]

In D126343#4450731, @mpdenton wrote:

The Chrome sandbox disallows process_vm_readv() (I think most seccomp sandboxes would) so this causes crashes when trying to collect backtraces at runtime.

What would happen if I caused process_vm_read() to return EPERM here? Would the unwinder still be able to unwind past the sigreturn trampoline with heuristics?

If not, is it possible to introduce a fallback that reads from the address directly, or possibly uses mincore() to check if the address is valid (a small race is possible)?

With the code as currently written, I believe we'd just fail to unwind past the sigreturn frame if process_vm_readv failed.

How would mincore work in this scenario? It tells you if a page is resident, but a non-resident page could still be accessible, right? On the flip side, ENOMEM would tell you if the page was unmapped, but an execute-only page would be mapped but still unreadable.

[mpdenton]

In D126343#4450750, @smeenai wrote:

With the code as currently written, I believe we'd just fail to unwind past the sigreturn frame if process_vm_readv failed.

Ah, too bad.

How would mincore work in this scenario? It tells you if a page is resident, but a non-resident page could still be accessible, right? On the flip side, ENOMEM would tell you if the page was unmapped, but an execute-only page would be mapped but still unreadable.

I didn't realize ARM supported execute-only pages. I was thinking that if mincore returned EFAULT this would work to detect if an address was readable. It looks like a "write-to-pipe" works for checking if an address is readable: https://github.com/libunwind/libunwind/blob/0e9119698dfad47f8afb88c2a7ee3d36f9efd568/src/mi/Gaddress_validator.c#L97

libunwind/src/UnwindCursor.hpp

 #ifdef _AIX
 #include <dlfcn.h>
 #include <sys/debug.h>
 #include <sys/pseg.h>
 #endif
 
 #if defined(_LIBUNWIND_TARGET_LINUX) &&                                        \
     (defined(_LIBUNWIND_TARGET_AARCH64) || defined(_LIBUNWIND_TARGET_S390X))
+#include <sys/syscall.h>
+#include <sys/uio.h>
+#include <unistd.h>
 #define _LIBUNWIND_CHECK_LINUX_SIGRETURN
 #endif
 
 #if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND)
 // Provide a definition for the DISPATCHER_CONTEXT struct for old (Win7 and
 // earlier) SDKs.
 // MinGW-w64 has always provided this struct.
   #if defined(_WIN32) && defined(_LIBUNWIND_TARGET_X86_64) && \
bool UnwindCursor<A, R>::setInfoForSigReturn(Registers_arm64 &) {
   // constant for the PC needs to be defined before DWARF can handle a signal
   // trampoline. This code may segfault if the target PC is unreadable, e.g.:
   //  - The PC points at a function compiled without unwind info, and which is
   //    part of an execute-only mapping (e.g. using -Wl,--execute-only).
   //  - The PC is invalid and happens to point to unreadable or unmapped memory.
   //
   // [1] https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/vdso/sigreturn.S
   const pint_t pc = static_cast<pint_t>(this->getReg(UNW_REG_IP));
+  // The PC might contain an invalid address if the unwind info is bad, so
+  // directly accessing it could cause a segfault. Use process_vm_readv to read
+  // the memory safely instead. process_vm_readv was added in Linux 3.2, and
+  // AArch64 supported was added in Linux 3.7, so the syscall is guaranteed to
+  // be present. Unfortunately, there are Linux AArch64 environments where the
+  // libc wrapper for the syscall might not be present (e.g. Android 5), so call
+  // the syscall directly instead.
+  uint32_t instructions[2];
+  struct iovec local_iov = {&instructions, sizeof instructions};
+  struct iovec remote_iov = {reinterpret_cast<void *>(pc), sizeof instructions};
+  long bytesRead =
+      syscall(SYS_process_vm_readv, getpid(), &local_iov, 1, &remote_iov, 1, 0);
   // Look for instructions: mov x8, #0x8b; svc #0x0
-  if (_addressSpace.get32(pc) == 0xd2801168 &&
-      _addressSpace.get32(pc + 4) == 0xd4000001) {
-    _info = {};
-    _info.start_ip = pc;
-    _info.end_ip = pc + 4;
-    _isSigReturn = true;
-    return true;
-  }
-  return false;
-}
+  if (bytesRead != sizeof instructions || instructions[0] != 0xd2801168 ||
+      instructions[1] != 0xd4000001)
+    return false;
+
 
 template <typename A, typename R>
 int UnwindCursor<A, R>::stepThroughSigReturn(Registers_arm64 &) {
   // In the signal trampoline frame, sp points to an rt_sigframe[1], which is:
   //  - 128-byte siginfo struct
   //  - ucontext struct:
   //     - 8-byte long (uc_flags)
   //     - 8-byte pointer (uc_link)

libunwind/test/bad_unwind_info.pass.cpp

+// -*- 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
+//
+//===----------------------------------------------------------------------===//
+
+// Ensure that libunwind doesn't crash on invalid info; the Linux aarch64
+// sigreturn frame check would previously attempt to access invalid memory in
+// this scenario.
+// REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}})
+
+// TODO: Figure out why this fails with Memory Sanitizer.
+// XFAIL: msan
+
+#undef NDEBUG
+#include <assert.h>
+#include <libunwind.h>
+#include <stdio.h>
+
+#if defined(__x86_64__)
+__asm__(".globl bad_unwind_info\n"
+        "bad_unwind_info:\n"
+        "        .cfi_startproc\n"
+        "        pushq   %rbx\n"
+        "        .cfi_def_cfa_offset 16\n"
+        "        movq    8(%rsp), %rbx\n"
+        "        # purposely corrupt return value on stack\n"
+        "        movq    $4, 8(%rsp)\n"
+        "        callq   stepper\n"
+        "        movq    %rbx, 8(%rsp)\n"
+        "        popq    %rbx\n"
+        "        .cfi_def_cfa_offset 8\n"
+        "        ret\n"
+        "        .cfi_endproc\n");
+#elif defined(__aarch64__)
+__asm__(".globl bad_unwind_info\n"
+        "bad_unwind_info:\n"
+        "        .cfi_startproc\n"
+        "        // not using 0 because unwinder was already resilient to it\n"
+        "        mov     x8, #4\n"
+        "        stp     x30, x8, [sp, #-16]!\n"
+        "        .cfi_def_cfa_offset 16\n"
+        "        // purposely use incorrect offset for x30\n"
+        "        .cfi_offset x30, -8\n"
+        "        bl      stepper\n"
+        "        ldr     x30, [sp], #16\n"
+        "        .cfi_def_cfa_offset 0\n"
+        "        .cfi_restore x30\n"
+        "        ret\n"
+        "        .cfi_endproc\n");
+#else
+#error "This test is only supported for aarch64 or x86-64"
+#endif
+
+extern "C" {
+void stepper() {
+  unw_cursor_t cursor;
+  unw_context_t uc;
+  unw_getcontext(&uc);
+  unw_init_local(&cursor, &uc);
+  // stepping to bad_unwind_info should succeed
+  assert(unw_step(&cursor) > 0);
+  // stepping past bad_unwind_info should fail but not crash
+  assert(unw_step(&cursor) <= 0);
+}
+
+void bad_unwind_info();
+}
+
+int main() { bad_unwind_info(); }