[PATCH 0/6] syscall_trace fixes and cleanups

[PATCH 0/6] syscall_trace fixes and cleanups

[Will Deacon]

Hello all,

This series makes a number of fixes and cleanups to the system call
tracing code on ARM, as well as fixing the interactions with audit.

The first two patches should probably go into -stable whilst the rest
can wait for later. Note that this will conflict horribly with the
tracehook code added recently (sitting in -next) and also with the
additional tracehook patch sitting in the patch system (7375/1).

I'd like to get the two stable patches in ASAP and then help with the
conflict resolution for the merge window (although other ideas welcome).
I can rebase the rest of the series as required.

I lightly tested the audit stuff and gave strace a decent run tracing
through forks.

All comments welcome,

Will


Will Deacon (6):
  ARM: audit: fix treatment of saved ip register during syscall tracing
  ARM: audit: use only AUDIT_ARCH_ARM regardless of endianness
  audit: arm: only allow syscall auditing for pure EABI userspace
  ARM: entry: don't bother with syscall tracing on ret_from_fork path
  ARM: audit: move syscall auditing until after ptrace SIGTRAP handling
  ARM: ptrace: provide separate functions for tracing syscall
    {entry,exit}

 arch/arm/kernel/entry-common.S |   20 +++++-----------
 arch/arm/kernel/ptrace.c       |   49 +++++++++++++++++++++++----------------
 init/Kconfig                   |    2 +-
 3 files changed, 36 insertions(+), 35 deletions(-)

-- 
1.7.4.1

[PATCH 1/6] ARM: audit: fix treatment of saved ip register during syscall tracing

[Will Deacon]

The ARM audit code incorrectly uses the saved application ip register
value to infer syscall entry or exit. Additionally, the saved value will
be clobbered if the current task is not being traced, which can lead to
libc corruption if ip is live (apparently glibc uses it for the TLS
pointer).

This patch fixes the syscall tracing code so that the why parameter is
used to infer the syscall direction and the saved ip is only updated if
we know that we will be signalling a ptrace trap.

