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

Groups > linux.kernel > #1427154 > unrolled thread

[PATCH v3 0/3] ptrace-vs-syscall-restart fixes, v3

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] ptrace-vs-syscall-restart fixes, v3 Andy Lutomirski <luto@kernel.org> - 2016-06-21 02:10 +0200
    [PATCH v3 3/3] x86/ptrace, x86/signal: Remove TS_I386_REGS_POKED Andy Lutomirski <luto@kernel.org> - 2016-06-21 02:10 +0200
    [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace code Andy Lutomirski <luto@kernel.org> - 2016-06-21 02:10 +0200
      Re: [PATCH v3 1/3] x86/ptrace: Stop setting TS_COMPAT in ptrace  code Oleg Nesterov <oleg@redhat.com> - 2016-06-23 00:20 +0200

#1427154 — [PATCH v3 0/3] ptrace-vs-syscall-restart fixes, v3

Now that I have a complete fix and cleanup that I like, here it is.
Patch 1 is the same as before except for a config-dependent build
fix.

This series is a cleanup and fixes at least two bugs:

 - 64-bit gdb attached to a 32-bit program malfunctions if the user
   types something like "print foo()" while stopped on exit from an
   interrupted ERESTART_RESTARTBLOCK-using syscall.

 - A 32-bit tracer that writes orig_eax while the tracee is stopped
   at syscall entry will break in_ia32_syscall() and
   syscall_get_arch().

After this is applied, it might be safe to drop all of the TS_COMPAT
checks in syscall.h, but I'd want to think carefully about that.

The reason that patch 1 is here at all instead of being folded into
the other patches is that it's intended to be cleanly and safely
backported if needed.

Patch 1 is for 4.7 or 4.8 at the maintainers' discretion.  It could
also make sense for -stable -- I thing it fixes a bug that could be
exploited to confuse the syscall auit logs.  Patch 2 and 3 are
intended for 4.8.

Pedro, can you try to test this series a bit?  I'm having trouble
getting ptrace-tests to pass even on an unmodified kernel.

Andy Lutomirski (3):
  x86/ptrace: Stop setting TS_COMPAT in ptrace code
  x86/signal: Rewire the restart_block() syscall to have a constant nr
  x86/ptrace, x86/signal: Remove TS_I386_REGS_POKED

 arch/x86/entry/syscalls/syscall_32.tbl |  2 ++
 arch/x86/entry/syscalls/syscall_64.tbl |  3 +++
 arch/x86/include/asm/syscall.h         |  3 ---
 arch/x86/kernel/ptrace.c               | 34 +++++++++++++++++++++-------------
 arch/x86/kernel/signal.c               | 16 ++++++++++------
 5 files changed, 36 insertions(+), 22 deletions(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1427156 — [PATCH v3 3/3] x86/ptrace, x86/signal: Remove TS_I386_REGS_POKED

System call restart has some oddities wrt ptrace:

1. For whatever reason, the kernel delivers signals and triggers
   ptrace before handling syscall restart.  This means that
   -ERESTART_RESTARTBLOCK, etc is visible to userspace.  We could
   plausibly get away with changing that, but it seems quite risky.

2. As a result of (1), gdb (quite reasonably) expects that it can
   snapshot user state on signal delivery, adjust regs to call a
   function, and then restore user state.

3. Presumably as a result of (2), we do syscall restart if indicated
   by the register state on ptrace resume even if we're *not* resuming
   a syscall.

4. Also as a result of (1), gdb expects that writing -1 to orig_eax
   via POKEUSER or similar will *disable* syscall restart, which is
   necessary to get function calling on syscall exit to work.

The combination of (1) and (4) means that, if we have a 32-bit tracer,
we need to skip syscall restart if orig_eax == -1 (in a 32-bit signed
sense).  The combination of (1) and (2) means that, if we have a
32-bit tracer, we need to enable syscall restart if orig_eax > 0 (in a
32-bit signed sense) and eax contains a -ERESTART* code (again in a
signed sense).

The current state of affairs is a mess.  Setting a temporary per-task
flag when ptrace changes orig_eax is messy.  It does the wrong thing
when ptrace only writes eax.  It's also seriously overcomplicated IMO.

Instead, just unconditionally sign-extending them in the ptrace code
and not worrying about ptrace in the signal handling code.

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     |  2 +-
 arch/x86/include/asm/thread_info.h |  3 ---
 arch/x86/kernel/ptrace.c           | 37 +++++++++++++++++++++----------------
 4 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 0db497a8ff19..ec138e538c44 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -270,12 +270,8 @@ __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|TS_I386_REGS_POKED);
+	ti->status &= ~TS_COMPAT;
 #endif
 
 	user_enter();
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 4e23dd15c661..4216bb7cbcba 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|TS_I386_REGS_POKED))
+	if (task_thread_info(task)->status & TS_COMPAT)
 		/*
 		 * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
 		 * and will match correctly in comparisons.
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 4bca518d11f4..30c133ac05cd 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -228,9 +228,6 @@ 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 f79576a541ff..c95aba795f88 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -891,6 +891,10 @@ long arch_ptrace(struct task_struct *child, long request,
 	case offsetof(struct user32, regs.l):				\
 		regs->q = value; break
 
+#define R32_SIGNED(l,q)							\
+	case offsetof(struct user32, regs.l):				\
+		regs->q = (long)(s32)value; break
+
 #define SEG32(rs)							\
 	case offsetof(struct user32, regs.rs):				\
 		return set_segment_reg(child,				\
@@ -917,25 +921,26 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
 	R32(edi, di);
 	R32(esi, si);
 	R32(ebp, bp);
-	R32(eax, ax);
 	R32(eip, ip);
 	R32(esp, sp);
 
-	case offsetof(struct user32, regs.orig_eax):
-		/*
-		 * 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_I386_REGS_POKED;
-		break;
+	/*
+	 * A 32-bit ptracer has the following expectations:
+	 *
+	 * - Storing -1 (i.e. 0xffffffff) to orig_eax will prevent
+	 *   syscall restart handling.
+	 *
+	 * - Restoring regs saved on exit from an interrupted
+	 *   restartable syscall will trigger syscall restart.  Such
+	 *   regs will have non-negative orig_eax and negative eax.
+	 *
+	 * The kernel's syscall restart code treats regs->orig_ax and
+	 * regs->ax as 64-bit signed quantities.  32-bit user code
+	 * doesn't care about the high bits.  Keep it simple and just
+	 * sign-extend both values.
+	 */
+	R32_SIGNED(orig_eax, orig_ax);
+	R32_SIGNED(eax, ax);
 
 	case offsetof(struct user32, regs.eflags):
 		return set_flags(child, value);
-- 
2.5.5

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

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

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.

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

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

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

Andy, sorry for delay. And for the noise.

I just want to say that I'll try very much to read this series
tomorrow. I have some concerns at first glance... but I feel that
most probably this is only because I already need to sleep ;)

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web