| Message ID | 20201030154919.1246645-4-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: remove set_fs callers and implementation | expand |
On Fri, Oct 30, 2020 at 04:49:14PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The system call number is used in a a couple of places, in particular > ptrace, seccomp and /proc/<pid>/syscall. > > The last one apparently never worked reliably on ARM for tasks > that are not currently getting traced. > > Storing the syscall number in the normal entry path makes it work, > as well as allowing us to see if the current system call is for > OABI compat mode, which is the next thing I want to hook into. I'm not sure this patch is correct. Tracing the existing code for OABI: asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno) { current_thread_info()->syscall = scno; /* Legacy ABI only. */ USER( ldr scno, [saved_pc, #-4] ) @ get SWI instruction bic scno, scno, #0xff000000 @ mask off SWI op-code eor scno, scno, #__NR_SYSCALL_BASE @ check OS number tst r10, #_TIF_SYSCALL_WORK @ are we tracing syscalls? bne __sys_trace __sys_trace: mov r1, scno add r0, sp, #S_OFF bl syscall_trace_enter So, thread_info->syscall does not include __NR_SYSCALL_BASE. The reason for this is the code that makes use of that via syscall_get_nr(). kernel/trace/trace_syscalls.c: syscall_nr = trace_get_syscall_nr(current, regs); if (syscall_nr < 0 || syscall_nr >= NR_syscalls) return; and NR_syscalls is the number of syscalls, which doesn't include the __NR_SYSCALL_BASE offset. So, I think this patch actually breaks OABI.
On Fri, Oct 30, 2020 at 5:53 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Fri, Oct 30, 2020 at 04:49:14PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > The system call number is used in a a couple of places, in particular > > ptrace, seccomp and /proc/<pid>/syscall. > > > > The last one apparently never worked reliably on ARM for tasks > > that are not currently getting traced. > > > > Storing the syscall number in the normal entry path makes it work, > > as well as allowing us to see if the current system call is for > > OABI compat mode, which is the next thing I want to hook into. > > I'm not sure this patch is correct. I'm not following where you still see a mismatch, I was hoping I had fixed them all after your previous review :( The thread_info->syscall entry should now consistently contain __NR_SYSCALL_BASE on an EABI kernel, and all users of that should consistently mask it out. > Tracing the existing code for OABI: > > asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno) > { > current_thread_info()->syscall = scno; This no longer stores to current_thread_info()->syscall but instead reads the number from syscall_get_nr(). > /* Legacy ABI only. */ > USER( ldr scno, [saved_pc, #-4] ) @ get SWI instruction > bic scno, scno, #0xff000000 @ mask off SWI op-code > eor scno, scno, #__NR_SYSCALL_BASE @ check OS number > tst r10, #_TIF_SYSCALL_WORK @ are we tracing syscalls? > bne __sys_trace > > __sys_trace: > mov r1, scno > add r0, sp, #S_OFF > bl syscall_trace_enter > > So, thread_info->syscall does not include __NR_SYSCALL_BASE. The > reason for this is the code that makes use of that via syscall_get_nr(). > kernel/trace/trace_syscalls.c: On both CONFIG_OABI_COMPAT and on !CONFIG_AEABI kernels, I store the value before masking out __NR_SYSCALL_BASE after my change. For EABI-only kernels there is no need for the mask. > syscall_nr = trace_get_syscall_nr(current, regs); > if (syscall_nr < 0 || syscall_nr >= NR_syscalls) > return; > > and NR_syscalls is the number of syscalls, which doesn't include the > __NR_SYSCALL_BASE offset. > > So, I think this patch actually breaks OABI. The value returned from trace_get_syscall_nr() is always in the 0...NR_syscalls range without the __NR_SYSCALL_BASE for a valid syscall. because of the added static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs) { - return task_thread_info(task)->syscall; + return task_thread_info(task)->syscall & ~__NR_OABI_SYSCALL_BASE; } (plus the corresponding logic for OABI_COMPAT. Which of the above do you think I got wrong? Arnd
diff --git a/arch/arm/include/asm/syscall.h b/arch/arm/include/asm/syscall.h index fd02761ba06c..89898497edd6 100644 --- a/arch/arm/include/asm/syscall.h +++ b/arch/arm/include/asm/syscall.h @@ -22,7 +22,10 @@ extern const unsigned long sys_call_table[]; static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs) { - return task_thread_info(task)->syscall; + 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; } static inline void syscall_rollback(struct task_struct *task, diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c index a1570c8bab25..97af6735172b 100644 --- a/arch/arm/kernel/asm-offsets.c +++ b/arch/arm/kernel/asm-offsets.c @@ -46,6 +46,7 @@ int main(void) DEFINE(TI_CPU, offsetof(struct thread_info, cpu)); DEFINE(TI_CPU_DOMAIN, offsetof(struct thread_info, cpu_domain)); DEFINE(TI_CPU_SAVE, offsetof(struct thread_info, cpu_context)); + DEFINE(TI_SYSCALL, offsetof(struct thread_info, syscall)); DEFINE(TI_USED_CP, offsetof(struct thread_info, used_cp)); DEFINE(TI_TP_VALUE, offsetof(struct thread_info, tp_value)); DEFINE(TI_FPSTATE, offsetof(struct thread_info, fpstate)); diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S index 271cb8a1eba1..9a76467bbb47 100644 --- a/arch/arm/kernel/entry-common.S +++ b/arch/arm/kernel/entry-common.S @@ -223,6 +223,7 @@ ENTRY(vector_swi) /* saved_psr and saved_pc are now dead */ uaccess_disable tbl + get_thread_info tsk adr tbl, sys_call_table @ load syscall table pointer @@ -234,13 +235,17 @@ ENTRY(vector_swi) * get the old ABI syscall table address. */ bics r10, r10, #0xff000000 + 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) bic scno, scno, #0xff000000 @ mask off SWI op-code + str scno, [tsk, #TI_SYSCALL] eor scno, scno, #__NR_SYSCALL_BASE @ check OS number +#else + str scno, [tsk, #TI_SYSCALL] #endif - get_thread_info tsk /* * Reload the registers that may have been corrupted on entry to * the syscall assembly (by tracing or context tracking.) @@ -285,7 +290,6 @@ ENDPROC(vector_swi) * context switches, and waiting for our parent to respond. */ __sys_trace: - mov r1, scno add r0, sp, #S_OFF bl syscall_trace_enter mov scno, r0 diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index 2771e682220b..683edb8b627d 100644 --- 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 @@ -885,9 +886,9 @@ static void tracehook_report_syscall(struct pt_regs *regs, regs->ARM_ip = ip; } -asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno) +asmlinkage int syscall_trace_enter(struct pt_regs *regs) { - current_thread_info()->syscall = scno; + int scno; if (test_thread_flag(TIF_SYSCALL_TRACE)) tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER); @@ -898,11 +899,11 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno) 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);