[PATCH] ARM: pass syscall return value to sys_exit tracepoint

[Will Deacon]

On Tue, Dec 04, 2012 at 10:40:38AM +0000, Will Deacon wrote:
> On Tue, Dec 04, 2012 at 10:38:09AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Dec 04, 2012 at 09:48:28AM +0000, Gabbasov, Andrew wrote:
> > > The other registers in "regs" structure are not saved around ptrace_syscall_trace,
> > > and both audit_syscall_exit and trace_sys_exit get the register values, potentially
> > > changed by a debugger. Does it make sense to save the isolated return value
> > > for trace_sys_exit call only and not to save other registers, passed, for example,
> > > to audit_syscall_exit function that takes the return value from regs structure?
> > > Isn't it a reasonable assumption that a debugger will preserve important
> > > register values (or intentionally change them for some purpose) in case
> > > of syscall_exit, as we rely on this for syscall_enter case?
> > 
> > Actually, why are we doing things in a different order to x86?  If we
> > assume that x86 has this stuff correctly ordered, then shouldn't we
> > be following the sequence presented there?
> > 
> > x86 on entry does:
> > - secure_computing
> > - tracehook_report_syscall_entry
> > - trace_sys_enter
> > - audit_syscall_entry
> > and on exit:
> > - audit_syscall_exit
> > - trace_sys_exit
> > - tracehook_report_syscall_exit
> > 
> > We appear to be doing the _exact_ reverse ordering in our syscall exit
> > path - why is that?
> 
> 
> I've just been looking at that and the problem stems around having ->syscall
> set for the current thread before calling the sys_exit tracepoint. The audit
> code should definitely be moved earlier.
> 
> Working on a patch...

Ok, how about something like the following?

Will

--->8

commit 0109b339755b29f92733c1f257911615fc231727
Author: Will Deacon <will.deacon at arm.com>
Date:   Tue Dec 4 11:47:14 2012 +0000

    ARM: syscall: rework ordering in syscall_trace_exit
    
    syscall_trace_exit is currently doing things back-to-front; invoking
    the audit hook *after* signalling the debugger, which presents an
    opportunity for the registers to be re-written by userspace in order to
    bypass auditing constaints.
    
    This patch fixes the ordering by moving the audit code first and the
    tracehook code last. On the face of it, it looks like
    current_thread_info()->syscall may be incorrect for the sys_exit
    tracepoint, but that's actually not an issue because it will have been
    set during syscall entry and cannot have changed since then.
    
    Reported-by: Andrew Gabbasov <Andrew_Gabbasov at mentor.com>
    Signed-off-by: Will Deacon <will.deacon at arm.com>

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 8355d4b..1dd5122 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -452,7 +452,6 @@ __sys_trace:
 
 __sys_trace_return:
 	str	r0, [sp, #S_R0 + S_OFF]!	@ save returned r0
-	mov	r1, scno
 	mov	r0, sp
 	bl	syscall_trace_exit
 	b	ret_slow_syscall
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 518536d..0e9c779 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -957,17 +957,22 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 	return scno;
 }
 
-asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
+asmlinkage void syscall_trace_exit(struct pt_regs *regs)
 {
-	current_thread_info()->syscall = scno;
-
-	if (test_thread_flag(TIF_SYSCALL_TRACE))
-		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
+	/*
+	 * Audit the syscall before anything else, as a debugger may
+	 * come in and change the current registers.
+	 */
+	audit_syscall_exit(regs);
 
+	/*
+	 * Note that we haven't updated the ->syscall field for the
+	 * current thread. This isn't a problem because it will have
+	 * been set on syscall entry and there hasn't been an opportunity
+	 * for a PTRACE_SET_SYSCALL since then.
+	 */
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
-		trace_sys_exit(regs, scno);
+		trace_sys_exit(regs, regs_return_value(regs));
 
-	audit_syscall_exit(regs);
-
-	return scno;
+	tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
 }