[PATCH v1 0/4] um: Add seccomp support

[PATCH v1 0/4] um: Add seccomp support

[Mickaël Salaün]

This series add seccomp support to User-mode Linux (i386 and x86_64
subarchitectures) and fix ptrace issues. This apply on v4.4-rc4 and pass all
the 48 tests from selftest/seccomp.

Regards,
 Mickaël

Mickaël Salaün (4):
  um: Fix ptrace GETREGS/SETREGS bugs
  selftests/seccomp: Remove the need for HAVE_ARCH_TRACEHOOK
  um: Add full asm/syscall.h support
  um: Add seccomp support

 .../seccomp/seccomp-filter/arch-support.txt        |   2 +-
 arch/um/Kconfig.common                             |   1 +
 arch/um/Kconfig.um                                 |  16 +++
 arch/um/include/asm/syscall-generic.h              | 138 +++++++++++++++++++++
 arch/um/include/asm/thread_info.h                  |   2 +
 arch/um/kernel/skas/syscall.c                      |  16 ++-
 arch/um/os-Linux/skas/process.c                    |   7 --
 arch/x86/um/asm/syscall.h                          |   1 +
 tools/testing/selftests/seccomp/seccomp_bpf.c      |  27 +++-
 9 files changed, 198 insertions(+), 12 deletions(-)
 create mode 100644 arch/um/include/asm/syscall-generic.h

-- 
2.6.4

[PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Mickaël Salaün]

This fix two related bugs:
* PTRACE_GETREGS doesn't get the right orig_ax (syscall) value
* PTRACE_SETREGS can't set the orig_ax value (erased by initial value)

Remove the now useless and error-prone get_syscall().

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Thomas Meyer <thomas@m3y3r.de>
Cc: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Cc: Anton Ivanov <aivanov@brocade.com>
Cc: linux-kernel@vger.kernel.org
Cc: user-mode-linux-devel@lists.sourceforge.net
Cc: user-mode-linux-user@lists.sourceforge.net
Cc: Meredydd Luff <meredydd@senatehouse.org>
Cc: David Drysdale <drysdale@google.com>
---
 arch/um/kernel/skas/syscall.c   | 7 ++++++-
 arch/um/os-Linux/skas/process.c | 7 -------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
index 1683b8e..65f0d1a 100644
--- a/arch/um/kernel/skas/syscall.c
+++ b/arch/um/kernel/skas/syscall.c
@@ -7,6 +7,7 @@
 #include <linux/ptrace.h>
 #include <kern_util.h>
 #include <sysdep/ptrace.h>
+#include <sysdep/ptrace_user.h>
 #include <sysdep/syscalls.h>
 #include <os.h>
 
@@ -16,12 +17,16 @@ void handle_syscall(struct uml_pt_regs *r)
 	long result;
 	int syscall;
 
+	/* Save the syscall register. */
+	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
+
 	if (syscall_trace_enter(regs)) {
 		result = -ENOSYS;
 		goto out;
 	}
 
-	syscall = get_syscall(r);
+	/* Get the syscall after being potentially updated with ptrace. */
+	syscall = UPT_SYSCALL_NR(r);
 
 	if ((syscall > __NR_syscall_max) || syscall < 0)
 		result = -ENOSYS;
diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
index b856c66..23025d6 100644
--- a/arch/um/os-Linux/skas/process.c
+++ b/arch/um/os-Linux/skas/process.c
@@ -172,13 +172,6 @@ static void handle_trap(int pid, struct uml_pt_regs *regs,
 	handle_syscall(regs);
 }
 
-int get_syscall(struct uml_pt_regs *regs)
-{
-	UPT_SYSCALL_NR(regs) = PT_SYSCALL_NR(regs->gp);
-
-	return UPT_SYSCALL_NR(regs);
-}
-
 extern char __syscall_stub_start[];
 
 static int userspace_tramp(void *stack)
-- 
2.6.4

[PATCH v1 2/4] selftests/seccomp: Remove the need for HAVE_ARCH_TRACEHOOK

[Mickaël Salaün]

Some architectures do not implement PTRACE_GETREGSET nor
PTRACE_SETREGSET (required by HAVE_ARCH_TRACEHOOK) but only implement
PTRACE_GETREGS and PTRACE_SETREGS (e.g. User-mode Linux).

