[PATCH] MIPS: fix forced successful syscalls

[PATCH] MIPS: fix forced successful syscalls

[Tanguy Bouzeloc]

On mips any syscalls who return a value between -MAXERRNO (1133) and
-1, is considered as an error (the error flag is set and return value
is the positive value of the error number).

But some syscalls can return values between -MAXERRNO and -1 like
sys_time and sys_times. In this case the userspace return value is
-return value of the syscall and the error flag set.

This patch add a TIF_NOERROR thread flag which indicates that the
return value of a syscall is always correct.

All ABIs are updated (kernel 32 o32, kernel64 o32, kernel64 n32
and kernel64 n64).
---
 arch/mips/include/asm/ptrace.h      |  5 +++++
 arch/mips/include/asm/thread_info.h |  2 ++
 arch/mips/kernel/scall32-o32.S      | 14 ++++++++++++++
 arch/mips/kernel/scall64-64.S       | 14 ++++++++++++++
 arch/mips/kernel/scall64-n32.S      | 14 ++++++++++++++
 arch/mips/kernel/scall64-o32.S      | 14 ++++++++++++++
 6 files changed, 63 insertions(+)

diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index 5e6cd09..51552f2 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -94,6 +94,11 @@ static inline void die_if_kernel(const char *str, struct pt_regs *regs)
 		die(str, regs);
 }
 
