linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] sh: Fix validation of system call number
@ 2020-07-22 23:13 Michael Karcher
  2020-07-22 23:13 ` [PATCH 2/4] sh: Rearrange blocks in entry-common.S Michael Karcher
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Michael Karcher @ 2020-07-22 23:13 UTC (permalink / raw)
  To: linux-sh, linux-kernel
  Cc: Yoshinori Sato, Rich Felker, Adrian Glaubitz, Michael Karcher

The slow path for traced system call entries accessed a wrong memory
location to get the number of the maximum allowed system call number.
Renumber the numbered "local" label for the correct location to avoid
collisions with actual local labels.

Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
---
 arch/sh/kernel/entry-common.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
index 956a7a03b0c8..9bac5bbb67f3 100644
--- a/arch/sh/kernel/entry-common.S
+++ b/arch/sh/kernel/entry-common.S
@@ -199,7 +199,7 @@ syscall_trace_entry:
 	mov.l	@(OFF_R7,r15), r7   ! arg3
 	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
 	!
-	mov.l	2f, r10			! Number of syscalls
+	mov.l	6f, r10			! Number of syscalls
 	cmp/hs	r10, r3
 	bf	syscall_call
 	mov	#-ENOSYS, r0
@@ -353,7 +353,7 @@ ENTRY(system_call)
 	tst	r9, r8
 	bf	syscall_trace_entry
 	!
-	mov.l	2f, r8			! Number of syscalls
+	mov.l	6f, r8			! Number of syscalls
 	cmp/hs	r8, r3
 	bt	syscall_badsys
 	!
@@ -392,7 +392,7 @@ syscall_exit:
 #if !defined(CONFIG_CPU_SH2)
 1:	.long	TRA
 #endif
-2:	.long	NR_syscalls
+6:	.long	NR_syscalls
 3:	.long	sys_call_table
 7:	.long	do_syscall_trace_enter
 8:	.long	do_syscall_trace_leave
-- 
2.28.0.rc1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/4] sh: Rearrange blocks in entry-common.S
  2020-07-22 23:13 [PATCH 1/4] sh: Fix validation of system call number Michael Karcher
@ 2020-07-22 23:13 ` Michael Karcher
  2020-07-22 23:20   ` John Paul Adrian Glaubitz
  2020-07-22 23:13 ` [PATCH 3/4] sh: Add SECCOMP_FILTER Michael Karcher
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Michael Karcher @ 2020-07-22 23:13 UTC (permalink / raw)
  To: linux-sh, linux-kernel
  Cc: Yoshinori Sato, Rich Felker, Adrian Glaubitz, Michael Karcher

This avoids out-of-range jumps that get auto-replaced by the assembler
and prepares for the changes needed to implement SECCOMP_FILTER cleanly.

Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
---
 arch/sh/kernel/entry-common.S | 57 ++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
index 9bac5bbb67f3..c4d88d61890d 100644
--- a/arch/sh/kernel/entry-common.S
+++ b/arch/sh/kernel/entry-common.S
@@ -178,34 +178,6 @@ syscall_exit_work:
 	bra	resume_userspace
 	 nop
 
-	.align	2
-syscall_trace_entry:
-	!                     	Yes it is traced.
-	mov     r15, r4
-	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
-	jsr	@r11	    	! superior (will chomp R[0-7])
-	 nop
-	mov.l	r0, @(OFF_R0,r15)	! Save return value
-	!			Reload R0-R4 from kernel stack, where the
-	!   	    	    	parent may have modified them using
-	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
-	!   	    	    	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
-	mov.l	@(OFF_R7,r15), r7   ! arg3
-	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
-	!
-	mov.l	6f, r10			! Number of syscalls
-	cmp/hs	r10, r3
-	bf	syscall_call
-	mov	#-ENOSYS, r0
-	bra	syscall_exit
-	 mov.l	r0, @(OFF_R0,r15)	! Return value
-
 __restore_all:
 	mov	#OFF_SR, r0
 	mov.l	@(r0,r15), r0	! get status register
@@ -388,6 +360,35 @@ syscall_exit:
 	bf	syscall_exit_work
 	bra	__restore_all
 	 nop
+
+	.align	2
+syscall_trace_entry:
+	!                     	Yes it is traced.
+	mov     r15, r4
+	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
+	jsr	@r11	    	! superior (will chomp R[0-7])
+	 nop
+	mov.l	r0, @(OFF_R0,r15)	! Save return value
+	!			Reload R0-R4 from kernel stack, where the
+	!   	    	    	parent may have modified them using
+	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
+	!   	    	    	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
+	mov.l	@(OFF_R7,r15), r7   ! arg3
+	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
+	!
+	mov.l	6f, r10			! Number of syscalls
+	cmp/hs	r10, r3
+	bf	syscall_call
+	mov	#-ENOSYS, r0
+	bra	syscall_exit
+	 mov.l	r0, @(OFF_R0,r15)	! Return value
+
 	.align	2
 #if !defined(CONFIG_CPU_SH2)
 1:	.long	TRA
-- 
2.28.0.rc1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-07-22 23:13 [PATCH 1/4] sh: Fix validation of system call number Michael Karcher
  2020-07-22 23:13 ` [PATCH 2/4] sh: Rearrange blocks in entry-common.S Michael Karcher
@ 2020-07-22 23:13 ` Michael Karcher
  2020-07-22 23:20   ` John Paul Adrian Glaubitz
  2020-08-28 15:50   ` Rich Felker
  2020-07-22 23:13 ` [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures Michael Karcher
  2020-07-22 23:19 ` [PATCH 1/4] sh: Fix validation of system call number John Paul Adrian Glaubitz
  3 siblings, 2 replies; 20+ messages in thread
From: Michael Karcher @ 2020-07-22 23:13 UTC (permalink / raw)
  To: linux-sh, linux-kernel
  Cc: Yoshinori Sato, Rich Felker, Adrian Glaubitz, Michael Karcher

Port sh to use the new SECCOMP_FILTER code.

Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
---
 arch/sh/Kconfig                               | 1 +
 arch/sh/kernel/entry-common.S                 | 2 ++
 arch/sh/kernel/ptrace_32.c                    | 5 +++--
 tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +++++++-
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 32d959849df9..10b510c16841 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -27,6 +27,7 @@ config SUPERH
 	select GENERIC_SMP_IDLE_THREAD
 	select GUP_GET_PTE_LOW_HIGH if X2TLB
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_DEBUG_BUGVERBOSE
diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
index c4d88d61890d..ad963104d22d 100644
--- a/arch/sh/kernel/entry-common.S
+++ b/arch/sh/kernel/entry-common.S
@@ -368,6 +368,8 @@ syscall_trace_entry:
 	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
 	jsr	@r11	    	! superior (will chomp R[0-7])
 	 nop
+	cmp/eq	#-1, r0
+	bt	syscall_exit
 	mov.l	r0, @(OFF_R0,r15)	! Save return value
 	!			Reload R0-R4 from kernel stack, where the
 	!   	    	    	parent may have modified them using
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 64bfb714943e..25ccfbd02bfa 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -485,8 +485,6 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
 {
 	long ret = 0;
 
-	secure_computing_strict(regs->regs[0]);
-
 	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
 	    tracehook_report_syscall_entry(regs))
 		/*
@@ -496,6 +494,9 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
 		 */
 		ret = -1L;
 
