diff mbox series

[3/4] sh: Add SECCOMP_FILTER

Message ID 20200722231322.419642-3-kernel@mkarcher.dialup.fu-berlin.de (mailing list archive)
State New, archived
Headers show
Series [1/4] sh: Fix validation of system call number | expand

Commit Message

Michael Karcher July 22, 2020, 11:13 p.m. UTC
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(-)

Comments

dalias@libc.org Aug. 28, 2020, 3:50 p.m. UTC | #1
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
John Paul Adrian Glaubitz Aug. 28, 2020, 4:21 p.m. UTC | #2
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
dalias@libc.org Aug. 28, 2020, 4:30 p.m. UTC | #3
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
John Paul Adrian Glaubitz Aug. 28, 2020, 4:38 p.m. UTC | #4
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
dalias@libc.org Aug. 28, 2020, 5:03 p.m. UTC | #5
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)
dalias@libc.org Aug. 29, 2020, 12:49 a.m. UTC | #6
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
John Paul Adrian Glaubitz Aug. 29, 2020, 11:09 a.m. UTC | #7
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
dalias@libc.org Sept. 3, 2020, 3:56 a.m. UTC | #8
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
dalias@libc.org Sept. 3, 2020, 5:46 a.m. UTC | #9
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
John Paul Adrian Glaubitz Sept. 3, 2020, 6:03 a.m. UTC | #10
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 Sept. 3, 2020, 6:04 a.m. UTC | #11
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
dalias@libc.org Sept. 3, 2020, 6:17 a.m. UTC | #12
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
diff mbox series

Patch

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;
 	}