diff mbox series

[v4,08/11] selftests/seccomp: Make kcmp() less required

Message ID 20200616032524.460144-9-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series Add seccomp notifier ioctl that enables adding fds | expand

Commit Message

Kees Cook June 16, 2020, 3:25 a.m. UTC
The seccomp tests are a bit noisy without CONFIG_CHECKPOINT_RESTORE (due
to missing the kcmp() syscall). The seccomp tests are more accurate with
kcmp(), but it's not strictly required. Refactor the tests to use
alternatives (comparing fd numbers), and provide a central test for
kcmp() so there is a single XFAIL instead of many. Continue to produce
warnings for the other tests, though.

Additionally adds some more bad flag EINVAL tests to the addfd selftest.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 100 +++++++++++-------
 1 file changed, 64 insertions(+), 36 deletions(-)

Comments

Tycho Andersen June 16, 2020, 2:57 p.m. UTC | #1
On Mon, Jun 15, 2020 at 08:25:21PM -0700, Kees Cook wrote:
> The seccomp tests are a bit noisy without CONFIG_CHECKPOINT_RESTORE (due
> to missing the kcmp() syscall). The seccomp tests are more accurate with
> kcmp(), but it's not strictly required. Refactor the tests to use
> alternatives (comparing fd numbers), and provide a central test for
> kcmp() so there is a single XFAIL instead of many. Continue to produce
> warnings for the other tests, though.
> 
> Additionally adds some more bad flag EINVAL tests to the addfd selftest.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

This looks fine, but I wonder if this is enough motivation for taking
kcmp() out of CONFIG_CHECKPOINT_RESTORE guards?

Tycho
Kees Cook June 16, 2020, 4:03 p.m. UTC | #2
On Tue, Jun 16, 2020 at 08:57:25AM -0600, Tycho Andersen wrote:
> On Mon, Jun 15, 2020 at 08:25:21PM -0700, Kees Cook wrote:
> > The seccomp tests are a bit noisy without CONFIG_CHECKPOINT_RESTORE (due
> > to missing the kcmp() syscall). The seccomp tests are more accurate with
> > kcmp(), but it's not strictly required. Refactor the tests to use
> > alternatives (comparing fd numbers), and provide a central test for
> > kcmp() so there is a single XFAIL instead of many. Continue to produce
> > warnings for the other tests, though.
> > 
> > Additionally adds some more bad flag EINVAL tests to the addfd selftest.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> This looks fine, but I wonder if this is enough motivation for taking
> kcmp() out of CONFIG_CHECKPOINT_RESTORE guards?

Do you mean in the kernel? I'd rather not -- it's a relatively powerful
primitive. Maybe if there were other users needing it, but there doesn't
seem to have been much demand.
diff mbox series

Patch

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index c4e264b37c30..40ed846744e4 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -258,6 +258,27 @@  int seccomp(unsigned int op, unsigned int flags, void *args)
 #define SIBLING_EXIT_FAILURE	0xbadface
 #define SIBLING_EXIT_NEWPRIVS	0xbadfeed
 
+static int filecmp(pid_t pid1, pid_t pid2, int fd1, int fd2)
+{
+#ifdef __NR_kcmp
+	errno = 0;
+	return syscall(__NR_kcmp, pid1, pid2, KCMP_FILE, fd1, fd2);
+#else
+	errno = ENOSYS;
+	return -1;
+#endif
+}
+
+TEST(kcmp)
+{
+	int ret;
+
+	ret = filecmp(getpid(), getpid(), 1, 1);
+	EXPECT_EQ(ret, 0);
+	if (ret != 0 && errno == ENOSYS)
+		XFAIL(return, "Kernel does not support kcmp() (missing CONFIG_CHECKPOINT_RESTORE?)");
+}
+
 TEST(mode_strict_support)
 {
 	long ret;
@@ -3606,16 +3627,6 @@  TEST(seccomp_get_notif_sizes)
 	EXPECT_EQ(sizes.seccomp_notif_resp, sizeof(struct seccomp_notif_resp));
 }
 
