Re: [PATCH v4 3/4] x86: introduce TS_COMPAT_RESTART to fix

[Andy Lutomirski]

On Mon, Feb 1, 2021 at 9:47 AM Oleg Nesterov <oleg@xxxxxxxxxx> 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@xxxxxxxxxxxxxx:/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@xxxxxxxxxx>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
>  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
>