[PATCH v2 0/3] Don't let system calls clobber userspace registers

[PATCH v2 0/3] Don't let system calls clobber userspace registers

[Bobby Bingham]

When invoking syscall handlers on sh32, the saved userspace registers
are at the top of the stack. This seems to have been intentional, as it
is an easy way to pass r0, r1, ... to the handler as parameters 5, 6,
...

It causes problems, however, because the compiler is allowed to generate
code for a function which clobbers that function's own parameters. For
example, gcc generates the following code for clone:

    <SyS_clone>:
        mov.l   8c020714 <SyS_clone+0xc>,r1  ! 8c020540 <do_fork>
        mov.l   r7,@r15
        mov     r6,r7
        jmp     @r1
        mov     #0,r6
        nop
        .word 0x0540
        .word 0x8c02

The `mov.l r7,@r15` clobbers the saved value of r0 passed from
userspace. For most system calls, this might not be a problem, because
we'll be overwriting r0 with the return value anyway. But in the case
of clone, copy_thread will need the original value of r0 if the
CLONE_SETTLS flag was specified.

The first patch in this series fixes this issue for system calls by
pushing to the stack and extra copy of r0-r2 before invoking the
handler. We discard this copy before restoring the userspace registers,
so it is not a problem if they are clobbered.

Exception handlers also receive the userspace register values in a
similar manner, and may hit the same problem. The second patch removes
the do_fpu_error handler, which looks susceptible to this problem and
which, as far as I can tell, has not been used in some time. The third
patch addresses other exception handlers.

Changes since V1:
- Update messages for [2/3] to quote the short log of the  previous
  commit that left do_fpu_error unused.

Bobby Bingham (3):
  sh: push extra copy of r0-r2 for syscall parameters
  sh: remove unused do_fpu_error
  sh: don't pass saved userspace state to exception handlers

 arch/sh/include/asm/syscalls_32.h | 12 +++---------
 arch/sh/include/asm/traps_32.h    | 16 ++++------------
 arch/sh/kernel/entry-common.S     | 15 +++++++++++----
 arch/sh/kernel/signal_32.c        | 12 ++++--------
 arch/sh/kernel/sys_sh32.c         |  7 ++-----
 arch/sh/kernel/traps_32.c         | 23 +++++++----------------
 arch/sh/math-emu/math.c           | 18 ------------------
 7 files changed, 31 insertions(+), 72 deletions(-)

-- 
1.8.5.5

[PATCH v2 1/3] sh: push extra copy of r0-r2 for syscall parameters

[Bobby Bingham]

The userspace registers are stored at the top of the stack when the syscall
handler is invoked, which allows r0-r2 to act as parameters 5-7. Parameters
passed on the stack may be clobbered by the syscall handler. The solution
is to push an extra copy of the registers which might be used as syscall
parameters to the stack, so that the authoritative set of saved register
values does not get clobbered.

A few system call handlers are also updated to get the userspace registers
using current_pt_regs() instead of from the stack.

Signed-off-by: Bobby Bingham <koorogi@koorogi.info>
---
 arch/sh/include/asm/syscalls_32.h | 12 +++---------
 arch/sh/kernel/entry-common.S     | 15 +++++++++++----
 arch/sh/kernel/signal_32.c        | 12 ++++--------
 arch/sh/kernel/sys_sh32.c         |  7 ++-----
 4 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/arch/sh/include/asm/syscalls_32.h b/arch/sh/include/asm/syscalls_32.h
index 4f97df8..4f643aa 100644
--- a/arch/sh/include/asm/syscalls_32.h
+++ b/arch/sh/include/asm/syscalls_32.h
@@ -9,15 +9,9 @@
 
 struct pt_regs;
 
