[PATCH 0/4] MIPS: syscall tracing fixes

[PATCH 0/4] MIPS: syscall tracing fixes

[James Hogan]

These patches fix some system call tracing issues around seccomp and
ptrace on MIPS.

Patch 1 fixes an issue introduced in v4.13-rc1, where o32 indirect
syscall arguments aren't shifted when filling out seccomp_data struct.
Arguably the samples/bpf/tracex5 case that was being fixed in -rc1 is
flawed, or else other arches are broken too. thoughts welcome on that,
but either way this fix should be okay. It'd be good to get this fix
in particular into v4.13.

Patches 2 and 3 fix changing of system calls by ptrace and
SECCOMP_RET_TRACE so that seccomp & syscall trace don't use the stale
system call number, which appears to have been conceptually broken since
v3.19 when thread_info::syscall was introduced, but also prevented the
change in v4.8 to re-run the seccomp filter against a changed syscall
from being effective on MIPS.
First (patch 2) syscall_trace_enter() is fixed to re-read the syscall
number from thread_info:syscall, then (patch 3) ptrace is fixed to
update thread_info::syscall when the relevant registers are altered.

Finally patch 4 fixes an API gap for MIPS which prevents a
SECCOMP_RET_TRACE tracer from being able to cancel a system call, since
you can't set both the system call number (v0) to -1 and the return
value (v0) to the chosen error code. A PTRACE_SET_SYSCALL is added which
allows thread_info::syscall to be set to -1 after the return value has
already been set in the v0 register to some other value.

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: David Daney <david.daney@cavium.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lars Persson <lars.persson@axis.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org

James Hogan (4):
  MIPS/seccomp: Fix indirect syscall args
  MIPS/ptrace: Pick up ptrace/seccomp changed syscalls
  MIPS/ptrace: Update syscall nr on register changes
  MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

 arch/mips/include/asm/syscall.h     | 29 ++++++++++++++++++++----
 arch/mips/include/uapi/asm/ptrace.h |  1 +
 arch/mips/kernel/ptrace.c           | 45 +++++++++++++++++++++++++++++--------
 arch/mips/kernel/ptrace32.c         | 18 +++++++++++++++
 4 files changed, 80 insertions(+), 13 deletions(-)

-- 
2.13.2

[PATCH 1/4] MIPS/seccomp: Fix indirect syscall args

[James Hogan]

