selftests/seccomp: Enhance per-arch ptrace syscall skip tests

PATCH In review Kees Cook 3 participants 4 messages Jan 25, 2019

Subsystems (3)

Kselftest Kernel janitors Secure computing

lore

Share & tools

lore

Kees Cook

author

2019-01-25 18:33 UTC #0 lore raw

PATCH 1/1 selftests/seccomp: Enhance per-arch ptrace syscall skip tests

Passing EPERM during syscall skipping was confusing since the test wasn't actually exercising the errno evaluation -- it was just passing a literal "1" (EPERM). Instead, expand the tests to check both direct value returns (positive, 45000 in this case), and errno values (negative, -ESRCH in this case) to check both fake success and fake failure during syscall skipping.

Reported-by Colin Ian King <[email protected]>

Fixes a33b2d0359a0 ("selftests/seccomp: Add tests for basic ptrace actions")

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
Colin, does this end up working on s390? Based on your bug report, I suspect the positive value tests will fail, but the errno tests will pass. If that's true, I think something is wrong in the s390 handling. (And it may just be that the ptrace code to rewrite syscalls on s390 in the test is wrong...) But splitting these tests up should tell us more.

---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 72 +++++++++++++++----
 1 file changed, 57 insertions(+), 15 deletions(-)

tools/testing/selftests/seccomp/seccomp_bpf.c +57 ~~-15~~

index 496a9a8c773a..7e632b465ab4 100644
@@ -1608,7 +1608,16 @@ TEST_F(TRACE_poke, getpid_runs_normally)
 #ifdef SYSCALL_NUM_RET_SHARE_REG
 # define EXPECT_SYSCALL_RETURN(val, action)	EXPECT_EQ(-1, action)
 #else
-# define EXPECT_SYSCALL_RETURN(val, action)	EXPECT_EQ(val, action)
+# define EXPECT_SYSCALL_RETURN(val, action)		\
+	do {						\
+		errno = 0;				\
+		if (val < 0) {				\
+			EXPECT_EQ(-1, action);		\
+			EXPECT_EQ(-(val), errno);	\
+		} else {				\
+			EXPECT_EQ(val, action);		\
+		}					\
+	} while (0)
 #endif
 
 /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for

@@ -1647,7 +1656,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 
 /* Architecture-specific syscall changing routine. */
 void change_syscall(struct __test_metadata *_metadata,
-		    pid_t tracee, int syscall)
+		    pid_t tracee, int syscall, int result)
 {
 	int ret;
 	ARCH_REGS regs;

@@ -1706,7 +1715,7 @@ void change_syscall(struct __test_metadata *_metadata,
 #ifdef SYSCALL_NUM_RET_SHARE_REG
 		TH_LOG("Can't modify syscall return on this architecture");
 #else
-		regs.SYSCALL_RET = EPERM;
+		regs.SYSCALL_RET = result;
 #endif
 
 #ifdef HAVE_GETREGS

@@ -1734,14 +1743,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee,
 	case 0x1002:
 		/* change getpid to getppid. */
 		EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee));
-		change_syscall(_metadata, tracee, __NR_getppid);
+		change_syscall(_metadata, tracee, __NR_getppid, 0);
 		break;
 	case 0x1003:
-		/* skip gettid. */
+		/* skip gettid with valid return code. */
 		EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee));
-		change_syscall(_metadata, tracee, -1);
+		change_syscall(_metadata, tracee, -1, 45000);
 		break;
 	case 0x1004:
+		/* skip openat with error. */
+		EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee));
+		change_syscall(_metadata, tracee, -1, -ESRCH);
+		break;
+	case 0x1005:
 		/* do nothing (allow getppid) */
 		EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee));
 		break;

@@ -1774,9 +1788,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
 	nr = get_syscall(_metadata, tracee);
 
 	if (nr = __NR_getpid)
-		change_syscall(_metadata, tracee, __NR_getppid);
+		change_syscall(_metadata, tracee, __NR_getppid, 0);
+	if (nr = __NR_gettid)
+		change_syscall(_metadata, tracee, -1, 45000);
 	if (nr = __NR_openat)
