ARM: Call syscall_trace_exit even when system call skipped

On Thu, Mar 15, 2018 at 01:14:31PM -0700, Kees Cook wrote:

On Thu, Mar 15, 2018 at 3:38 AM,  [off-list ref] wrote:

quoted

On 15 March 2018 00:45:01 Kees Cook [off-list ref] wrote:

quoted

quoted

quoted

--- 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 <redacted>
Tested-by: Kees Cook <redacted>

Tested-by: Eugene Syromyatnikov <redacted>

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