Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1449190 > unrolled thread

Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code Andy Lutomirski <luto@amacapital.net> - 2016-07-24 20:50 +0200
    Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code Ingo Molnar <mingo@kernel.org> - 2016-07-25 08:40 +0200
      Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code Oleg Nesterov <oleg@redhat.com> - 2016-07-25 18:40 +0200
      Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code Andy Lutomirski <luto@amacapital.net> - 2016-07-25 19:00 +0200
        Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code Andy Lutomirski <luto@amacapital.net> - 2016-07-26 02:30 +0200

#1449190 — Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code

On Mon, Jun 20, 2016 at 4:39 PM, Andy Lutomirski <luto@kernel.org> wrote:
> Setting TS_COMPAT in ptrace is wrong: if we happen to do it during
> syscall entry, then we'll confuse seccomp and audit.  (The former
> isn't a security problem: seccomp is currently entirely insecure if a
> malicious ptracer is attached.)  As a minimal fix, this patch adds a
> new flag TS_I386_REGS_POKED that handles the ptrace special case.

Hi Ingo-

Could you apply this one patch for 4.8?  While I don't think it's a
significant security issue in 4.7 or earlier, leaving it unfixed in
4.8 will introduce a potentially unpleasant interaction with some
seccomp changes that are queued up in the
security tree for 4.8.

It will have a trivially-resolvable conflict with -mm.

The rest of the series this is in can wait.

--Andy

