[PATCH 4/4] seccomp: publish user notifications after copying them to userspace

[Andrea Parri <parri.andrea@gmail.com>]

SECCOMP_FILTER_FLAG_RESTART_BEFORE_RECV restarts a syscall when a signal
interrupts its notification before the supervisor receives it. However,
seccomp_notify_recv() marks the notification SENT before copy_to_user().
Unless WAIT_KILLABLE_RECV is also set, a signal in that window makes the
target return EINTR while the supervisor can receive a stale notification
ID; with it, the signal is held back until the reply.

  CPU 1 (target)                   CPU 2 (supervisor)
  ------------------------------   ------------------------------
                                   mark notification SENT
  receive signal
  observe SENT                     copy notification to user
  remove notification
  return EINTR                     return success with stale ID

Cure this by reserving the notification in a new RECEIVING state. Move it
to SENT after copy_to_user() succeeds and it remains on the notification
list. The target's interrupted wait treats RECEIVING like INIT, while the
receiver returns ENOENT if the target withdraws the notification during
the copy.

When it returns ENOENT, the receiver clears the copied notification, so
userspace can retry with the same zeroed buffer, and returns EFAULT if
clearing fails.

The copy failure path no longer wakes a killable waiter. The target only
switches to a killable wait once its notification is SENT, which now
happens after the copy, and a SENT notification never returns to INIT.

Filters without RESTART_BEFORE_RECV see the same change. Without
WAIT_KILLABLE_RECV, the receiver fails with ENOENT instead of returning a
withdrawn ID. With it, a signal during the copy is handled normally, as
documented for signals sent before userspace receives the notification.

This should be safe because notify_lock protects every state transition
and the post-copy list lookup. seccomp_notify_recv() works from its
by-value copy across the unlocked copy and looks the notification up again
by ID, so it does not dereference the target's stack object after the
target can remove it.

Add a regression test which uses userfaultfd to stop copy_to_user() in the
race window and checks that the buffer is zeroed again on ENOENT. All four
flag variants fail without the kernel change and pass with it when run with
the following command.

  ./seccomp_bpf -t during_receive

The test needs CONFIG_USERFAULTFD, so add it to the seccomp selftest
config.

The race was found and the fix was written with the help of an LLM.

Fixes: f7952251ecc3 ("seccomp: allow restarting interrupted unreceived notifications")
Cc: stable@vger.kernel.org
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
 kernel/seccomp.c                              |  59 ++++----
 tools/testing/selftests/seccomp/config        |   1 +
 tools/testing/selftests/seccomp/seccomp_bpf.c | 132 ++++++++++++++++++
 3 files changed, 161 insertions(+), 31 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 75a758128db2d..7dfd5eec6d640 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -54,6 +54,7 @@
 
 enum notify_state {
 	SECCOMP_NOTIFY_INIT,
+	SECCOMP_NOTIFY_RECEIVING,
 	SECCOMP_NOTIFY_SENT,
 	SECCOMP_NOTIFY_REPLIED,
 };