This improve seccomp selftest portability for architectures without
HAVE_ARCH_TRACEHOOK support by defining a new trigger HAVE_GETREGS. For
now, this is only enabled for i386 and x86_64 architectures. This is
required to be able to run this tests on User-mode Linux.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: linux-kernel@vger.kernel.org
Cc: user-mode-linux-devel@lists.sourceforge.net
Cc: user-mode-linux-user@lists.sourceforge.net
Cc: linux-api@vger.kernel.org
Cc: Meredydd Luff <meredydd@senatehouse.org>
Cc: David Drysdale <drysdale@google.com>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 882fe83..b9453b8 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -1246,11 +1246,24 @@ TEST_F(TRACE_poke, getpid_runs_normally)
 # error "Do not know how to find your architecture's registers and syscalls"
 #endif
 
+/* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for
+ * architectures without HAVE_ARCH_TRACEHOOK (e.g. User-mode Linux).
+ */
+#if defined(__x86_64__) || defined(__i386__)
+#define HAVE_GETREGS
+#endif
+
 /* Architecture-specific syscall fetching routine. */
 int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 {
-	struct iovec iov;
 	ARCH_REGS regs;
+#ifdef HAVE_GETREGS
+	EXPECT_EQ(0, ptrace(PTRACE_GETREGS, tracee, 0, &regs)) {
+		TH_LOG("PTRACE_GETREGS failed");
+		return -1;
+	}
+#else
+	struct iovec iov;
 
 	iov.iov_base = &regs;
 	iov.iov_len = sizeof(regs);
@@ -1258,6 +1271,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 		TH_LOG("PTRACE_GETREGSET failed");
 		return -1;
 	}
+#endif
 
 	return regs.SYSCALL_NUM;
 }
