| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALCETrWrPyd1HLXfKLc17CF85r2336YoEpe6bo6dNGdG_2A2bQ@mail.gmail.com>
Date: Mon, 1 Feb 2021 10:32:58 -0800
From: Andy Lutomirski <luto@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Andy Lutomirski <luto@...nel.org>,
Ingo Molnar <mingo@...nel.org>,
Jan Kratochvil <jan.kratochvil@...hat.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Pedro Alves <palves@...hat.com>, Peter Anvin <hpa@...or.com>,
LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>
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@...hat.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@...rceware.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@...hat.com>
> Signed-off-by: Oleg Nesterov <oleg@...hat.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
>
Powered by blists - more mailing lists