-		change_syscall(_metadata, tracee, -1);
+		change_syscall(_metadata, tracee, -1, -ESRCH);
 }
 
 FIXTURE_DATA(TRACE_syscall) {

@@ -1793,8 +1809,10 @@ FIXTURE_SETUP(TRACE_syscall)
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002),
 		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1),
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003),
-		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1),
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005),
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
 	};

@@ -1842,15 +1860,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected)
 	EXPECT_NE(self->mypid, syscall(__NR_getpid));
 }
 
-TEST_F(TRACE_syscall, ptrace_syscall_dropped)
+TEST_F(TRACE_syscall, ptrace_syscall_errno)
+{
+	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
+	teardown_trace_fixture(_metadata, self->tracer);
+	self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
+					   true);
+
+	/* Tracer should skip the open syscall, resulting in ESRCH. */
+	EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, ptrace_syscall_faked)
 {
 	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
 	teardown_trace_fixture(_metadata, self->tracer);
 	self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
 					   true);
 
-	/* Tracer should skip the open syscall, resulting in EPERM. */
-	EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat));
+	/* Tracer should skip the gettid syscall, resulting fake pid. */
+	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }
 
 TEST_F(TRACE_syscall, syscall_allowed)

@@ -1883,7 +1912,21 @@ TEST_F(TRACE_syscall, syscall_redirected)
 	EXPECT_NE(self->mypid, syscall(__NR_getpid));
 }
 
-TEST_F(TRACE_syscall, syscall_dropped)
+TEST_F(TRACE_syscall, syscall_errno)
+{
+	long ret;
+
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	/* openat has been skipped and an errno return. */
+	EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, syscall_faked)
 {
 	long ret;

@@ -1894,8 +1937,7 @@ TEST_F(TRACE_syscall, syscall_dropped)
 	ASSERT_EQ(0, ret);
 
 	/* gettid has been skipped and an altered return value stored. */
-	EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid));
-	EXPECT_NE(self->mytid, syscall(__NR_gettid));
+	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }
 
 TEST_F(TRACE_syscall, skip_after_RET_TRACE)

Signature · 5 lines

2.17.1


-- 
Kees Cook

Colin Ian King

Tested-by on v1 1/1

2019-01-25 18:42 UTC #1 lore raw

Quoted 164 lines · Kees Cook

Reported-by Colin Ian King <[email protected]>

Fixes a33b2d0359a0 ("selftests/seccomp: Add tests for basic ptrace actions")

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 72 +++++++++++++++----
 1 file changed, 57 insertions(+), 15 deletions(-)

tools/testing/selftests/seccomp/seccomp_bpf.c +56 ~~-13~~

index 496a9a8c773a..7e632b465ab4 100644
@@ -1608,7 +1608,16 @@ TEST_F(TRACE_poke, getpid_runs_normally)
 #ifdef SYSCALL_NUM_RET_SHARE_REG
 # define EXPECT_SYSCALL_RETURN(val, action)	EXPECT_EQ(-1, action)
 #else
-# define EXPECT_SYSCALL_RETURN(val, action)	EXPECT_EQ(val, action)
+# define EXPECT_SYSCALL_RETURN(val, action)		\
+	do {						\
+		errno = 0;				\
+		if (val < 0) {				\
+			EXPECT_EQ(-1, action);		\
+			EXPECT_EQ(-(val), errno);	\
+		} else {				\
+			EXPECT_EQ(val, action);		\
+		}					\
+	} while (0)
 #endif
 
 /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for

@@ -1647,7 +1656,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 
 /* Architecture-specific syscall changing routine. */
 void change_syscall(struct __test_metadata *_metadata,
-		    pid_t tracee, int syscall)
+		    pid_t tracee, int syscall, int result)
 {
 	int ret;
 	ARCH_REGS regs;

@@ -1706,7 +1715,7 @@ void change_syscall(struct __test_metadata *_metadata,
 #ifdef SYSCALL_NUM_RET_SHARE_REG
 		TH_LOG("Can't modify syscall return on this architecture");
 #else
-		regs.SYSCALL_RET = EPERM;
+		regs.SYSCALL_RET = result;
 #endif
 
 #ifdef HAVE_GETREGS

@@ -1734,14 +1743,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee,
 	case 0x1002:
 		/* change getpid to getppid. */
 		EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee));