@@ -1266,13 +1280,16 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 void change_syscall(struct __test_metadata *_metadata,
 		    pid_t tracee, int syscall)
 {
-	struct iovec iov;
 	int ret;
 	ARCH_REGS regs;
-
+#ifdef HAVE_GETREGS
+	ret = ptrace(PTRACE_GETREGS, tracee, 0, &regs);
+#else
+	struct iovec iov;
 	iov.iov_base = &regs;
 	iov.iov_len = sizeof(regs);
 	ret = ptrace(PTRACE_GETREGSET, tracee, NT_PRSTATUS, &iov);
+#endif
 	EXPECT_EQ(0, ret);
 
 #if defined(__x86_64__) || defined(__i386__) || defined(__powerpc__) || \
@@ -1312,9 +1329,13 @@ void change_syscall(struct __test_metadata *_metadata,
 	if (syscall == -1)
 		regs.SYSCALL_RET = 1;
 
+#ifdef HAVE_GETREGS
+	ret = ptrace(PTRACE_SETREGS, tracee, 0, &regs);
+#else
 	iov.iov_base = &regs;
 	iov.iov_len = sizeof(regs);
 	ret = ptrace(PTRACE_SETREGSET, tracee, NT_PRSTATUS, &iov);
+#endif
 	EXPECT_EQ(0, ret);
 }
 
-- 
2.6.4

[PATCH v1 3/4] um: Add full asm/syscall.h support

[Mickaël Salaün]

Add subarchitecture-independent implementation of asm-generic/syscall.h
allowing access to user system call parameters and results:
* syscall_get_nr()
* syscall_rollback()
* syscall_get_error()
* syscall_get_return_value()
* syscall_set_return_value()
* syscall_get_arguments()
* syscall_set_arguments()
* syscall_get_arch() provided by arch/x86/um/asm/syscall.h

This provides the necessary syscall helpers needed by
HAVE_ARCH_SECCOMP_FILTER plus syscall_get_error().

This is inspired from Meredydd Luff's patch
(https://gerrit.chromium.org/gerrit/21425).

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-kernel@vger.kernel.org
Cc: user-mode-linux-devel@lists.sourceforge.net
Cc: user-mode-linux-user@lists.sourceforge.net
Cc: Meredydd Luff <meredydd@senatehouse.org>
Cc: David Drysdale <drysdale@google.com>
---
 arch/um/include/asm/syscall-generic.h | 138 ++++++++++++++++++++++++++++++++++
 arch/x86/um/asm/syscall.h             |   1 +
 2 files changed, 139 insertions(+)
 create mode 100644 arch/um/include/asm/syscall-generic.h

diff --git a/arch/um/include/asm/syscall-generic.h b/arch/um/include/asm/syscall-generic.h
new file mode 100644
index 0000000..9fb9cf8
--- /dev/null
+++ b/arch/um/include/asm/syscall-generic.h
@@ -0,0 +1,138 @@
+/*
+ * Access to user system call parameters and results
+ *
+ * See asm-generic/syscall.h for function descriptions.
+ *
+ * Copyright (C) 2015 Mickaël Salaün <mic@digikod.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __UM_SYSCALL_GENERIC_H
+#define __UM_SYSCALL_GENERIC_H
+
+#include <asm/ptrace.h>
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <sysdep/ptrace.h>
+
+static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
+{
+
+	return PT_REGS_SYSCALL_NR(regs);
+}
+
+static inline void syscall_rollback(struct task_struct *task,
+				    struct pt_regs *regs)
+{
+	/* do nothing */
+}
+
+static inline long syscall_get_error(struct task_struct *task,
+				     struct pt_regs *regs)
+{
+	const long error = regs_return_value(regs);
+
+	return IS_ERR_VALUE(error) ? error : 0;
+}
+
+static inline long syscall_get_return_value(struct task_struct *task,
+					    struct pt_regs *regs)
+{
+	return regs_return_value(regs);
+}
+
+static inline void syscall_set_return_value(struct task_struct *task,
+					    struct pt_regs *regs,
+					    int error, long val)
+{
+	PT_REGS_SET_SYSCALL_RETURN(regs, (long) error ?: val);
+}
+
+static inline void syscall_get_arguments(struct task_struct *task,
+					 struct pt_regs *regs,
+					 unsigned int i, unsigned int n,
+					 unsigned long *args)
+{
+	const struct uml_pt_regs *r = &regs->regs;
+
+	switch (i) {
+	case 0:
+		if (!n--)
+			break;
+		*args++ = UPT_SYSCALL_ARG1(r);
+	case 1:
+		if (!n--)
+			break;
+		*args++ = UPT_SYSCALL_ARG2(r);
+	case 2:
+		if (!n--)
+			break;
+		*args++ = UPT_SYSCALL_ARG3(r);
+	case 3:
+		if (!n--)
+			break;
+		*args++ = UPT_SYSCALL_ARG4(r);
+	case 4:
+		if (!n--)
+			break;
+		*args++ = UPT_SYSCALL_ARG5(r);
+	case 5:
+		if (!n--)
+			break;
+		*args++ = UPT_SYSCALL_ARG6(r);
+	case 6:
+		if (!n--)
+			break;
+	default:
+		BUG();
+		break;
+	}
+}
+
+static inline void syscall_set_arguments(struct task_struct *task,
+					 struct pt_regs *regs,
+					 unsigned int i, unsigned int n,
+					 const unsigned long *args)
+{
+	struct uml_pt_regs *r = &regs->regs;
+
+	switch (i) {
+	case 0:
+		if (!n--)
+			break;
+		UPT_SYSCALL_ARG1(r) = *args++;
+	case 1:
+		if (!n--)
+			break;
+		UPT_SYSCALL_ARG2(r) = *args++;
+	case 2:
+		if (!n--)
+			break;
+		UPT_SYSCALL_ARG3(r) = *args++;
+	case 3:
+		if (!n--)
+			break;
+		UPT_SYSCALL_ARG4(r) = *args++;
+	case 4:
+		if (!n--)
+			break;
+		UPT_SYSCALL_ARG5(r) = *args++;
+	case 5:
+		if (!n--)
+			break;
+		UPT_SYSCALL_ARG6(r) = *args++;
+	case 6:
+		if (!n--)
+			break;
+	default:
+		BUG();
+		break;
+	}
+}
+
+/* See arch/x86/um/asm/syscall.h for syscall_get_arch() definition. */
+
+#endif	/* __UM_SYSCALL_GENERIC_H */
diff --git a/arch/x86/um/asm/syscall.h b/arch/x86/um/asm/syscall.h
index 81d6562..11ab90d 100644
--- a/arch/x86/um/asm/syscall.h
+++ b/arch/x86/um/asm/syscall.h
@@ -1,6 +1,7 @@
 #ifndef __UM_ASM_SYSCALL_H
 #define __UM_ASM_SYSCALL_H
 
+#include <asm/syscall-generic.h>
 #include <uapi/linux/audit.h>
 
 typedef asmlinkage long (*sys_call_ptr_t)(unsigned long, unsigned long,
-- 
2.6.4

[PATCH v1 4/4] um: Add seccomp support

[Mickaël Salaün]

This brings SECCOMP_MODE_STRICT and SECCOMP_MODE_FILTER support through
prctl(2) and seccomp(2) to User-mode Linux for i386 and x86_64
subarchitectures.

secure_computing() is called first in handle_syscall() so that the
syscall emulation will be aborted quickly if matching a seccomp rule.

This is inspired from Meredydd Luff's patch
(https://gerrit.chromium.org/gerrit/21425).

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: James Hogan <james.hogan@imgtec.com>
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: user-mode-linux-devel@lists.sourceforge.net
Cc: user-mode-linux-user@lists.sourceforge.net
Cc: Meredydd Luff <meredydd@senatehouse.org>
Cc: David Drysdale <drysdale@google.com>
---
 .../features/seccomp/seccomp-filter/arch-support.txt     |  2 +-
 arch/um/Kconfig.common                                   |  1 +
 arch/um/Kconfig.um                                       | 16 ++++++++++++++++
 arch/um/include/asm/thread_info.h                        |  2 ++
 arch/um/kernel/skas/syscall.c                            |  9 +++++++++
 5 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/Documentation/features/seccomp/seccomp-filter/arch-support.txt b/Documentation/features/seccomp/seccomp-filter/arch-support.txt
index 76d39d6..4f66ec1 100644
--- a/Documentation/features/seccomp/seccomp-filter/arch-support.txt
+++ b/Documentation/features/seccomp/seccomp-filter/arch-support.txt
@@ -33,7 +33,7 @@
     |          sh: | TODO |
     |       sparc: | TODO |
     |        tile: |  ok  |
-    |          um: | TODO |
+    |          um: |  ok  |
     |   unicore32: | TODO |
     |         x86: |  ok  |
     |      xtensa: | TODO |
diff --git a/arch/um/Kconfig.common b/arch/um/Kconfig.common
index d195a87..cc00134 100644
--- a/arch/um/Kconfig.common
+++ b/arch/um/Kconfig.common
@@ -2,6 +2,7 @@ config UML
 	bool
 	default y
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_UID16
 	select HAVE_FUTEX_CMPXCHG if FUTEX
 	select GENERIC_IRQ_SHOW
diff --git a/arch/um/Kconfig.um b/arch/um/Kconfig.um
index 28a9885..4b2ed58 100644
--- a/arch/um/Kconfig.um
+++ b/arch/um/Kconfig.um
@@ -104,3 +104,19 @@ config PGTABLE_LEVELS
 	int
 	default 3 if 3_LEVEL_PGTABLES
 	default 2
+
+config SECCOMP
+	def_bool y
+	prompt "Enable seccomp to safely compute untrusted bytecode"
+	---help---
+	  This kernel feature is useful for number crunching applications
+	  that may need to compute untrusted bytecode during their
+	  execution. By using pipes or other transports made available to
+	  the process as file descriptors supporting the read/write
+	  syscalls, it's possible to isolate those applications in
+	  their own address space using seccomp. Once seccomp is
+	  enabled via prctl(PR_SET_SECCOMP), it cannot be disabled
+	  and the task is only allowed to execute a few safe syscalls
+	  defined by each seccomp mode.
+
+	  If unsure, say Y.
diff --git a/arch/um/include/asm/thread_info.h b/arch/um/include/asm/thread_info.h
index 53968aa..053baff 100644
--- a/arch/um/include/asm/thread_info.h
+++ b/arch/um/include/asm/thread_info.h
@@ -62,11 +62,13 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SYSCALL_AUDIT	6
 #define TIF_RESTORE_SIGMASK	7
 #define TIF_NOTIFY_RESUME	8
+#define TIF_SECCOMP		9	/* secure computing */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_MEMDIE		(1 << TIF_MEMDIE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
+#define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 
 #endif
diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
index 65f0d1a..af567da 100644
--- a/arch/um/kernel/skas/syscall.c
+++ b/arch/um/kernel/skas/syscall.c
@@ -5,6 +5,7 @@
 
 #include <linux/kernel.h>
 #include <linux/ptrace.h>
+#include <linux/seccomp.h>
 #include <kern_util.h>
 #include <sysdep/ptrace.h>
 #include <sysdep/ptrace_user.h>
@@ -20,6 +21,14 @@ void handle_syscall(struct uml_pt_regs *r)
 	/* Save the syscall register. */
 	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
 
+	/* Do the secure computing check first; failures should be fast. */
+	if (secure_computing() == -1) {
+		/* Do not put secure_computing() into syscall_trace_enter() to
+		 * avoid forced syscall return value.
+		 */
+		return;
+	}
+
 	if (syscall_trace_enter(regs)) {
 		result = -ENOSYS;
 		goto out;
-- 
2.6.4

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Richard Weinberger]

Am 21.12.2015 um 01:03 schrieb Mickaël Salaün:
> This fix two related bugs:
> * PTRACE_GETREGS doesn't get the right orig_ax (syscall) value
> * PTRACE_SETREGS can't set the orig_ax value (erased by initial value)
> 
> Remove the now useless and error-prone get_syscall().
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Jeff Dike <jdike@addtoit.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: Thomas Meyer <thomas@m3y3r.de>
> Cc: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> Cc: Anton Ivanov <aivanov@brocade.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: user-mode-linux-devel@lists.sourceforge.net
> Cc: user-mode-linux-user@lists.sourceforge.net
> Cc: Meredydd Luff <meredydd@senatehouse.org>
> Cc: David Drysdale <drysdale@google.com>
> ---
>  arch/um/kernel/skas/syscall.c   | 7 ++++++-
>  arch/um/os-Linux/skas/process.c | 7 -------
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
> index 1683b8e..65f0d1a 100644
> --- a/arch/um/kernel/skas/syscall.c
> +++ b/arch/um/kernel/skas/syscall.c
> @@ -7,6 +7,7 @@
>  #include <linux/ptrace.h>
>  #include <kern_util.h>
>  #include <sysdep/ptrace.h>
> +#include <sysdep/ptrace_user.h>
>  #include <sysdep/syscalls.h>
>  #include <os.h>
>  
> @@ -16,12 +17,16 @@ void handle_syscall(struct uml_pt_regs *r)
>  	long result;
>  	int syscall;
>  
> +	/* Save the syscall register. */
> +	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
> +
>  	if (syscall_trace_enter(regs)) {
>  		result = -ENOSYS;
>  		goto out;
>  	}
>  
> -	syscall = get_syscall(r);
> +	/* Get the syscall after being potentially updated with ptrace. */
> +	syscall = UPT_SYSCALL_NR(r);

Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?

Thanks,
//richard

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]


On 21/12/2015 01:20, Richard Weinberger wrote:
> Am 21.12.2015 um 01:03 schrieb Mickaël Salaün:
>> diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
>> index 1683b8e..65f0d1a 100644
>> --- a/arch/um/kernel/skas/syscall.c
>> +++ b/arch/um/kernel/skas/syscall.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/ptrace.h>
>>  #include <kern_util.h>
>>  #include <sysdep/ptrace.h>
>> +#include <sysdep/ptrace_user.h>
>>  #include <sysdep/syscalls.h>
>>  #include <os.h>
>>  
>> @@ -16,12 +17,16 @@ void handle_syscall(struct uml_pt_regs *r)
>>  	long result;
>>  	int syscall;
>>  
>> +	/* Save the syscall register. */
>> +	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
>> +
>>  	if (syscall_trace_enter(regs)) {
>>  		result = -ENOSYS;
>>  		goto out;
>>  	}
>>  
>> -	syscall = get_syscall(r);
>> +	/* Get the syscall after being potentially updated with ptrace. */
>> +	syscall = UPT_SYSCALL_NR(r);
> 
> Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?

The logic is unchanged except updating the UPT_SYSCALL_NR before syscall_trace_enter(). I did my last tests with the x86_32 subarchitecture and all tests (from selftest/seccomp), including PTRACE_SETREGS for syscall numbers tests, passed. However, 2 of this tests still fail for x86_64 (only).

Regards,
 Mickaël


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Richard Weinberger]

Hi!

Am 21.12.2015 um 09:49 schrieb Mickaël Salaün:
> 
> On 21/12/2015 01:20, Richard Weinberger wrote:
>> Am 21.12.2015 um 01:03 schrieb Mickaël Salaün:
>>> diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
>>> index 1683b8e..65f0d1a 100644
>>> --- a/arch/um/kernel/skas/syscall.c
>>> +++ b/arch/um/kernel/skas/syscall.c
>>> @@ -7,6 +7,7 @@
>>>  #include <linux/ptrace.h>
>>>  #include <kern_util.h>
>>>  #include <sysdep/ptrace.h>
>>> +#include <sysdep/ptrace_user.h>
>>>  #include <sysdep/syscalls.h>
>>>  #include <os.h>
>>>  
>>> @@ -16,12 +17,16 @@ void handle_syscall(struct uml_pt_regs *r)
>>>  	long result;
>>>  	int syscall;
>>>  
>>> +	/* Save the syscall register. */
>>> +	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
>>> +
>>>  	if (syscall_trace_enter(regs)) {
>>>  		result = -ENOSYS;
>>>  		goto out;
>>>  	}
>>>  
>>> -	syscall = get_syscall(r);
>>> +	/* Get the syscall after being potentially updated with ptrace. */
>>> +	syscall = UPT_SYSCALL_NR(r);
>>
>> Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?
> 
> The logic is unchanged except updating the UPT_SYSCALL_NR before syscall_trace_enter(). I did my last tests with the x86_32 subarchitecture and all tests (from selftest/seccomp), including PTRACE_SETREGS for syscall numbers tests, passed. However, 2 of this tests still fail for x86_64 (only).

No, you chagned the logic.
syscall_trace_enter() enters the ptrace() path, and here EAX/RAX can be changed.
Hence, "syscall = UPT_SYSCALL_NR(r)" will still see the old syscall number.
--> changing syscall numbers got broken by you. :-)