+	if (secure_computing() == -1)
+		return -1;
+
 	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
 		trace_sys_enter(regs, regs->regs[0]);
 
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 252140a52553..6eb21685c88f 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -122,6 +122,8 @@ struct seccomp_data {
 #  define __NR_seccomp 358
 # elif defined(__s390__)
 #  define __NR_seccomp 348
+# elif defined(__sh__)
+#  define __NR_seccomp 372
 # else
 #  warning "seccomp syscall number unknown for this architecture"
 #  define __NR_seccomp 0xffff
@@ -1622,6 +1624,10 @@ TEST_F(TRACE_poke, getpid_runs_normally)
 # define SYSCALL_SYSCALL_NUM regs[4]
 # define SYSCALL_RET	regs[2]
 # define SYSCALL_NUM_RET_SHARE_REG
+#elif defined(__sh__)
+# define ARCH_REGS	struct pt_regs
+# define SYSCALL_NUM	gpr[3]
+# define SYSCALL_RET	gpr[0]
 #else
 # error "Do not know how to find your architecture's registers and syscalls"
 #endif
@@ -1693,7 +1699,7 @@ void change_syscall(struct __test_metadata *_metadata,
 	EXPECT_EQ(0, ret) {}
 
 #if defined(__x86_64__) || defined(__i386__) || defined(__powerpc__) || \
-	defined(__s390__) || defined(__hppa__) || defined(__riscv)
+	defined(__s390__) || defined(__hppa__) || defined(__riscv) || defined(__sh__)
 	{
 		regs.SYSCALL_NUM = syscall;
 	}
-- 
2.28.0.rc1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures
  2020-07-22 23:13 [PATCH 1/4] sh: Fix validation of system call number Michael Karcher
  2020-07-22 23:13 ` [PATCH 2/4] sh: Rearrange blocks in entry-common.S Michael Karcher
  2020-07-22 23:13 ` [PATCH 3/4] sh: Add SECCOMP_FILTER Michael Karcher
@ 2020-07-22 23:13 ` Michael Karcher
  2020-07-22 23:20   ` John Paul Adrian Glaubitz
  2020-07-22 23:19 ` [PATCH 1/4] sh: Fix validation of system call number John Paul Adrian Glaubitz
  3 siblings, 1 reply; 20+ messages in thread
From: Michael Karcher @ 2020-07-22 23:13 UTC (permalink / raw)
  To: linux-sh, linux-kernel
  Cc: Yoshinori Sato, Rich Felker, Adrian Glaubitz, Michael Karcher

Other architectures expect that syscall_set_return_value gets an already
negative value as error. That's also what kernel/seccomp.c provides.

Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
---
 arch/sh/include/asm/syscall_32.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/sh/include/asm/syscall_32.h b/arch/sh/include/asm/syscall_32.h
index 0b5b8e75edac..cb51a7528384 100644
--- a/arch/sh/include/asm/syscall_32.h
+++ b/arch/sh/include/asm/syscall_32.h
@@ -40,10 +40,7 @@ static inline void syscall_set_return_value(struct task_struct *task,
 					    struct pt_regs *regs,
 					    int error, long val)
 {
-	if (error)
-		regs->regs[0] = -error;
-	else
-		regs->regs[0] = val;
+	regs->regs[0] = (long) error ?: val;
 }
 
 static inline void syscall_get_arguments(struct task_struct *task,
-- 
2.28.0.rc1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/4] sh: Fix validation of system call number
  2020-07-22 23:13 [PATCH 1/4] sh: Fix validation of system call number Michael Karcher
                   ` (2 preceding siblings ...)
  2020-07-22 23:13 ` [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures Michael Karcher
@ 2020-07-22 23:19 ` John Paul Adrian Glaubitz
  3 siblings, 0 replies; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-22 23:19 UTC (permalink / raw)
  To: Michael Karcher, linux-sh, linux-kernel; +Cc: Yoshinori Sato, Rich Felker

On 7/23/20 1:13 AM, Michael Karcher wrote:
> The slow path for traced system call entries accessed a wrong memory
> location to get the number of the maximum allowed system call number.
> Renumber the numbered "local" label for the correct location to avoid
> collisions with actual local labels.
> 
> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> ---
>  arch/sh/kernel/entry-common.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
> index 956a7a03b0c8..9bac5bbb67f3 100644
> --- a/arch/sh/kernel/entry-common.S
> +++ b/arch/sh/kernel/entry-common.S
> @@ -199,7 +199,7 @@ syscall_trace_entry:
>  	mov.l	@(OFF_R7,r15), r7   ! arg3
>  	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
>  	!
> -	mov.l	2f, r10			! Number of syscalls
> +	mov.l	6f, r10			! Number of syscalls
>  	cmp/hs	r10, r3
>  	bf	syscall_call
>  	mov	#-ENOSYS, r0
> @@ -353,7 +353,7 @@ ENTRY(system_call)
>  	tst	r9, r8
>  	bf	syscall_trace_entry
>  	!
> -	mov.l	2f, r8			! Number of syscalls
> +	mov.l	6f, r8			! Number of syscalls
>  	cmp/hs	r8, r3
>  	bt	syscall_badsys
>  	!
> @@ -392,7 +392,7 @@ syscall_exit:
>  #if !defined(CONFIG_CPU_SH2)
>  1:	.long	TRA
>  #endif
> -2:	.long	NR_syscalls
> +6:	.long	NR_syscalls
>  3:	.long	sys_call_table
>  7:	.long	do_syscall_trace_enter
>  8:	.long	do_syscall_trace_leave
> 

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] sh: Rearrange blocks in entry-common.S
  2020-07-22 23:13 ` [PATCH 2/4] sh: Rearrange blocks in entry-common.S Michael Karcher
@ 2020-07-22 23:20   ` John Paul Adrian Glaubitz
  0 siblings, 0 replies; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-22 23:20 UTC (permalink / raw)
  To: Michael Karcher, linux-sh, linux-kernel; +Cc: Yoshinori Sato, Rich Felker

On 7/23/20 1:13 AM, Michael Karcher wrote:
> This avoids out-of-range jumps that get auto-replaced by the assembler
> and prepares for the changes needed to implement SECCOMP_FILTER cleanly.
> 
> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> ---
>  arch/sh/kernel/entry-common.S | 57 ++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
> index 9bac5bbb67f3..c4d88d61890d 100644
> --- a/arch/sh/kernel/entry-common.S
> +++ b/arch/sh/kernel/entry-common.S
> @@ -178,34 +178,6 @@ syscall_exit_work:
>  	bra	resume_userspace
>  	 nop
>  
> -	.align	2
> -syscall_trace_entry:
> -	!                     	Yes it is traced.
> -	mov     r15, r4
> -	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
> -	jsr	@r11	    	! superior (will chomp R[0-7])
> -	 nop
> -	mov.l	r0, @(OFF_R0,r15)	! Save return value
> -	!			Reload R0-R4 from kernel stack, where the
> -	!   	    	    	parent may have modified them using
> -	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
> -	!   	    	    	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
> -	mov.l	@(OFF_R7,r15), r7   ! arg3
> -	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
> -	!
> -	mov.l	6f, r10			! Number of syscalls
> -	cmp/hs	r10, r3
> -	bf	syscall_call
> -	mov	#-ENOSYS, r0
> -	bra	syscall_exit
> -	 mov.l	r0, @(OFF_R0,r15)	! Return value
> -
>  __restore_all:
>  	mov	#OFF_SR, r0
>  	mov.l	@(r0,r15), r0	! get status register
> @@ -388,6 +360,35 @@ syscall_exit:
>  	bf	syscall_exit_work
>  	bra	__restore_all
>  	 nop
> +
> +	.align	2
> +syscall_trace_entry:
> +	!                     	Yes it is traced.
> +	mov     r15, r4
> +	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
> +	jsr	@r11	    	! superior (will chomp R[0-7])
> +	 nop
> +	mov.l	r0, @(OFF_R0,r15)	! Save return value
> +	!			Reload R0-R4 from kernel stack, where the
> +	!   	    	    	parent may have modified them using
> +	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
> +	!   	    	    	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
> +	mov.l	@(OFF_R7,r15), r7   ! arg3
> +	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
> +	!
> +	mov.l	6f, r10			! Number of syscalls
> +	cmp/hs	r10, r3
> +	bf	syscall_call
> +	mov	#-ENOSYS, r0
> +	bra	syscall_exit
> +	 mov.l	r0, @(OFF_R0,r15)	! Return value
> +
>  	.align	2
>  #if !defined(CONFIG_CPU_SH2)
>  1:	.long	TRA
> 

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-07-22 23:13 ` [PATCH 3/4] sh: Add SECCOMP_FILTER Michael Karcher
@ 2020-07-22 23:20   ` John Paul Adrian Glaubitz
  2020-08-28 15:50   ` Rich Felker
  1 sibling, 0 replies; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-22 23:20 UTC (permalink / raw)
  To: Michael Karcher, linux-sh, linux-kernel; +Cc: Yoshinori Sato, Rich Felker

On 7/23/20 1:13 AM, Michael Karcher wrote:
> Port sh to use the new SECCOMP_FILTER code.
> 
> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> ---
>  arch/sh/Kconfig                               | 1 +
>  arch/sh/kernel/entry-common.S                 | 2 ++
>  arch/sh/kernel/ptrace_32.c                    | 5 +++--
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +++++++-
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
> index 32d959849df9..10b510c16841 100644
> --- a/arch/sh/Kconfig
> +++ b/arch/sh/Kconfig
> @@ -27,6 +27,7 @@ config SUPERH
>  	select GENERIC_SMP_IDLE_THREAD
>  	select GUP_GET_PTE_LOW_HIGH if X2TLB
>  	select HAVE_ARCH_AUDITSYSCALL
> +	select HAVE_ARCH_SECCOMP_FILTER
>  	select HAVE_ARCH_KGDB
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_DEBUG_BUGVERBOSE
> diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
> index c4d88d61890d..ad963104d22d 100644
> --- a/arch/sh/kernel/entry-common.S
> +++ b/arch/sh/kernel/entry-common.S
> @@ -368,6 +368,8 @@ syscall_trace_entry:
>  	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
>  	jsr	@r11	    	! superior (will chomp R[0-7])
>  	 nop
> +	cmp/eq	#-1, r0
> +	bt	syscall_exit
>  	mov.l	r0, @(OFF_R0,r15)	! Save return value
>  	!			Reload R0-R4 from kernel stack, where the
>  	!   	    	    	parent may have modified them using
> diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
> index 64bfb714943e..25ccfbd02bfa 100644
> --- a/arch/sh/kernel/ptrace_32.c
> +++ b/arch/sh/kernel/ptrace_32.c
> @@ -485,8 +485,6 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>  {
>  	long ret = 0;
>  
> -	secure_computing_strict(regs->regs[0]);
> -
>  	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
>  	    tracehook_report_syscall_entry(regs))
>  		/*
> @@ -496,6 +494,9 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>  		 */
>  		ret = -1L;
>  
> +	if (secure_computing() == -1)
> +		return -1;
> +
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>  		trace_sys_enter(regs, regs->regs[0]);
>  
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 252140a52553..6eb21685c88f 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -122,6 +122,8 @@ struct seccomp_data {
>  #  define __NR_seccomp 358
>  # elif defined(__s390__)
>  #  define __NR_seccomp 348
> +# elif defined(__sh__)
> +#  define __NR_seccomp 372
>  # else
>  #  warning "seccomp syscall number unknown for this architecture"
>  #  define __NR_seccomp 0xffff
> @@ -1622,6 +1624,10 @@ TEST_F(TRACE_poke, getpid_runs_normally)
>  # define SYSCALL_SYSCALL_NUM regs[4]
>  # define SYSCALL_RET	regs[2]
>  # define SYSCALL_NUM_RET_SHARE_REG
> +#elif defined(__sh__)
> +# define ARCH_REGS	struct pt_regs
> +# define SYSCALL_NUM	gpr[3]
> +# define SYSCALL_RET	gpr[0]
>  #else
>  # error "Do not know how to find your architecture's registers and syscalls"
>  #endif
> @@ -1693,7 +1699,7 @@ void change_syscall(struct __test_metadata *_metadata,
>  	EXPECT_EQ(0, ret) {}
>  
>  #if defined(__x86_64__) || defined(__i386__) || defined(__powerpc__) || \
> -	defined(__s390__) || defined(__hppa__) || defined(__riscv)
> +	defined(__s390__) || defined(__hppa__) || defined(__riscv) || defined(__sh__)
>  	{
>  		regs.SYSCALL_NUM = syscall;
>  	}
> 

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures
  2020-07-22 23:13 ` [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures Michael Karcher
@ 2020-07-22 23:20   ` John Paul Adrian Glaubitz
  0 siblings, 0 replies; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-07-22 23:20 UTC (permalink / raw)
  To: Michael Karcher, linux-sh, linux-kernel; +Cc: Yoshinori Sato, Rich Felker

On 7/23/20 1:13 AM, Michael Karcher wrote:
> Other architectures expect that syscall_set_return_value gets an already
> negative value as error. That's also what kernel/seccomp.c provides.
> 
> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> ---
>  arch/sh/include/asm/syscall_32.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/sh/include/asm/syscall_32.h b/arch/sh/include/asm/syscall_32.h
> index 0b5b8e75edac..cb51a7528384 100644
> --- a/arch/sh/include/asm/syscall_32.h
> +++ b/arch/sh/include/asm/syscall_32.h
> @@ -40,10 +40,7 @@ static inline void syscall_set_return_value(struct task_struct *task,
>  					    struct pt_regs *regs,
>  					    int error, long val)
>  {
> -	if (error)
> -		regs->regs[0] = -error;
> -	else
> -		regs->regs[0] = val;
> +	regs->regs[0] = (long) error ?: val;
>  }
>  
>  static inline void syscall_get_arguments(struct task_struct *task,
> 

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-07-22 23:13 ` [PATCH 3/4] sh: Add SECCOMP_FILTER Michael Karcher
  2020-07-22 23:20   ` John Paul Adrian Glaubitz
@ 2020-08-28 15:50   ` Rich Felker
  2020-08-28 16:21     ` John Paul Adrian Glaubitz
  2020-08-28 16:30     ` Rich Felker
  1 sibling, 2 replies; 20+ messages in thread
From: Rich Felker @ 2020-08-28 15:50 UTC (permalink / raw)
  To: Michael Karcher; +Cc: linux-sh, linux-kernel, Yoshinori Sato, Adrian Glaubitz

On Thu, Jul 23, 2020 at 01:13:21AM +0200, Michael Karcher wrote:
> Port sh to use the new SECCOMP_FILTER code.
> 
> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> ---
>  arch/sh/Kconfig                               | 1 +
>  arch/sh/kernel/entry-common.S                 | 2 ++
>  arch/sh/kernel/ptrace_32.c                    | 5 +++--
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +++++++-
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
> index 32d959849df9..10b510c16841 100644
> --- a/arch/sh/Kconfig
> +++ b/arch/sh/Kconfig
> @@ -27,6 +27,7 @@ config SUPERH
>  	select GENERIC_SMP_IDLE_THREAD
>  	select GUP_GET_PTE_LOW_HIGH if X2TLB
>  	select HAVE_ARCH_AUDITSYSCALL
> +	select HAVE_ARCH_SECCOMP_FILTER
>  	select HAVE_ARCH_KGDB
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_DEBUG_BUGVERBOSE
> diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
> index c4d88d61890d..ad963104d22d 100644
> --- a/arch/sh/kernel/entry-common.S
> +++ b/arch/sh/kernel/entry-common.S
> @@ -368,6 +368,8 @@ syscall_trace_entry:
>  	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
>  	jsr	@r11	    	! superior (will chomp R[0-7])
>  	 nop
> +	cmp/eq	#-1, r0
> +	bt	syscall_exit
>  	mov.l	r0, @(OFF_R0,r15)	! Save return value
>  	!			Reload R0-R4 from kernel stack, where the
>  	!   	    	    	parent may have modified them using
> diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
> index 64bfb714943e..25ccfbd02bfa 100644
> --- a/arch/sh/kernel/ptrace_32.c
> +++ b/arch/sh/kernel/ptrace_32.c
> @@ -485,8 +485,6 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>  {
>  	long ret = 0;
>  
> -	secure_computing_strict(regs->regs[0]);
> -
>  	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
>  	    tracehook_report_syscall_entry(regs))
>  		/*
> @@ -496,6 +494,9 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>  		 */
>  		ret = -1L;
>  
> +	if (secure_computing() == -1)
> +		return -1;
> +
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>  		trace_sys_enter(regs, regs->regs[0]);
>  

This patch broke strace - it spews out bogus syscalls and gets the
tracee hung. I suspect the last hunk is wrong and breaks all
non-seccomp tracing. I'll follow up with further analysis and possibly
a fix if you don't find one sooner.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-28 15:50   ` Rich Felker
@ 2020-08-28 16:21     ` John Paul Adrian Glaubitz
  2020-08-28 16:30     ` Rich Felker
  1 sibling, 0 replies; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-08-28 16:21 UTC (permalink / raw)
  To: Rich Felker, Michael Karcher; +Cc: linux-sh, linux-kernel, Yoshinori Sato

On 8/28/20 5:50 PM, Rich Felker wrote:
> This patch broke strace - it spews out bogus syscalls and gets the
> tracee hung. I suspect the last hunk is wrong and breaks all
> non-seccomp tracing. I'll follow up with further analysis and possibly
> a fix if you don't find one sooner.
Hmm, it does not hang for me but it looks suspicious:

root@tirpitz:~> strace ./test
execve("./test", ["./test"], 0x7be59690 /* 24 vars */) = 0
brk(NULL)                               = 0x52abc000
uname({sysname="Linux", nodename="tirpitz", ...}) = 0
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=67578, ...}) = 0
mmap2(NULL, 67578, PROT_READ, MAP_PRIVATE, 3, 0) = 0x29584000
close(3)                                = 0
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 EPERM (Operation not permitted)
openat(AT_FDCWD, "/lib/sh4-linux-gnu/tls/sh4a/libc.so.6", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = -1 ENOENT (No such file or directory)
stat64("/lib/sh4-linux-gnu/tls/sh4a", 0x7b89fb2c) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib/sh4-linux-gnu/tls/libc.so.6", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = -1 ENOENT (No such file or directory)
stat64("/lib/sh4-linux-gnu/tls", 0x7b89fb2c) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib/sh4-linux-gnu/sh4a/libc.so.6", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = -1 ENOENT (No such file or directory)
stat64("/lib/sh4-linux-gnu/sh4a", 0x7b89fb2c) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib/sh4-linux-gnu/libc.so.6", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0*\0\1\0\0\0t\306\1\0004\0\0\0"..., 512) = 512
pread64(3, "\4\0\0\0\20\0\0\0\1\0\0\0GNU\0\0\0\0\0\3\0\0\0\2\0\0\0\0\0\0\0", 32, 1290836) = 32
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 EPERM (Operation not permitted)
close(3)                                = 0
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 EPERM (Operation not permitted)
writev(2, [{iov_base="./test", iov_len=6}, {iov_base=": ", iov_len=2}, {iov_base="error while loading shared libra"..., iov_len=36}, {iov_base=": ", iov_len=2}, {iov_base="", iov_len=0}, {iov_base="", iov_len=0}, {iov_base="out of memory", iov_len=13}, {iov_base=": ", iov_len=2}, {iov_base="Operation not permitted", iov_len=23}, {iov_base="\n", iov_len=1}], 10./test: error while loading shared libraries: out of memory: Operation not permitted
) = 85
exit_group(127)                         = ?
+++ exited with 127 +++
root@tirpitz:~>

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-28 15:50   ` Rich Felker
  2020-08-28 16:21     ` John Paul Adrian Glaubitz