-		change_syscall(_metadata, tracee, __NR_getppid);
+		change_syscall(_metadata, tracee, __NR_getppid, 0);
 		break;
 	case 0x1003:
-		/* skip gettid. */
+		/* skip gettid with valid return code. */
 		EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee));
-		change_syscall(_metadata, tracee, -1);
+		change_syscall(_metadata, tracee, -1, 45000);
 		break;
 	case 0x1004:
+		/* skip openat with error. */
+		EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee));
+		change_syscall(_metadata, tracee, -1, -ESRCH);
+		break;
+	case 0x1005:
 		/* do nothing (allow getppid) */
 		EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee));
 		break;

@@ -1774,9 +1788,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
 	nr = get_syscall(_metadata, tracee);
 
 	if (nr = __NR_getpid)
-		change_syscall(_metadata, tracee, __NR_getppid);
+		change_syscall(_metadata, tracee, __NR_getppid, 0);
+	if (nr = __NR_gettid)
+		change_syscall(_metadata, tracee, -1, 45000);
 	if (nr = __NR_openat)
-		change_syscall(_metadata, tracee, -1);
+		change_syscall(_metadata, tracee, -1, -ESRCH);
 }
 
 FIXTURE_DATA(TRACE_syscall) {

@@ -1793,8 +1809,10 @@ FIXTURE_SETUP(TRACE_syscall)
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002),
 		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1),
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003),
-		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1),
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005),
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
 	};

@@ -1842,15 +1860,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected)
 	EXPECT_NE(self->mypid, syscall(__NR_getpid));
 }
 
-TEST_F(TRACE_syscall, ptrace_syscall_dropped)
+TEST_F(TRACE_syscall, ptrace_syscall_errno)
+{
+	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
+	teardown_trace_fixture(_metadata, self->tracer);
+	self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
+					   true);
+
+	/* Tracer should skip the open syscall, resulting in ESRCH. */
+	EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, ptrace_syscall_faked)
 {
 	/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
 	teardown_trace_fixture(_metadata, self->tracer);
 	self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
 					   true);
 
-	/* Tracer should skip the open syscall, resulting in EPERM. */
-	EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat));
+	/* Tracer should skip the gettid syscall, resulting fake pid. */
+	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }
 
 TEST_F(TRACE_syscall, syscall_allowed)

@@ -1883,7 +1912,21 @@ TEST_F(TRACE_syscall, syscall_redirected)
 	EXPECT_NE(self->mypid, syscall(__NR_getpid));
 }
 
-TEST_F(TRACE_syscall, syscall_dropped)
+TEST_F(TRACE_syscall, syscall_errno)
+{
+	long ret;
+
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	/* openat has been skipped and an errno return. */
+	EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, syscall_faked)
 {
 	long ret;

tools/testing/selftests/seccomp/seccomp_bpf.c TEST_F(TRACE_syscall, syscall_dropped) · line 1943

Quoted 9 lines

tools/testing/selftests/seccomp/seccomp_bpf.c +1 -2

@@ -1894,8 +1937,7 @@ TEST_F(TRACE_syscall, syscall_dropped)
 	ASSERT_EQ(0, ret);
 
 	/* gettid has been skipped and an altered return value stored. */
-	EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid));
-	EXPECT_NE(self->mytid, syscall(__NR_gettid));
+	EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }
 
 TEST_F(TRACE_syscall, skip_after_RET_TRACE)

Works perfectly. Thanks Kees.

Tested-by Colin Ian King <[email protected]>

Kees Cook

author

2019-01-25 18:46 UTC #2 lore raw

Quoted 173 lines · Colin Ian King