Thanks,
//richard

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Richard Weinberger]

[sending the mail again, Thunderbird crashed :-\]

Am 21.12.2015 um 09:49 schrieb Mickaël Salaün:
> 
> On 21/12/2015 01:20, Richard Weinberger wrote:
>> Am 21.12.2015 um 01:03 schrieb Mickaël Salaün:
>>> diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
>>> index 1683b8e..65f0d1a 100644
>>> --- a/arch/um/kernel/skas/syscall.c
>>> +++ b/arch/um/kernel/skas/syscall.c
>>> @@ -7,6 +7,7 @@
>>>  #include <linux/ptrace.h>
>>>  #include <kern_util.h>
>>>  #include <sysdep/ptrace.h>
>>> +#include <sysdep/ptrace_user.h>
>>>  #include <sysdep/syscalls.h>
>>>  #include <os.h>
>>>  
>>> @@ -16,12 +17,16 @@ void handle_syscall(struct uml_pt_regs *r)
>>>  	long result;
>>>  	int syscall;
>>>  
>>> +	/* Save the syscall register. */
>>> +	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
>>> +
>>>  	if (syscall_trace_enter(regs)) {
>>>  		result = -ENOSYS;
>>>  		goto out;
>>>  	}
>>>  
>>> -	syscall = get_syscall(r);
>>> +	/* Get the syscall after being potentially updated with ptrace. */
>>> +	syscall = UPT_SYSCALL_NR(r);
>>
>> Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?
> 
> The logic is unchanged except updating the UPT_SYSCALL_NR before syscall_trace_enter(). I did my last tests with the x86_32 subarchitecture and all tests (from selftest/seccomp), including PTRACE_SETREGS for syscall numbers tests, passed. However, 2 of this tests still fail for x86_64 (only).

