2020-07-22 23:22:22

by Michael Karcher

[permalink] [raw]
Subject: [PATCH 1/4] sh: Fix validation of system call number

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 <[email protected]>
---
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


Subject: Re: [PATCH 1/4] sh: Fix validation of system call number

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 <[email protected]>
> ---
> 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 <[email protected]>

--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

2020-07-22 23:23:09

by Michael Karcher

[permalink] [raw]
Subject: [PATCH 4/4] sh: bring syscall_set_return_value in line with other architectures

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 <[email protected]>
---
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

2020-07-22 23:24:53

by Michael Karcher

[permalink] [raw]
Subject: [PATCH 3/4] sh: Add SECCOMP_FILTER

Port sh to use the new SECCOMP_FILTER code.

Signed-off-by: Michael Karcher <[email protected]>
---
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

2020-08-28 15:53:24

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 <[email protected]>
> ---
> 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

Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

2020-08-28 16:32:02

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 <[email protected]>
> > ---
> > 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

Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

2020-08-28 17:04:03

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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)

2020-08-29 00:51:38

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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

Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

2020-09-03 03:57:00

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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

2020-09-03 05:47:27

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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

Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913

2020-09-03 06:18:50

by Rich Felker

[permalink] [raw]
Subject: Re: [PATCH 3/4] sh: Add SECCOMP_FILTER

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