seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV
Subsystems (2)
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
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
@@ -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
Re: [PATCH 1/4] seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited
Quoted 7 lines · Andrei Vagin
SECCOMP_IOCTL_NOTIF_RECVpromptly 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.Reviewed-by Christian Brauner <[email protected]>Signed-off-by Andrei Vagin <[email protected]>--- kernel/seccomp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
PATCH v3 2/4 seccomp: release task filters when the task exits
In reply to Andrei VaginPreviously, 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
@@ -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
@@ -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
PATCH v3 3/4 selftests/seccomp: add test for NOTIF_RECV and unused filters
In reply to Andrei VaginAdd 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
@@ -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
PATCH v3 4/4 selftests/seccomp: check that a zombie leader doesn't affect others
In reply to Andrei VaginEnsure 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
@@ -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
Quoted 8 lines · Andrei Vagin
This patch set addresses two problems with the
SECCOMP_IOCTL_NOTIF_RECVioctl:
* 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
Tycho Andersen
Reviewed-byQuoted 9 lines · Kees Cook
Quoted 7 lines · Andrei Vagin
This patch set addresses two problems with the
SECCOMP_IOCTL_NOTIF_RECVioctl:
* 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,