Since commit 669c4092225f ("MIPS: Give __secure_computing() access to
syscall arguments."), upon syscall entry when seccomp is enabled,
syscall_trace_enter() passes a carefully prepared struct seccomp_data
containing syscall arguments to __secure_computing(). Unfortunately it
directly uses mips_get_syscall_arg() and fails to take into account the
indirect O32 system calls (i.e. syscall(2)) which put the system call
number in a0 and have the arguments shifted up by one entry.

We can't just revert that commit as samples/bpf/tracex5 would break
again, so use syscall_get_arguments() which already takes indirect
syscalls into account instead of directly using mips_get_syscall_arg(),
similar to what populate_seccomp_data() does.

This also removes the redundant error checking of the
mips_get_syscall_arg() return value (get_user() already zeroes the
result if an argument from the stack can't be loaded).

Reported-by: James Cowgill <James.Cowgill@imgtec.com>
Fixes: 669c4092225f ("MIPS: Give __secure_computing() access to syscall arguments.")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: David Daney <david.daney@cavium.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
---
It would have been much simpler for MIPS arch code to just pass a NULL
seccomp_data to secure_computing() so populate_seccomp_data() would take
care of fetching arguments, as it did for MIPS prior to commit
669c4092225f ("MIPS: Give __secure_computing() access to syscall
arguments."), but as that commit mentions it breaks samples/bpf/tracex5,
which relies on sd being non-NULL at entry to __seccomp_filter().

Arguably the samples/bpf/tracex5 test is flawed, at least for every arch
except x86 (and now MIPS).
---
 arch/mips/kernel/ptrace.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index 6dd13641a418..1395654cfc8d 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -872,15 +872,13 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
 	if (unlikely(test_thread_flag(TIF_SECCOMP))) {
 		int ret, i;
 		struct seccomp_data sd;
+		unsigned long args[6];
 
 		sd.nr = syscall;
 		sd.arch = syscall_get_arch();
-		for (i = 0; i < 6; i++) {
-			unsigned long v, r;
-
-			r = mips_get_syscall_arg(&v, current, regs, i);
-			sd.args[i] = r ? 0 : v;
-		}
+		syscall_get_arguments(current, regs, 0, 6, args);
+		for (i = 0; i < 6; i++)
+			sd.args[i] = args[i];
 		sd.instruction_pointer = KSTK_EIP(current);
 
 		ret = __secure_computing(&sd);
-- 
2.13.2

[PATCH 2/4] MIPS/ptrace: Pick up ptrace/seccomp changed syscalls

[James Hogan]

The MIPS syscall_trace_enter() allows the system call number to be
altered or cancelled by a ptrace tracer, via the normal ptrace hook
(PTRACE_SYSCALL) and changing the system call number register on entry,
and similarly via seccomp (PTRACE_EVENT_SECCOMP when a seccomp filter
returns SECCOMP_RET_TRACE).

Be sure to update the syscall local variable if this happens, so that
seccomp will filter the correct system call number if the normal ptrace
hook changes it first, and so that if either the normal ptrace hook or
seccomp change it the correct system call number is passed to the trace
event.

This won't have any effect until the next commit, which fixes ptrace to
update thread_info::syscall.

Fixes: c2d9f1775731 ("MIPS: Fix syscall_get_nr for the syscall exit tracing.")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Lars Persson <lars.persson@axis.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-mips@linux-mips.org
---
 arch/mips/kernel/ptrace.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index 1395654cfc8d..be5d5fefcc7c 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -864,9 +864,11 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
 
 	current_thread_info()->syscall = syscall;
 
-	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
-	    tracehook_report_syscall_entry(regs))
-		return -1;
+	if (test_thread_flag(TIF_SYSCALL_TRACE)) {
+		if (tracehook_report_syscall_entry(regs))
+			return -1;
+		syscall = current_thread_info()->syscall;
+	}
 
 #ifdef CONFIG_SECCOMP
 	if (unlikely(test_thread_flag(TIF_SECCOMP))) {
@@ -884,6 +886,7 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
 		ret = __secure_computing(&sd);
 		if (ret == -1)
 			return ret;
+		syscall = current_thread_info()->syscall;
 	}
 #endif
 
-- 
2.13.2

[PATCH 3/4] MIPS/ptrace: Update syscall nr on register changes

[James Hogan]

Update the thread_info::syscall field when registers are modified via
ptrace to change or cancel the system call being entered.

This is important to allow seccomp and the syscall entry and exit trace
events to observe the new syscall number changed by the normal ptrace
hook or seccomp. That includes allowing seccomp's recheck of the system
call number after SECCOMP_RET_TRACE to notice if the syscall is changed
to a denied one, which happens in seccomp since commit ce6526e8afa4
("seccomp: recheck the syscall after RET_TRACE") in v4.8.

In the process of doing this, the logic to determine whether an indirect
system call is in progress (i.e. the O32 ABI's syscall()) is abstracted
into mips_syscall_is_indirect(), and a new mips_syscall_update_nr() is
used to update the thread_info::syscall based on the register state.

The following ptrace operations are updated:
 - PTRACE_SETREGS (ptrace_setregs()).
 - PTRACE_SETREGSET with NT_PRSTATUS (gpr32_set() and gpr64_set()).
 - PTRACE_POKEUSR with 2/v0 or 4/a0 for indirect syscall
   ([compat_]arch_ptrace()).

Fixes: c2d9f1775731 ("MIPS: Fix syscall_get_nr for the syscall exit tracing.")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Lars Persson <larper@axis.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-mips@linux-mips.org
---
 arch/mips/include/asm/syscall.h | 29 +++++++++++++++++++++++++----
 arch/mips/kernel/ptrace.c       | 15 +++++++++++++++
 arch/mips/kernel/ptrace32.c     |  7 +++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index 7c713025b23f..0170602a1e4e 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -26,12 +26,34 @@
 #define __NR_syscall 4000
 #endif
 
+static inline bool mips_syscall_is_indirect(struct task_struct *task,
+					    struct pt_regs *regs)
+{
+	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
+	return (IS_ENABLED(CONFIG_32BIT) ||
+		test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
+		(regs->regs[2] == __NR_syscall);
+}
+
 static inline long syscall_get_nr(struct task_struct *task,
 				  struct pt_regs *regs)
 {
 	return current_thread_info()->syscall;
 }
 
+static inline void mips_syscall_update_nr(struct task_struct *task,
+					  struct pt_regs *regs)
+{
+	/*
+	 * v0 is the system call number, except for O32 ABI syscall(), where it
+	 * ends up in a0.
+	 */
+	if (mips_syscall_is_indirect(task, regs))
+		task_thread_info(task)->syscall = regs->regs[4];
+	else
+		task_thread_info(task)->syscall = regs->regs[2];
+}
+
 static inline unsigned long mips_get_syscall_arg(unsigned long *arg,
 	struct task_struct *task, struct pt_regs *regs, unsigned int n)
 {
@@ -98,10 +120,9 @@ static inline void syscall_get_arguments(struct task_struct *task,
 					 unsigned long *args)
 {
 	int ret;
-	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
-	if ((IS_ENABLED(CONFIG_32BIT) ||
-	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
-	    (regs->regs[2] == __NR_syscall))
+
+	/* O32 ABI syscall() */
+	if (mips_syscall_is_indirect(task, regs))
 		i++;
 
 	while (n--)
diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index be5d5fefcc7c..465fc5633e61 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -144,6 +144,9 @@ int ptrace_setregs(struct task_struct *child, struct user_pt_regs __user *data)
 
 	/* badvaddr, status, and cause may not be written.  */
 
+	/* System call number may have been changed */
+	mips_syscall_update_nr(child, regs);
+
 	return 0;
 }
 
@@ -345,6 +348,9 @@ static int gpr32_set(struct task_struct *target,
 		}
 	}
 
+	/* System call number may have been changed */
+	mips_syscall_update_nr(target, regs);
+
 	return 0;
 }
 
@@ -405,6 +411,9 @@ static int gpr64_set(struct task_struct *target,
 		}
 	}
 
+	/* System call number may have been changed */
+	mips_syscall_update_nr(target, regs);
+
 	return 0;
 }
 
@@ -753,6 +762,12 @@ long arch_ptrace(struct task_struct *child, long request,
 		switch (addr) {
 		case 0 ... 31:
 			regs->regs[addr] = data;
+			/* System call number may have been changed */
+			if (addr == 2)
+				mips_syscall_update_nr(child, regs);
+			else if (addr == 4 &&
+				 mips_syscall_is_indirect(child, regs))
+				mips_syscall_update_nr(child, regs);
 			break;
 		case FPR_BASE ... FPR_BASE + 31: {
 			union fpureg *fregs = get_fpu_regs(child);
diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
index 40e212d6b26b..2b9260f92ccd 100644
--- a/arch/mips/kernel/ptrace32.c
+++ b/arch/mips/kernel/ptrace32.c
@@ -33,6 +33,7 @@
 #include <asm/pgtable.h>
 #include <asm/page.h>
 #include <asm/reg.h>
+#include <asm/syscall.h>
 #include <linux/uaccess.h>
 #include <asm/bootinfo.h>
 
@@ -195,6 +196,12 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 		switch (addr) {
 		case 0 ... 31:
 			regs->regs[addr] = data;
+			/* System call number may have been changed */
+			if (addr == 2)
+				mips_syscall_update_nr(child, regs);
+			else if (addr == 4 &&
+				 mips_syscall_is_indirect(child, regs))
+				mips_syscall_update_nr(child, regs);
 			break;
 		case FPR_BASE ... FPR_BASE + 31: {
 			union fpureg *fregs = get_fpu_regs(child);
-- 
2.13.2

[PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

[James Hogan]

Add a PTRACE_SET_SYSCALL ptrace operation to allow the system call to be
cancelled independently to the value of the v0 system call number
register.

This is needed for SECCOMP_RET_TRACE when the tracer wants to cancel the
system call, since it has to set both the system call number to -1 and
the chosen return value, both of which reside in the same register (v0).
The tracer should set the return value first, followed by
PTRACE_SET_SYSCALL to set the system call number to -1.

That is in contrast to the normal ptrace syscall hook which triggers the
tracer on both entry and exit, allowing the system call to be cancelled
during the entry hook (setting system call number register to -1, or
optionally using PTRACE_SET_SYSCALL), separately to setting the return
value during the exit hook.

Positive values (to change the syscall that should be executed instead
of cancelling it entirely) are explicitly disallowed at the moment. The
same thing can be done safely already by writing the v0 system call
number register and the argument registers, and allowing
thread_info::syscall to be changed to a different value independently of
the v0 register would potentially allow seccomp or the syscall trace
events to be fooled into thinking a different system call was being
executed.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-mips@linux-mips.org
---
 arch/mips/include/uapi/asm/ptrace.h |  1 +
 arch/mips/kernel/ptrace.c           | 11 +++++++++++
 arch/mips/kernel/ptrace32.c         | 11 +++++++++++
 3 files changed, 23 insertions(+)

diff --git a/arch/mips/include/uapi/asm/ptrace.h b/arch/mips/include/uapi/asm/ptrace.h
index 91a3d197ede3..23af103c4e8d 100644
--- a/arch/mips/include/uapi/asm/ptrace.h
+++ b/arch/mips/include/uapi/asm/ptrace.h
@@ -58,6 +58,7 @@ struct pt_regs {
 
 #define PTRACE_GET_THREAD_AREA	25
 #define PTRACE_SET_THREAD_AREA	26
+#define PTRACE_SET_SYSCALL	27
 
 /* Calls to trace a 64bit program from a 32bit program.	 */
 #define PTRACE_PEEKTEXT_3264	0xc0
diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index 465fc5633e61..9bf31a990c6e 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -853,6 +853,17 @@ long arch_ptrace(struct task_struct *child, long request,
 		ret = put_user(task_thread_info(child)->tp_value, datalp);
 		break;
 
+	case PTRACE_SET_SYSCALL:
+		/*
+		 * This is currently only useful to cancel the syscall from a
+		 * seccomp RET_TRACE tracer.
+		 */
+		if ((long)data >= 0)
+			return -EINVAL;
+		task_thread_info(child)->syscall = -1;
+		ret = 0;
+		break;
+
 	case PTRACE_GET_WATCH_REGS:
 		ret = ptrace_get_watch_regs(child, addrp);
 		break;
diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
index 2b9260f92ccd..cca76aec9c10 100644
--- a/arch/mips/kernel/ptrace32.c
+++ b/arch/mips/kernel/ptrace32.c
@@ -287,6 +287,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 				(unsigned int __user *) (unsigned long) data);
 		break;
 
+	case PTRACE_SET_SYSCALL:
+		/*
+		 * This is currently only useful to cancel the syscall from a
+		 * seccomp RET_TRACE tracer.
+		 */
+		if ((long)data >= 0)
+			return -EINVAL;
+		task_thread_info(child)->syscall = -1;
+		ret = 0;
+		break;
+
 	case PTRACE_GET_THREAD_AREA_3264:
 		ret = put_user(task_thread_info(child)->tp_value,
 				(unsigned long __user *) (unsigned long) data);
-- 
2.13.2

Re: [PATCH 1/4] MIPS/seccomp: Fix indirect syscall args

[Kees Cook]

On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Since commit 669c4092225f ("MIPS: Give __secure_computing() access to
> syscall arguments."), upon syscall entry when seccomp is enabled,
> syscall_trace_enter() passes a carefully prepared struct seccomp_data
> containing syscall arguments to __secure_computing(). Unfortunately it
> directly uses mips_get_syscall_arg() and fails to take into account the
> indirect O32 system calls (i.e. syscall(2)) which put the system call
> number in a0 and have the arguments shifted up by one entry.
>
> We can't just revert that commit as samples/bpf/tracex5 would break
> again, so use syscall_get_arguments() which already takes indirect
> syscalls into account instead of directly using mips_get_syscall_arg(),
> similar to what populate_seccomp_data() does.
>
> This also removes the redundant error checking of the
> mips_get_syscall_arg() return value (get_user() already zeroes the
> result if an argument from the stack can't be loaded).
>
> Reported-by: James Cowgill <James.Cowgill@imgtec.com>
> Fixes: 669c4092225f ("MIPS: Give __secure_computing() access to syscall arguments.")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> ---
> It would have been much simpler for MIPS arch code to just pass a NULL
> seccomp_data to secure_computing() so populate_seccomp_data() would take
> care of fetching arguments, as it did for MIPS prior to commit
> 669c4092225f ("MIPS: Give __secure_computing() access to syscall
> arguments."), but as that commit mentions it breaks samples/bpf/tracex5,
> which relies on sd being non-NULL at entry to __seccomp_filter().
>
> Arguably the samples/bpf/tracex5 test is flawed, at least for every arch
> except x86 (and now MIPS).

Weird. Yeah, that sample is broken. Allowing NULL sd is totally fine.
The point is that seccomp will use syscall_get_arguments() when it's
NULL (which is effectively what this is doing...)

The reason sd can be _non_-NULL is when an architecture has access to
the args in some way that might be faster than calling
syscall_get_arguments().

Regardless, I'm fine with this change. It should either be this or
reverting 669c4092225f, but it looks like kprobes of
__seccomp_filter() is desired on MIPS...

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  arch/mips/kernel/ptrace.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 6dd13641a418..1395654cfc8d 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -872,15 +872,13 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
>         if (unlikely(test_thread_flag(TIF_SECCOMP))) {
>                 int ret, i;
>                 struct seccomp_data sd;
> +               unsigned long args[6];
>
>                 sd.nr = syscall;
>                 sd.arch = syscall_get_arch();
> -               for (i = 0; i < 6; i++) {
> -                       unsigned long v, r;
> -
> -                       r = mips_get_syscall_arg(&v, current, regs, i);
> -                       sd.args[i] = r ? 0 : v;
> -               }
> +               syscall_get_arguments(current, regs, 0, 6, args);
> +               for (i = 0; i < 6; i++)
> +                       sd.args[i] = args[i];
>                 sd.instruction_pointer = KSTK_EIP(current);
>
>                 ret = __secure_computing(&sd);
> --
> 2.13.2
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 2/4] MIPS/ptrace: Pick up ptrace/seccomp changed syscalls

[Kees Cook]

On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> wrote:
> The MIPS syscall_trace_enter() allows the system call number to be
> altered or cancelled by a ptrace tracer, via the normal ptrace hook
> (PTRACE_SYSCALL) and changing the system call number register on entry,
> and similarly via seccomp (PTRACE_EVENT_SECCOMP when a seccomp filter
> returns SECCOMP_RET_TRACE).
>
> Be sure to update the syscall local variable if this happens, so that
> seccomp will filter the correct system call number if the normal ptrace
> hook changes it first, and so that if either the normal ptrace hook or
> seccomp change it the correct system call number is passed to the trace
> event.
>
> This won't have any effect until the next commit, which fixes ptrace to
> update thread_info::syscall.
>
> Fixes: c2d9f1775731 ("MIPS: Fix syscall_get_nr for the syscall exit tracing.")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Lars Persson <lars.persson@axis.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: linux-mips@linux-mips.org

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  arch/mips/kernel/ptrace.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 1395654cfc8d..be5d5fefcc7c 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -864,9 +864,11 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
>
>         current_thread_info()->syscall = syscall;
>
> -       if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> -           tracehook_report_syscall_entry(regs))
> -               return -1;
> +       if (test_thread_flag(TIF_SYSCALL_TRACE)) {
> +               if (tracehook_report_syscall_entry(regs))
> +                       return -1;
> +               syscall = current_thread_info()->syscall;
> +       }
>
>  #ifdef CONFIG_SECCOMP
>         if (unlikely(test_thread_flag(TIF_SECCOMP))) {
> @@ -884,6 +886,7 @@ asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall)
>                 ret = __secure_computing(&sd);
>                 if (ret == -1)
>                         return ret;
> +               syscall = current_thread_info()->syscall;
>         }
>  #endif
>
> --
> 2.13.2
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

[Kees Cook]

On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Add a PTRACE_SET_SYSCALL ptrace operation to allow the system call to be
> cancelled independently to the value of the v0 system call number
> register.
>
> This is needed for SECCOMP_RET_TRACE when the tracer wants to cancel the
> system call, since it has to set both the system call number to -1 and
> the chosen return value, both of which reside in the same register (v0).
> The tracer should set the return value first, followed by
> PTRACE_SET_SYSCALL to set the system call number to -1.
>
> That is in contrast to the normal ptrace syscall hook which triggers the
> tracer on both entry and exit, allowing the system call to be cancelled
> during the entry hook (setting system call number register to -1, or
> optionally using PTRACE_SET_SYSCALL), separately to setting the return
> value during the exit hook.
>
> Positive values (to change the syscall that should be executed instead
> of cancelling it entirely) are explicitly disallowed at the moment. The
> same thing can be done safely already by writing the v0 system call
> number register and the argument registers, and allowing
> thread_info::syscall to be changed to a different value independently of
> the v0 register would potentially allow seccomp or the syscall trace
> events to be fooled into thinking a different system call was being
> executed.

Wouldn't the sycall be reloaded, so no spoofing could occur?

Regardless, can you update
tools/testing/selftests/seccomp/seccomp_bpf.c to update or eliminate
the MIPS-only SYSCALL_NUM_RET_SHARE_REG special-case? (Or maybe it
needs to be further special-cased to split syscall-changing from
syscall-cancelling?)

-Kees

>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: linux-mips@linux-mips.org
> ---
>  arch/mips/include/uapi/asm/ptrace.h |  1 +
>  arch/mips/kernel/ptrace.c           | 11 +++++++++++
>  arch/mips/kernel/ptrace32.c         | 11 +++++++++++
>  3 files changed, 23 insertions(+)
>
> diff --git a/arch/mips/include/uapi/asm/ptrace.h b/arch/mips/include/uapi/asm/ptrace.h
> index 91a3d197ede3..23af103c4e8d 100644
> --- a/arch/mips/include/uapi/asm/ptrace.h
> +++ b/arch/mips/include/uapi/asm/ptrace.h
> @@ -58,6 +58,7 @@ struct pt_regs {
>
>  #define PTRACE_GET_THREAD_AREA 25
>  #define PTRACE_SET_THREAD_AREA 26
> +#define PTRACE_SET_SYSCALL     27
>
>  /* Calls to trace a 64bit program from a 32bit program.         */
>  #define PTRACE_PEEKTEXT_3264   0xc0
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 465fc5633e61..9bf31a990c6e 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -853,6 +853,17 @@ long arch_ptrace(struct task_struct *child, long request,
>                 ret = put_user(task_thread_info(child)->tp_value, datalp);
>                 break;
>
> +       case PTRACE_SET_SYSCALL:
> +               /*
> +                * This is currently only useful to cancel the syscall from a
> +                * seccomp RET_TRACE tracer.
> +                */
> +               if ((long)data >= 0)
> +                       return -EINVAL;
> +               task_thread_info(child)->syscall = -1;
> +               ret = 0;
> +               break;
> +
>         case PTRACE_GET_WATCH_REGS:
>                 ret = ptrace_get_watch_regs(child, addrp);
>                 break;
> diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
> index 2b9260f92ccd..cca76aec9c10 100644
> --- a/arch/mips/kernel/ptrace32.c
> +++ b/arch/mips/kernel/ptrace32.c
> @@ -287,6 +287,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>                                 (unsigned int __user *) (unsigned long) data);
>                 break;
>
> +       case PTRACE_SET_SYSCALL:
> +               /*
> +                * This is currently only useful to cancel the syscall from a
> +                * seccomp RET_TRACE tracer.
> +                */
> +               if ((long)data >= 0)
> +                       return -EINVAL;
> +               task_thread_info(child)->syscall = -1;
> +               ret = 0;
> +               break;
> +
>         case PTRACE_GET_THREAD_AREA_3264:
>                 ret = put_user(task_thread_info(child)->tp_value,
>                                 (unsigned long __user *) (unsigned long) data);
> --
> 2.13.2
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

[James Hogan]

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

On Fri, Aug 11, 2017 at 03:23:34PM -0700, Kees Cook wrote:
> On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> wrote:
> > Add a PTRACE_SET_SYSCALL ptrace operation to allow the system call to be
> > cancelled independently to the value of the v0 system call number
> > register.
> >
> > This is needed for SECCOMP_RET_TRACE when the tracer wants to cancel the
> > system call, since it has to set both the system call number to -1 and
> > the chosen return value, both of which reside in the same register (v0).
> > The tracer should set the return value first, followed by
> > PTRACE_SET_SYSCALL to set the system call number to -1.
> >
> > That is in contrast to the normal ptrace syscall hook which triggers the
> > tracer on both entry and exit, allowing the system call to be cancelled
> > during the entry hook (setting system call number register to -1, or
> > optionally using PTRACE_SET_SYSCALL), separately to setting the return
> > value during the exit hook.
> >
> > Positive values (to change the syscall that should be executed instead
> > of cancelling it entirely) are explicitly disallowed at the moment. The
> > same thing can be done safely already by writing the v0 system call
> > number register and the argument registers, and allowing
> > thread_info::syscall to be changed to a different value independently of
> > the v0 register would potentially allow seccomp or the syscall trace
> > events to be fooled into thinking a different system call was being
> > executed.
> 
> Wouldn't the sycall be reloaded, so no spoofing could occur?

The case I was thinking of was:
- PTRACE_POKEUSR v0 = __NR_some_disallowed_syscall
- PTRACE_SET_SYSCALL __NR_some_allowed_syscall

syscall_get_nr() will return __NR_some_allowed_syscall, so seccomp will
allow, but when syscall_trace_enter() returns to syscall_trace_entry in
arch/mips/kernel/scall32-o32.S, it will reload the syscall number from
v0 (i.e. __NR_some_disallowed_syscall).

> 
> Regardless, can you update
> tools/testing/selftests/seccomp/seccomp_bpf.c to update or eliminate
> the MIPS-only SYSCALL_NUM_RET_SHARE_REG special-case? (Or maybe it
> needs to be further special-cased to split syscall-changing from
> syscall-cancelling?)

Sure, i'll look into that,

Thanks for reviewing,

Cheers
James

> 
> -Kees
> 
> >
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: linux-mips@linux-mips.org
> > ---
> >  arch/mips/include/uapi/asm/ptrace.h |  1 +
> >  arch/mips/kernel/ptrace.c           | 11 +++++++++++
> >  arch/mips/kernel/ptrace32.c         | 11 +++++++++++
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/arch/mips/include/uapi/asm/ptrace.h b/arch/mips/include/uapi/asm/ptrace.h
> > index 91a3d197ede3..23af103c4e8d 100644
> > --- a/arch/mips/include/uapi/asm/ptrace.h
> > +++ b/arch/mips/include/uapi/asm/ptrace.h
> > @@ -58,6 +58,7 @@ struct pt_regs {
> >
> >  #define PTRACE_GET_THREAD_AREA 25
> >  #define PTRACE_SET_THREAD_AREA 26
> > +#define PTRACE_SET_SYSCALL     27
> >
> >  /* Calls to trace a 64bit program from a 32bit program.         */
> >  #define PTRACE_PEEKTEXT_3264   0xc0
> > diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> > index 465fc5633e61..9bf31a990c6e 100644
> > --- a/arch/mips/kernel/ptrace.c
> > +++ b/arch/mips/kernel/ptrace.c
> > @@ -853,6 +853,17 @@ long arch_ptrace(struct task_struct *child, long request,
> >                 ret = put_user(task_thread_info(child)->tp_value, datalp);
> >                 break;
> >
> > +       case PTRACE_SET_SYSCALL:
> > +               /*
> > +                * This is currently only useful to cancel the syscall from a
> > +                * seccomp RET_TRACE tracer.
> > +                */
> > +               if ((long)data >= 0)
> > +                       return -EINVAL;
> > +               task_thread_info(child)->syscall = -1;
> > +               ret = 0;
> > +               break;
> > +
> >         case PTRACE_GET_WATCH_REGS:
> >                 ret = ptrace_get_watch_regs(child, addrp);
> >                 break;
> > diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
> > index 2b9260f92ccd..cca76aec9c10 100644
> > --- a/arch/mips/kernel/ptrace32.c
> > +++ b/arch/mips/kernel/ptrace32.c
> > @@ -287,6 +287,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> >                                 (unsigned int __user *) (unsigned long) data);
> >                 break;
> >
> > +       case PTRACE_SET_SYSCALL:
> > +               /*
> > +                * This is currently only useful to cancel the syscall from a
> > +                * seccomp RET_TRACE tracer.
> > +                */
> > +               if ((long)data >= 0)
> > +                       return -EINVAL;
> > +               task_thread_info(child)->syscall = -1;
> > +               ret = 0;
> > +               break;
> > +
> >         case PTRACE_GET_THREAD_AREA_3264:
> >                 ret = put_user(task_thread_info(child)->tp_value,
> >                                 (unsigned long __user *) (unsigned long) data);
> > --
> > 2.13.2
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

[Kees Cook]

On Mon, Aug 14, 2017 at 2:41 AM, James Hogan <james.hogan@imgtec.com> wrote:
> On Fri, Aug 11, 2017 at 03:23:34PM -0700, Kees Cook wrote:
>> On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> wrote:
>> > Add a PTRACE_SET_SYSCALL ptrace operation to allow the system call to be
>> > cancelled independently to the value of the v0 system call number
>> > register.
>> >
>> > This is needed for SECCOMP_RET_TRACE when the tracer wants to cancel the
>> > system call, since it has to set both the system call number to -1 and
>> > the chosen return value, both of which reside in the same register (v0).
>> > The tracer should set the return value first, followed by
>> > PTRACE_SET_SYSCALL to set the system call number to -1.
>> >
>> > That is in contrast to the normal ptrace syscall hook which triggers the
>> > tracer on both entry and exit, allowing the system call to be cancelled
>> > during the entry hook (setting system call number register to -1, or
>> > optionally using PTRACE_SET_SYSCALL), separately to setting the return
>> > value during the exit hook.
>> >
>> > Positive values (to change the syscall that should be executed instead
>> > of cancelling it entirely) are explicitly disallowed at the moment. The
>> > same thing can be done safely already by writing the v0 system call
>> > number register and the argument registers, and allowing
>> > thread_info::syscall to be changed to a different value independently of
>> > the v0 register would potentially allow seccomp or the syscall trace
>> > events to be fooled into thinking a different system call was being
>> > executed.
>>
>> Wouldn't the sycall be reloaded, so no spoofing could occur?
>
> The case I was thinking of was:
> - PTRACE_POKEUSR v0 = __NR_some_disallowed_syscall
> - PTRACE_SET_SYSCALL __NR_some_allowed_syscall
>
> syscall_get_nr() will return __NR_some_allowed_syscall, so seccomp will
> allow, but when syscall_trace_enter() returns to syscall_trace_entry in
> arch/mips/kernel/scall32-o32.S, it will reload the syscall number from
> v0 (i.e. __NR_some_disallowed_syscall).

IIUC, the issue is that v0 holds syscall on entry and syscall return
on exit. Isn't it possible to rework all the entry logic to examine
only thread_info->syscall and ignore v0 during the ptrace and seccomp
events? i.e. SET_SYSCALL can modify ti->syscall, and only if it goes
to -1 only then will v0 be examined for a result? (If I'm reading
scall32-o32.S, I think this means loading the new syscall from
thread_info instead of registers after syscall_trace_enter.)

If that is possible, it doesn't have to happen in this patch,
obviously. Incremental is fine. :)

>> Regardless, can you update
>> tools/testing/selftests/seccomp/seccomp_bpf.c to update or eliminate
>> the MIPS-only SYSCALL_NUM_RET_SHARE_REG special-case? (Or maybe it
>> needs to be further special-cased to split syscall-changing from
>> syscall-cancelling?)
>
> Sure, i'll look into that,
>
> Thanks for reviewing,

Sure thing, thanks!

-Kees

-- 
Kees Cook
Pixel Security