From 5c12d5f9ab77e526f852cbca82f454a42e3a6e30 Mon Sep 17 00:00:00 2001 From: Kyle Huey Date: Tue, 25 Aug 2020 10:57:12 -0700 Subject: [PATCH] Set GPRs through PTRACE_SETREGSET. When the GPRs are manipulated through PTRACE_GETREGS/PTRACE_SETREGS, whether the register set is the 32 bit user_regs_struct or the 64 bit user_regs_struct depends on whether rr is a 32 bit or 64 bit program. But when they're manipulated through PTRACE_GETREGSET/PTRACE_SETREGSET with the NT_PRSTATUS regset, it depends on whether the *tracee* is a 32 bit or 64 bit program. Starting with kernel 5.9, if a 64 bit rr PTRACE_SETREGS a 32 bit tracee the fs/gsbase values in that user_regs_struct are used. This is a problem because we don't track them and thus they're always zero, regardless of what the correct value is. See LKML "x86/cpu fsgsbase breaks TLS in 32 bit rr tracees on a 64 bit system" for more discussion. If we use PTRACE_SETREGSET we can pass in the 32 bit version of user_regs_struct and the kernel will figure out the correct fs/gsbase as before. It's still more convenient to use PTRACE_GETREGS rather than PTRACE_GETREGSET so we can be sure where the CS register is in memory for testing whether we have a 64/32 bit process, so that and the conversion code stick around. Fixes #2642 (again). --- src/Registers.cc | 11 +++++++++++ src/Registers.h | 3 +++ src/Task.cc | 12 ++++++------ src/kernel_abi.cc | 16 ++++++++++++++++ src/kernel_abi.h | 3 +++ src/record_syscall.cc | 8 ++++---- 6 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/Registers.cc b/src/Registers.cc index 790475a75e8..ee2a380fcd4 100644 --- a/src/Registers.cc +++ b/src/Registers.cc @@ -577,6 +577,17 @@ NativeArch::user_regs_struct Registers::get_ptrace() const { return result.linux_api; } +iovec Registers::get_ptrace_iovec() { + if (arch() == NativeArch::arch()) { + iovec iov = { &u, sizeof(NativeArch::user_regs_struct) }; + return iov; + } + + DEBUG_ASSERT(arch() == x86 && NativeArch::arch() == x86_64); + iovec iov = { &u.x86regs, sizeof(u.x86regs) }; + return iov; +} + Registers::InternalData Registers::get_ptrace_for_self_arch() const { switch (arch_) { case x86: diff --git a/src/Registers.h b/src/Registers.h index 682ad5c64a4..da6f7e11714 100644 --- a/src/Registers.h +++ b/src/Registers.h @@ -16,6 +16,8 @@ #include "remote_code_ptr.h" #include "remote_ptr.h" +struct iovec; + namespace rr { class ReplayTask; @@ -86,6 +88,7 @@ class Registers { * to the rr build (e.g. ARM vs x86). */ NativeArch::user_regs_struct get_ptrace() const; + iovec get_ptrace_iovec(); /** * Get a user_regs_struct for a particular Arch from these Registers. diff --git a/src/Task.cc b/src/Task.cc index cefa59c06db..79ebad95aef 100644 --- a/src/Task.cc +++ b/src/Task.cc @@ -673,17 +673,17 @@ void Task::on_syscall_exit_arch(int syscallno, const Registers& regs) { switch ((int)regs.arg3()) { case NT_PRSTATUS: { auto set = ptrace_get_regs_set( - this, regs, sizeof(typename Arch::user_regs_struct)); + this, regs, user_regs_struct_size(tracee->arch())); Registers r = tracee->regs(); - r.set_from_ptrace_for_arch(Arch::arch(), set.data(), set.size()); + r.set_from_ptrace_for_arch(tracee->arch(), set.data(), set.size()); tracee->set_regs(r); break; } case NT_FPREGSET: { auto set = ptrace_get_regs_set( - this, regs, sizeof(typename Arch::user_fpregs_struct)); + this, regs, user_fpregs_struct_size(tracee->arch())); ExtraRegisters r = tracee->extra_regs(); - r.set_user_fpregs_struct(this, Arch::arch(), set.data(), + r.set_user_fpregs_struct(this, tracee->arch(), set.data(), set.size()); tracee->set_extra_regs(r); break; @@ -1442,9 +1442,9 @@ void Task::set_regs(const Registers& regs) { void Task::flush_regs() { if (registers_dirty) { LOG(debug) << "Flushing registers for tid " << tid << " " << registers; - auto ptrace_regs = registers.get_ptrace(); + auto ptrace_regs = registers.get_ptrace_iovec(); #if defined(__i386__) || defined(__x86_64__) - if (ptrace_if_alive(PTRACE_SETREGS, nullptr, &ptrace_regs)) { + if (ptrace_if_alive(PTRACE_SETREGSET, NT_PRSTATUS, &ptrace_regs)) { /* It's ok for flush regs to fail, e.g. if the task got killed underneath * us - we just need to remember not to trust any value we would load * from ptrace otherwise */ diff --git a/src/kernel_abi.cc b/src/kernel_abi.cc index f8b9d9c942a..6486c166fc8 100644 --- a/src/kernel_abi.cc +++ b/src/kernel_abi.cc @@ -335,4 +335,20 @@ template static size_t sigaction_sigset_size_arch() { size_t sigaction_sigset_size(SupportedArch arch) { RR_ARCH_FUNCTION(sigaction_sigset_size_arch, arch); } + +template static size_t user_regs_struct_size_arch() { + return sizeof(typename Arch::user_regs_struct); +} + +size_t user_regs_struct_size(SupportedArch arch) { + RR_ARCH_FUNCTION(user_regs_struct_size_arch, arch) +} + +template static size_t user_fpregs_struct_size_arch() { + return sizeof(typename Arch::user_fpregs_struct); +} + +size_t user_fpregs_struct_size(SupportedArch arch) { + RR_ARCH_FUNCTION(user_fpregs_struct_size_arch, arch) +} } diff --git a/src/kernel_abi.h b/src/kernel_abi.h index 45865d377b6..ce7704b228f 100644 --- a/src/kernel_abi.h +++ b/src/kernel_abi.h @@ -2224,6 +2224,9 @@ void set_arch_siginfo(const siginfo_t& siginfo, SupportedArch a, void* dest, size_t sigaction_sigset_size(SupportedArch arch); +size_t user_regs_struct_size(SupportedArch arch); +size_t user_fpregs_struct_size(SupportedArch arch); + #if defined(__i386__) typedef X86Arch NativeArch; #elif defined(__x86_64__) diff --git a/src/record_syscall.cc b/src/record_syscall.cc index d41dfad3234..4f6695e1089 100644 --- a/src/record_syscall.cc +++ b/src/record_syscall.cc @@ -2585,7 +2585,7 @@ static Switchable prepare_ptrace(RecordTask* t, case NT_PRSTATUS: { RecordTask* tracee = verify_ptrace_target(t, syscall_state, pid); if (tracee) { - auto regs = tracee->regs().get_ptrace_for_arch(Arch::arch()); + auto regs = tracee->regs().get_ptrace_for_arch(tracee->arch()); ptrace_get_reg_set(t, syscall_state, regs); } break; @@ -2594,7 +2594,7 @@ static Switchable prepare_ptrace(RecordTask* t, RecordTask* tracee = verify_ptrace_target(t, syscall_state, pid); if (tracee) { auto regs = - tracee->extra_regs().get_user_fpregs_struct(Arch::arch()); + tracee->extra_regs().get_user_fpregs_struct(tracee->arch()); ptrace_get_reg_set(t, syscall_state, regs); } break; @@ -2667,7 +2667,7 @@ static Switchable prepare_ptrace(RecordTask* t, RecordTask* tracee = verify_ptrace_target(t, syscall_state, pid); if (tracee) { ptrace_verify_set_reg_set( - t, sizeof(typename Arch::user_regs_struct), syscall_state); + t, user_regs_struct_size(tracee->arch()), syscall_state); } break; } @@ -2675,7 +2675,7 @@ static Switchable prepare_ptrace(RecordTask* t, RecordTask* tracee = verify_ptrace_target(t, syscall_state, pid); if (tracee) { ptrace_verify_set_reg_set( - t, sizeof(typename Arch::user_fpregs_struct), syscall_state); + t, user_fpregs_struct_size(tracee->arch()), syscall_state); } break; }