Cc: Eric Paris <eparis@redhat.com>
Reported-and-tested-by: Jon Masters <jcm@jonmasters.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 80abafb..d8dbe9c 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 {
 	unsigned long ip;
 
-	/*
-	 * Save IP.  IP is used to denote syscall entry/exit:
-	 *  IP = 0 -> entry, = 1 -> exit
-	 */
-	ip = regs->ARM_ip;
-	regs->ARM_ip = why;
-
-	if (!ip)
+	if (why)
 		audit_syscall_exit(regs);
 	else
 		audit_syscall_entry(AUDIT_ARCH_NR, scno, regs->ARM_r0,
@@ -936,6 +929,13 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 
 	current_thread_info()->syscall = scno;
 
+	/*
+	 * IP is used to denote syscall entry/exit:
+	 * IP = 0 -> entry, =1 -> exit
+	 */
+	ip = regs->ARM_ip;
+	regs->ARM_ip = why;
+
 	/* the 0x80 provides a way for the tracing parent to distinguish
 	   between a syscall stop and SIGTRAP delivery */
 	ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
-- 
1.7.4.1

[PATCH 2/6] ARM: audit: use only AUDIT_ARCH_ARM regardless of endianness

[Will Deacon]

The machine endianness has no direct correspondence to the syscall ABI,
so use only AUDIT_ARCH_ARM when identifying the ABI to the audit tools
in userspace.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index d8dbe9c..9650c14 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -906,12 +906,6 @@ long arch_ptrace(struct task_struct *child, long request,
 	return ret;
 }
 
-#ifdef __ARMEB__
-#define AUDIT_ARCH_NR AUDIT_ARCH_ARMEB
-#else
-#define AUDIT_ARCH_NR AUDIT_ARCH_ARM
-#endif
-
 asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 {
 	unsigned long ip;
@@ -919,7 +913,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 	if (why)
 		audit_syscall_exit(regs);
 	else
-		audit_syscall_entry(AUDIT_ARCH_NR, scno, regs->ARM_r0,
+		audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0,
 				    regs->ARM_r1, regs->ARM_r2, regs->ARM_r3);
 
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-- 
1.7.4.1

[PATCH 3/6] audit: arm: only allow syscall auditing for pure EABI userspace

[Will Deacon]

The audit tools support only EABI userspace and, since there are no
AUDIT_ARCH_* defines for the ARM OABI, it makes sense to allow syscall
auditing on ARM only for EABI at the moment.

Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 init/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 6cfd71d..5e83be4 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -355,7 +355,7 @@ config AUDIT
 
 config AUDITSYSCALL
 	bool "Enable system-call auditing support"
-	depends on AUDIT && (X86 || PPC || S390 || IA64 || UML || SPARC64 || SUPERH || ARM)
+	depends on AUDIT && (X86 || PPC || S390 || IA64 || UML || SPARC64 || SUPERH || (ARM && AEABI && !OABI_COMPAT))
 	default y if SECURITY_SELINUX
 	help
 	  Enable low-overhead system-call auditing infrastructure that
-- 
1.7.4.1

[PATCH 4/6] ARM: entry: don't bother with syscall tracing on ret_from_fork path

[Will Deacon]

ret_from_fork is setup for a freshly spawned child task via copy_thread,
called from copy_process. The latter function clears TIF_SYSCALL_TRACE
and also resets the child task's audit_context to NULL, meaning that
there is little point invoking the system call tracing routines.
Furthermore, getting hold of the syscall number is a complete pain and
it looks like the current code doesn't even bother.

This patch removes the syscall tracing checks from ret_from_fork.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/entry-common.S |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 54ee265..93962cc 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -91,13 +91,7 @@ ENDPROC(ret_to_user)
 ENTRY(ret_from_fork)
 	bl	schedule_tail
 	get_thread_info tsk
-	ldr	r1, [tsk, #TI_FLAGS]		@ check for syscall tracing
 	mov	why, #1
-	tst	r1, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
-	beq	ret_slow_syscall
-	mov	r1, sp
-	mov	r0, #1				@ trace exit [IP = 1]
-	bl	syscall_trace
 	b	ret_slow_syscall
 ENDPROC(ret_from_fork)
 
-- 
1.7.4.1

[PATCH 5/6] ARM: audit: move syscall auditing until after ptrace SIGTRAP handling

[Will Deacon]

When auditing system calls on ARM, the audit code is called before
notifying the parent process in the case that the current task is being
ptraced. At this point, the parent (debugger) may choose to change the
system call being issued via the SET_SYSCALL ptrace request, causing
the wrong system call to be reported to the audit tools.

This patch moves the audit calls after the ptrace SIGTRAP handling code
in the syscall tracing implementation.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 9650c14..e7d687e 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -910,16 +910,10 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 {
 	unsigned long ip;
 
-	if (why)
-		audit_syscall_exit(regs);
-	else
-		audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0,
-				    regs->ARM_r1, regs->ARM_r2, regs->ARM_r3);
-
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-		return scno;
+		goto out_no_trace;
 	if (!(current->ptrace & PT_PTRACED))
-		return scno;
+		goto out_no_trace;
 
 	current_thread_info()->syscall = scno;
 
@@ -943,7 +937,15 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 		send_sig(current->exit_code, current, 1);
 		current->exit_code = 0;
 	}
+
 	regs->ARM_ip = ip;
+	scno = current_thread_info()->syscall;
 
-	return current_thread_info()->syscall;
+out_no_trace:
+	if (why)
+		audit_syscall_exit(regs);
+	else
+		audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0,
+				    regs->ARM_r1, regs->ARM_r2, regs->ARM_r3);
+	return scno;
 }
-- 
1.7.4.1

[PATCH 6/6] ARM: ptrace: provide separate functions for tracing syscall {entry, exit}

[Will Deacon]

The syscall_trace on ARM takes a `why' parameter to indicate whether or
not we are entering or exiting a system call. This can be confusing for
people looking at the code since (a) it conflicts with the why register
alias in the entry assembly code and (b) it is not immediately clear
what it represents.

This patch splits up the syscall_trace function into separate wrappers
for syscall entry and exit, allowing the low-level syscall handling
code to branch to the appropriate function.

Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/entry-common.S |   14 ++++++--------
 arch/arm/kernel/ptrace.c       |   37 +++++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 93962cc..d0b97ec 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -466,10 +466,9 @@ ENDPROC(vector_swi)
 	 * context switches, and waiting for our parent to respond.
 	 */
 __sys_trace:
-	mov	r2, scno
-	add	r1, sp, #S_OFF
-	mov	r0, #0				@ trace entry [IP = 0]
-	bl	syscall_trace
+	mov	r1, scno
+	add	r0, sp, #S_OFF
+	bl	syscall_trace_enter
 
 	adr	lr, BSYM(__sys_trace_return)	@ return address
 	mov	scno, r0			@ syscall number (possibly new)
@@ -481,10 +480,9 @@ __sys_trace:
 
 __sys_trace_return:
 	str	r0, [sp, #S_R0 + S_OFF]!	@ save returned r0
-	mov	r2, scno
-	mov	r1, sp
-	mov	r0, #1				@ trace exit [IP = 1]
-	bl	syscall_trace
+	mov	r1, scno
+	mov	r0, sp
+	bl	syscall_trace_exit
 	b	ret_slow_syscall
 
 	.align	5
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index e7d687e..3fa40c3 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -906,14 +906,20 @@ long arch_ptrace(struct task_struct *child, long request,
 	return ret;
 }
 
-asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
+enum ptrace_syscall_dir {
+	PTRACE_SYSCALL_ENTER = 0,
+	PTRACE_SYSCALL_EXIT,
+};
+
+static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
+				enum ptrace_syscall_dir dir)
 {
 	unsigned long ip;
 
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-		goto out_no_trace;
+		return scno;
 	if (!(current->ptrace & PT_PTRACED))
-		goto out_no_trace;
+		return scno;
 
 	current_thread_info()->syscall = scno;
 
@@ -922,7 +928,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 	 * IP = 0 -> entry, =1 -> exit
 	 */
 	ip = regs->ARM_ip;
-	regs->ARM_ip = why;
+	regs->ARM_ip = dir;
 
 	/* the 0x80 provides a way for the tracing parent to distinguish
 	   between a syscall stop and SIGTRAP delivery */
@@ -939,13 +945,20 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 	}
 
 	regs->ARM_ip = ip;
-	scno = current_thread_info()->syscall;
+	return current_thread_info()->syscall;
+}
 
-out_no_trace:
-	if (why)
-		audit_syscall_exit(regs);
-	else
-		audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0,
-				    regs->ARM_r1, regs->ARM_r2, regs->ARM_r3);
-	return scno;
+asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
+{
+	int ret = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
+			    regs->ARM_r2, regs->ARM_r3);
+	return ret;
+}
+
+asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
+{
+	int ret = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+	audit_syscall_exit(regs);
+	return ret;
 }
-- 
1.7.4.1