Netdev List
 help / color / mirror / Atom feed
From: James Hogan <james.hogan@imgtec.com>
To: <linux-mips@linux-mips.org>
Cc: <linux-kernel@vger.kernel.org>,
	James Hogan <james.hogan@imgtec.com>,
	Ralf Baechle <ralf@linux-mips.org>,
	David Daney <david.daney@cavium.com>,
	Kees Cook <keescook@chromium.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Will Drewry <wad@chromium.org>, Oleg Nesterov <oleg@redhat.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>, <netdev@vger.kernel.org>
Subject: [PATCH 1/4] MIPS/seccomp: Fix indirect syscall args
Date: Fri, 11 Aug 2017 21:56:50 +0100	[thread overview]
Message-ID: <20170811205653.21873-2-james.hogan@imgtec.com> (raw)
In-Reply-To: <20170811205653.21873-1-james.hogan@imgtec.com>

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

  reply	other threads:[~2017-08-11 20:56 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-11 20:56 [PATCH 0/4] MIPS: syscall tracing fixes James Hogan
2017-08-11 20:56 ` James Hogan [this message]
2017-08-11 22:17   ` [PATCH 1/4] MIPS/seccomp: Fix indirect syscall args Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170811205653.21873-2-james.hogan@imgtec.com \
    --to=james.hogan@imgtec.com \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=david.daney@cavium.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    --cc=luto@amacapital.net \
    --cc=netdev@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=ralf@linux-mips.org \
    --cc=wad@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox