[PATCH 1/2] seccomp: Use FIFO semantics to order notifications

[PATCH 1/2] seccomp: Use FIFO semantics to order notifications

[Sargun Dhillon]

Previously, the seccomp notifier used LIFO semantics, where each
notification would be added on top of the stack, and notifications
were popped off the top of the stack. This could result one process
that generates a large number of notifications preventing other
notifications from being handled. This patch moves from LIFO (stack)
semantics to FIFO (queue semantics).

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 kernel/seccomp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index db10e73d06e0..2cb3bcd90eb3 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1101,7 +1101,7 @@ static int seccomp_do_user_notification(int this_syscall,
 	n.data = sd;
 	n.id = seccomp_next_notify_id(match);
 	init_completion(&n.ready);
-	list_add(&n.list, &match->notif->notifications);
+	list_add_tail(&n.list, &match->notif->notifications);
 	INIT_LIST_HEAD(&n.addfd);
 
 	up(&match->notif->request);
-- 
2.25.1

[PATCH 2/2] selftests/seccomp: Ensure that notifications come in FIFO order

[Sargun Dhillon]

When multiple notifications are waiting, ensure they show up in order, as
defined by the (predictable) seccomp notification ID. This ensures FIFO
ordering of notification delivery as notification ids are monitonic and
decided when the notification is generated (as opposed to received).

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 9d126d7fabdb..33fb3d0c3347 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4231,6 +4231,115 @@ TEST(user_notification_addfd_rlimit)
 	close(memfd);
 }
 
+static char get_proc_stat(int pid)
+{
+	char proc_path[100] = {0};
+	char *line = NULL;
+	size_t len = 0;
+	ssize_t nread;
+	char status;
+	FILE *f;
+	int i;
+
+	snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid);
+	f = fopen(proc_path, "r");
+	if (f == NULL)
+		ksft_exit_fail_msg("%s - Could not open %s\n",
+				   strerror(errno), proc_path);
+
+	for (i = 0; i < 3; i++) {
+		nread = getdelim(&line, &len, ' ', f);
+		if (nread <= 0)
+			ksft_exit_fail_msg("Failed to read status: %s\n",
+					   strerror(errno));
+	}
+
+	status = *line;
+	free(line);
+	fclose(f);
+
+	return status;
+}
+
+TEST(user_notification_fifo)
+{
+	struct seccomp_notif_resp resp = {};
+	struct seccomp_notif req = {};
+	int i, status, listener;
+	pid_t pid, pids[3];
+	__u64 baseid;
+	long ret;
+	/* 100 ms */
+	struct timespec delay = { .tv_nsec = 100000000 };
+
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
+	/* Setup a listener */
+	listener = user_notif_syscall(__NR_getppid,
+				      SECCOMP_FILTER_FLAG_NEW_LISTENER);
+	ASSERT_GE(listener, 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		ret = syscall(__NR_getppid);
+		exit(ret != USER_NOTIF_MAGIC);
+	}
+
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+	baseid = req.id + 1;
+
+	resp.id = req.id;
+	resp.error = 0;
+	resp.val = USER_NOTIF_MAGIC;
+
+	/* check that we make sure flags == 0 */
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFEXITED(status));
+	EXPECT_EQ(0, WEXITSTATUS(status));
+
+	/* Start children, and them generate notifications */
+	for (i = 0; i < ARRAY_SIZE(pids); i++) {
+		pid = fork();
+		if (pid == 0) {
+			ret = syscall(__NR_getppid);
+			exit(ret != USER_NOTIF_MAGIC);
+		}
+		pids[i] = pid;
+	}
+
+	/* This spins until all of the children are sleeping */
+restart_wait:
+	for (i = 0; i < ARRAY_SIZE(pids); i++) {
+		if (get_proc_stat(pids[i]) != 'S') {
+			nanosleep(&delay, NULL);
+			goto restart_wait;
+		}
+	}
+
+	/* Read the notifications in order (and respond) */
+	for (i = 0; i < ARRAY_SIZE(pids); i++) {
+		memset(&req, 0, sizeof(req));
+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+		EXPECT_EQ(req.id, baseid + i);
+		resp.id = req.id;
+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+	}
+
+	/* Make sure notifications were received */
+	for (i = 0; i < ARRAY_SIZE(pids); i++) {
+		EXPECT_EQ(waitpid(pids[i], &status, 0), pids[i]);
+		EXPECT_EQ(true, WIFEXITED(status));
+		EXPECT_EQ(0, WEXITSTATUS(status));
+	}
+}
+
 /*
  * TODO:
  * - expand NNP testing
-- 
2.25.1

Re: [PATCH 2/2] selftests/seccomp: Ensure that notifications come in FIFO order

[Tycho Andersen]

On Wed, Apr 27, 2022 at 06:54:47PM -0700, Sargun Dhillon wrote:
> +	/* Start children, and them generate notifications */
                           ^^ - they maybe?

> +	for (i = 0; i < ARRAY_SIZE(pids); i++) {
> +		pid = fork();
> +		if (pid == 0) {
> +			ret = syscall(__NR_getppid);
> +			exit(ret != USER_NOTIF_MAGIC);
> +		}
> +		pids[i] = pid;
> +	}
> +
> +	/* This spins until all of the children are sleeping */
> +restart_wait:
> +	for (i = 0; i < ARRAY_SIZE(pids); i++) {
> +		if (get_proc_stat(pids[i]) != 'S') {
> +			nanosleep(&delay, NULL);
> +			goto restart_wait;
> +		}
> +	}

I wonder if we should/can combine this loop with the previous one, and
wait for the child to sleep in getppid() before we fork the next one.
Otherwise isn't racy in the case that your loop continues to the next
iteration before the child processes are scheduled, so things might be
out of order? Maybe I'm missing something.

In any case, this change seems reasonable to me.

Tycho

Re: [PATCH 2/2] selftests/seccomp: Ensure that notifications come in FIFO order

[Sargun Dhillon]

On Thu, Apr 28, 2022 at 6:15 AM Tycho Andersen <tycho@tycho.pizza> wrote:
>
> On Wed, Apr 27, 2022 at 06:54:47PM -0700, Sargun Dhillon wrote:
> > +     /* Start children, and them generate notifications */
>                            ^^ - they maybe?
>
Whoops, this was supposed to be:
 /* Start children, and generate notifications */
> > +     for (i = 0; i < ARRAY_SIZE(pids); i++) {
> > +             pid = fork();
> > +             if (pid == 0) {
> > +                     ret = syscall(__NR_getppid);
> > +                     exit(ret != USER_NOTIF_MAGIC);
> > +             }
> > +             pids[i] = pid;
> > +     }
> > +
> > +     /* This spins until all of the children are sleeping */
> > +restart_wait:
> > +     for (i = 0; i < ARRAY_SIZE(pids); i++) {
> > +             if (get_proc_stat(pids[i]) != 'S') {
> > +                     nanosleep(&delay, NULL);
> > +                     goto restart_wait;
> > +             }
> > +     }
>
> I wonder if we should/can combine this loop with the previous one, and
> wait for the child to sleep in getppid() before we fork the next one.
> Otherwise isn't racy in the case that your loop continues to the next
> iteration before the child processes are scheduled, so things might be
> out of order? Maybe I'm missing something.
>
> In any case, this change seems reasonable to me.
>
> Tycho
It's okay if the child processes are started out of order. The test just
verifies that the calls are delivered in FIFO order according to when
the syscall was called (not when the process was started), and we do
this by just looking at the notification ID. It doesn't care about which
process generated the notification.

Re: [PATCH 2/2] selftests/seccomp: Ensure that notifications come in FIFO order

[Tycho Andersen]

On Thu, Apr 28, 2022 at 09:38:10AM -0700, Sargun Dhillon wrote:
> On Thu, Apr 28, 2022 at 6:15 AM Tycho Andersen <tycho@tycho.pizza> wrote:
> > > +     for (i = 0; i < ARRAY_SIZE(pids); i++) {
> > > +             pid = fork();
> > > +             if (pid == 0) {
> > > +                     ret = syscall(__NR_getppid);
> > > +                     exit(ret != USER_NOTIF_MAGIC);
> > > +             }
> > > +             pids[i] = pid;
> > > +     }
> > > +
> > > +     /* This spins until all of the children are sleeping */
> > > +restart_wait:
> > > +     for (i = 0; i < ARRAY_SIZE(pids); i++) {
> > > +             if (get_proc_stat(pids[i]) != 'S') {
> > > +                     nanosleep(&delay, NULL);
> > > +                     goto restart_wait;
> > > +             }
> > > +     }
> >
> > I wonder if we should/can combine this loop with the previous one, and
> > wait for the child to sleep in getppid() before we fork the next one.
> > Otherwise isn't racy in the case that your loop continues to the next
> > iteration before the child processes are scheduled, so things might be
> > out of order? Maybe I'm missing something.
> >
> > In any case, this change seems reasonable to me.
> >
> > Tycho
> It's okay if the child processes are started out of order. The test just
> verifies that the calls are delivered in FIFO order according to when
> the syscall was called (not when the process was started), and we do
> this by just looking at the notification ID. It doesn't care about which
> process generated the notification.

I totally missed that you don't this, I just assumed you did. Thanks.

Anyway, you can add:

Acked-by: Tycho Andersen <tycho@tycho.pizza>

to both patches.

Tycho

Re: [PATCH 1/2] seccomp: Use FIFO semantics to order notifications

[Christian Brauner]

On Wed, Apr 27, 2022 at 06:54:46PM -0700, Sargun Dhillon wrote:
> Previously, the seccomp notifier used LIFO semantics, where each
> notification would be added on top of the stack, and notifications
> were popped off the top of the stack. This could result one process
> that generates a large number of notifications preventing other
> notifications from being handled. This patch moves from LIFO (stack)
> semantics to FIFO (queue semantics).
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---

It has a minimal user-visible impact in a sense but I don't think it
should be an issue. Makes sense to me,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH 1/2] seccomp: Use FIFO semantics to order notifications

[Kees Cook]

On Wed, 27 Apr 2022 18:54:46 -0700, Sargun Dhillon wrote:
> Previously, the seccomp notifier used LIFO semantics, where each
> notification would be added on top of the stack, and notifications
> were popped off the top of the stack. This could result one process
> that generates a large number of notifications preventing other
> notifications from being handled. This patch moves from LIFO (stack)
> semantics to FIFO (queue semantics).
> 
> [...]

Applied (which comment typo fix) to for-next/seccomp, thanks!

[1/2] seccomp: Use FIFO semantics to order notifications
      https://git.kernel.org/kees/c/4cbf6f621150
[2/2] selftests/seccomp: Ensure that notifications come in FIFO order
      https://git.kernel.org/kees/c/662340ef9218

-- 
Kees Cook