Skip to contentSkip to conversation

seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

PATCH Applied Andrei Vagin 4 participants 8 messages Jun 28, 2024 – Jul 2, 2024

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v3 cover seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

This patch set addresses two problems with the SECCOMP_IOCTL_NOTIF_RECV ioctl:
* it doesn't return when the seccomp filter becomes unused (all tasks have exited).
* EPOLLHUP is triggered not when a task exits, but rather when its zombie is collected.

v2: - Remove unnecessary checks of PF_EXITING.
- Take siglock with disabling irqs.
Thanks to Oleg for the review and the help with the first version.
v3: - a few fixes suggested by Kees.
- add a test to ensure that a dead thread leader doesn't prevent installing new filters with SECCOMP_FILTER_FLAG_TSYNC.

Andrei Vagin (3):
seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited
seccomp: release task filters when the task exits selftests/seccomp: add test for NOTIF_RECV and unused filters selftests/seccomp: check that a zombie leader doesn't affect others

 kernel/exit.c                                 |   3 +-
 kernel/seccomp.c                              |  30 +++-
 tools/testing/selftests/seccomp/seccomp_bpf.c | 131 ++++++++++++++++++
 3 files changed, 157 insertions(+), 7 deletions(-)
Signature · 1 line
2.45.0.rc1.225.g2a3ae87e7f-goog
#1 lore raw

PATCH v3 1/4 seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited

SECCOMP_IOCTL_NOTIF_RECV promptly returns when a seccomp filter becomes unused, as a filter without users can't trigger any events.

Previously, event listeners had to rely on epoll to detect when all processes had exited.

The change is based on the 'commit 99cdb8b9a573 ("seccomp: notify about unused filter")' which implemented (E)POLLHUP notifications.

---
 kernel/seccomp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
kernel/seccomp.c +6 -1
index e30b60b57614..60990264fef0 100644
@@ -1466,7 +1466,7 @@ static int recv_wake_function(wait_queue_entry_t *wait, unsigned int mode, int s
 				  void *key)
 {
 	/* Avoid a wakeup if event not interesting for us. */
-	if (key && !(key_to_poll(key) & (EPOLLIN | EPOLLERR)))
+	if (key && !(key_to_poll(key) & (EPOLLIN | EPOLLERR | EPOLLHUP)))
 		return 0;
 	return autoremove_wake_function(wait, mode, sync, key);
 }
@@ -1476,6 +1476,9 @@ static int recv_wait_event(struct seccomp_filter *filter)
 	DEFINE_WAIT_FUNC(wait, recv_wake_function);
 	int ret;
 
+	if (refcount_read(&filter->users) == 0)
+		return 0;
+
 	if (atomic_dec_if_positive(&filter->notif->requests) >= 0)
 		return 0;
 
@@ -1484,6 +1487,8 @@ static int recv_wait_event(struct seccomp_filter *filter)
 
 		if (atomic_dec_if_positive(&filter->notif->requests) >= 0)
 			break;
+		if (refcount_read(&filter->users) == 0)
+			break;
 
 		if (ret)
 			return ret;
Signature · 1 line
2.45.2.803.g4e1b14247a-goog
Reviewed-by on v3 1/4
#2 lore raw

Re: [PATCH 1/4] seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited

Quoted 7 lines · Andrei Vagin

SECCOMP_IOCTL_NOTIF_RECV promptly returns when a seccomp filter becomes unused, as a filter without users can't trigger any events.

Previously, event listeners had to rely on epoll to detect when all processes had exited.

Quoted 8 lines

The change is based on the 'commit 99cdb8b9a573 ("seccomp: notify about unused filter")' which implemented (E)POLLHUP notifications.

---
 kernel/seccomp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
#3 lore raw

PATCH v3 2/4 seccomp: release task filters when the task exits

In reply to Andrei Vagin

Previously, seccomp filters were released in release_task(), which required the process to exit and its zombie to be collected. However, exited threads/processes can't trigger any seccomp events, making it more logical to release filters upon task exits.

This adjustment simplifies scenarios where a parent is tracing its child process. The parent process can now handle all events from a seccomp listening descriptor and then call wait to collect a child zombie.

seccomp_filter_release takes the siglock to avoid races with seccomp_sync_threads. There was an idea to bypass taking the lock by checking PF_EXITING, but it can be set without holding siglock if threads have SIGNAL_GROUP_EXIT. This means it can happen concurently with seccomp_filter_release.

This change also fixes another minor problem. Suppose that a group leader installs the new filter without SECCOMP_FILTER_FLAG_TSYNC, exits, and becomes a zombie. Without this change, SECCOMP_FILTER_FLAG_TSYNC from any other thread can never succeed, seccomp_can_sync_threads() will check a zombie leader and is_ancestor() will fail.