@ 2020-08-28 16:30     ` Rich Felker
  2020-08-28 16:38       ` John Paul Adrian Glaubitz
  1 sibling, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-08-28 16:30 UTC (permalink / raw)
  To: Michael Karcher; +Cc: linux-sh, linux-kernel, Yoshinori Sato, Adrian Glaubitz

On Fri, Aug 28, 2020 at 11:50:25AM -0400, Rich Felker wrote:
> On Thu, Jul 23, 2020 at 01:13:21AM +0200, Michael Karcher wrote:
> > Port sh to use the new SECCOMP_FILTER code.
> > 
> > Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> > ---
> >  arch/sh/Kconfig                               | 1 +
> >  arch/sh/kernel/entry-common.S                 | 2 ++
> >  arch/sh/kernel/ptrace_32.c                    | 5 +++--
> >  tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +++++++-
> >  4 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
> > index 32d959849df9..10b510c16841 100644
> > --- a/arch/sh/Kconfig
> > +++ b/arch/sh/Kconfig
> > @@ -27,6 +27,7 @@ config SUPERH
> >  	select GENERIC_SMP_IDLE_THREAD
> >  	select GUP_GET_PTE_LOW_HIGH if X2TLB
> >  	select HAVE_ARCH_AUDITSYSCALL
> > +	select HAVE_ARCH_SECCOMP_FILTER
> >  	select HAVE_ARCH_KGDB
> >  	select HAVE_ARCH_TRACEHOOK
> >  	select HAVE_DEBUG_BUGVERBOSE
> > diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
> > index c4d88d61890d..ad963104d22d 100644
> > --- a/arch/sh/kernel/entry-common.S
> > +++ b/arch/sh/kernel/entry-common.S
> > @@ -368,6 +368,8 @@ syscall_trace_entry:
> >  	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
> >  	jsr	@r11	    	! superior (will chomp R[0-7])
> >  	 nop
> > +	cmp/eq	#-1, r0
> > +	bt	syscall_exit
> >  	mov.l	r0, @(OFF_R0,r15)	! Save return value
> >  	!			Reload R0-R4 from kernel stack, where the
> >  	!   	    	    	parent may have modified them using
> > diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
> > index 64bfb714943e..25ccfbd02bfa 100644
> > --- a/arch/sh/kernel/ptrace_32.c
> > +++ b/arch/sh/kernel/ptrace_32.c
> > @@ -485,8 +485,6 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> >  {
> >  	long ret = 0;
> >  
> > -	secure_computing_strict(regs->regs[0]);
> > -
> >  	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> >  	    tracehook_report_syscall_entry(regs))
> >  		/*
> > @@ -496,6 +494,9 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> >  		 */
> >  		ret = -1L;
> >  
> > +	if (secure_computing() == -1)
> > +		return -1;
> > +
> >  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> >  		trace_sys_enter(regs, regs->regs[0]);
> >  
> 
> This patch broke strace - it spews out bogus syscalls and gets the
> tracee hung. I suspect the last hunk is wrong and breaks all
> non-seccomp tracing. I'll follow up with further analysis and possibly
> a fix if you don't find one sooner.