No, the logic is different.
syscall_trace_enter(regs) enters the ptrace() path and here registers can be changed.
Hence "syscall = UPT_SYSCALL_NR(r);" will see the old syscall number.
UPT_SYSCALL_NR() returns the syscall number before the ptrace() path...

Thanks,
//richard

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]


On 21/12/2015 10:00, Richard Weinberger wrote:
> [sending the mail again, Thunderbird crashed :-\]
> 
> Am 21.12.2015 um 09:49 schrieb Mickaël Salaün:
>>
>> On 21/12/2015 01:20, Richard Weinberger wrote:
>>> Am 21.12.2015 um 01:03 schrieb Mickaël Salaün:
>>>> diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
>>>> index 1683b8e..65f0d1a 100644
>>>> --- a/arch/um/kernel/skas/syscall.c
>>>> +++ b/arch/um/kernel/skas/syscall.c
>>>> @@ -7,6 +7,7 @@
>>>>  #include <linux/ptrace.h>
>>>>  #include <kern_util.h>
>>>>  #include <sysdep/ptrace.h>
>>>> +#include <sysdep/ptrace_user.h>
>>>>  #include <sysdep/syscalls.h>
>>>>  #include <os.h>
>>>>  
>>>> @@ -16,12 +17,16 @@ void handle_syscall(struct uml_pt_regs *r)
>>>>  	long result;
>>>>  	int syscall;
>>>>  
>>>> +	/* Save the syscall register. */
>>>> +	UPT_SYSCALL_NR(r) = PT_SYSCALL_NR(r->gp);
>>>> +
>>>>  	if (syscall_trace_enter(regs)) {
>>>>  		result = -ENOSYS;
>>>>  		goto out;
>>>>  	}
>>>>  
>>>> -	syscall = get_syscall(r);
>>>> +	/* Get the syscall after being potentially updated with ptrace. */
>>>> +	syscall = UPT_SYSCALL_NR(r);
>>>
>>> Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?
>>
>> The logic is unchanged except updating the UPT_SYSCALL_NR before syscall_trace_enter(). I did my last tests with the x86_32 subarchitecture and all tests (from selftest/seccomp), including PTRACE_SETREGS for syscall numbers tests, passed. However, 2 of this tests still fail for x86_64 (only).
> 
> No, the logic is different.
> syscall_trace_enter(regs) enters the ptrace() path and here registers can be changed.
> Hence "syscall = UPT_SYSCALL_NR(r);" will see the old syscall number.
> UPT_SYSCALL_NR() returns the syscall number before the ptrace() path...

