[PATCH 0/2] selftests/seccomp: Report event mismatches more clearly

[PATCH 0/2] selftests/seccomp: Report event mismatches more clearly

[Kees Cook]

Hi,

This expands the seccomp selftests slightly to add additional debug
reporting detail and a new "immediate fatal SIGSYS under tracing" test.
I expect to be taking these via my seccomp tree.

Thanks,

-Kees

Kees Cook (2):
  selftests/seccomp: Stop USER_NOTIF test if kcmp() fails
  selftests/seccomp: Report event mismatches more clearly

 tools/testing/selftests/seccomp/seccomp_bpf.c | 56 +++++++++++++++++--
 1 file changed, 50 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH 1/2] selftests/seccomp: Stop USER_NOTIF test if kcmp() fails

[Kees Cook]

If kcmp() fails during the USER_NOTIF test, the test is likely to hang,
so switch from EXPECT to ASSERT.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 1d64891e6492..d999643d577c 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4087,7 +4087,7 @@ TEST(user_notification_addfd)
 	 * lowest available fd to be assigned here.
 	 */
 	EXPECT_EQ(fd, nextfd++);
-	EXPECT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
+	ASSERT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
 
 	/*
 	 * This sets the ID of the ADD FD to the last request plus 1. The
-- 
2.30.2

[PATCH 2/2] selftests/seccomp: Report event mismatches more clearly

[Kees Cook]

When running under tracer, more explicitly report the status and event
mismatches to help with debugging. Additionally add an "immediate kill"
test when under tracing to verify that fatal SIGSYS behaves the same
under ptrace or seccomp tracing.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 54 +++++++++++++++++--
 1 file changed, 49 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index d999643d577c..60b8d5899fe3 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -1487,7 +1487,7 @@ TEST_F(precedence, log_is_fifth_in_any_order)
 #define PTRACE_EVENT_SECCOMP 7
 #endif
 
-#define IS_SECCOMP_EVENT(status) ((status >> 16) == PTRACE_EVENT_SECCOMP)
+#define PTRACE_EVENT_MASK(status) ((status) >> 16)
 bool tracer_running;
 void tracer_stop(int sig)
 {
@@ -1539,12 +1539,22 @@ void start_tracer(struct __test_metadata *_metadata, int fd, pid_t tracee,
 
 		if (wait(&status) != tracee)
 			continue;
-		if (WIFSIGNALED(status) || WIFEXITED(status))
-			/* Child is dead. Time to go. */
+
+		if (WIFSIGNALED(status)) {
+			/* Child caught a fatal signal. */
+			return;
+		}
+		if (WIFEXITED(status)) {
+			/* Child exited with code. */
 			return;
+		}
 
-		/* Check if this is a seccomp event. */
-		ASSERT_EQ(!ptrace_syscall, IS_SECCOMP_EVENT(status));
+		/* Check if we got an expected event. */
+		ASSERT_EQ(WIFCONTINUED(status), false);
+		ASSERT_EQ(WIFSTOPPED(status), true);
+		ASSERT_EQ(WSTOPSIG(status) & SIGTRAP, SIGTRAP) {
+			TH_LOG("Unexpected WSTOPSIG: %d", WSTOPSIG(status));
+		}
 
 		tracer_func(_metadata, tracee, status, args);
 
@@ -1961,6 +1971,11 @@ void tracer_seccomp(struct __test_metadata *_metadata, pid_t tracee,
 	int ret;
 	unsigned long msg;
 
+	EXPECT_EQ(PTRACE_EVENT_MASK(status), PTRACE_EVENT_SECCOMP) {
+		TH_LOG("Unexpected ptrace event: %d", PTRACE_EVENT_MASK(status));
+		return;
+	}
+
 	/* Make sure we got the right message. */
 	ret = ptrace(PTRACE_GETEVENTMSG, tracee, NULL, &msg);
 	EXPECT_EQ(0, ret);
@@ -2011,6 +2026,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
 	long *syscall_nr = NULL, *syscall_ret = NULL;
 	FIXTURE_DATA(TRACE_syscall) *self = args;
 
+	EXPECT_EQ(WSTOPSIG(status) & 0x80, 0x80) {
+		TH_LOG("Unexpected WSTOPSIG: %d", WSTOPSIG(status));
+		return;
+	}
+
 	/*
 	 * The traditional way to tell PTRACE_SYSCALL entry/exit
 	 * is by counting.
@@ -2128,6 +2148,7 @@ FIXTURE_SETUP(TRACE_syscall)
 	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
 	ASSERT_EQ(0, ret);
 
+	/* Do not install seccomp rewrite filters, as we'll use ptrace instead. */
 	if (variant->use_ptrace)
 		return;
 
@@ -2186,6 +2207,29 @@ TEST_F(TRACE_syscall, syscall_faked)
 	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }
 
