All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix indirect syscall handler for syscalls with > 4 args
@ 2017-03-31 16:09 ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-03-31 16:09 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips; +Cc: James.Cowgill

These two patches fix a corner case in the o32 indirect syscall handler where
incorrect arguments might get passed to the underlying syscall function if more
than 4 arguments are passed to a restartable syscall.

The first patch hopefully removes the last use of pt_regs for reading syscall
arguments and the second patch removes the special pt_regs handling in the
indrect syscall handler which is no longer needed.


James Cowgill (2):
  MIPS: opt into HAVE_COPY_THREAD_TLS
  MIPS: Remove pt_regs adjustments in indirect syscall handler

 arch/mips/Kconfig              |  1 +
 arch/mips/kernel/process.c     |  6 +++---
 arch/mips/kernel/scall32-o32.S | 11 -----------
 arch/mips/kernel/scall64-o32.S |  6 ------
 4 files changed, 4 insertions(+), 20 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH 0/2] Fix indirect syscall handler for syscalls with > 4 args
@ 2017-03-31 16:09 ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-03-31 16:09 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips; +Cc: James.Cowgill

These two patches fix a corner case in the o32 indirect syscall handler where
incorrect arguments might get passed to the underlying syscall function if more
than 4 arguments are passed to a restartable syscall.

The first patch hopefully removes the last use of pt_regs for reading syscall
arguments and the second patch removes the special pt_regs handling in the
indrect syscall handler which is no longer needed.


James Cowgill (2):
  MIPS: opt into HAVE_COPY_THREAD_TLS
  MIPS: Remove pt_regs adjustments in indirect syscall handler

 arch/mips/Kconfig              |  1 +
 arch/mips/kernel/process.c     |  6 +++---
 arch/mips/kernel/scall32-o32.S | 11 -----------
 arch/mips/kernel/scall64-o32.S |  6 ------
 4 files changed, 4 insertions(+), 20 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH 1/2] MIPS: opt into HAVE_COPY_THREAD_TLS
@ 2017-03-31 16:09   ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-03-31 16:09 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips; +Cc: James.Cowgill