The thing is, PTRACE_SETREGS give access to *orig_ax* in the user_regs_struct from arch/x86/include/asm/user_*.h and selftest/seccomp only update this (virtual) register, not the EAX/RAX. Am I missing something?

 Mickaël


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Am 21.12.2015 um 10:23 schrieb Mickaël Salaün:
>>>> Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?
>>>
>>> The logic is unchanged except updating the UPT_SYSCALL_NR before syscall_trace_enter(). I did my last tests with the x86_32 subarchitecture and all tests (from selftest/seccomp), including PTRACE_SETREGS for syscall numbers tests, passed. However, 2 of this tests still fail for x86_64 (only).
>>
>> No, the logic is different.
>> syscall_trace_enter(regs) enters the ptrace() path and here registers can be changed.
>> Hence "syscall = UPT_SYSCALL_NR(r);" will see the old syscall number.
>> UPT_SYSCALL_NR() returns the syscall number before the ptrace() path...
> 
> The thing is, PTRACE_SETREGS give access to *orig_ax* in the user_regs_struct from arch/x86/include/asm/user_*.h and selftest/seccomp only update this (virtual) register, not the EAX/RAX. Am I missing something?

Sorry, meant orig...

Please see the attached program. It proves that your patch is breaking stuff.
The test is extracted from UML's selftests.

