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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jKmDKZ5DPDC87ZqHtmRVac7t3kQ3nqY4YzargZZPQmW6Q@mail.gmail.com>
Date:   Fri, 11 Aug 2017 15:17:11 -0700
From:   Kees Cook <keescook@...omium.org>
To:     James Hogan <james.hogan@...tec.com>
Cc:     Linux MIPS Mailing List <linux-mips@...ux-mips.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        David Daney <david.daney@...ium.com>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/4] MIPS/seccomp: Fix indirect syscall args

On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@...tec.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@...tec.com>
> Fixes: 669c4092225f ("MIPS: Give __secure_computing() access to syscall arguments.")
> Signed-off-by: James Hogan <james.hogan@...tec.com>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: David Daney <david.daney@...ium.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Andy Lutomirski <luto@...capital.net>
> Cc: Will Drewry <wad@...omium.org>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: Alexei Starovoitov <ast@...nel.org>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: netdev@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-mips@...ux-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@...omium.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