-asmlinkage int sys_sigreturn(unsigned long r4, unsigned long r5,
-			     unsigned long r6, unsigned long r7,
-			     struct pt_regs __regs);
-asmlinkage int sys_rt_sigreturn(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs);
-asmlinkage int sys_sh_pipe(unsigned long r4, unsigned long r5,
-			   unsigned long r6, unsigned long r7,
-			   struct pt_regs __regs);
+asmlinkage int sys_sigreturn(void);
+asmlinkage int sys_rt_sigreturn(void);
+asmlinkage int sys_sh_pipe(void);
 asmlinkage ssize_t sys_pread_wrapper(unsigned int fd, char __user *buf,
 				     size_t count, long dummy, loff_t pos);
 asmlinkage ssize_t sys_pwrite_wrapper(unsigned int fd, const char __user *buf,
diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
index ca46834..13047a4 100644
--- a/arch/sh/kernel/entry-common.S
+++ b/arch/sh/kernel/entry-common.S
@@ -193,10 +193,10 @@ syscall_trace_entry:
 	!			Reload R0-R4 from kernel stack, where the
 	!   	    	    	parent may have modified them using
 	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
-	!   	    	    	used by the system call handler directly
-	!   	    	    	from the kernel stack anyway, so don't need
-	!   	    	    	to be reloaded here.)  This allows the parent
-	!   	    	    	to rewrite system calls and args on the fly.
+	!   	    	    	reloaded from the kernel stack by syscall_call
+	!   	    	    	below, so don't need to be reloaded here.)
+	!   	    	    	This allows the parent to rewrite system calls
+	!   	    	    	and args on the fly.
 	mov.l	@(OFF_R4,r15), r4   ! arg0
 	mov.l	@(OFF_R5,r15), r5
 	mov.l	@(OFF_R6,r15), r6
@@ -357,8 +357,15 @@ syscall_call:
 	mov.l	3f, r8		! Load the address of sys_call_table
 	add	r8, r3
 	mov.l	@r3, r8
+	mov.l	@(OFF_R2,r15), r2
+	mov.l	@(OFF_R1,r15), r1
+	mov.l	@(OFF_R0,r15), r0
+	mov.l	r2, @-r15
+	mov.l	r1, @-r15
+	mov.l	r0, @-r15
 	jsr	@r8	    	! jump to specific syscall handler
 	 nop
+	add	#12, r15
 	mov.l	@(OFF_R0,r15), r12		! save r0
 	mov.l	r0, @(OFF_R0,r15)		! save the return value
 	!
diff --git a/arch/sh/kernel/signal_32.c b/arch/sh/kernel/signal_32.c
index 6af6e7c..594cd37 100644
--- a/arch/sh/kernel/signal_32.c
+++ b/arch/sh/kernel/signal_32.c
@@ -148,11 +148,9 @@ restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc, int *r0_p
 	return err;
 }
 
-asmlinkage int sys_sigreturn(unsigned long r4, unsigned long r5,
-			     unsigned long r6, unsigned long r7,
-			     struct pt_regs __regs)
+asmlinkage int sys_sigreturn(void)
 {
-	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
+	struct pt_regs *regs = current_pt_regs();
 	struct sigframe __user *frame = (struct sigframe __user *)regs->regs[15];
 	sigset_t set;
 	int r0;
@@ -180,11 +178,9 @@ badframe:
 	return 0;
 }
 