It looks like the problem is actually the hunk in entry-common.S, but
this code has been wrong since ab99c733ae in 2008: it was storing the
return value of do_syscall_trace_enter, which is supposed to replace
the syscall number and make it fail, in r0 (the 5th argument) rather
than r3 (the syscall number). This looks like the reason you put the
(apparently wrong) branch to syscall_exit in there -- the existing
code was not actually causing ENOSYS when do_syscall_trace_enter tried
to replace nr with -1, because the -1 was put in the wrong place.

I'm guessing something in syscall_exit assumes the registers have been
reloaded (the code skipped by your branch) and blows up when they
haven't.

I think the right change is going to be something like replacing 
mov.l r0, @(OFF_R0,r15) with mov r0, r3 and getting rid of the r3
reload below. do_syscall_trace_enter should also be returning
regs->regs[3] in the success case, not regs->regs[0] as it's doing, at
least if it's to match other archs (that return the original syscall
number on success). In any case, returning the 5th argument register
is nonsense.

I'm about to test a patch along these lines and will report what I
find.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-28 16:30     ` Rich Felker
@ 2020-08-28 16:38       ` John Paul Adrian Glaubitz
  2020-08-28 17:03         ` Rich Felker
  0 siblings, 1 reply; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-08-28 16:38 UTC (permalink / raw)
  To: Rich Felker, Michael Karcher; +Cc: linux-sh, linux-kernel, Yoshinori Sato

Hi!

On 8/28/20 6:30 PM, Rich Felker wrote:
> I'm about to test a patch along these lines and will report what I
> find.

Let me know when you have something to test and I will test the patch as
well, making sure we're not breaking seccomp again.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-28 16:38       ` John Paul Adrian Glaubitz
@ 2020-08-28 17:03         ` Rich Felker
  2020-08-29  0:49           ` Rich Felker
  0 siblings, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-08-28 17:03 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz
  Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