Quoted 171 lines · Kees Cook

Reported-by Colin Ian King <[email protected]>

Fixes a33b2d0359a0 ("selftests/seccomp: Add tests for basic ptrace actions")

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 72 +++++++++++++++----
 1 file changed, 57 insertions(+), 15 deletions(-)

tools/testing/selftests/seccomp/seccomp_bpf.c +57 ~~-15~~

index 496a9a8c773a..7e632b465ab4 100644
@@ -1608,7 +1608,16 @@ TEST_F(TRACE_poke, getpid_runs_normally)
 #ifdef SYSCALL_NUM_RET_SHARE_REG
 # define EXPECT_SYSCALL_RETURN(val, action)  EXPECT_EQ(-1, action)
 #else
-# define EXPECT_SYSCALL_RETURN(val, action)  EXPECT_EQ(val, action)
+# define EXPECT_SYSCALL_RETURN(val, action)          \
+     do {                                            \
+             errno = 0;                              \
+             if (val < 0) {                          \
+                     EXPECT_EQ(-1, action);          \
+                     EXPECT_EQ(-(val), errno);       \
+             } else {                                \
+                     EXPECT_EQ(val, action);         \
+             }                                       \
+     } while (0)
 #endif
 
 /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for

@@ -1647,7 +1656,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 
 /* Architecture-specific syscall changing routine. */
 void change_syscall(struct __test_metadata *_metadata,
-                 pid_t tracee, int syscall)
+                 pid_t tracee, int syscall, int result)
 {
      int ret;
      ARCH_REGS regs;

@@ -1706,7 +1715,7 @@ void change_syscall(struct __test_metadata *_metadata,
 #ifdef SYSCALL_NUM_RET_SHARE_REG
              TH_LOG("Can't modify syscall return on this architecture");
 #else
-             regs.SYSCALL_RET = EPERM;
+             regs.SYSCALL_RET = result;
 #endif
 
 #ifdef HAVE_GETREGS

@@ -1734,14 +1743,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee,
      case 0x1002:
              /* change getpid to getppid. */
              EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee));
-             change_syscall(_metadata, tracee, __NR_getppid);
+             change_syscall(_metadata, tracee, __NR_getppid, 0);
              break;
      case 0x1003:
-             /* skip gettid. */
+             /* skip gettid with valid return code. */
              EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee));
-             change_syscall(_metadata, tracee, -1);
+             change_syscall(_metadata, tracee, -1, 45000);
              break;
      case 0x1004:
+             /* skip openat with error. */
+             EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee));
+             change_syscall(_metadata, tracee, -1, -ESRCH);
+             break;
+     case 0x1005:
              /* do nothing (allow getppid) */
              EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee));
              break;

@@ -1774,9 +1788,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
      nr = get_syscall(_metadata, tracee);
 
      if (nr = __NR_getpid)
-             change_syscall(_metadata, tracee, __NR_getppid);
+             change_syscall(_metadata, tracee, __NR_getppid, 0);
+     if (nr = __NR_gettid)
+             change_syscall(_metadata, tracee, -1, 45000);
      if (nr = __NR_openat)
-             change_syscall(_metadata, tracee, -1);
+             change_syscall(_metadata, tracee, -1, -ESRCH);
 }
 
 FIXTURE_DATA(TRACE_syscall) {

@@ -1793,8 +1809,10 @@ FIXTURE_SETUP(TRACE_syscall)
              BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002),
              BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1),
              BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003),
-             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1),
              BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004),
+             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+             BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005),
              BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
      };

@@ -1842,15 +1860,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected)
      EXPECT_NE(self->mypid, syscall(__NR_getpid));
 }
 
-TEST_F(TRACE_syscall, ptrace_syscall_dropped)
+TEST_F(TRACE_syscall, ptrace_syscall_errno)
+{
+     /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
+     teardown_trace_fixture(_metadata, self->tracer);
+     self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
+                                        true);
+
+     /* Tracer should skip the open syscall, resulting in ESRCH. */
+     EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, ptrace_syscall_faked)
 {
      /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
      teardown_trace_fixture(_metadata, self->tracer);
      self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
                                         true);
 
