linux-kernel - Re: [PATCH v2 0/9] ARM: remove set

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: <CAK8P3a3ZFvXPbKRJzjh5Gj08TcQYNV+MoSTLXjTOnkyVb4WuNA@mail.gmail.com>
Date:   Fri, 25 Sep 2020 17:30:38 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [PATCH v2 0/9] ARM: remove set_fs callers and implementation

On Fri, Sep 25, 2020 at 4:08 PM Arnd Bergmann <arnd@...db.de> wrote:
> On Sat, Sep 19, 2020 at 10:19 AM Russell King - ARM Linux admin
> <linux@...linux.org.uk> wrote:
> >
> > On Fri, Sep 18, 2020 at 02:46:15PM +0200, Arnd Bergmann wrote:
> > > Hi Christoph, Russell,
> > >
> > > Here is an updated series for removing set_fs() from arch/arm,
> > > based on the previous feedback.
> > >
> > > I have tested the oabi-compat changes using the LTP tests for the three
> > > modified syscalls using an Armv7 kernel and a Debian 5 OABI user space,
> > > and I have lightly tested the get_kernel_nofault infrastructure by
> > > loading the test_lockup.ko module after setting CONFIG_DEBUG_SPINLOCK.
> >
> > I'm not too keen on always saving the syscall number, but for the gain
> > of getting rid of set_fs() I think it's worth it. However...
> >
> > I think there are some things to check - what value do you end up
> > with as the first number in /proc/self/syscall when you do:
> >
> > strace cat /proc/self/syscall
> >
> > ?
>
> > It should be 3, not 0x900003. I suspect you're getting the latter
> > with these changes.  IIRC, task_thread_info(task)->syscall needs to
> > be the value _without_ the offset, otherwise tracing will break.
>
> It seems broken in different ways, depending on the combination
> of kernel and userland:
>
> 1. EABI armv5-versatile kernel, EABI Debian 5:
> $ cat /proc/self/syscall
> 0 0x1500000000003 0x1500000000400 0x1500000000400 0x60000013c7800480
> 0xc0008668c0112f8c 0xc0112d14c68e1f68 0xbeab06f8 0xb6e80d4c
> $ strace -f cat /proc/self/syscall
> execve("/bin/cat", ["cat", "/proc/self/syscall"], [/* 16 vars */]) =
> -1 EINTR (Interrupted system call)
> dup(2)                                  = -1 EINTR (Interrupted system call)
> write(2, "strace: exec: Interrupted system "..., 38) = -1 EINTR
> (Interrupted system call)
> exit_group(1)                           = ?

Both the missing number and the broken strace are fixed with

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 610e32273c81..2c0bde14fba6 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -226,7 +226,8 @@ ENTRY(vector_swi)
         * get the old ABI syscall table address.
         */
        bics    r10, r10, #0xff000000
-       str     r10, [tsk, #TI_SYSCALL]
+       strne   r10, [tsk, #TI_SYSCALL]
+       streq   scno, [tsk, #TI_SYSCALL]
        eorne   scno, r10, #__NR_OABI_SYSCALL_BASE
        ldrne   tbl, =sys_oabi_call_table
 #elif !defined(CONFIG_AEABI)

It was already working with CONFIG_AEABI=y and
CONFIG_OABI_COMPAT=n

> 2. EABI kernel, OABI Debian 5:
> $ cat /proc/self/syscall
> 3 0x1500000000003 0x13ccc00000400 0x1500000000400 0x60000013c7800480
> 0xc0008de0c0112f8c 0xc0112d14c7313f68 0xbeed27d0 0xb6eab324
> $ strace cat /proc/self/syscall
> execve("/bin/cat", ["cat", "/proc/self/syscall"], [/* 16 vars */]) = -1090648236
> --- SIGILL (Illegal instruction) @ 0 (0) ---
> +++ killed by SIGILL +++

This was caused by me after all, here is my fix:

--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -25,6 +25,7 @@
 #include <linux/tracehook.h>
 #include <linux/unistd.h>

+#include <asm/syscall.h>
 #include <asm/traps.h>

 #define CREATE_TRACE_POINTS
@@ -898,11 +899,11 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs)
                return -1;
 #else
        /* XXX: remove this once OABI gets fixed */
-       secure_computing_strict(current_thread_info()->syscall);
+       secure_computing_strict(syscall_get_nr(current, regs));
 #endif

        /* Tracer or seccomp may have changed syscall. */
-       scno = current_thread_info()->syscall;
+       scno = syscall_get_nr(current, regs);

        if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
                trace_sys_enter(regs, scno);

> 3. OABI kernel, OABI Debian 5:
>  cat /proc/self/syscall
> 9437187 0x1500000000003 0x13ccc00000400 0x1500000000400 0x100060000013
> 0x15000c72cff6c 0xc72cfe9000000000 0xbece27d0 0xb6f2f324

This one is fixed by

--- a/arch/arm/include/asm/syscall.h
+++ b/arch/arm/include/asm/syscall.h
@@ -22,7 +22,7 @@ extern const unsigned long sys_call_table[];
 static inline int syscall_get_nr(struct task_struct *task,
                                 struct pt_regs *regs)
 {
-       if (!IS_ENABLED(CONFIG_OABI_COMPAT))
+       if (IS_ENABLED(CONFIG_AEABI) && !IS_ENABLED(CONFIG_OABI_COMPAT))
                return task_thread_info(task)->syscall;

        return task_thread_info(task)->syscall & ~__NR_OABI_SYSCALL_BASE;



I'll send an updated patch once I've addressed Christoph's comments.

      Arnd