+TEST_F_SIGNAL(TRACE_syscall, kill_immediate, SIGSYS)
+{
+	struct sock_filter filter[] = {
+		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+			offsetof(struct seccomp_data, nr)),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_mknodat, 0, 1),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL_THREAD),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog prog = {
+		.len = (unsigned short)ARRAY_SIZE(filter),
+		.filter = filter,
+	};
+	long ret;
+
+	/* Install "kill on mknodat" filter. */
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	/* This should immediately die with SIGSYS, regardless of tracer. */
+	EXPECT_EQ(-1, syscall(__NR_mknodat, -1, NULL, 0, 0));
+}
+
 TEST_F(TRACE_syscall, skip_after)
 {
 	struct sock_filter filter[] = {
-- 
2.30.2

Re: [PATCH 0/2] selftests/seccomp: Report event mismatches more clearly

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> Hi,
>
> This expands the seccomp selftests slightly to add additional debug
> reporting detail and a new "immediate fatal SIGSYS under tracing" test.
> I expect to be taking these via my seccomp tree.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I am a little fuzzy on the details but I understand what and why
you are testing (I broken it).  So this is my 10,000 foot ack.

Eric




> Thanks,
>
> -Kees
>
> Kees Cook (2):
>   selftests/seccomp: Stop USER_NOTIF test if kcmp() fails
>   selftests/seccomp: Report event mismatches more clearly
>
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 56 +++++++++++++++++--
>  1 file changed, 50 insertions(+), 6 deletions(-)

Re: [PATCH 0/2] selftests/seccomp: Report event mismatches more clearly

[Kees Cook]

On Wed, Nov 03, 2021 at 01:37:51PM -0500, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > Hi,
> >
> > This expands the seccomp selftests slightly to add additional debug
> > reporting detail and a new "immediate fatal SIGSYS under tracing" test.
> > I expect to be taking these via my seccomp tree.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> I am a little fuzzy on the details but I understand what and why
> you are testing (I broken it).  So this is my 10,000 foot ack.

Thanks! Yeah, and the other tests did catch it, but it was kind of a
"side effect", so I added the specific "direct" case where it can be
seen more clearly.

-- 
Kees Cook

Re: [PATCH 0/2] selftests/seccomp: Report event mismatches more clearly

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Wed, Nov 03, 2021 at 01:37:51PM -0500, Eric W. Biederman wrote:
>> Kees Cook <keescook@chromium.org> writes:
>> 
>> > Hi,
>> >
>> > This expands the seccomp selftests slightly to add additional debug
>> > reporting detail and a new "immediate fatal SIGSYS under tracing" test.
>> > I expect to be taking these via my seccomp tree.
>> 
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> 
>> I am a little fuzzy on the details but I understand what and why
>> you are testing (I broken it).  So this is my 10,000 foot ack.
>
> Thanks! Yeah, and the other tests did catch it, but it was kind of a
> "side effect", so I added the specific "direct" case where it can be
> seen more clearly.

Hey.  Did you happen to understand the bit about racing with sigaction?

As much as I care about not braking ptrace.  What really decided me was
the on SA_IMMUTABLE was closing the race with sigaction changing the
signal handler.  Especially for something like seccomp.

It is a race so probably very fickle to write a test for, but if we can
figure out how to write a reliable test I expect it will be a good idea.
Do you have any ideas?

I am concerned there is some threaded program somewhere using seccomp
that is allowed to call sigaction, can use sigaction to keep from
being killed (before I send the fix to Linus). 

Eric