-     /* Tracer should skip the open syscall, resulting in EPERM. */
-     EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat));
+     /* Tracer should skip the gettid syscall, resulting fake pid. */
+     EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }
 
 TEST_F(TRACE_syscall, syscall_allowed)

@@ -1883,7 +1912,21 @@ TEST_F(TRACE_syscall, syscall_redirected)
      EXPECT_NE(self->mypid, syscall(__NR_getpid));
 }
 
-TEST_F(TRACE_syscall, syscall_dropped)
+TEST_F(TRACE_syscall, syscall_errno)
+{
+     long ret;
+
+     ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+     ASSERT_EQ(0, ret);
+
+     ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0);
+     ASSERT_EQ(0, ret);
+
+     /* openat has been skipped and an errno return. */
+     EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, syscall_faked)
 {
      long ret;

@@ -1894,8 +1937,7 @@ TEST_F(TRACE_syscall, syscall_dropped)
      ASSERT_EQ(0, ret);
 
      /* gettid has been skipped and an altered return value stored. */
-     EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid));
-     EXPECT_NE(self->mytid, syscall(__NR_gettid));
+     EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
 }

Quoted 5 lines

tools/testing/selftests/seccomp/seccomp_bpf.c TEST_F(TRACE_syscall, syscall_dropped) · line 1943
Quoted 2 lines
tools/testing/selftests/seccomp/seccomp_bpf.c +0 -0
 TEST_F(TRACE_syscall, skip_after_RET_TRACE)
Works perfectly. Thanks Kees.
Tested-by Colin Ian King <[email protected]>

Oh excellent! Thanks. :) Shuah can you queue this up?

Signature · 1 line

Kees Cook

shuah

maintainer

2019-01-25 19:11 UTC #3 lore raw

Quoted 174 lines · Kees Cook

Quoted 173 lines · Colin Ian King

Quoted 171 lines · Kees Cook

Reported-by Colin Ian King <[email protected]>

Fixes a33b2d0359a0 ("selftests/seccomp: Add tests for basic ptrace actions")

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
  tools/testing/selftests/seccomp/seccomp_bpf.c | 72 +++++++++++++++----
  1 file changed, 57 insertions(+), 15 deletions(-)

tools/testing/selftests/seccomp/seccomp_bpf.c +57 ~~-15~~

index 496a9a8c773a..7e632b465ab4 100644
@@ -1608,7 +1608,16 @@ TEST_F(TRACE_poke, getpid_runs_normally)
  #ifdef SYSCALL_NUM_RET_SHARE_REG
  # define EXPECT_SYSCALL_RETURN(val, action)  EXPECT_EQ(-1, action)
  #else
-# define EXPECT_SYSCALL_RETURN(val, action)  EXPECT_EQ(val, action)
+# define EXPECT_SYSCALL_RETURN(val, action)          \
+     do {                                            \
+             errno = 0;                              \
+             if (val < 0) {                          \
+                     EXPECT_EQ(-1, action);          \
+                     EXPECT_EQ(-(val), errno);       \
+             } else {                                \
+                     EXPECT_EQ(val, action);         \
+             }                                       \
+     } while (0)
  #endif
 
  /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for

@@ -1647,7 +1656,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee)
 
  /* Architecture-specific syscall changing routine. */
  void change_syscall(struct __test_metadata *_metadata,
-                 pid_t tracee, int syscall)
+                 pid_t tracee, int syscall, int result)
  {
       int ret;
       ARCH_REGS regs;

@@ -1706,7 +1715,7 @@ void change_syscall(struct __test_metadata *_metadata,
  #ifdef SYSCALL_NUM_RET_SHARE_REG
               TH_LOG("Can't modify syscall return on this architecture");
  #else
-             regs.SYSCALL_RET = EPERM;
+             regs.SYSCALL_RET = result;
  #endif
 
  #ifdef HAVE_GETREGS

@@ -1734,14 +1743,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee,
       case 0x1002:
               /* change getpid to getppid. */
               EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee));