Thanks,
//richard

[-- Attachment #2: ptsc.c --]
[-- Type: text/x-csrc, Size: 4641 bytes --]

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/user.h>
#include <sys/syscall.h>
#include <sys/reg.h>

#define CATCH_EINTR(expr) while ((errno = 0, ((expr) < 0)) && (errno == EINTR))
#define PT_OFFSET(r) ((r) * sizeof(long))
#define PT_SYSCALL_NR_OFFSET PT_OFFSET(ORIG_RAX)
#define PTRACE_OLDSETOPTIONS      21

static void ptrace_child(void)
{
        int ret;
        int pid = syscall(__NR_getpid), ppid = getppid();
        int sc_result;

        if (ptrace(PTRACE_TRACEME, 0, 0, 0) < 0) {
                perror("ptrace");
                kill(pid, SIGKILL);
        }
        kill(pid, SIGSTOP);

        /*
         * This syscall will be intercepted by the parent. Don't call more than
         * once, please.
         */
        sc_result = syscall(__NR_getpid);

        if (sc_result == pid)
                /* Nothing modified by the parent, we are running normally. */
                ret = 1;
        else if (sc_result == ppid)
                /*
                 * Expected in check_ptrace and check_sysemu when they succeed
                 * in modifying the stack frame
                 */
                ret = 0;
        else
                /* Serious trouble! This could be caused by a bug in host 2.6
                 * SKAS3/2.6 patch before release -V6, together with a bug in
                 * the UML code itself.
                 */
                ret = 2;

        exit(ret);
}

static int stop_ptraced_child(int pid, int exitcode, int mustexit)
{
        int status, n, ret = 0;

        if (ptrace(PTRACE_CONT, pid, 0, 0) < 0) {
                perror("stop_ptraced_child : ptrace failed");
                return -1;
        }
        CATCH_EINTR(n = waitpid(pid, &status, 0));
        if (!WIFEXITED(status) || (WEXITSTATUS(status) != exitcode)) {
                int exit_with = WEXITSTATUS(status);
                if (exit_with == 2)
                        printf("check_ptrace : child exited with status 2. "
                                  "\nDisabling SYSEMU support.\n");
                printf("check_ptrace : child exited with exitcode %d, while "
                          "expecting %d; status 0x%x\n", exit_with,
                          exitcode, status);
                if (mustexit)
                        exit(1);
                ret = -1;
        }

        return ret;
}


static int start_ptraced_child(void)
{
        int pid, n, status;

        pid = fork();
        if (pid == 0)
                ptrace_child();
        else if (pid < 0)
                printf("start_ptraced_child : fork failed");

        CATCH_EINTR(n = waitpid(pid, &status, WUNTRACED));
        if (n < 0)
                printf("check_ptrace : waitpid failed");
        if (!WIFSTOPPED(status) || (WSTOPSIG(status) != SIGSTOP))
                printf("check_ptrace : expected SIGSTOP, got status = %d",
                      status);

        return pid;
}

static void check_ptrace(void)
{       
        int pid, syscall, n, status;
        
        printf("Checking that ptrace can change system call numbers...");
	fflush(stdout);

        pid = start_ptraced_child();

        if ((ptrace(PTRACE_OLDSETOPTIONS, pid, 0,
                   (void *) PTRACE_O_TRACESYSGOOD) < 0))
                printf("check_ptrace: PTRACE_OLDSETOPTIONS failed");

        while (1) {
                if (ptrace(PTRACE_SYSCALL, pid, 0, 0) < 0)
                        printf("check_ptrace : ptrace failed");
                
                CATCH_EINTR(n = waitpid(pid, &status, WUNTRACED));
                if (n < 0)
                        printf("check_ptrace : wait failed");
                
                if (!WIFSTOPPED(status) ||
                   (WSTOPSIG(status) != (SIGTRAP | 0x80)))
                        printf("check_ptrace : expected (SIGTRAP|0x80), "
                               "got status = %d", status);
                
                syscall = ptrace(PTRACE_PEEKUSER, pid, PT_SYSCALL_NR_OFFSET,
                                 0);
                if (syscall == __NR_getpid) {
                        n = ptrace(PTRACE_POKEUSER, pid, PT_SYSCALL_NR_OFFSET,
                                   __NR_getppid);
                        if (n < 0)
                                printf("check_ptrace : failed to modify "
                                             "system call");
                        break;
                }
        }
        stop_ptraced_child(pid, 0, 1);
        printf("OK\n");
}

int main(void)
{
	check_ptrace();

	return 0;
}

Re: [PATCH v1 1/4] um: Fix ptrace GETREGS/SETREGS bugs

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]