>
> Cc: Pedro Alves <palves@redhat.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/common.c            |  6 +++++-
>  arch/x86/include/asm/syscall.h     |  5 +----
>  arch/x86/include/asm/thread_info.h |  3 +++
>  arch/x86/kernel/ptrace.c           | 15 +++++++++------
>  arch/x86/kernel/signal.c           | 26 ++++++++++++++++++++++++--
>  5 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index ec138e538c44..0db497a8ff19 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -270,8 +270,12 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
>          * handling, because syscall restart has a fixup for compat
>          * syscalls.  The fixup is exercised by the ptrace_syscall_32
>          * selftest.
> +        *
> +        * We also need to clear TS_REGS_POKED_I386: the 32-bit tracer
> +        * special case only applies after poking regs and before the
> +        * very next return to user mode.
>          */
> -       ti->status &= ~TS_COMPAT;
> +       ti->status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
>  #endif
>
>         user_enter();
> diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
> index 999b7cd2e78c..4e23dd15c661 100644
> --- a/arch/x86/include/asm/syscall.h
> +++ b/arch/x86/include/asm/syscall.h
> @@ -60,7 +60,7 @@ static inline long syscall_get_error(struct task_struct *task,
>          * TS_COMPAT is set for 32-bit syscall entries and then
>          * remains set until we return to user mode.
>          */
> -       if (task_thread_info(task)->status & TS_COMPAT)
> +       if (task_thread_info(task)->status & (TS_COMPAT|TS_I386_REGS_POKED))
>                 /*
>                  * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
>                  * and will match correctly in comparisons.
> @@ -239,9 +239,6 @@ static inline int syscall_get_arch(void)
>          * TS_COMPAT is set for 32-bit syscall entry and then
>          * remains set until we return to user mode.
>          *
> -        * TIF_IA32 tasks should always have TS_COMPAT set at
> -        * system call time.
> -        *
>          * x32 tasks should be considered AUDIT_ARCH_X86_64.
>          */
>         if (task_thread_info(current)->status & TS_COMPAT)
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index 30c133ac05cd..4bca518d11f4 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -228,6 +228,9 @@ static inline unsigned long current_stack_pointer(void)
>   * have to worry about atomic accesses.
>   */
>  #define TS_COMPAT              0x0002  /* 32bit syscall active (64BIT)*/
> +#ifdef CONFIG_COMPAT
> +#define TS_I386_REGS_POKED     0x0004  /* regs poked by 32-bit ptracer */
> +#endif
>  #define TS_RESTORE_SIGMASK     0x0008  /* restore signal mask in do_signal() */
>
>  #ifndef __ASSEMBLY__
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 600edd225e81..f79576a541ff 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -923,15 +923,18 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
>
>         case offsetof(struct user32, regs.orig_eax):
>                 /*
> -                * A 32-bit debugger setting orig_eax means to restore
> -                * the state of the task restarting a 32-bit syscall.
> -                * Make sure we interpret the -ERESTART* codes correctly
> -                * in case the task is not actually still sitting at the
> -                * exit from a 32-bit syscall with TS_COMPAT still set.
> +                * Warning: bizarre corner case fixup here.  A 32-bit
> +                * debugger setting orig_eax to -1 wants to disable
> +                * syscall restart.  Make sure that the syscall
> +                * restart code sign-extends orig_ax.  Also make sure
> +                * we interpret the -ERESTART* codes correctly if
> +                * loaded into regs->ax in case the task is not
> +                * actually still sitting at the exit from a 32-bit
> +                * syscall with TS_COMPAT still set.
>                  */
>                 regs->orig_ax = value;
>                 if (syscall_get_nr(child, regs) >= 0)
> -                       task_thread_info(child)->status |= TS_COMPAT;
> +                       task_thread_info(child)->status |= TS_I386_REGS_POKED;
>                 break;
>
>         case offsetof(struct user32, regs.eflags):
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 22cc2f9f8aec..6b952e1d8db8 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -760,8 +760,30 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>
>  static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
>  {
> -#ifdef CONFIG_X86_64
> -       if (in_ia32_syscall())
> +       /*
> +        * 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))
>                 return __NR_ia32_restart_syscall;
>  #endif
>  #ifdef CONFIG_X86_X32_ABI
> --
> 2.5.5
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [next] | [standalone]

#1449303

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Mon, Jun 20, 2016 at 4:39 PM, Andy Lutomirski <luto@kernel.org> wrote:
> > Setting TS_COMPAT in ptrace is wrong: if we happen to do it during
> > syscall entry, then we'll confuse seccomp and audit.  (The former
> > isn't a security problem: seccomp is currently entirely insecure if a
> > malicious ptracer is attached.)  As a minimal fix, this patch adds a
> > new flag TS_I386_REGS_POKED that handles the ptrace special case.
> 
> Hi Ingo-
> 
> Could you apply this one patch for 4.8?  While I don't think it's a
> significant security issue in 4.7 or earlier, leaving it unfixed in
> 4.8 will introduce a potentially unpleasant interaction with some
> seccomp changes that are queued up in the
> security tree for 4.8.
> 
> It will have a trivially-resolvable conflict with -mm.
> 
> The rest of the series this is in can wait.

I don't mind the rest of the series either - could you please repost it (with the 
review feedback addressed)?

Looks like that with minor changes the series has Oleg's Acked-by?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1449643

On 07/25, Ingo Molnar wrote:
>
> Looks like that with minor changes the series has Oleg's Acked-by?

Yes, thanks, I think these changes are fine.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1449656

On Jul 24, 2016 11:38 PM, "Ingo Molnar" <mingo@kernel.org> wrote:
>
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
> > On Mon, Jun 20, 2016 at 4:39 PM, Andy Lutomirski <luto@kernel.org> wrote:
> > > Setting TS_COMPAT in ptrace is wrong: if we happen to do it during
> > > syscall entry, then we'll confuse seccomp and audit.  (The former
> > > isn't a security problem: seccomp is currently entirely insecure if a
> > > malicious ptracer is attached.)  As a minimal fix, this patch adds a
> > > new flag TS_I386_REGS_POKED that handles the ptrace special case.
> >
> > Hi Ingo-
> >
> > Could you apply this one patch for 4.8?  While I don't think it's a
> > significant security issue in 4.7 or earlier, leaving it unfixed in
> > 4.8 will introduce a potentially unpleasant interaction with some
> > seccomp changes that are queued up in the
> > security tree for 4.8.
> >
> > It will have a trivially-resolvable conflict with -mm.
> >
> > The rest of the series this is in can wait.
>
> I don't mind the rest of the series either - could you please repost it (with the
> review feedback addressed)?

I'm nervous about it for a couple reasons involving the fact that it's
user visible.

1. It doesn't make gdb work right in all the cases that gdb currently
gets wrong.  I haven't had time to think about whether there's a
minimal tweak that would fix this.

2. It might have annoying interactions with seccomp whitelists.  I
don't know that for sure, but I still don't love it.


Patch 1 is only user-visible in the case where the current behavior is
clearly wrong, so I'd personally be more comfortable applying just
patch 1 for 4.8.

--Andy

[toc] | [prev] | [next] | [standalone]

#1450263

On Mon, Jul 25, 2016 at 9:57 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Jul 24, 2016 11:38 PM, "Ingo Molnar" <mingo@kernel.org> wrote:
>>
>>
>> * Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> > On Mon, Jun 20, 2016 at 4:39 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> > > Setting TS_COMPAT in ptrace is wrong: if we happen to do it during
>> > > syscall entry, then we'll confuse seccomp and audit.  (The former
>> > > isn't a security problem: seccomp is currently entirely insecure if a
>> > > malicious ptracer is attached.)  As a minimal fix, this patch adds a
>> > > new flag TS_I386_REGS_POKED that handles the ptrace special case.
>> >
>> > Hi Ingo-
>> >
>> > Could you apply this one patch for 4.8?  While I don't think it's a
>> > significant security issue in 4.7 or earlier, leaving it unfixed in
>> > 4.8 will introduce a potentially unpleasant interaction with some
>> > seccomp changes that are queued up in the
>> > security tree for 4.8.
>> >
>> > It will have a trivially-resolvable conflict with -mm.
>> >
>> > The rest of the series this is in can wait.
>>
>> I don't mind the rest of the series either - could you please repost it (with the
>> review feedback addressed)?
>
> I'm nervous about it for a couple reasons involving the fact that it's
> user visible.
>
> 1. It doesn't make gdb work right in all the cases that gdb currently
> gets wrong.  I haven't had time to think about whether there's a
> minimal tweak that would fix this.

After re-reading the whole thread, I think that the rest of the series
needs a good self-test to make sure that we're providing whatever
behavior we think we're providing.  So I really don't what to apply it
yet.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web