On Fri, Aug 28, 2020 at 06:38:09PM +0200, John Paul Adrian Glaubitz wrote:
> Hi!
> 
> On 8/28/20 6:30 PM, Rich Felker wrote:
> > I'm about to test a patch along these lines and will report what I
> > find.
> 
> Let me know when you have something to test and I will test the patch as
> well, making sure we're not breaking seccomp again.

If you have a seccomp test setup, please try the following patch. I'm
not sure if the end result is entirely correct, but I believe it's
at least much closer to correct than the code was before or after
adding SECCOMP_FILTER.


diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
index ad963104d22d..0560a8054215 100644
--- a/arch/sh/kernel/entry-common.S
+++ b/arch/sh/kernel/entry-common.S
@@ -368,9 +368,6 @@ syscall_trace_entry:
 	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
 	jsr	@r11	    	! superior (will chomp R[0-7])
 	 nop
-	cmp/eq	#-1, r0
-	bt	syscall_exit
-	mov.l	r0, @(OFF_R0,r15)	! Save return value
 	!			Reload R0-R4 from kernel stack, where the
 	!   	    	    	parent may have modified them using
 	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
@@ -382,7 +379,7 @@ syscall_trace_entry:
 	mov.l	@(OFF_R5,r15), r5
 	mov.l	@(OFF_R6,r15), r6
 	mov.l	@(OFF_R7,r15), r7   ! arg3
-	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
+	mov	r0, r3              ! syscall_nr, possibly changed to -1
 	!
 	mov.l	6f, r10			! Number of syscalls
 	cmp/hs	r10, r3
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 25ccfbd02bfa..9e86cff041c7 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -503,7 +503,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
 	audit_syscall_entry(regs->regs[3], regs->regs[4], regs->regs[5],
 			    regs->regs[6], regs->regs[7]);
 
-	return ret ?: regs->regs[0];
+	return ret ?: regs->regs[3];
 }
 
 asmlinkage void do_syscall_trace_leave(struct pt_regs *regs)

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-28 17:03         ` Rich Felker
@ 2020-08-29  0:49           ` Rich Felker
  2020-08-29 11:09             ` John Paul Adrian Glaubitz
  0 siblings, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-08-29  0:49 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz
  Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

On Fri, Aug 28, 2020 at 01:03:00PM -0400, Rich Felker wrote:
> On Fri, Aug 28, 2020 at 06:38:09PM +0200, John Paul Adrian Glaubitz wrote:
> > Hi!
> > 
> > On 8/28/20 6:30 PM, Rich Felker wrote:
> > > I'm about to test a patch along these lines and will report what I
> > > find.
> > 
> > Let me know when you have something to test and I will test the patch as
> > well, making sure we're not breaking seccomp again.
> 
> If you have a seccomp test setup, please try the following patch. I'm
> not sure if the end result is entirely correct, but I believe it's
> at least much closer to correct than the code was before or after
> adding SECCOMP_FILTER.
> 
> 
> diff --git a/arch/sh/kernel/entry-common.S b/arch/sh/kernel/entry-common.S
> index ad963104d22d..0560a8054215 100644
> --- a/arch/sh/kernel/entry-common.S
> +++ b/arch/sh/kernel/entry-common.S
> @@ -368,9 +368,6 @@ syscall_trace_entry:
>  	mov.l	7f, r11		! Call do_syscall_trace_enter which notifies
>  	jsr	@r11	    	! superior (will chomp R[0-7])
>  	 nop
> -	cmp/eq	#-1, r0
> -	bt	syscall_exit
> -	mov.l	r0, @(OFF_R0,r15)	! Save return value
>  	!			Reload R0-R4 from kernel stack, where the
>  	!   	    	    	parent may have modified them using
>  	!   	    	    	ptrace(POKEUSR).  (Note that R0-R2 are
> @@ -382,7 +379,7 @@ syscall_trace_entry:
>  	mov.l	@(OFF_R5,r15), r5
>  	mov.l	@(OFF_R6,r15), r6
>  	mov.l	@(OFF_R7,r15), r7   ! arg3
> -	mov.l	@(OFF_R3,r15), r3   ! syscall_nr
> +	mov	r0, r3              ! syscall_nr, possibly changed to -1
>  	!
>  	mov.l	6f, r10			! Number of syscalls
>  	cmp/hs	r10, r3
> diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
> index 25ccfbd02bfa..9e86cff041c7 100644
> --- a/arch/sh/kernel/ptrace_32.c
> +++ b/arch/sh/kernel/ptrace_32.c
> @@ -503,7 +503,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>  	audit_syscall_entry(regs->regs[3], regs->regs[4], regs->regs[5],
>  			    regs->regs[6], regs->regs[7]);
>  
> -	return ret ?: regs->regs[0];
> +	return ret ?: regs->regs[3];
>  }
>  
>  asmlinkage void do_syscall_trace_leave(struct pt_regs *regs)

This restored my ability to use strace, and I've written and tested a
minimal strace-like hack using SECCOMP_RET_USER_NOTIF that works as
expected on both j2 and qemu-system-sh4, so I think the above is
correct.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-29  0:49           ` Rich Felker
@ 2020-08-29 11:09             ` John Paul Adrian Glaubitz
  2020-09-03  3:56               ` Rich Felker
  0 siblings, 1 reply; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-08-29 11:09 UTC (permalink / raw)
  To: Rich Felker; +Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

Hi!

On 8/29/20 2:49 AM, Rich Felker wrote:
> This restored my ability to use strace

I can confirm that. However ...

> and I've written and tested a minimal strace-like hack using
> SECCOMP_RET_USER_NOTIF that works as
> expected on both j2 and qemu-system-sh4, so I think the above is
> correct.

The seccomp live testsuite has regressed.

With your patch:

=============== Sat 29 Aug 2020 12:35:52 PM CEST ===============
Regression Test Report ("regression -T live")
 batch name: 01-sim-allow
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 02-sim-basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 03-sim-basic_chains
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 04-sim-multilevel_chains
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 05-sim-long_jumps
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 06-sim-actions
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 07-sim-db_bug_looping
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 08-sim-subtree_checks
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 09-sim-syscall_priority_pre
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 10-sim-syscall_priority_post
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 11-basic-basic_errors
 test mode:  c
 test type:  basic
 batch name: 12-sim-basic_masked_ops
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 13-basic-attrs
 test mode:  c
 test type:  basic
 batch name: 14-sim-reset
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 15-basic-resolver
 test mode:  c
 test type:  basic
 batch name: 16-sim-arch_basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 17-sim-arch_merge
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 18-sim-basic_allowlist
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 19-sim-missing_syscalls
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 20-live-basic_die
 test mode:  c
 test type:  live
