From mboxrd@z Thu Jan 1 00:00:00 1970 From: evgsyr@gmail.com (Eugene Syromyatnikov) Date: Sun, 30 Sep 2018 01:03:39 +0200 Subject: ARM: Call syscall_trace_exit even when system call skipped In-Reply-To: References: <20180203152112.2449-1-T.E.Baldwin99@members.leeds.ac.uk> <20180313235850.GA18373@altlinux.org> <162293d1df0.2772.8578bc6afb023d3c4d0bd68fb35b28aa@members.leeds.ac.uk> Message-ID: <20180929230337.GA3640@obsidian> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Mar 15, 2018 at 01:14:31PM -0700, Kees Cook wrote: > On Thu, Mar 15, 2018 at 3:38 AM, wrote: > > On 15 March 2018 00:45:01 Kees Cook wrote: > > > >>>> --- a/arch/arm/kernel/entry-common.S > >>>> +++ b/arch/arm/kernel/entry-common.S > >>>> @@ -288,16 +288,15 @@ __sys_trace: > >>>> cmp scno, #-1 @ skip the syscall? > >>>> bne 2b > >>>> add sp, sp, #S_OFF @ restore stack > >>>> - b ret_slow_syscall > >>>> > >>>> -__sys_trace_return: > >>>> - str r0, [sp, #S_R0 + S_OFF]! @ save returned r0 > >>>> +__sys_trace_return_nosave: > >>>> + enable_irq_notrace > >> > >> > >> Why is __sys_trace_return_nosave the correct destination here? The > >> original handle set up for lr a few lines above is for > >> __sys_trace_return. It's not clear to me why this change is made? > > > > > > __sys_trace_return stores the current r0 value on the stack which will > > reloaded on exit to user mode. However if skipping a system call r0 is -1 > > and storing it would destroy the users r0 value, unlike the case where the > > system call is made and r0 is the return value. > > > > The enabling of interrupts is redundant for this purpose, the reuse of code > > is a size optimization. > > Ah right. Cool, thanks! > > Reviewed-by: Kees Cook > Tested-by: Kees Cook Tested-by: Eugene Syromyatnikov Have checked the patch on an Orange Pi Zero board (Allwinner sun8i), kernel 4.14.73-sunxi armv7l (Armbian). Before the change, seccomp_ret_trap test from ldv/SECCOMP_RET_TRAP strace branch was failing with incorrect decoding of the second chdir() call: ---8<--- FAIL: seccomp_ret_trap.gen ========================== --- .././seccomp_ret_trap.expected 2018-09-29 20:16:27.675951746 +0000 +++ log 2018-09-29 21:09:49.669414847 +0000 @@ -1,3 +1,3 @@ chdir(".") = 0 -chdir("./") = 0 +chdir(NULL) = 0 +++ exited with 0 +++ seccomp_ret_trap.gen.test: failed test: ../../strace -a11 -esignal=none -echdir ../seccomp_ret_trap output mismatch FAIL seccomp_ret_trap.gen.test (exit status: 1) --->8--- After the change, test passes and chdir() call after the filtered nanosleep() call is decoded correctly: ---8<--- $ ./strace -echdir,nanosleep,exit_group tests/seccomp_ret_trap chdir(".") = 0 nanosleep({tv_sec=0, tv_nsec=0}, NULL) = 5143188 --- SIGSYS {si_signo=SIGSYS, si_code=SYS_SECCOMP, si_call_addr=0xb6f6df42, si_syscall=__NR_nanosleep, si_arch=AUDIT_ARCH_ARM} --- chdir("./") = 0 exit_group(0) = ? +++ exited with 0 +++ --->8--- > I assume Dmitry can add a Tested-by too? > > This seems good to go into the ARM patch tracker... > > -Kees > > -- > Kees Cook > Pixel Security