@@ -74,11 +75,12 @@ struct seccomp_knotif {
 
 	/*
 	 * Notification states. When SECCOMP_RET_USER_NOTIF is returned, a
-	 * struct seccomp_knotif is created and starts out in INIT. Once the
-	 * handler reads the notification off of an FD, it transitions to SENT.
-	 * If a signal is received the state transitions back to INIT and
-	 * another message is sent. When the userspace handler replies, state
-	 * transitions to REPLIED.
+	 * struct seccomp_knotif is created and starts out in INIT. A receiver
+	 * reserves it by moving it to RECEIVING. Once the notification has been
+	 * copied to userspace, it transitions to SENT; if copying fails, it
+	 * returns to INIT. An interrupted wait may withdraw the notification in
+	 * any state, including RECEIVING. When the userspace handler replies,
+	 * state transitions to REPLIED.
 	 */
 	enum notify_state state;
 
@@ -1225,7 +1227,7 @@ static int seccomp_do_user_notification(int this_syscall,
 			 */
 			if (!(!wait_killable && should_sleep_killable(match, &n))) {
 				if (err == -ERESTARTSYS && match->restart_before_recv &&
-				    n.state == SECCOMP_NOTIFY_INIT)
+				    n.state <= SECCOMP_NOTIFY_RECEIVING)
 					err = -ERESTARTNOINTR;
 				goto interrupted;
 			}
@@ -1600,40 +1602,35 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
 	 * when we were able to acquire the rw lock.
 	 */
 	if (!knotif) {
-		ret = -ENOENT;
-		goto out;
+		mutex_unlock(&filter->notify_lock);
+		return -ENOENT;
 	}
 
 	unotif.id = knotif->id;
 	unotif.pid = task_pid_vnr(knotif->task);
 	unotif.data = *(knotif->data);
-
-	seccomp_notify_set_sent(filter, knotif);
-	ret = 0;
-out:
+	knotif->state = SECCOMP_NOTIFY_RECEIVING;
 	mutex_unlock(&filter->notify_lock);
 
-	if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
-		ret = -EFAULT;
+	ret = copy_to_user(buf, &unotif, sizeof(unotif)) ? -EFAULT : 0;
 
-		/*
-		 * Userspace screwed up. To make sure that we keep this
-		 * notification alive, let's reset it back to INIT. It
-		 * may have died when we released the lock, so we need to make
-		 * sure it's still around.
-		 */
-		mutex_lock(&filter->notify_lock);
-		knotif = find_notification(filter, unotif.id);
-		if (knotif) {
-			/* Reset the process to make sure it's not stuck */
-			if (should_sleep_killable(filter, knotif))
-				complete(&knotif->ready);
-			knotif->state = SECCOMP_NOTIFY_INIT;
-			atomic_inc(&filter->notif->requests);
-			wake_up_poll(&filter->wqh, EPOLLIN | EPOLLRDNORM);
-		}
-		mutex_unlock(&filter->notify_lock);
+	mutex_lock(&filter->notify_lock);
+	knotif = find_notification(filter, unotif.id);
+	if (!knotif) {
+		if (!ret)
+			ret = -ENOENT;
+	} else if (!ret) {
+		seccomp_notify_set_sent(filter, knotif);
+	} else {
+		knotif->state = SECCOMP_NOTIFY_INIT;
+		atomic_inc(&filter->notif->requests);
+		wake_up_poll(&filter->wqh, EPOLLIN | EPOLLRDNORM);
 	}
+	mutex_unlock(&filter->notify_lock);
+
+	/* Leave the buffer zeroed so that userspace can retry unchanged. */
+	if (ret == -ENOENT && clear_user(buf, sizeof(unotif)))
+		ret = -EFAULT;
 
 	return ret;
 }
diff --git a/tools/testing/selftests/seccomp/config b/tools/testing/selftests/seccomp/config
index ad431a5178fbe..c6e14eb473fd9 100644
--- a/tools/testing/selftests/seccomp/config
+++ b/tools/testing/selftests/seccomp/config
@@ -1,4 +1,5 @@
 CONFIG_PID_NS=y
 CONFIG_SECCOMP=y
 CONFIG_SECCOMP_FILTER=y
+CONFIG_USERFAULTFD=y
 CONFIG_USER_NS=y
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 123eddb0c0d64..1ac5b6960a1ce 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -22,6 +22,7 @@
 
 #include <errno.h>
 #include <linux/filter.h>
+#include <linux/userfaultfd.h>
 #include <sys/epoll.h>
 #include <sys/prctl.h>
 #include <sys/ptrace.h>
@@ -5137,6 +5138,137 @@ TEST_F(notification_restart, failed_receive)
 	notification_result(_metadata, self, -1, EAGAIN);
 }
 