Test 20-live-basic_die%%001-00001 result:   SUCCESS
Test 20-live-basic_die%%002-00001 result:   SUCCESS
Test 20-live-basic_die%%003-00001 result:   FAILURE 20-live-basic_die 1 ERRNO rc=38
 batch name: 21-live-basic_allow
 test mode:  c
 test type:  live
Test 21-live-basic_allow%%001-00001 result:   SUCCESS
 batch name: 22-sim-basic_chains_array
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 23-sim-arch_all_le_basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 24-live-arg_allow
 test mode:  c
 test type:  live
Test 24-live-arg_allow%%001-00001 result:   SUCCESS
 batch name: 25-sim-multilevel_chains_adv
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 26-sim-arch_all_be_basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 27-sim-bpf_blk_state
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 28-sim-arch_x86
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 29-sim-pseudo_syscall
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 30-sim-socket_syscalls
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 31-basic-version_check
 test mode:  c
 test type:  basic
 batch name: 32-live-tsync_allow
 test mode:  c
 test type:  live
Test 32-live-tsync_allow%%001-00001 result:   SUCCESS
 batch name: 33-sim-socket_syscalls_be
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 34-sim-basic_denylist
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 35-sim-negative_one
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 36-sim-ipc_syscalls
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 37-sim-ipc_syscalls_be
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 38-basic-pfc_coverage
 test mode:  c
 test type:  basic
 batch name: 39-basic-api_level
 test mode:  c
 test type:  basic
 batch name: 40-sim-log
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 41-sim-syscall_priority_arch
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 42-sim-adv_chains
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 43-sim-a2_order
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 44-live-a2_order
 test mode:  c
 test type:  live
Test 44-live-a2_order%%001-00001 result:   FAILURE 44-live-a2_order 1 ALLOW rc=1
 batch name: 45-sim-chain_code_coverage
 test mode:  c
 test type:  bpf-sim
 batch name: 46-sim-kill_process
 test mode:  c
 test type:  bpf-sim
 batch name: 47-live-kill_process
 test mode:  c
 test type:  live
Test 47-live-kill_process%%001-00001 result:   SUCCESS
 batch name: 48-sim-32b_args
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 49-sim-64b_comparisons
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 50-sim-hash_collision
 test mode:  c
 test type:  bpf-sim
 batch name: 51-live-user_notification
 test mode:  c
 test type:  live
Test 51-live-user_notification%%001-00001 result:   FAILURE 51-live-user_notification 5 ALLOW rc=14
 batch name: 52-basic-load
 test mode:  c
 test type:  basic
 batch name: 53-sim-binary_tree
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 54-live-binary_tree
 test mode:  c
 test type:  live
Test 54-live-binary_tree%%001-00001 result:   SUCCESS
 batch name: 55-basic-pfc_binary_tree
 test mode:  c
 test type:  basic
 batch name: 56-basic-iterate_syscalls
 test mode:  c
 test type:  basic
 batch name: 57-basic-rawsysrc
 test mode:  c
 test type:  basic
 batch name: 58-live-tsync_notify
 test mode:  c
 test type:  live
Test 58-live-tsync_notify%%001-00001 result:   FAILURE 58-live-tsync_notify 6 ALLOW rc=14
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 7
 tests failed: 4
 tests errored: 0
============================================================

And without:

=============== Sat 29 Aug 2020 01:03:07 PM CEST ===============
Regression Test Report ("regression -T live")
 batch name: 01-sim-allow
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 02-sim-basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 03-sim-basic_chains
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 04-sim-multilevel_chains
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 05-sim-long_jumps
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 06-sim-actions
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 07-sim-db_bug_looping
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 08-sim-subtree_checks
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 09-sim-syscall_priority_pre
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 10-sim-syscall_priority_post
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 11-basic-basic_errors
 test mode:  c
 test type:  basic
 batch name: 12-sim-basic_masked_ops
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 13-basic-attrs
 test mode:  c
 test type:  basic
 batch name: 14-sim-reset
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 15-basic-resolver
 test mode:  c
 test type:  basic
 batch name: 16-sim-arch_basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 17-sim-arch_merge
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 18-sim-basic_allowlist
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 19-sim-missing_syscalls
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 20-live-basic_die
 test mode:  c
 test type:  live
Test 20-live-basic_die%%001-00001 result:   SUCCESS
Test 20-live-basic_die%%002-00001 result:   SUCCESS
Test 20-live-basic_die%%003-00001 result:   SUCCESS
 batch name: 21-live-basic_allow
 test mode:  c
 test type:  live
Test 21-live-basic_allow%%001-00001 result:   SUCCESS
 batch name: 22-sim-basic_chains_array
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 23-sim-arch_all_le_basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 24-live-arg_allow
 test mode:  c
 test type:  live
Test 24-live-arg_allow%%001-00001 result:   SUCCESS
 batch name: 25-sim-multilevel_chains_adv
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 26-sim-arch_all_be_basic
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 27-sim-bpf_blk_state
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 28-sim-arch_x86
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 29-sim-pseudo_syscall
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 30-sim-socket_syscalls
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 31-basic-version_check
 test mode:  c
 test type:  basic
 batch name: 32-live-tsync_allow
 test mode:  c
 test type:  live
Test 32-live-tsync_allow%%001-00001 result:   SUCCESS
 batch name: 33-sim-socket_syscalls_be
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 34-sim-basic_denylist
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 35-sim-negative_one
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 36-sim-ipc_syscalls
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 37-sim-ipc_syscalls_be
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 38-basic-pfc_coverage
 test mode:  c
 test type:  basic
 batch name: 39-basic-api_level
 test mode:  c
 test type:  basic
 batch name: 40-sim-log
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 41-sim-syscall_priority_arch
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 42-sim-adv_chains
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 43-sim-a2_order
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 44-live-a2_order
 test mode:  c
 test type:  live
Test 44-live-a2_order%%001-00001 result:   SUCCESS
 batch name: 45-sim-chain_code_coverage
 test mode:  c
 test type:  bpf-sim
 batch name: 46-sim-kill_process
 test mode:  c
 test type:  bpf-sim
 batch name: 47-live-kill_process
 test mode:  c
 test type:  live
Test 47-live-kill_process%%001-00001 result:   SUCCESS
 batch name: 48-sim-32b_args
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-sim-fuzz
 test mode:  c
 test type:  bpf-valgrind
 batch name: 49-sim-64b_comparisons
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 50-sim-hash_collision
 test mode:  c
 test type:  bpf-sim
 batch name: 51-live-user_notification
 test mode:  c
 test type:  live
Test 51-live-user_notification%%001-00001 result:   SUCCESS
 batch name: 52-basic-load
 test mode:  c
 test type:  basic
 batch name: 53-sim-binary_tree
 test mode:  c
 test type:  bpf-sim
 test mode:  c
 test type:  bpf-valgrind
 batch name: 54-live-binary_tree
 test mode:  c
 test type:  live
Test 54-live-binary_tree%%001-00001 result:   SUCCESS
 batch name: 55-basic-pfc_binary_tree
 test mode:  c
 test type:  basic
 batch name: 56-basic-iterate_syscalls
 test mode:  c
 test type:  basic
 batch name: 57-basic-rawsysrc
 test mode:  c
 test type:  basic
 batch name: 58-live-tsync_notify
 test mode:  c
 test type:  live
Test 58-live-tsync_notify%%001-00001 result:   SUCCESS
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0
============================================================