+#define force_successful_syscall_return()   \
+	do { \
+		set_thread_flag(TIF_NOERROR); \
+	} while(0)
+
 #define current_pt_regs()						\
 ({									\
 	unsigned long sp = (unsigned long)__builtin_frame_address(0);	\
diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
index 61215a3..6764fc1 100644
--- a/arch/mips/include/asm/thread_info.h
+++ b/arch/mips/include/asm/thread_info.h
@@ -116,6 +116,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_32BIT_ADDR		23	/* 32-bit address space (o32/n32) */
 #define TIF_FPUBOUND		24	/* thread bound to FPU-full CPU set */
 #define TIF_LOAD_WATCH		25	/* If set, load watch registers */
+#define TIF_NOERROR		30	/* Force successful syscall return */
 #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -131,6 +132,7 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_32BIT_REGS		(1<<TIF_32BIT_REGS)
 #define _TIF_32BIT_ADDR		(1<<TIF_32BIT_ADDR)
 #define _TIF_FPUBOUND		(1<<TIF_FPUBOUND)
+#define _TIF_NOERROR		(1<<TIF_NOERROR)
 #define _TIF_LOAD_WATCH		(1<<TIF_LOAD_WATCH)
 
 #define _TIF_WORK_SYSCALL_ENTRY	(_TIF_NOHZ | _TIF_SYSCALL_TRACE |	\
diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
index e774bb1..4ac30ac 100644
--- a/arch/mips/kernel/scall32-o32.S
+++ b/arch/mips/kernel/scall32-o32.S
@@ -58,8 +58,22 @@ stack_done:
 
 	jalr	t2			# Do The Real Thing (TM)
 
+	li	t0, 0			# no error for negative return ?
+	lw	t2, TI_FLAGS($28)
+	li	t1, _TIF_NOERROR
+	and	t2, t1
+	bnez	t2, syscall_ret_noerror
+
 	li	t0, -EMAXERRNO - 1	# error?
 	sltu	t0, t0, v0
+	b	syscall_ret_error
+
+syscall_ret_noerror:
+	li	t1, ~_TIF_NOERROR	# clear TIF_NOERROR
+	and	t2, t1
+	sw	t2, TI_FLAGS($28)
+
+syscall_ret_error:
 	sw	t0, PT_R7(sp)		# set error flag
 	beqz	t0, 1f
 
diff --git a/arch/mips/kernel/scall64-64.S b/arch/mips/kernel/scall64-64.S
index be6627e..793ba7e 100644
--- a/arch/mips/kernel/scall64-64.S
+++ b/arch/mips/kernel/scall64-64.S
@@ -61,8 +61,22 @@ NESTED(handle_sys64, PT_SIZE, sp)
 
 	jalr	t2			# Do The Real Thing (TM)
 
+	li	t0, 0			# no error for negative return ?
+	LONG_L	t2, TI_FLAGS($28)
+	li	t1, _TIF_NOERROR
+	and	t2, t1
+	bnez	t2, syscall_ret_noerror
+
 	li	t0, -EMAXERRNO - 1	# error?
 	sltu	t0, t0, v0
+	b	syscall_ret_error
+
+syscall_ret_noerror:
+	li	t1, ~_TIF_NOERROR	# clear TIF_NOERROR
+	and	t2, t1
+	LONG_S	t2, TI_FLAGS($28)
+
+syscall_ret_error:
 	sd	t0, PT_R7(sp)		# set error flag
 	beqz	t0, 1f
 
diff --git a/arch/mips/kernel/scall64-n32.S b/arch/mips/kernel/scall64-n32.S
index cab1507..ccac0c6 100644
--- a/arch/mips/kernel/scall64-n32.S
+++ b/arch/mips/kernel/scall64-n32.S
@@ -54,8 +54,22 @@ NESTED(handle_sysn32, PT_SIZE, sp)
 
 	jalr	t2			# Do The Real Thing (TM)
 
+	li	t0, 0			# no error for negative return ?
+	LONG_L	t2, TI_FLAGS($28)
+	li	t1, _TIF_NOERROR
+	and	t2, t1
+	bnez	t2, syscall_ret_noerror
+
 	li	t0, -EMAXERRNO - 1	# error?
 	sltu	t0, t0, v0
+	b	syscall_ret_error
+
+syscall_ret_noerror:
+	li	t1, ~_TIF_NOERROR	# clear TIF_NOERROR
+	and	t2, t1
+	LONG_S	t2, TI_FLAGS($28)
+
+syscall_ret_error:
 	sd	t0, PT_R7(sp)		# set error flag
 	beqz	t0, 1f
 
diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
index 37605dc..b1d3d51 100644
--- a/arch/mips/kernel/scall64-o32.S
+++ b/arch/mips/kernel/scall64-o32.S
@@ -88,8 +88,22 @@ NESTED(handle_sys, PT_SIZE, sp)
 
 	jalr	t2			# Do The Real Thing (TM)
 
+	li	t0, 0			# no error for negative return ?
+	LONG_L	t2, TI_FLAGS($28)
+	li	t1, _TIF_NOERROR
+	and	t2, t1
+	bnez	t2, syscall_ret_noerror
+
 	li	t0, -EMAXERRNO - 1	# error?
 	sltu	t0, t0, v0
+	b	syscall_ret_error
+
+syscall_ret_noerror:
+	li	t1, ~_TIF_NOERROR	# clear TIF_NOERROR
+	and	t2, t1
+	LONG_S	t2, TI_FLAGS($28)
+
+syscall_ret_error:
 	sd	t0, PT_R7(sp)		# set error flag
 	beqz	t0, 1f
 
-- 
1.8.3.1

Re: [PATCH] MIPS: fix forced successful syscalls

[Ralf Baechle]

On Mon, Sep 30, 2013 at 04:22:49PM +0200, Tanguy Bouzeloc wrote:
> Date:   Mon, 30 Sep 2013 16:22:49 +0200
> From: Tanguy Bouzeloc <tanguy.bouzeloc@efixo.com>
> To: ralf@linux-mips.org
> Cc: linux-mips@linux-mips.org, Tanguy Bouzeloc <tanguy.bouzeloc@efixo.com>
> Subject: [PATCH] MIPS: fix forced successful syscalls
> 
> On mips any syscalls who return a value between -MAXERRNO (1133) and
> -1, is considered as an error (the error flag is set and return value
> is the positive value of the error number).
> 
> But some syscalls can return values between -MAXERRNO and -1 like
> sys_time and sys_times. In this case the userspace return value is
> -return value of the syscall and the error flag set.
> 
> This patch add a TIF_NOERROR thread flag which indicates that the
> return value of a syscall is always correct.

To my personal embarassment I have to admit that I knew about this since the
day the syscall wrapper was written - but was considering it an acceptable
bug ...

Where it really bits is sigreturn and similar which use the following
stunt:

        /*
         * Don't let your children do this ...
         */
        __asm__ __volatile__(
                "move\t$29, %0\n\t"
                "j\tsyscall_exit"
                :/* no outputs */
                :"r" (&regs));
        /* Unreached */

to keep the syscall return path from tampering with the return value.

The scall*.S part of your patch is clearing TIF_NOERROR using a non-atomic
LW/SW sequence.  This needs to be done atomically or the thread's flags
variable might get corrupted.  This is complicated by MIPS I, R5900 and
afair some older oddball not-quite MIPS II CPUs lacking LL/SC rsp. LLD/SCD.

  Ralf

Re: [PATCH] MIPS: fix forced successful syscalls

[Tanguy Bouzeloc]

On 10/02/2013 11:19 AM, Ralf Baechle wrote:
> To my personal embarassment I have to admit that I knew about this since the
> day the syscall wrapper was written - but was considering it an acceptable
> bug ...
>
> Where it really bits is sigreturn and similar which use the following
> stunt:
>
>          /*
>           * Don't let your children do this ...
>           */
>          __asm__ __volatile__(
>                  "move\t$29, %0\n\t"
>                  "j\tsyscall_exit"
>                  :/* no outputs */
>                  :"r" (&regs));
>          /* Unreached */
>
> to keep the syscall return path from tampering with the return value.
>
> The scall*.S part of your patch is clearing TIF_NOERROR using a non-atomic
> LW/SW sequence.  This needs to be done atomically or the thread's flags
> variable might get corrupted.  This is complicated by MIPS I, R5900 and
> afair some older oddball not-quite MIPS II CPUs lacking LL/SC rsp. LLD/SCD.
>
>    Ralf
>

I discover the issue when changing the HZ of the kernel to 100HZ, in 
this case the jiffies returned to the userland are the same as the 
kernel ticks and it'll wrap after 5 minutes of uptime. With kernel HZ at 
250 or 1000H it'll make happen the ticks wrap after 230~260j.

Unfortunately programs relying on ticks (they shouldn't but that 
happens) have unpredictable behavior for 11.3s before the wrap.

I can update the patch in order to access atomically the thread flags, 
the point is ... it'll make the kernel incompatible with old hardware.

Regards,
Tanguy.

Re: [PATCH] MIPS: fix forced successful syscalls

[Ralf Baechle]

On Thu, Oct 03, 2013 at 04:53:31PM +0200, Tanguy Bouzeloc wrote:

> On 10/02/2013 11:19 AM, Ralf Baechle wrote:
> >To my personal embarassment I have to admit that I knew about this since the
> >day the syscall wrapper was written - but was considering it an acceptable
> >bug ...
> >
> >Where it really bits is sigreturn and similar which use the following
> >stunt:
> >
> >         /*
> >          * Don't let your children do this ...
> >          */
> >         __asm__ __volatile__(
> >                 "move\t$29, %0\n\t"
> >                 "j\tsyscall_exit"
> >                 :/* no outputs */
> >                 :"r" (&regs));
> >         /* Unreached */
> >
> >to keep the syscall return path from tampering with the return value.
> >
> >The scall*.S part of your patch is clearing TIF_NOERROR using a non-atomic
> >LW/SW sequence.  This needs to be done atomically or the thread's flags
> >variable might get corrupted.  This is complicated by MIPS I, R5900 and
> >afair some older oddball not-quite MIPS II CPUs lacking LL/SC rsp. LLD/SCD.
> >
> >   Ralf
> >
> 
> I discover the issue when changing the HZ of the kernel to 100HZ, in
> this case the jiffies returned to the userland are the same as the
> kernel ticks and it'll wrap after 5 minutes of uptime. With kernel
> HZ at 250 or 1000H it'll make happen the ticks wrap after 230~260j.
> 
> Unfortunately programs relying on ticks (they shouldn't but that
> happens) have unpredictable behavior for 11.3s before the wrap.
> 
> I can update the patch in order to access atomically the thread
> flags, the point is ... it'll make the kernel incompatible with old
> hardware.

The price to pay: an ifdef ...

#include <asm/sgidefs.H>
...
#if (_MIPS_ISA > _MIPS_ISA_MIPS1)
... LL/SC code goes here
#else
... LL/SC-less code goes here
#endif

But I'm wondering if we can move this hopefully relativly rare special
case into do_notify_resume() and handle it in plain C there.

  Ralf