-static int filecmp(pid_t pid1, pid_t pid2, int fd1, int fd2)
-{
-#ifdef __NR_kcmp
-	return syscall(__NR_kcmp, pid1, pid2, KCMP_FILE, fd1, fd2);
-#else
-	errno = ENOSYS;
-	return -1;
-#endif
-}
-
 TEST(user_notification_continue)
 {
 	pid_t pid;
@@ -3640,20 +3651,20 @@  TEST(user_notification_continue)
 		int dup_fd, pipe_fds[2];
 		pid_t self;
 
-		ret = pipe(pipe_fds);
-		if (ret < 0)
-			exit(1);
+		ASSERT_GE(pipe(pipe_fds), 0);
 
 		dup_fd = dup(pipe_fds[0]);
-		if (dup_fd < 0)
-			exit(1);
+		ASSERT_GE(dup_fd, 0);
+		EXPECT_NE(pipe_fds[0], dup_fd);
 
 		self = getpid();
-
 		ret = filecmp(self, self, pipe_fds[0], dup_fd);
-		if (ret)
-			exit(2);
-
+		if (ret != 0) {
+			if (ret < 0 && errno == ENOSYS) {
+				TH_LOG("kcmp() syscall missing (test is less accurate)");
+			} else
+				ASSERT_EQ(ret, 0);
+		}
 		exit(0);
 	}
 
@@ -3700,12 +3711,7 @@  TEST(user_notification_continue)
 skip:
 	EXPECT_EQ(waitpid(pid, &status, 0), pid);
 	EXPECT_EQ(true, WIFEXITED(status));
-	EXPECT_EQ(0, WEXITSTATUS(status)) {
-		if (WEXITSTATUS(status) == 2) {
-			XFAIL(return, "Kernel does not support kcmp() syscall");
-			return;
-		}
-	}
+	EXPECT_EQ(0, WEXITSTATUS(status));
 }
 
 TEST(user_notification_filter_empty)
@@ -3847,7 +3853,7 @@  TEST(user_notification_sendfd)
 {
 	pid_t pid;
 	long ret;
-	int status, listener, memfd;
+	int status, listener, memfd, fd;
 	struct seccomp_notif_addfd addfd = {};
 	struct seccomp_notif req = {};
 	struct seccomp_notif_resp resp = {};
@@ -3880,34 +3886,56 @@  TEST(user_notification_sendfd)
 
 	addfd.size = sizeof(addfd);
 	addfd.srcfd = memfd;
-	addfd.newfd_flags = O_CLOEXEC;
 	addfd.newfd = 0;
 	addfd.id = req.id;
-	addfd.flags = 0xff;
+	addfd.flags = 0;
+
+	/* Verify bad newfd_flags cannot be set */
+	addfd.newfd_flags = ~O_CLOEXEC;
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
+	EXPECT_EQ(errno, EINVAL);
+	addfd.newfd_flags = O_CLOEXEC;
 
 	/* Verify bad flags cannot be set */
+	addfd.flags = 0xff;
 	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
 	EXPECT_EQ(errno, EINVAL);
+	addfd.flags = 0;
 
 	/* Verify that remote_fd cannot be set without setting flags */
-	addfd.flags = 0;
 	addfd.newfd = 1;
 	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), -1);
 	EXPECT_EQ(errno, EINVAL);
-
-	/* Verify we can set an arbitrary remote fd */
 	addfd.newfd = 0;
 
-	ret = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd);
-	EXPECT_GE(ret, 0);
-	EXPECT_EQ(filecmp(getpid(), pid, memfd, ret), 0);
+	/* Verify we can set an arbitrary remote fd */
+	fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd);
+	/*
+	 * The child has fds 0(stdin), 1(stdout), 2(stderr), 3(memfd),
+	 * 4(listener), so the newly allocated fd should be 5.
+	 */
+	EXPECT_EQ(fd, 5);
+	ret = filecmp(getpid(), pid, memfd, fd);
+	if (ret != 0) {
+		if (ret < 0 && errno == ENOSYS) {
+			TH_LOG("kcmp() syscall missing (test is less accurate)");
+		} else
+			EXPECT_EQ(ret, 0);
+	}
 
 	/* Verify we can set a specific remote fd */
 	addfd.newfd = 42;
 	addfd.flags = SECCOMP_ADDFD_FLAG_SETFD;
 
-	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd), 42);
-	EXPECT_EQ(filecmp(getpid(), pid, memfd, 42), 0);
+	fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd);
+	EXPECT_EQ(fd, 42);
+	ret = filecmp(getpid(), pid, memfd, fd);
+	if (ret != 0) {
+		if (ret < 0 && errno == ENOSYS) {
+			TH_LOG("kcmp() syscall missing (test is less accurate)");
+		} else
+			EXPECT_EQ(ret, 0);
+	}
 
 	resp.id = req.id;
 	resp.error = 0;