To test libseccomp, check out my superh branch from here:

> https://github.com/glaubitz/libseccomp/tree/superh

then build and test with:

# ./autogen.sh && ./configure && make && make check && make check-build && cd tests && ./regression -T live

Maybe Michael Karcher has any idea what's wrong with the strace stuff?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-08-29 11:09             ` John Paul Adrian Glaubitz
@ 2020-09-03  3:56               ` Rich Felker
  2020-09-03  5:46                 ` Rich Felker
  2020-09-03  6:03                 ` John Paul Adrian Glaubitz
  0 siblings, 2 replies; 20+ messages in thread
From: Rich Felker @ 2020-09-03  3:56 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz
  Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

On Sat, Aug 29, 2020 at 01:09:43PM +0200, John Paul Adrian Glaubitz wrote:
> Hi!
> 
> On 8/29/20 2:49 AM, Rich Felker wrote:
> > This restored my ability to use strace
> 
> I can confirm that. However ...
> 
> > and I've written and tested a minimal strace-like hack using
> > SECCOMP_RET_USER_NOTIF that works as
> > expected on both j2 and qemu-system-sh4, so I think the above is
> > correct.
> 
> The seccomp live testsuite has regressed.
> 
> With your patch:
> 
> =============== Sat 29 Aug 2020 12:35:52 PM CEST ===============
> Regression Test Report ("regression -T live")
>  batch name: 01-sim-allow
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 02-sim-basic
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 03-sim-basic_chains
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 04-sim-multilevel_chains
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 05-sim-long_jumps
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 06-sim-actions
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 07-sim-db_bug_looping
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 08-sim-subtree_checks
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 09-sim-syscall_priority_pre
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 10-sim-syscall_priority_post
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 11-basic-basic_errors
>  test mode:  c
>  test type:  basic
>  batch name: 12-sim-basic_masked_ops
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 13-basic-attrs
>  test mode:  c
>  test type:  basic
>  batch name: 14-sim-reset
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 15-basic-resolver
>  test mode:  c
>  test type:  basic
>  batch name: 16-sim-arch_basic
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 17-sim-arch_merge
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 18-sim-basic_allowlist
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 19-sim-missing_syscalls
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 20-live-basic_die
>  test mode:  c
>  test type:  live
> Test 20-live-basic_die%%001-00001 result:   SUCCESS
> Test 20-live-basic_die%%002-00001 result:   SUCCESS
> Test 20-live-basic_die%%003-00001 result:   FAILURE 20-live-basic_die 1 ERRNO rc=38
>  batch name: 21-live-basic_allow
>  test mode:  c
>  test type:  live
> Test 21-live-basic_allow%%001-00001 result:   SUCCESS
>  batch name: 22-sim-basic_chains_array
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 23-sim-arch_all_le_basic
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 24-live-arg_allow
>  test mode:  c
>  test type:  live
> Test 24-live-arg_allow%%001-00001 result:   SUCCESS
>  batch name: 25-sim-multilevel_chains_adv
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 26-sim-arch_all_be_basic
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 27-sim-bpf_blk_state
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 28-sim-arch_x86
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 29-sim-pseudo_syscall
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 30-sim-socket_syscalls
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 31-basic-version_check
>  test mode:  c
>  test type:  basic
>  batch name: 32-live-tsync_allow
>  test mode:  c
>  test type:  live
> Test 32-live-tsync_allow%%001-00001 result:   SUCCESS
>  batch name: 33-sim-socket_syscalls_be
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 34-sim-basic_denylist
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 35-sim-negative_one
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 36-sim-ipc_syscalls
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 37-sim-ipc_syscalls_be
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 38-basic-pfc_coverage
>  test mode:  c
>  test type:  basic
>  batch name: 39-basic-api_level
>  test mode:  c
>  test type:  basic
>  batch name: 40-sim-log
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 41-sim-syscall_priority_arch
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 42-sim-adv_chains
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 43-sim-a2_order
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 44-live-a2_order
>  test mode:  c
>  test type:  live
> Test 44-live-a2_order%%001-00001 result:   FAILURE 44-live-a2_order 1 ALLOW rc=1
>  batch name: 45-sim-chain_code_coverage
>  test mode:  c
>  test type:  bpf-sim
>  batch name: 46-sim-kill_process
>  test mode:  c
>  test type:  bpf-sim
>  batch name: 47-live-kill_process
>  test mode:  c
>  test type:  live
> Test 47-live-kill_process%%001-00001 result:   SUCCESS
>  batch name: 48-sim-32b_args
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-sim-fuzz
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 49-sim-64b_comparisons
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 50-sim-hash_collision
>  test mode:  c
>  test type:  bpf-sim
>  batch name: 51-live-user_notification
>  test mode:  c
>  test type:  live
> Test 51-live-user_notification%%001-00001 result:   FAILURE 51-live-user_notification 5 ALLOW rc=14

AFAICT, this test is buggy and cannot possibly work. It attempts to
have SYS_getpid return a 64-bit value and check that the returned
value matches. On 32-bit archs this will be truncated to 32 bits, but
the comparison in the caller still compares against the full 64-bit
value. I have no idea how this seemed to work before.

>  batch name: 52-basic-load
>  test mode:  c
>  test type:  basic
>  batch name: 53-sim-binary_tree
>  test mode:  c
>  test type:  bpf-sim
>  test mode:  c
>  test type:  bpf-valgrind
>  batch name: 54-live-binary_tree
>  test mode:  c
>  test type:  live
> Test 54-live-binary_tree%%001-00001 result:   SUCCESS
>  batch name: 55-basic-pfc_binary_tree
>  test mode:  c
>  test type:  basic
>  batch name: 56-basic-iterate_syscalls
>  test mode:  c
>  test type:  basic
>  batch name: 57-basic-rawsysrc
>  test mode:  c
>  test type:  basic
>  batch name: 58-live-tsync_notify
>  test mode:  c
>  test type:  live
> Test 58-live-tsync_notify%%001-00001 result:   FAILURE 58-live-tsync_notify 6 ALLOW rc=14

This is similar to 51.

I think the commonality of all the failures is that they deal with
return values set by seccomp filters for blocked syscalls, which are
getting clobbered by ENOSYS from the failed syscall here. So I do need
to keep the code path that jumps over the actual syscall if
do_syscall_trace_enter returns -1, but that means
do_syscall_trace_enter must now be responsible for setting the return
value in non-seccomp failure paths.

I'll experiment to see what's still needed if that change is made.

> [...]
> ============================================================
> 
> To test libseccomp, check out my superh branch from here:
> 
> > https://github.com/glaubitz/libseccomp/tree/superh
> 
> then build and test with:
> 
> # ./autogen.sh && ./configure && make && make check && make check-build && cd tests && ./regression -T live
> 
> Maybe Michael Karcher has any idea what's wrong with the strace stuff?

I'd welcome any input, but I think I'm on track to solving this either
way.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-09-03  3:56               ` Rich Felker
@ 2020-09-03  5:46                 ` Rich Felker
  2020-09-03  6:04                   ` John Paul Adrian Glaubitz
  2020-09-03  6:03                 ` John Paul Adrian Glaubitz
  1 sibling, 1 reply; 20+ messages in thread
From: Rich Felker @ 2020-09-03  5:46 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz
  Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

On Wed, Sep 02, 2020 at 11:56:04PM -0400, Rich Felker wrote:
> On Sat, Aug 29, 2020 at 01:09:43PM +0200, John Paul Adrian Glaubitz wrote:
> > Hi!
> > 
> > On 8/29/20 2:49 AM, Rich Felker wrote:
> > > This restored my ability to use strace
> > 
> > I can confirm that. However ...
> > 
> > > and I've written and tested a minimal strace-like hack using
> > > SECCOMP_RET_USER_NOTIF that works as
> > > expected on both j2 and qemu-system-sh4, so I think the above is
> > > correct.
> > 
> > The seccomp live testsuite has regressed.
> > 
> > [...]
> > Test 58-live-tsync_notify%%001-00001 result:   FAILURE 58-live-tsync_notify 6 ALLOW rc=14
> 
> This is similar to 51.
> 
> I think the commonality of all the failures is that they deal with
> return values set by seccomp filters for blocked syscalls, which are
> getting clobbered by ENOSYS from the failed syscall here. So I do need
> to keep the code path that jumps over the actual syscall if
> do_syscall_trace_enter returns -1, but that means
> do_syscall_trace_enter must now be responsible for setting the return
> value in non-seccomp failure paths.
> 
> I'll experiment to see what's still needed if that change is made.

OK, I think I have an explanation for the mechanism of the bug, and it
really is a combination of the 2008 bug (confusion of r0 vs r3) and
the SECCOMP_FILTER commit. When the syscall_trace_entry code path is
in use, a syscall with argument 5 having value -1 causes
do_syscall_trace_enter to return -1 (because it returns regs[0], which
contains argument 5), which the change in entry-common.S interprets as
a sign to skip the syscall and jump to syscall_exit, and things blow
up from there. In particular, SYS_mmap2 is almost always called with
-1 as the 5th argument (fd), and this is even more common on nommu
where SYS_brk does not work.

I'll follow up with a new proposed patch.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-09-03  3:56               ` Rich Felker
  2020-09-03  5:46                 ` Rich Felker
@ 2020-09-03  6:03                 ` John Paul Adrian Glaubitz
  1 sibling, 0 replies; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-09-03  6:03 UTC (permalink / raw)
  To: Rich Felker; +Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