-             change_syscall(_metadata, tracee, __NR_getppid);
+             change_syscall(_metadata, tracee, __NR_getppid, 0);
               break;
       case 0x1003:
-             /* skip gettid. */
+             /* skip gettid with valid return code. */
               EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee));
-             change_syscall(_metadata, tracee, -1);
+             change_syscall(_metadata, tracee, -1, 45000);
               break;
       case 0x1004:
+             /* skip openat with error. */
+             EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee));
+             change_syscall(_metadata, tracee, -1, -ESRCH);
+             break;
+     case 0x1005:
               /* do nothing (allow getppid) */
               EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee));
               break;

@@ -1774,9 +1788,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
       nr = get_syscall(_metadata, tracee);
 
       if (nr = __NR_getpid)
-             change_syscall(_metadata, tracee, __NR_getppid);
+             change_syscall(_metadata, tracee, __NR_getppid, 0);
+     if (nr = __NR_gettid)
+             change_syscall(_metadata, tracee, -1, 45000);
       if (nr = __NR_openat)
-             change_syscall(_metadata, tracee, -1);
+             change_syscall(_metadata, tracee, -1, -ESRCH);
  }
 
  FIXTURE_DATA(TRACE_syscall) {

@@ -1793,8 +1809,10 @@ FIXTURE_SETUP(TRACE_syscall)
               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002),
               BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1),
               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003),
-             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1),
               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004),
+             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1),
+             BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005),
               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
       };

@@ -1842,15 +1860,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected)
       EXPECT_NE(self->mypid, syscall(__NR_getpid));
  }
 
-TEST_F(TRACE_syscall, ptrace_syscall_dropped)
+TEST_F(TRACE_syscall, ptrace_syscall_errno)
+{
+     /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
+     teardown_trace_fixture(_metadata, self->tracer);
+     self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
+                                        true);
+
+     /* Tracer should skip the open syscall, resulting in ESRCH. */
+     EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, ptrace_syscall_faked)
  {
       /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
       teardown_trace_fixture(_metadata, self->tracer);
       self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL,
                                          true);
 
-     /* Tracer should skip the open syscall, resulting in EPERM. */
-     EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat));
+     /* Tracer should skip the gettid syscall, resulting fake pid. */
+     EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
  }
 
  TEST_F(TRACE_syscall, syscall_allowed)

@@ -1883,7 +1912,21 @@ TEST_F(TRACE_syscall, syscall_redirected)
       EXPECT_NE(self->mypid, syscall(__NR_getpid));
  }
 
-TEST_F(TRACE_syscall, syscall_dropped)
+TEST_F(TRACE_syscall, syscall_errno)
+{
+     long ret;
+
+     ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+     ASSERT_EQ(0, ret);
+
+     ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0);
+     ASSERT_EQ(0, ret);
+
+     /* openat has been skipped and an errno return. */
+     EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat));
+}
+
+TEST_F(TRACE_syscall, syscall_faked)
  {
       long ret;

@@ -1894,8 +1937,7 @@ TEST_F(TRACE_syscall, syscall_dropped)
       ASSERT_EQ(0, ret);
 
       /* gettid has been skipped and an altered return value stored. */
-     EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid));
-     EXPECT_NE(self->mytid, syscall(__NR_gettid));
+     EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid));
  }

Quoted 8 lines

Quoted 5 lines
tools/testing/selftests/seccomp/seccomp_bpf.c TEST_F(TRACE_syscall, syscall_dropped) · line 1943
Quoted 2 lines
tools/testing/selftests/seccomp/seccomp_bpf.c +0 -0
  TEST_F(TRACE_syscall, skip_after_RET_TRACE)
Works perfectly. Thanks Kees.
Tested-by Colin Ian King <[email protected]>
Oh excellent! Thanks. :) Shuah can you queue this up?

Yup will do - I am planning to apply bunch of fixes this afternoon. Hoping to get them into rc5

thanks,
-- Shuah