---
 kernel/exit.c    |  3 ++-
 kernel/seccomp.c | 23 ++++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)
kernel/exit.c +2 -1
index f95a2c1338a8..b945ab81eb92 100644
@@ -277,7 +277,6 @@ void release_task(struct task_struct *p)
 	}
 
 	write_unlock_irq(&tasklist_lock);
-	seccomp_filter_release(p);
 	proc_flush_pid(thread_pid);
 	put_pid(thread_pid);
 	release_thread(p);
@@ -832,6 +831,8 @@ void __noreturn do_exit(long code)
 	io_uring_files_cancel();
 	exit_signals(tsk);  /* sets PF_EXITING */
 
+	seccomp_filter_release(tsk);
+
 	acct_update_integrals(tsk);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
kernel/seccomp.c +18 -5
index 60990264fef0..dc51e521bc1d 100644
@@ -502,6 +502,9 @@ static inline pid_t seccomp_can_sync_threads(void)
 		/* Skip current, since it is initiating the sync. */
 		if (thread == caller)
 			continue;
+		/* Skip exited threads. */
+		if (thread->flags & PF_EXITING)
+			continue;
 
 		if (thread->seccomp.mode == SECCOMP_MODE_DISABLED ||
 		    (thread->seccomp.mode == SECCOMP_MODE_FILTER &&
@@ -563,18 +566,21 @@ static void __seccomp_filter_release(struct seccomp_filter *orig)
  * @tsk: task the filter should be released from.
  *
  * This function should only be called when the task is exiting as
- * it detaches it from its filter tree. As such, READ_ONCE() and
- * barriers are not needed here, as would normally be needed.
+ * it detaches it from its filter tree. PF_EXITING has to be set
+ * for the task.
  */
 void seccomp_filter_release(struct task_struct *tsk)
 {
-	struct seccomp_filter *orig = tsk->seccomp.filter;
+	struct seccomp_filter *orig;
 
-	/* We are effectively holding the siglock by not having any sighand. */
-	WARN_ON(tsk->sighand != NULL);
+	if (WARN_ON((tsk->flags & PF_EXITING) == 0))
+		return;
 
+	spin_lock_irq(&tsk->sighand->siglock);
+	orig = tsk->seccomp.filter;
 	/* Detach task from its filter tree. */
 	tsk->seccomp.filter = NULL;
+	spin_unlock_irq(&tsk->sighand->siglock);
 	__seccomp_filter_release(orig);
 }
 
@@ -602,6 +608,13 @@ static inline void seccomp_sync_threads(unsigned long flags)
 		if (thread == caller)
 			continue;
 
+		/*
+		 * Skip exited threads. seccomp_filter_release could have
+		 * been already called for this task.
+		 */
+		if (thread->flags & PF_EXITING)
+			continue;
+
 		/* Get a task reference for the new leaf node. */
 		get_seccomp_filter(caller);
 
Signature · 1 line
2.45.2.803.g4e1b14247a-goog
#4 lore raw

PATCH v3 3/4 selftests/seccomp: add test for NOTIF_RECV and unused filters

In reply to Andrei Vagin

Add a new test case to check that SECCOMP_IOCTL_NOTIF_RECV returns when all tasks have gone.

---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
tools/testing/selftests/seccomp/seccomp_bpf.c +54 -0
index 783ebce8c4de..390781d7c951 100644
@@ -3954,6 +3954,60 @@ TEST(user_notification_filter_empty)
 	EXPECT_GT((pollfd.revents & POLLHUP) ?: 0, 0);
 }
 
+TEST(user_ioctl_notification_filter_empty)
+{
+	pid_t pid;
+	long ret;
+	int status, p[2];
+	struct __clone_args args = {
+		.flags = CLONE_FILES,
+		.exit_signal = SIGCHLD,
+	};
+	struct seccomp_notif req = {};
+
+	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!");
+	}
+
+	if (__NR_clone3 < 0)
+		SKIP(return, "Test not built with clone3 support");
+
+	ASSERT_EQ(0, pipe(p));
+
+	pid = sys_clone3(&args, sizeof(args));
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		int listener;
+
+		listener = user_notif_syscall(__NR_mknodat, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+		if (listener < 0)
+			_exit(EXIT_FAILURE);
+
+		if (dup2(listener, 200) != 200)
+			_exit(EXIT_FAILURE);
+		close(p[1]);
+		close(listener);
+		sleep(1);
+
+		_exit(EXIT_SUCCESS);
+	}
+	if (read(p[0], &status, 1) != 0)
+		_exit(EXIT_SUCCESS);
+	close(p[0]);
+	/*
+	 * The seccomp filter has become unused so we should be notified once
+	 * the kernel gets around to cleaning up task struct.
+	 */
+	EXPECT_EQ(ioctl(200, SECCOMP_IOCTL_NOTIF_RECV, &req), -1);
+	EXPECT_EQ(errno, ENOENT);
+
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFEXITED(status));
+	EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
 static void *do_thread(void *data)
 {
 	return NULL;
Signature · 1 line
2.45.2.803.g4e1b14247a-goog
#5 lore raw

PATCH v3 4/4 selftests/seccomp: check that a zombie leader doesn't affect others

In reply to Andrei Vagin

Ensure that a dead thread leader doesn't prevent installing new filters with SECCOMP_FILTER_FLAG_TSYNC from other threads.

---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
tools/testing/selftests/seccomp/seccomp_bpf.c +77 -0
index 390781d7c951..e3f97f90d8db 100644
@@ -4809,6 +4809,83 @@ TEST(user_notification_wait_killable_fatal)
 	EXPECT_EQ(SIGTERM, WTERMSIG(status));
 }
 