Hi Richi!

On 9/3/20 5:56 AM, Rich Felker wrote:
>> Test 51-live-user_notification%%001-00001 result:   FAILURE 51-live-user_notification 5 ALLOW rc=14
> 
> AFAICT, this test is buggy and cannot possibly work. It attempts to
> have SYS_getpid return a 64-bit value and check that the returned
> value matches. On 32-bit archs this will be truncated to 32 bits, but
> the comparison in the caller still compares against the full 64-bit
> value. I have no idea how this seemed to work before.

You're actually right, I forgot about that. Michael discovered this bug as well
and it was consequently fixed:

> https://github.com/seccomp/libseccomp/commit/bee43d3e884788569860a384e6a38357785a3995

>> Test 58-live-tsync_notify%%001-00001 result:   FAILURE 58-live-tsync_notify 6 ALLOW rc=14
> 
> This is similar to 51.
> 
> I think the commonality of all the failures is that they deal with
> return values set by seccomp filters for blocked syscalls, which are
> getting clobbered by ENOSYS from the failed syscall here. So I do need
> to keep the code path that jumps over the actual syscall if
> do_syscall_trace_enter returns -1, but that means
> do_syscall_trace_enter must now be responsible for setting the return
> value in non-seccomp failure paths.

Same here:

> https://github.com/seccomp/libseccomp/commit/f0686d9de911e7ffcdc7364566c1d146e44657c2

Not sure about the other two tests. I can re-base and re-test.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-09-03  5:46                 ` Rich Felker
@ 2020-09-03  6:04                   ` John Paul Adrian Glaubitz
  2020-09-03  6:17                     ` Rich Felker
  0 siblings, 1 reply; 20+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-09-03  6:04 UTC (permalink / raw)
  To: Rich Felker; +Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

On 9/3/20 7:46 AM, Rich Felker wrote:
> 
> OK, I think I have an explanation for the mechanism of the bug, and it
> really is a combination of the 2008 bug (confusion of r0 vs r3) and
> the SECCOMP_FILTER commit. When the syscall_trace_entry code path is
> in use, a syscall with argument 5 having value -1 causes
> do_syscall_trace_enter to return -1 (because it returns regs[0], which
> contains argument 5), which the change in entry-common.S interprets as
> a sign to skip the syscall and jump to syscall_exit, and things blow
> up from there. In particular, SYS_mmap2 is almost always called with
> -1 as the 5th argument (fd), and this is even more common on nommu
> where SYS_brk does not work.
> 
> I'll follow up with a new proposed patch.

I'm not sure whether we need another revision of your first patch. Your
previous analysis was at least right regarding the tests 51 and 58
but those have been fixed now.

But there were two other tests failing, weren't there?

I have to recheck later, I just got up (it's 8 AM CEST).

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] sh: Add SECCOMP_FILTER
  2020-09-03  6:04                   ` John Paul Adrian Glaubitz
@ 2020-09-03  6:17                     ` Rich Felker
  0 siblings, 0 replies; 20+ messages in thread
From: Rich Felker @ 2020-09-03  6:17 UTC (permalink / raw)
  To: John Paul Adrian Glaubitz
  Cc: Michael Karcher, linux-sh, linux-kernel, Yoshinori Sato

On Thu, Sep 03, 2020 at 08:04:44AM +0200, John Paul Adrian Glaubitz wrote:
> On 9/3/20 7:46 AM, Rich Felker wrote:
> > 
> > OK, I think I have an explanation for the mechanism of the bug, and it
> > really is a combination of the 2008 bug (confusion of r0 vs r3) and
> > the SECCOMP_FILTER commit. When the syscall_trace_entry code path is
> > in use, a syscall with argument 5 having value -1 causes
> > do_syscall_trace_enter to return -1 (because it returns regs[0], which
> > contains argument 5), which the change in entry-common.S interprets as
> > a sign to skip the syscall and jump to syscall_exit, and things blow
> > up from there. In particular, SYS_mmap2 is almost always called with
> > -1 as the 5th argument (fd), and this is even more common on nommu
> > where SYS_brk does not work.
> > 
> > I'll follow up with a new proposed patch.
> 
> I'm not sure whether we need another revision of your first patch. Your
> previous analysis was at least right regarding the tests 51 and 58
> but those have been fixed now.
> 
> But there were two other tests failing, weren't there?
> 
> I have to recheck later, I just got up (it's 8 AM CEST).

The first patch was surely not right; setting syscall_nr to -1 and
letting it -ENOSYS clobbered any return value set by the seccomp
filters. The one I've sent now should be right. I'll follow up after
testing with libseccomp test cases.

Rich

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-09-03  6:17 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22 23:13 [PATCH 1/4] sh: Fix validation of system call number Michael Karcher
2020-07-22 23:13 ` [PATCH 2/4] sh: Rearrange blocks in entry-common.S Michael Karcher
2020-07-22 23:20   ` John Paul Adrian Glaubitz
2020-07-22 23:13 ` [PATCH 3/4] sh: Add SECCOMP_FILTER Michael Karcher
2020-07-22 23:20   ` John Paul Adrian Glaubitz
2020-08-28 15:50   ` Rich Felker
2020-08-28 16:21     ` John Paul Adrian Glaubitz
2020-08-28 16:30     ` Rich Felker
2020-08-28 16:38       ` John Paul Adrian Glaubitz
2020-08-28 17:03         ` Rich Felker
2020-08-29  0:49           ` Rich Felker
2020-08-29 11:09             ` John Paul Adrian Glaubitz
2020-09-03  3:56               ` Rich Felker
2020-09-03  5:46                 ` Rich Felker
2020-09-03  6:04                   ` John Paul Adrian Glaubitz
2020-09-03  6:17                     ` Rich Felker
2020-09-03  6:03                 ` John Paul Adrian Glaubitz
2020-07-22 23:13 ` [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures Michael Karcher
2020-07-22 23:20   ` John Paul Adrian Glaubitz
2020-07-22 23:19 ` [PATCH 1/4] sh: Fix validation of system call number John Paul Adrian Glaubitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).