Page MenuHomePhabricator

[libunwind] Unwind through aarch64/Linux sigreturn frame
ClosedPublic

Authored by rprichard on Nov 5 2020, 8:27 PM.

Details

Summary

An AArch64 sigreturn trampoline frame can't currently be described
in a DWARF .eh_frame section, because the AArch64 DWARF spec currently
doesn't define a constant for the PC register. (PC and LR may need to
be restored to different values.)

Instead, use the same technique as libgcc or github.com/libunwind and
detect the sigreturn frame by looking for the sigreturn instructions:

mov x8, #0x8b
svc #0x0

If a sigreturn frame is detected, libunwind restores all the GPRs by
assuming that sp points at an rt_sigframe Linux kernel struct. This
behavior is a fallback mode that is only used if there is no ordinary
unwind info for sigreturn.

If libunwind can't find unwind info for a PC, it assumes that the PC is
readable, and would crash if it isn't. This could happen if:

  • 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.

In the tests, ignore a failed dladdr call so that the tests can run on
user-mode qemu for AArch64, which uses a stack-allocated trampoline
instead of a vDSO.

Diff Detail

Event Timeline

rprichard created this revision.Nov 5 2020, 8:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 5 2020, 8:27 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard requested review of this revision.Nov 5 2020, 8:27 PM

Could you add a bit of a comment explaining why the XOM case is not protected against? libunwind can be used for generating backtraces during regular execution (e.g. profiling) and crashing if the XOM is in use seems unfortunate. We could check the memory mapping and just give up in the XOM case.

libunwind/include/__libunwind_config.h
31

What do you think of having an else clause which sets this to 0 to allow using the macro as a condition rather than checking the definedness of it?

libunwind/src/UnwindCursor.hpp
940

What do you think of Registers instead of R2?

2020

It really would be amazing to have a link to the VDSO code here to make it easier to understand the check - I figured it was that this is the implementation, but had to chase through the files.

danielkiss added inline comments.Nov 9 2020, 7:26 AM
libunwind/src/UnwindCursor.hpp
2020

VDSO is an elf, so we could do something like this:

void* handle = dlopen("linux-vdso.so.1", RTLD_LAZY);
const pint_t ptr = (const pint_t)dlsym(handle, "__kernel_rt_sigreturn");
if( pc == ptr ) ...

This could help with the XOM issue too.

Could you add a bit of a comment explaining why the XOM case is not protected against?

I think I can expand on the comment.

libunwind can be used for generating backtraces during regular execution (e.g. profiling) and crashing if the XOM is in use seems unfortunate.

For profiling, I assume the unwind info needs to be accurate at every instruction? I know that's definitely not the case for AArch64, but I thought it's still only mostly-accurate on other archs? I don't *really* know that unwinding through a signal handler is important for Android, but I don't know how to rule it out either, and we do have a test for it. I think the test predated Android's libunwindstack unwinder. Maybe someone could be using _Unwind_Backtrace in a segfault handler, and that probably avoids the async-unwind-info problem most of the time. But I also don't think that's how third-party Android crash dumpers work.

It would only crash on an XOM function if that function also doesn't have unwind info.

We could check the memory mapping and just give up in the XOM case.

That could work. Some similar approaches:

  • process_vm_readv
  • pipe+write
  • open /proc/self/mem

They all have the drawback that /proc might be unavailable or that SELinux/seccomp/etc might restrict some system calls.

libunwind/include/__libunwind_config.h
31

D31078 was a previous attempt to establish a different macro convention for booleans (each macro is either undefined or defined to empty), but it seems to have left a lot of the macros defined to 1 anyway. e.g. _LIBUNWIND_TARGET_I386 was changed from 1 to empty, but none of the other target macros were affected.

I don't currently have a opinion on what the convention should be, so I was trying to match what I thought the convention was (boolean macros are undefined or defined to 1, then checked with defined(...)).

libunwind/src/UnwindCursor.hpp
940

Both R and Registers would be in scope and would be the names of Registers_* classes, but that seems OK to me.

2020

I think the vDSO is the most canonical implementation -- Bionic uses it, and it looks like glibc also does. It's not universal, though. e.g.:

FWIW, Bionic provides its own restorer function on arm/x86/x86_64 and only uses the kernel's restorer for arm64. This patch is sufficient to fix unwinding through a signal handler for Bionic, but not other targets. e.g. The situations above, but I think x86(-32) with glibc is also affected if there is no vDSO.

I think I can add a link to the vDSO, probably to:
https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/vdso/sigreturn.S

2020