On 21/12/2015 11:13, Richard Weinberger wrote:
> Am 21.12.2015 um 10:23 schrieb Mickaël Salaün:
>>>>> Doesn't this break the support for changing syscall numbers using PTRACE_SETREGS?
>>>>
>>>> The logic is unchanged except updating the UPT_SYSCALL_NR before syscall_trace_enter(). I did my last tests with the x86_32 subarchitecture and all tests (from selftest/seccomp), including PTRACE_SETREGS for syscall numbers tests, passed. However, 2 of this tests still fail for x86_64 (only).
>>>
>>> No, the logic is different.
>>> syscall_trace_enter(regs) enters the ptrace() path and here registers can be changed.
>>> Hence "syscall = UPT_SYSCALL_NR(r);" will see the old syscall number.
>>> UPT_SYSCALL_NR() returns the syscall number before the ptrace() path...
>>
>> The thing is, PTRACE_SETREGS give access to *orig_ax* in the user_regs_struct from arch/x86/include/asm/user_*.h and selftest/seccomp only update this (virtual) register, not the EAX/RAX. Am I missing something?
> 
> Sorry, meant orig...
> 
> Please see the attached program. It proves that your patch is breaking stuff.
> The test is extracted from UML's selftests.

OK, I found the origin of this misunderstanding. On x86_32, PTRACE_SETREGS set regs->syscall when updating orig_eax, which is not the case on x86_64, hence the difference of behavior. I fixed this bug in the v2 series. The ptsc test and all the seccomp tests pass for 32 and 64 bits!

Where can we find the UML selftests?

Thanks,
 Mickaël


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]