Messages in this thread | | | From | Andy Lutomirski <> | Date | Mon, 1 Feb 2021 10:32:58 -0800 | Subject | Re: [PATCH v4 3/4] x86: introduce TS_COMPAT_RESTART to fix |
| |
On Mon, Feb 1, 2021 at 9:47 AM Oleg Nesterov <oleg@redhat.com> wrote: > > The comment in get_nr_restart_syscall() says: > > * The problem is that we can get here when ptrace pokes > * syscall-like values into regs even if we're not in a syscall > * at all. > > Yes. but if we are not in syscall then the > > status & (TS_COMPAT|TS_I386_REGS_POKED) > > check below can't really help: > > - TS_COMPAT can't be set > > - TS_I386_REGS_POKED is only set if regs->orig_ax was changed by > 32bit debugger; and even in this case get_nr_restart_syscall() > is only correct if the tracee is 32bit too. > > Suppose that 64bit debugger plays with 32bit tracee and
At the risk of asking an obnoxious question here:
> > * Tracee calls sleep(2) // TS_COMPAT is set > * User interrupts the tracee by CTRL-C after 1 sec and does > "(gdb) call func()" > * gdb saves the regs by PTRACE_GETREGS
It seems to me that a better solution may be for gdb to see the post-restart-setup state. In other words, shouldn't the GETREGS return with the ax pointing to the restart syscall already?
Now maybe this ship has sailed a long long time ago and we can't do this, but is it at all worth considering?
> * does PTRACE_SETREGS to set %rip='func' and %orig_rax=-1 > * PTRACE_CONT // TS_COMPAT is cleared > * func() hits int3. > * Debugger catches SIGTRAP. > * Restore original regs by PTRACE_SETREGS. > * PTRACE_CONT > > get_nr_restart_syscall() wrongly returns __NR_restart_syscall==219, the > tracee calls ia32_sys_call_table[219] == sys_madvise. > > This patch adds the sticky TS_COMPAT_RESTART flag which survives after > return to user mode, hopefully it allows us to kill TS_I386_REGS_POKED > but this needs another patch. > > Test-case: > > $ cvs -d :pserver:anoncvs:anoncvs@sourceware.org:/cvs/systemtap co ptrace-tests > $ gcc -o erestartsys-trap-debuggee ptrace-tests/tests/erestartsys-trap-debuggee.c --m32 > $ gcc -o erestartsys-trap-debugger ptrace-tests/tests/erestartsys-trap-debugger.c -lutil > $ ./erestartsys-trap-debugger > Unexpected: retval 1, errno 22 > erestartsys-trap-debugger: ptrace-tests/tests/erestartsys-trap-debugger.c:421 > > Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com> > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > arch/x86/include/asm/thread_info.h | 14 +++++++++++++- > arch/x86/kernel/signal.c | 24 +----------------------- > 2 files changed, 14 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h > index c2dc29e215ea..30d1d187019f 100644 > --- a/arch/x86/include/asm/thread_info.h > +++ b/arch/x86/include/asm/thread_info.h > @@ -214,10 +214,22 @@ static inline int arch_within_stack_frames(const void * const stack, > */ > #define TS_COMPAT 0x0002 /* 32bit syscall active (64BIT)*/ > > +#ifndef __ASSEMBLY__ > #ifdef CONFIG_COMPAT > #define TS_I386_REGS_POKED 0x0004 /* regs poked by 32-bit ptracer */ > +#define TS_COMPAT_RESTART 0x0008 > + > +#define arch_set_restart_data arch_set_restart_data > + > +static inline void arch_set_restart_data(struct restart_block *restart) > +{ > + struct thread_info *ti = current_thread_info(); > + if (ti->status & TS_COMPAT) > + ti->status |= TS_COMPAT_RESTART; > + else > + ti->status &= ~TS_COMPAT_RESTART; > +} > #endif > -#ifndef __ASSEMBLY__ > > #ifdef CONFIG_X86_32 > #define in_ia32_syscall() true > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c > index ea794a083c44..6c26d2c3a2e4 100644 > --- a/arch/x86/kernel/signal.c > +++ b/arch/x86/kernel/signal.c > @@ -766,30 +766,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) > > static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs) > { > - /* > - * This function is fundamentally broken as currently > - * implemented. > - * > - * The idea is that we want to trigger a call to the > - * restart_block() syscall and that we want in_ia32_syscall(), > - * in_x32_syscall(), etc. to match whatever they were in the > - * syscall being restarted. We assume that the syscall > - * instruction at (regs->ip - 2) matches whatever syscall > - * instruction we used to enter in the first place. > - * > - * The problem is that we can get here when ptrace pokes > - * syscall-like values into regs even if we're not in a syscall > - * at all. > - * > - * For now, we maintain historical behavior and guess based on > - * stored state. We could do better by saving the actual > - * syscall arch in restart_block or (with caveats on x32) by > - * checking if regs->ip points to 'int $0x80'. The current > - * behavior is incorrect if a tracer has a different bitness > - * than the tracee. > - */ > #ifdef CONFIG_IA32_EMULATION > - if (current_thread_info()->status & (TS_COMPAT|TS_I386_REGS_POKED)) > + if (current_thread_info()->status & TS_COMPAT_RESTART) > return __NR_ia32_restart_syscall; > #endif > #ifdef CONFIG_X86_X32_ABI > -- > 2.25.1.362.g51ebf55 >
| |