+struct tsync_vs_thread_leader_args {
+	pthread_t leader;
+};
+
+static void *tsync_vs_dead_thread_leader_sibling(void *_args)
+{
+	struct sock_filter allow_filter[] = {
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog allow_prog = {
+		.len = (unsigned short)ARRAY_SIZE(allow_filter),
+		.filter = allow_filter,
+	};
+	struct tsync_vs_thread_leader_args *args = _args;
+	void *retval;
+	long ret;
+
+	ret = pthread_join(args->leader, &retval);
+	if (ret)
+		exit(1);
+	if (retval != _args)
+		exit(2);
+	ret = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, &allow_prog);
+	if (ret)
+		exit(3);
+
+	exit(0);
+}
+
+/*
+ * Ensure that a dead thread leader doesn't prevent installing new filters with
+ * SECCOMP_FILTER_FLAG_TSYNC from other threads.
+ */
+TEST(tsync_vs_dead_thread_leader)
+{
+	int status;
+	pid_t pid;
+	long ret;
+
+	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!");
+	}
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		struct sock_filter allow_filter[] = {
+			BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+		};
+		struct sock_fprog allow_prog = {
+			.len = (unsigned short)ARRAY_SIZE(allow_filter),
+			.filter = allow_filter,
+		};
+		struct  tsync_vs_thread_leader_args *args;
+		pthread_t sibling;
+
+		args = malloc(sizeof(*args));
+		ASSERT_NE(NULL, args);
+		args->leader = pthread_self();
+
+		ret = pthread_create(&sibling, NULL,
+				     tsync_vs_dead_thread_leader_sibling, args);
+		ASSERT_EQ(0, ret);
+
+		/* Install a new filter just to the leader thread. */
+		ret = seccomp(SECCOMP_SET_MODE_FILTER, 0, &allow_prog);
+		ASSERT_EQ(0, ret);
+		pthread_exit(args);
+		exit(1);
+	}
+
+	EXPECT_EQ(pid, waitpid(pid, &status, 0));
+	EXPECT_EQ(0, status);
+}
+
 /*
  * TODO:
  * - expand NNP testing
Signature · 1 line
2.45.2.803.g4e1b14247a-goog
maintainer Applied
#6 lore raw
In reply to Andrei Vagin
Quoted 8 lines · Andrei Vagin

This patch set addresses two problems with the SECCOMP_IOCTL_NOTIF_RECV ioctl:
* it doesn't return when the seccomp filter becomes unused (all tasks have exited).
* EPOLLHUP is triggered not when a task exits, but rather when its zombie is collected.

[...]

Applied to for-next/seccomp, thanks!

[1/4] seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited

      https://git.kernel.org/kees/c/6dbfc08fd57a
[2/4] seccomp: release task filters when the task exits
      https://git.kernel.org/kees/c/13eb42e2529e
[3/4] selftests/seccomp: add test for NOTIF_RECV and unused filters
      https://git.kernel.org/kees/c/9b366d69118b
[4/4] selftests/seccomp: check that a zombie leader doesn't affect others
      https://git.kernel.org/kees/c/22cc0f3b4d0c

Take care,

Signature · 1 line
Kees Cook
Reviewed-by
#7 lore raw
Quoted 9 lines · Kees Cook
Quoted 7 lines · Andrei Vagin

This patch set addresses two problems with the SECCOMP_IOCTL_NOTIF_RECV ioctl:
* it doesn't return when the seccomp filter becomes unused (all tasks have exited).
* EPOLLHUP is triggered not when a task exits, but rather when its zombie is collected.

Quoted 3 lines
Quoted 1 line

[...]

Applied to for-next/seccomp, thanks!

A little late to the party, but,