[libcxx-commits] [PATCH] D123428: [libunwind] Add configuration to disable sigreturn frame check

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai abandoned this revision.
smeenai added a comment.

In D123428#3487134 <https://reviews.llvm.org/D123428#3487134>, @rprichard wrote:

> In D123428#3476371 <https://reviews.llvm.org/D123428#3476371>, @rprichard wrote:
>
>> In D123428#3475346 <https://reviews.llvm.org/D123428#3475346>, @smeenai wrote:
>>
>>> In D123428#3444212 <https://reviews.llvm.org/D123428#3444212>, @MaskRay wrote:
>>>
>>>> https://github.com/abseil/abseil-cpp/blob/master/absl/debugging/internal/address_is_readable.cc has gone through several iterations. We can use `rt_sigprocmask`.
>>>
>>> @rprichard, do you know if this would work for Android? It has the TOCTOU issue, but I imagine it's much simpler than having to manage and synchronize the pipe fd, and we could live with the TOCTOU in practice.
>>
>> I'm glad @MaskRay found this -- I think it will probably work, and it seems better than assuming the PC is readable.
>>
>> I see rt_procsigmask listed in bionic/libc/SYSCALLS.TXT, and I don't see it mentioned in any of the bionic/libc/SECCOMP*.txt files. I think that means seccomp is allowing the system call. I looked at kernel/signal.c, and AFAICT it's not doing any security checks that could be a problem. Bionic itself uses `rt_sigprocmask` for (at least) spawning new processes, creating/exiting threads, TLS-related locking, POSIX timers, and abort(). I think any seccomp-like blocking of rt_sigprocmask would have to be very targeted, so I think the syscall is probably allowed everywhere on Android.
>>
>> It is assuming that the kernel will validate the address before validating the `how`. The kernel has a principle of not breaking userland -- is there a more specific guarantee we can rely on? e.g. The code has this comment:
>>
>>   // This strategy depends on Linux implementation details,
>>   // so we rely on the test to alert us if it stops working.
>>
>> The kernel source is structured as two wrappers around sigprocmask, `SYSCALL_DEFINE4(rt_sigprocmask, ...)` and `COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, ...)`. The wrappers copy user memory to/from kernel memory before calling `sigprocmask`, so it makes sense that they would validate the address first.
>
> I talked with cferris today (the maintainer of Android's libunwindstack), and I think LLVM's libunwind could just use `process_vm_readv`, which is more straightforward and would also avoid the TOCTOU issue.
>
> `process_vm_readv` is used by libunwindstack, and cferris wasn't aware of any situation where `process_vm_readv` was blocked.
>
> `process_vm_readv` is presumably slower than `rt_procsigmask`, but that's probably acceptable? It's only used when the unwinder can't find DWARF unwind info for a PC.
>
> `process_vm_readv` requires a 3.2 kernel or newer, but this is syscall only needed for arm64, and arm64 support wasn't added until a later kernel version (3.7, it appears? <https://lwn.net/Articles/506148/>).
>
> I think the kernel special-cases `process_vm_readv` so it doesn't need ptrace permissions when reading the calling process's address space. When `mm == current->mm`, the kernel skips the `ptrace_may_access` call:
>
> - https://github.com/torvalds/linux/blob/v5.17/mm/process_vm_access.c#L202
> - https://github.com/torvalds/linux/blob/v5.17/kernel/fork.c#L1324

Thanks for all the research and info here! I'm abandoning this since the `process_vm_readv` route seems like a much better path. (I likely won't have time to take a swing at that for a couple of weeks, but I'm also happy for anyone else to try it if they have the time, of course.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123428/new/

https://reviews.llvm.org/D123428