-asmlinkage int sys_rt_sigreturn(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs)
+asmlinkage int sys_rt_sigreturn(void)
 {
-	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
+	struct pt_regs *regs = current_pt_regs();
 	struct rt_sigframe __user *frame = (struct rt_sigframe __user *)regs->regs[15];
 	sigset_t set;
 	int r0;
diff --git a/arch/sh/kernel/sys_sh32.c b/arch/sh/kernel/sys_sh32.c
index 497bab3..b66d1c6 100644
--- a/arch/sh/kernel/sys_sh32.c
+++ b/arch/sh/kernel/sys_sh32.c
@@ -21,17 +21,14 @@
  * sys_pipe() is the normal C calling standard for creating
  * a pipe. It's not the way Unix traditionally does this, though.
  */
-asmlinkage int sys_sh_pipe(unsigned long r4, unsigned long r5,
-	unsigned long r6, unsigned long r7,
-	struct pt_regs __regs)
+asmlinkage int sys_sh_pipe(void)
 {
-	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
 	int fd[2];
 	int error;
 
 	error = do_pipe_flags(fd, 0);
 	if (!error) {
-		regs->regs[1] = fd[1];
+		current_pt_regs()->regs[1] = fd[1];
 		return fd[0];
 	}
 	return error;
-- 
1.8.5.5

[PATCH v2 2/3] sh: remove unused do_fpu_error

[Bobby Bingham]

This does not appear to have been used since commit
74d99a5e262229ee865f6f68528d10b82471ead6 (sh: SH-2A FPU support) in
2007.

Signed-off-by: Bobby Bingham <koorogi@koorogi.info>
---
 arch/sh/math-emu/math.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/arch/sh/math-emu/math.c b/arch/sh/math-emu/math.c
index b876780..04aa55f 100644
--- a/arch/sh/math-emu/math.c
+++ b/arch/sh/math-emu/math.c
@@ -574,24 +574,6 @@ static int ieee_fpe_handler(struct pt_regs *regs)
 	return 0;
 }
 
-asmlinkage void do_fpu_error(unsigned long r4, unsigned long r5,
-			     unsigned long r6, unsigned long r7,
-			     struct pt_regs regs)
-{
-	struct task_struct *tsk = current;
-	siginfo_t info;
-
-	if (ieee_fpe_handler (&regs))
-		return;
-
-	regs.pc += 2;
-	info.si_signo = SIGFPE;
-	info.si_errno = 0;
-	info.si_code = FPE_FLTINV;
-	info.si_addr = (void __user *)regs.pc;
-	force_sig_info(SIGFPE, &info, tsk);
-}
-
 /**
  * fpu_init - Initialize FPU registers
  * @fpu: Pointer to software emulated FPU registers.
-- 
1.8.5.5

[PATCH v2 3/3] sh: don't pass saved userspace state to exception handlers

[Bobby Bingham]

The compiler is permitted to generate code which overwrites the parameters
to a function. If those parameters include the only saved copy we have of
userspace's registers, we're in trouble.

Signed-off-by: Bobby Bingham <koorogi@koorogi.info>
---
 arch/sh/include/asm/traps_32.h | 16 ++++------------
 arch/sh/kernel/traps_32.c      | 23 +++++++----------------
 2 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/arch/sh/include/asm/traps_32.h b/arch/sh/include/asm/traps_32.h
index cfd55ff..17e129f 100644
--- a/arch/sh/include/asm/traps_32.h
+++ b/arch/sh/include/asm/traps_32.h
@@ -42,18 +42,10 @@ static inline void trigger_address_error(void)
 asmlinkage void do_address_error(struct pt_regs *regs,
 				 unsigned long writeaccess,
 				 unsigned long address);
-asmlinkage void do_divide_error(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs);
-asmlinkage void do_reserved_inst(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs);
-asmlinkage void do_illegal_slot_inst(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs);
-asmlinkage void do_exception_error(unsigned long r4, unsigned long r5,
-				   unsigned long r6, unsigned long r7,
-				   struct pt_regs __regs);
+asmlinkage void do_divide_error(unsigned long r4);
+asmlinkage void do_reserved_inst(void);
+asmlinkage void do_illegal_slot_inst(void);
+asmlinkage void do_exception_error(void);
 
 #define BUILD_TRAP_HANDLER(name)					\
 asmlinkage void name##_trap_handler(unsigned long r4, unsigned long r5,	\
diff --git a/arch/sh/kernel/traps_32.c b/arch/sh/kernel/traps_32.c
index 68e99f0..ff63934 100644
--- a/arch/sh/kernel/traps_32.c
+++ b/arch/sh/kernel/traps_32.c
@@ -594,9 +594,7 @@ int is_dsp_inst(struct pt_regs *regs)
 #endif /* CONFIG_SH_DSP */
 
 #ifdef CONFIG_CPU_SH2A
-asmlinkage void do_divide_error(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs)
+asmlinkage void do_divide_error(unsigned long r4)
 {
 	siginfo_t info;
 
@@ -613,11 +611,9 @@ asmlinkage void do_divide_error(unsigned long r4, unsigned long r5,
 }
 #endif
 
-asmlinkage void do_reserved_inst(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs)
+asmlinkage void do_reserved_inst(void)
 {
-	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
+	struct pt_regs *regs = current_pt_regs();
 	unsigned long error_code;
 	struct task_struct *tsk = current;
 
@@ -701,11 +697,9 @@ static int emulate_branch(unsigned short inst, struct pt_regs *regs)
 }
 #endif
 
-asmlinkage void do_illegal_slot_inst(unsigned long r4, unsigned long r5,
-				unsigned long r6, unsigned long r7,
-				struct pt_regs __regs)
+asmlinkage void do_illegal_slot_inst(void)
 {
-	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
+	struct pt_regs *regs = current_pt_regs();
 	unsigned long inst;
 	struct task_struct *tsk = current;
 
@@ -730,15 +724,12 @@ asmlinkage void do_illegal_slot_inst(unsigned long r4, unsigned long r5,
 	die_if_no_fixup("illegal slot instruction", regs, inst);
 }
 
-asmlinkage void do_exception_error(unsigned long r4, unsigned long r5,
-				   unsigned long r6, unsigned long r7,
-				   struct pt_regs __regs)
+asmlinkage void do_exception_error(void)
 {
-	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
 	long ex;
 
 	ex = lookup_exception_vector();
-	die_if_kernel("exception", regs, ex);
+	die_if_kernel("exception", current_pt_regs(), ex);
 }
 
 void per_cpu_trap_init(void)
-- 
1.8.5.5