+#ifdef __NR_userfaultfd
+struct notification_receive_args {
+	struct seccomp_notif *req;
+	int listener;
+	int ret;
+	int err;
+};
+
+static void *notification_receive(void *data)
+{
+	struct notification_receive_args *args = data;
+
+	args->ret = ioctl(args->listener, SECCOMP_IOCTL_NOTIF_RECV, args->req);
+	args->err = errno;
+	return NULL;
+}
+
+/*
+ * Write-protect the range through a new userfaultfd and return it, or
+ * return -1 with a skip reason if this kernel cannot report the fault.
+ */
+static int notification_uffd_wp(struct __test_metadata *_metadata,
+				const struct uffdio_range *range,
+				char *reason, size_t reason_len)
+{
+	struct uffdio_writeprotect wp = {};
+	struct uffdio_register reg = {};
+	struct uffdio_api api = {
+		.api = UFFD_API,
+		.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP,
+	};
+	int ret, uffd;
+
+	/* Kernel-originated faults require a privileged userfaultfd. */
+	uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
+	if (uffd < 0) {
+		snprintf(reason, reason_len,
+			 "userfaultfd for kernel faults unavailable: %s",
+			 strerror(errno));
+		return -1;
+	}
+	ret = ioctl(uffd, UFFDIO_API, &api);
+	if (ret < 0 && errno == EINVAL) {
+		snprintf(reason, reason_len, "userfaultfd API is not available");
+		goto skip;
+	}
+	ASSERT_EQ(ret, 0);
+	if (!(api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP)) {
+		snprintf(reason, reason_len,
+			 "userfaultfd write protection is not available");
+		goto skip;
+	}
+
+	reg.range = *range;
+	reg.mode = UFFDIO_REGISTER_MODE_WP;
+	ASSERT_EQ(ioctl(uffd, UFFDIO_REGISTER, &reg), 0);
+	wp.range = *range;
+	wp.mode = UFFDIO_WRITEPROTECT_MODE_WP;
+	ASSERT_EQ(ioctl(uffd, UFFDIO_WRITEPROTECT, &wp), 0);
+	return uffd;
+
+skip:
+	ASSERT_EQ(close(uffd), 0);
+	return -1;
+}
+
+TEST_F(notification_restart, during_receive)
+{
+	struct notification_receive_args args = { .listener = self->listener };
+	struct seccomp_notif req = {};
+	struct seccomp_notif_resp resp = {};
+	struct uffdio_writeprotect wp = {};
+	struct uffdio_range range = {};
+	struct pollfd pfd = { .events = POLLIN };
+	struct uffd_msg msg;
+	char reason[128];
+	pthread_t thread;
+	long page_size;
+	int uffd;
+
+	page_size = sysconf(_SC_PAGESIZE);
+	ASSERT_GT(page_size, 0);
+	args.req = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	ASSERT_NE(args.req, MAP_FAILED);
+	memset(args.req, 0, page_size);
+
+	range.start = (unsigned long)args.req;
+	range.len = page_size;
+	uffd = notification_uffd_wp(_metadata, &range, reason, sizeof(reason));
+	if (uffd < 0) {
+		ASSERT_EQ(munmap(args.req, page_size), 0);
+		SKIP(return, "%s", reason);
+	}
+
+	notification_restart_child(_metadata, self);
+	notification_pending(_metadata, self->listener);
+	ASSERT_EQ(pthread_create(&thread, NULL, notification_receive, &args), 0);
+	pfd.fd = uffd;
+	ASSERT_EQ(poll(&pfd, 1, 5000), 1);
+	ASSERT_EQ(read(uffd, &msg, sizeof(msg)), sizeof(msg));
+	ASSERT_EQ(msg.event, UFFD_EVENT_PAGEFAULT);
+	ASSERT_TRUE(msg.arg.pagefault.flags & UFFD_PAGEFAULT_FLAG_WP);
+
+	/* Withdraw the notification while copy_to_user() is stalled. */
+	notification_signal(_metadata, self);
+	wp.range = range;
+	wp.mode = 0;
+	ASSERT_EQ(ioctl(uffd, UFFDIO_WRITEPROTECT, &wp), 0);
+	ASSERT_EQ(pthread_join(thread, NULL), 0);
+	ASSERT_EQ(args.ret, -1);
+	EXPECT_EQ(args.err, ENOENT);
+	EXPECT_EQ(memcmp(args.req, &req, sizeof(req)), 0);
+	ASSERT_EQ(close(uffd), 0);
+	ASSERT_EQ(munmap(args.req, page_size), 0);
+
+	if (!variant->restart) {
+		notification_result(_metadata, self, -1, EINTR);
+		return;
+	}
+
+	notification_pending(_metadata, self->listener);
+	ASSERT_EQ(ioctl(self->listener, SECCOMP_IOCTL_NOTIF_RECV,
+			&req), 0);
+	resp.id = req.id;
+	resp.flags = SECCOMP_USER_NOTIF_FLAG_CONTINUE;
+	ASSERT_EQ(ioctl(self->listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+	notification_result(_metadata, self, getpid(), 0);
+}
+#endif
+
 TEST_F(notification_restart, after_receive)
 {
 	struct seccomp_notif req = {};
-- 
2.53.0