It looks like dlopen("linux-vdso.so.1", ...) does find the vDSO with Bionic and glibc, but not with musl. (I wondered about Bionic, because Bionic's dl_iterate_phdr reports the name of the vDSO as [vdso] rather than linux-vdso.so.1. However, when I tested, it looks like Bionic's dlopen can open linux-vdso.so but not [vdso].)

This approach might have trouble with static linking? For statically-linked Bionic programs, a special limited dl_iterate_phdr is provided in libc.a, but there is no dlopen linked in by default. There is a stub dlopen in libdl.a, but the stub just returns nullptr. For glibc, dlopen also requires linking with -ldl, and then ld.bfd/ld.gold apparently print a warning for static executables:

/tmp/test-f61321.o:test.c:function main: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

One approach I thought of: change findUnwindSections so that it reports a readable range of memory containing the lookup PC. I think this would break with qemu user-mode emulation but would work in most other configurations. (e.g. A static musl executable doesn't look like it shows the vDSO via dl_iterate_phdr, but that's OK if musl provides its own sigreturn function.)

rprichard updated this revision to Diff 309148.Dec 2 2020, 9:27 PM

Update comments.

That could work. Some similar approaches:

  • process_vm_readv
  • pipe+write
  • open /proc/self/mem

FWIW, Bionic doesn't expose the process_vm_readv syscall wrapper function until API 23 (M, Android 6.0). The first version of Android to support AArch64 is API 21 (L, Android 5.0). Maybe libunwind could call syscall instead.

Also, with respect to execute-only memory (XOM), there are a couple of mitigating factors:

  • As long as the function has unwind info, libunwind won't try to read the memory at the PC.
  • Execute-only memory broke another security mitigation, PAN (see here and here), so it only shipped in Android 10. It was removed from Android 11 and from the upstream Linux kernel. Even on Android 10, I think it was only supported on sufficiently-new devices (e.g. not the Pixel 2), and at least for the Pixel 3, the kernel doesn't seem to enforce XOM anymore as of Android 10 QPR3. e.g. I still see --x pages, but it's now possible to read them w/o segfaulting. The first page I linked mentioned backporting a 4.9 kernel patch disabling XOM. Maybe the feature would be resurrected eventually, though.
danielkiss accepted this revision.Dec 3 2020, 9:29 AM

unwind_leaffunction.pass.cpp could be enable too for aarch64.

LGTM.

rprichard updated this revision to Diff 316319.Tue, Jan 12, 9:44 PM

Enable unwind_leaffunction.pass.cpp for AArch64 Linux.

rprichard edited the summary of this revision. (Show Details)Tue, Jan 12, 9:45 PM

Can I get a code-owner to review this?

compnerd accepted this revision.Wed, Jan 13, 1:45 PM

Could you add a bit of a comment explaining why the XOM case is not protected against?

I think I can expand on the comment.

libunwind can be used for generating backtraces during regular execution (e.g. profiling) and crashing if the XOM is in use seems unfortunate.

For profiling, I assume the unwind info needs to be accurate at every instruction? I know that's definitely not the case for AArch64, but I thought it's still only mostly-accurate on other archs? I don't *really* know that unwinding through a signal handler is important for Android, but I don't know how to rule it out either, and we do have a test for it. I think the test predated Android's libunwindstack unwinder. Maybe someone could be using _Unwind_Backtrace in a segfault handler, and that probably avoids the async-unwind-info problem most of the time. But I also don't think that's how third-party Android crash dumpers work.

Sure, though, libunwind is generic and is used outside of android as well, including other AArch64 targets, so we do need to consider those use cases while making changes.

This revision is now accepted and ready to land.Wed, Jan 13, 1:45 PM
This revision was landed with ongoing or failed builds.Wed, Jan 13, 4:39 PM
This revision was automatically updated to reflect the committed changes.

Could you add a bit of a comment explaining why the XOM case is not protected against?

I think I can expand on the comment.

libunwind can be used for generating backtraces during regular execution (e.g. profiling) and crashing if the XOM is in use seems unfortunate.

For profiling, I assume the unwind info needs to be accurate at every instruction? I know that's definitely not the case for AArch64, but I thought it's still only mostly-accurate on other archs? I don't *really* know that unwinding through a signal handler is important for Android, but I don't know how to rule it out either, and we do have a test for it. I think the test predated Android's libunwindstack unwinder. Maybe someone could be using _Unwind_Backtrace in a segfault handler, and that probably avoids the async-unwind-info problem most of the time. But I also don't think that's how third-party Android crash dumpers work.

Sure, though, libunwind is generic and is used outside of android as well, including other AArch64 targets, so we do need to consider those use cases while making changes.

Makes sense. Thanks for the review!