This the mips version of commit c1bd55f922a2d ("x86: opt into
HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit").

Simply use the tls system call argument instead of extracting the tls
argument by magic from the pt_regs structure.

See commit 3033f14ab78c3 ("clone: support passing tls argument via C
rather than pt_regs magic") for more background.

Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
---
 arch/mips/Kconfig          | 1 +
 arch/mips/kernel/process.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index a008a9f03072..7baddfa0e229 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -69,6 +69,7 @@ config MIPS
 	select HAVE_EXIT_THREAD
 	select HAVE_REGS_AND_STACK_ACCESS_API
 	select HAVE_ARCH_HARDENED_USERCOPY
+	select HAVE_COPY_THREAD_TLS
 
 menu "Machine selection"
 
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index fb6b6b650719..2b8066806bc4 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -114,8 +114,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 /*
  * Copy architecture-specific thread state
  */
-int copy_thread(unsigned long clone_flags, unsigned long usp,
-	unsigned long kthread_arg, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
+	unsigned long kthread_arg, struct task_struct *p, unsigned long tls)
 {
 	struct thread_info *ti = task_thread_info(p);
 	struct pt_regs *childregs, *regs = current_pt_regs();
@@ -176,7 +176,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
 	atomic_set(&p->thread.bd_emu_frame, BD_EMUFRAME_NONE);
 
 	if (clone_flags & CLONE_SETTLS)
-		ti->tp_value = regs->regs[7];
+		ti->tp_value = tls;
 
 	return 0;
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 1/2] MIPS: opt into HAVE_COPY_THREAD_TLS
@ 2017-03-31 16:09   ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-03-31 16:09 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips; +Cc: James.Cowgill

This the mips version of commit c1bd55f922a2d ("x86: opt into
HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit").

Simply use the tls system call argument instead of extracting the tls
argument by magic from the pt_regs structure.

See commit 3033f14ab78c3 ("clone: support passing tls argument via C
rather than pt_regs magic") for more background.

Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
---
 arch/mips/Kconfig          | 1 +
 arch/mips/kernel/process.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index a008a9f03072..7baddfa0e229 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -69,6 +69,7 @@ config MIPS
 	select HAVE_EXIT_THREAD
 	select HAVE_REGS_AND_STACK_ACCESS_API
 	select HAVE_ARCH_HARDENED_USERCOPY
+	select HAVE_COPY_THREAD_TLS
 
 menu "Machine selection"
 
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index fb6b6b650719..2b8066806bc4 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -114,8 +114,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 /*
  * Copy architecture-specific thread state
  */
-int copy_thread(unsigned long clone_flags, unsigned long usp,
-	unsigned long kthread_arg, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
+	unsigned long kthread_arg, struct task_struct *p, unsigned long tls)
 {
 	struct thread_info *ti = task_thread_info(p);
 	struct pt_regs *childregs, *regs = current_pt_regs();
@@ -176,7 +176,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
 	atomic_set(&p->thread.bd_emu_frame, BD_EMUFRAME_NONE);
 
 	if (clone_flags & CLONE_SETTLS)
-		ti->tp_value = regs->regs[7];
+		ti->tp_value = tls;
 
 	return 0;
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler
@ 2017-03-31 16:09   ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-03-31 16:09 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips; +Cc: James.Cowgill

If a restartable syscall is called using the indirect o32 syscall
handler - eg: syscall(__NR_waitid, ...), then it is possible for the
incorrect arguments to be passed to the syscall after it has been
restarted. This is because the syscall handler tries to shift all the
registers down one place in pt_regs so that when the syscall is restarted,
the "real" syscall is called instead. Unfortunately it only shifts the
arguments passed in registers, not the arguments on the user stack. This
causes the 4th argument to be duplicated when the syscall is restarted.

Fix by removing all the pt_regs shifting so that the indirect syscall
handler is called again when the syscall is restarted. The comment "some
syscalls like execve get their arguments from struct pt_regs" is long
out of date so this should now be safe.

Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
---
 arch/mips/kernel/scall32-o32.S | 11 -----------
 arch/mips/kernel/scall64-o32.S |  6 ------
 2 files changed, 17 deletions(-)

diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
index c29d397eee86..d8d6336c4cc5 100644
--- a/arch/mips/kernel/scall32-o32.S
+++ b/arch/mips/kernel/scall32-o32.S
@@ -190,12 +190,6 @@ illegal_syscall:
 	sll	t1, t0, 2
 	beqz	v0, einval
 	lw	t2, sys_call_table(t1)		# syscall routine
-	sw	a0, PT_R2(sp)			# call routine directly on restart
-
-	/* Some syscalls like execve get their arguments from struct pt_regs
-	   and claim zero arguments in the syscall table. Thus we have to
-	   assume the worst case and shuffle around all potential arguments.
-	   If you want performance, don't use indirect syscalls. */
 
 	move	a0, a1				# shift argument registers
 	move	a1, a2
@@ -207,11 +201,6 @@ illegal_syscall:
 	sw	t4, 16(sp)
 	sw	t5, 20(sp)
 	sw	t6, 24(sp)
-	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
-	sw	a1, PT_R5(sp)			# syscalls expect them there
-	sw	a2, PT_R6(sp)
-	sw	a3, PT_R7(sp)
-	sw	a3, PT_R26(sp)			# update a3 for syscall restarting
 	jr	t2
 	/* Unreached */
 
diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
index 5a47042dd25f..6fd8ecca89e7 100644
--- a/arch/mips/kernel/scall64-o32.S
+++ b/arch/mips/kernel/scall64-o32.S
@@ -198,7 +198,6 @@ LEAF(sys32_syscall)
 	dsll	t1, t0, 3
 	beqz	v0, einval
 	ld	t2, sys32_call_table(t1)		# syscall routine
-	sd	a0, PT_R2(sp)		# call routine directly on restart
 
 	move	a0, a1			# shift argument registers
 	move	a1, a2
@@ -207,11 +206,6 @@ LEAF(sys32_syscall)
 	move	a4, a5
 	move	a5, a6
 	move	a6, a7
-	sd	a0, PT_R4(sp)		# ... and push back a0 - a3, some
-	sd	a1, PT_R5(sp)		# syscalls expect them there
-	sd	a2, PT_R6(sp)
-	sd	a3, PT_R7(sp)
-	sd	a3, PT_R26(sp)		# update a3 for syscall restarting
 	jr	t2
 	/* Unreached */
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler
@ 2017-03-31 16:09   ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-03-31 16:09 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips; +Cc: James.Cowgill

If a restartable syscall is called using the indirect o32 syscall
handler - eg: syscall(__NR_waitid, ...), then it is possible for the
incorrect arguments to be passed to the syscall after it has been
restarted. This is because the syscall handler tries to shift all the
registers down one place in pt_regs so that when the syscall is restarted,
the "real" syscall is called instead. Unfortunately it only shifts the
arguments passed in registers, not the arguments on the user stack. This
causes the 4th argument to be duplicated when the syscall is restarted.

Fix by removing all the pt_regs shifting so that the indirect syscall
handler is called again when the syscall is restarted. The comment "some
syscalls like execve get their arguments from struct pt_regs" is long
out of date so this should now be safe.

Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
---
 arch/mips/kernel/scall32-o32.S | 11 -----------
 arch/mips/kernel/scall64-o32.S |  6 ------
 2 files changed, 17 deletions(-)

diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
index c29d397eee86..d8d6336c4cc5 100644
--- a/arch/mips/kernel/scall32-o32.S
+++ b/arch/mips/kernel/scall32-o32.S
@@ -190,12 +190,6 @@ illegal_syscall:
 	sll	t1, t0, 2
 	beqz	v0, einval
 	lw	t2, sys_call_table(t1)		# syscall routine
-	sw	a0, PT_R2(sp)			# call routine directly on restart
-
-	/* Some syscalls like execve get their arguments from struct pt_regs
-	   and claim zero arguments in the syscall table. Thus we have to
-	   assume the worst case and shuffle around all potential arguments.
-	   If you want performance, don't use indirect syscalls. */
 
 	move	a0, a1				# shift argument registers
 	move	a1, a2
@@ -207,11 +201,6 @@ illegal_syscall:
 	sw	t4, 16(sp)
 	sw	t5, 20(sp)
 	sw	t6, 24(sp)
-	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
-	sw	a1, PT_R5(sp)			# syscalls expect them there
-	sw	a2, PT_R6(sp)
-	sw	a3, PT_R7(sp)
-	sw	a3, PT_R26(sp)			# update a3 for syscall restarting
 	jr	t2
 	/* Unreached */
 
diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
index 5a47042dd25f..6fd8ecca89e7 100644
--- a/arch/mips/kernel/scall64-o32.S
+++ b/arch/mips/kernel/scall64-o32.S
@@ -198,7 +198,6 @@ LEAF(sys32_syscall)
 	dsll	t1, t0, 3
 	beqz	v0, einval
 	ld	t2, sys32_call_table(t1)		# syscall routine
-	sd	a0, PT_R2(sp)		# call routine directly on restart
 
 	move	a0, a1			# shift argument registers
 	move	a1, a2
@@ -207,11 +206,6 @@ LEAF(sys32_syscall)
 	move	a4, a5
 	move	a5, a6
 	move	a6, a7
-	sd	a0, PT_R4(sp)		# ... and push back a0 - a3, some
-	sd	a1, PT_R5(sp)		# syscalls expect them there
-	sd	a2, PT_R6(sp)
-	sd	a3, PT_R7(sp)
-	sd	a3, PT_R26(sp)		# update a3 for syscall restarting
 	jr	t2
 	/* Unreached */
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] MIPS: opt into HAVE_COPY_THREAD_TLS
  2017-03-31 16:09   ` James Cowgill
  (?)
@ 2017-04-10 11:19   ` Ralf Baechle
  -1 siblings, 0 replies; 11+ messages in thread
From: Ralf Baechle @ 2017-04-10 11:19 UTC (permalink / raw)
  To: James Cowgill; +Cc: linux-mips


Thanks, applied.

  Ralf

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler
@ 2017-07-17 14:27     ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-07-17 14:27 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips


[-- Attachment #1.1: Type: text/plain, Size: 1067 bytes --]

On 31/03/17 17:09, James Cowgill wrote:
> If a restartable syscall is called using the indirect o32 syscall
> handler - eg: syscall(__NR_waitid, ...), then it is possible for the
> incorrect arguments to be passed to the syscall after it has been
> restarted. This is because the syscall handler tries to shift all the
> registers down one place in pt_regs so that when the syscall is restarted,
> the "real" syscall is called instead. Unfortunately it only shifts the
> arguments passed in registers, not the arguments on the user stack. This
> causes the 4th argument to be duplicated when the syscall is restarted.
> 
> Fix by removing all the pt_regs shifting so that the indirect syscall
> handler is called again when the syscall is restarted. The comment "some
> syscalls like execve get their arguments from struct pt_regs" is long
> out of date so this should now be safe.
> 
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>

Ping? The first patch in this series has been applied, but this patch
has not been.

Thanks,
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler
@ 2017-07-17 14:27     ` James Cowgill
  0 siblings, 0 replies; 11+ messages in thread
From: James Cowgill @ 2017-07-17 14:27 UTC (permalink / raw)
  To: Ralf Baechle, linux-mips


[-- Attachment #1.1: Type: text/plain, Size: 1067 bytes --]

On 31/03/17 17:09, James Cowgill wrote:
> If a restartable syscall is called using the indirect o32 syscall
> handler - eg: syscall(__NR_waitid, ...), then it is possible for the
> incorrect arguments to be passed to the syscall after it has been
> restarted. This is because the syscall handler tries to shift all the
> registers down one place in pt_regs so that when the syscall is restarted,
> the "real" syscall is called instead. Unfortunately it only shifts the
> arguments passed in registers, not the arguments on the user stack. This
> causes the 4th argument to be duplicated when the syscall is restarted.
> 
> Fix by removing all the pt_regs shifting so that the indirect syscall
> handler is called again when the syscall is restarted. The comment "some
> syscalls like execve get their arguments from struct pt_regs" is long
> out of date so this should now be safe.
> 
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>

Ping? The first patch in this series has been applied, but this patch
has not been.

Thanks,
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler
@ 2017-07-28 14:03     ` James Hogan
  0 siblings, 0 replies; 11+ messages in thread
From: James Hogan @ 2017-07-28 14:03 UTC (permalink / raw)
  To: James Cowgill; +Cc: Ralf Baechle, linux-mips

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]

On Fri, Mar 31, 2017 at 05:09:59PM +0100, James Cowgill wrote:
> If a restartable syscall is called using the indirect o32 syscall
> handler - eg: syscall(__NR_waitid, ...), then it is possible for the
> incorrect arguments to be passed to the syscall after it has been
> restarted. This is because the syscall handler tries to shift all the
> registers down one place in pt_regs so that when the syscall is restarted,
> the "real" syscall is called instead. Unfortunately it only shifts the
> arguments passed in registers, not the arguments on the user stack. This
> causes the 4th argument to be duplicated when the syscall is restarted.
> 
> Fix by removing all the pt_regs shifting so that the indirect syscall
> handler is called again when the syscall is restarted. The comment "some
> syscalls like execve get their arguments from struct pt_regs" is long
> out of date so this should now be safe.
> 
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>

Reviewed-by: James Hogan <james.hogan@imgtec.com>
Tested-by: James Hogan <james.hogan@imgtec.com>

This is safe to backport as far back as 4.2 too (I just tested), which
is I think as far back as patch 1 (commit f9c4e3a6dae1) can be
backported due to the commit 3033f14ab78c3 ("clone: support passing tls
argument via C rather than pt_regs magic") referenced in patch 1, so I
suggest adding:

Cc: <stable@vger.kernel.org> # f9c4e3a6dae1: MIPS: Opt into HAVE_COPY_THREAD_TLS
Cc: <stable@vger.kernel.org> # 4.2+

Thanks
James

> ---
>  arch/mips/kernel/scall32-o32.S | 11 -----------
>  arch/mips/kernel/scall64-o32.S |  6 ------
>  2 files changed, 17 deletions(-)
> 
> diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
> index c29d397eee86..d8d6336c4cc5 100644
> --- a/arch/mips/kernel/scall32-o32.S
> +++ b/arch/mips/kernel/scall32-o32.S
> @@ -190,12 +190,6 @@ illegal_syscall:
>  	sll	t1, t0, 2
>  	beqz	v0, einval
>  	lw	t2, sys_call_table(t1)		# syscall routine
> -	sw	a0, PT_R2(sp)			# call routine directly on restart
> -
> -	/* Some syscalls like execve get their arguments from struct pt_regs
> -	   and claim zero arguments in the syscall table. Thus we have to
> -	   assume the worst case and shuffle around all potential arguments.
> -	   If you want performance, don't use indirect syscalls. */
>  
>  	move	a0, a1				# shift argument registers
>  	move	a1, a2
> @@ -207,11 +201,6 @@ illegal_syscall:
>  	sw	t4, 16(sp)
>  	sw	t5, 20(sp)
>  	sw	t6, 24(sp)
> -	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
> -	sw	a1, PT_R5(sp)			# syscalls expect them there
> -	sw	a2, PT_R6(sp)
> -	sw	a3, PT_R7(sp)
> -	sw	a3, PT_R26(sp)			# update a3 for syscall restarting
>  	jr	t2
>  	/* Unreached */
>  
> diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
> index 5a47042dd25f..6fd8ecca89e7 100644
> --- a/arch/mips/kernel/scall64-o32.S
> +++ b/arch/mips/kernel/scall64-o32.S
> @@ -198,7 +198,6 @@ LEAF(sys32_syscall)
>  	dsll	t1, t0, 3
>  	beqz	v0, einval
>  	ld	t2, sys32_call_table(t1)		# syscall routine
> -	sd	a0, PT_R2(sp)		# call routine directly on restart
>  
>  	move	a0, a1			# shift argument registers
>  	move	a1, a2
> @@ -207,11 +206,6 @@ LEAF(sys32_syscall)
>  	move	a4, a5
>  	move	a5, a6
>  	move	a6, a7
> -	sd	a0, PT_R4(sp)		# ... and push back a0 - a3, some
> -	sd	a1, PT_R5(sp)		# syscalls expect them there
> -	sd	a2, PT_R6(sp)
> -	sd	a3, PT_R7(sp)
> -	sd	a3, PT_R26(sp)		# update a3 for syscall restarting
>  	jr	t2
>  	/* Unreached */
>  
> -- 
> 2.11.0
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler
@ 2017-07-28 14:03     ` James Hogan
  0 siblings, 0 replies; 11+ messages in thread
From: James Hogan @ 2017-07-28 14:03 UTC (permalink / raw)
  To: James Cowgill; +Cc: Ralf Baechle, linux-mips

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]

On Fri, Mar 31, 2017 at 05:09:59PM +0100, James Cowgill wrote:
> If a restartable syscall is called using the indirect o32 syscall
> handler - eg: syscall(__NR_waitid, ...), then it is possible for the
> incorrect arguments to be passed to the syscall after it has been
> restarted. This is because the syscall handler tries to shift all the
> registers down one place in pt_regs so that when the syscall is restarted,
> the "real" syscall is called instead. Unfortunately it only shifts the
> arguments passed in registers, not the arguments on the user stack. This
> causes the 4th argument to be duplicated when the syscall is restarted.
> 
> Fix by removing all the pt_regs shifting so that the indirect syscall
> handler is called again when the syscall is restarted. The comment "some
> syscalls like execve get their arguments from struct pt_regs" is long
> out of date so this should now be safe.
> 
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>

Reviewed-by: James Hogan <james.hogan@imgtec.com>
Tested-by: James Hogan <james.hogan@imgtec.com>

This is safe to backport as far back as 4.2 too (I just tested), which
is I think as far back as patch 1 (commit f9c4e3a6dae1) can be
backported due to the commit 3033f14ab78c3 ("clone: support passing tls
argument via C rather than pt_regs magic") referenced in patch 1, so I
suggest adding:

Cc: <stable@vger.kernel.org> # f9c4e3a6dae1: MIPS: Opt into HAVE_COPY_THREAD_TLS
Cc: <stable@vger.kernel.org> # 4.2+

Thanks
James

> ---
>  arch/mips/kernel/scall32-o32.S | 11 -----------
>  arch/mips/kernel/scall64-o32.S |  6 ------
>  2 files changed, 17 deletions(-)
> 
> diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
> index c29d397eee86..d8d6336c4cc5 100644
> --- a/arch/mips/kernel/scall32-o32.S
> +++ b/arch/mips/kernel/scall32-o32.S
> @@ -190,12 +190,6 @@ illegal_syscall:
>  	sll	t1, t0, 2
>  	beqz	v0, einval
>  	lw	t2, sys_call_table(t1)		# syscall routine
> -	sw	a0, PT_R2(sp)			# call routine directly on restart
> -
> -	/* Some syscalls like execve get their arguments from struct pt_regs
> -	   and claim zero arguments in the syscall table. Thus we have to
> -	   assume the worst case and shuffle around all potential arguments.
> -	   If you want performance, don't use indirect syscalls. */
>  
>  	move	a0, a1				# shift argument registers
>  	move	a1, a2
> @@ -207,11 +201,6 @@ illegal_syscall:
>  	sw	t4, 16(sp)
>  	sw	t5, 20(sp)
>  	sw	t6, 24(sp)
> -	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
> -	sw	a1, PT_R5(sp)			# syscalls expect them there
> -	sw	a2, PT_R6(sp)
> -	sw	a3, PT_R7(sp)
> -	sw	a3, PT_R26(sp)			# update a3 for syscall restarting
>  	jr	t2
>  	/* Unreached */
>  
> diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
> index 5a47042dd25f..6fd8ecca89e7 100644
> --- a/arch/mips/kernel/scall64-o32.S
> +++ b/arch/mips/kernel/scall64-o32.S
> @@ -198,7 +198,6 @@ LEAF(sys32_syscall)
>  	dsll	t1, t0, 3
>  	beqz	v0, einval
>  	ld	t2, sys32_call_table(t1)		# syscall routine
> -	sd	a0, PT_R2(sp)		# call routine directly on restart
>  
>  	move	a0, a1			# shift argument registers
>  	move	a1, a2
> @@ -207,11 +206,6 @@ LEAF(sys32_syscall)
>  	move	a4, a5
>  	move	a5, a6
>  	move	a6, a7
> -	sd	a0, PT_R4(sp)		# ... and push back a0 - a3, some
> -	sd	a1, PT_R5(sp)		# syscalls expect them there
> -	sd	a2, PT_R6(sp)
> -	sd	a3, PT_R7(sp)
> -	sd	a3, PT_R26(sp)		# update a3 for syscall restarting
>  	jr	t2
>  	/* Unreached */
>  
> -- 
> 2.11.0
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-07-28 14:03 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-31 16:09 [PATCH 0/2] Fix indirect syscall handler for syscalls with > 4 args James Cowgill
2017-03-31 16:09 ` James Cowgill
2017-03-31 16:09 ` [PATCH 1/2] MIPS: opt into HAVE_COPY_THREAD_TLS James Cowgill
2017-03-31 16:09   ` James Cowgill
2017-04-10 11:19   ` Ralf Baechle
2017-03-31 16:09 ` [PATCH 2/2] MIPS: Remove pt_regs adjustments in indirect syscall handler James Cowgill
2017-03-31 16:09   ` James Cowgill
2017-07-17 14:27   ` James Cowgill
2017-07-17 14:27     ` James Cowgill
2017-07-28 14:03   ` James Hogan
2017-07-28 14:03     ` James Hogan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.