[v11,11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC

Commit Message

Kees Cook July 16, 2014, 9:50 p.m. UTC

Applying restrictive seccomp filter programs to large or diverse
codebases often requires handling threads which may be started early in
the process lifetime (e.g., by code that is linked in). While it is
possible to apply permissive programs prior to process start up, it is
difficult to further restrict the kernel ABI to those threads after that
point.

This change adds a new seccomp syscall flag to SECCOMP_SET_MODE_FILTER for
synchronizing thread group seccomp filters at filter installation time.

When calling seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC,
filter) an attempt will be made to synchronize all threads in current's
threadgroup to its new seccomp filter program. This is possible iff all
threads are using a filter that is an ancestor to the filter current is
attempting to synchronize to. NULL filters (where the task is running as
SECCOMP_MODE_NONE) are also treated as ancestors allowing threads to be
transitioned into SECCOMP_MODE_FILTER. If prctrl(PR_SET_NO_NEW_PRIVS,
...) has been set on the calling thread, no_new_privs will be set for
all synchronized threads too. On success, 0 is returned. On failure,
the pid of one of the failing threads will be returned and no filters
will have been applied.

The race conditions against another thread are:
- requesting TSYNC (already handled by sighand lock)
- performing a clone (already handled by sighand lock)
- changing its filter (already handled by sighand lock)
- calling exec (handled by cred_guard_mutex)
The clone case is assisted by the fact that new threads will have their
seccomp state duplicated from their parent before appearing on the tasklist.

Holding cred_guard_mutex means that seccomp filters cannot be assigned
while in the middle of another thread's exec (potentially bypassing
no_new_privs or similar). The call to de_thread() may kill threads waiting
for the mutex.

Changes across threads to the filter pointer includes a barrier.

Based on patches by Will Drewry.

Suggested-by: Julien Tinnes <[e-mail]>
Signed-off-by: Kees Cook <[e-mail]>
Reviewed-by: Oleg Nesterov <[e-mail]>
Reviewed-by: Andy Lutomirski <[e-mail]>
---
 fs/exec.c                    |    2 +-
 include/linux/seccomp.h      |    2 +
 include/uapi/linux/seccomp.h |    3 +
 kernel/seccomp.c             |  130 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 135 insertions(+), 2 deletions(-)

Comments

David Drysdale July 17, 2014, 3:04 p.m. UTC | #1

On Wed, Jul 16, 2014 at 10:50 PM, Kees Cook <[e-mail]> wrote:
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 9065d2c79c56..2125b83ccfd4 100644
> +/**
> + * seccomp_can_sync_threads: checks if all threads can be synchronized
> + *
> + * Expects sighand and cred_guard_mutex locks to be held.
> + *
> + * Returns 0 on success, -ve on error, or the pid of a thread which was
> + * either not in the correct seccomp mode or it did not have an ancestral
> + * seccomp filter.
> + */
> +static inline pid_t seccomp_can_sync_threads(void)
> +{
> +       struct task_struct *thread, *caller;
> +
> +       BUG_ON(!mutex_is_locked(&current->signal->cred_guard_mutex));
> +       BUG_ON(!spin_is_locked(&current->sighand->siglock));
> +
> +       if (current->seccomp.mode != SECCOMP_MODE_FILTER)
> +               return -EACCES;

Quick question -- is it possible to apply the first filter and also synchronize
it across threads in the same operation?  If so, does this arm also need to
cope with seccomp.mode being SECCOMP_MODE_DISABLED?

[seccomp_set_mode_filter() looks to call this via seccomp_attach_filter()
before it does seccomp_assign_mode()]

> +
> +       /* Validate all threads being eligible for synchronization. */
> +       caller = current;
> +       for_each_thread(caller, thread) {
> +               pid_t failed;
> +
> +               if (thread->seccomp.mode == SECCOMP_MODE_DISABLED ||
> +                   (thread->seccomp.mode == SECCOMP_MODE_FILTER &&
> +                    is_ancestor(thread->seccomp.filter,
> +                                caller->seccomp.filter)))
> +                       continue;
> +
> +               /* Return the first thread that cannot be synchronized. */
> +               failed = task_pid_vnr(thread);
> +               /* If the pid cannot be resolved, then return -ESRCH */
> +               if (unlikely(WARN_ON(failed == 0)))
> +                       failed = -ESRCH;
> +               return failed;
> +       }
> +
> +       return 0;
> +}

Kees Cook July 17, 2014, 3:45 p.m. UTC | #2

On Thu, Jul 17, 2014 at 8:04 AM, David Drysdale <[e-mail]> wrote:
> On Wed, Jul 16, 2014 at 10:50 PM, Kees Cook <[e-mail]> wrote:
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 9065d2c79c56..2125b83ccfd4 100644
>> +/**
>> + * seccomp_can_sync_threads: checks if all threads can be synchronized
>> + *
>> + * Expects sighand and cred_guard_mutex locks to be held.
>> + *
>> + * Returns 0 on success, -ve on error, or the pid of a thread which was
>> + * either not in the correct seccomp mode or it did not have an ancestral
>> + * seccomp filter.
>> + */
>> +static inline pid_t seccomp_can_sync_threads(void)
>> +{
>> +       struct task_struct *thread, *caller;
>> +
>> +       BUG_ON(!mutex_is_locked(&current->signal->cred_guard_mutex));
>> +       BUG_ON(!spin_is_locked(&current->sighand->siglock));
>> +
>> +       if (current->seccomp.mode != SECCOMP_MODE_FILTER)
>> +               return -EACCES;
>
> Quick question -- is it possible to apply the first filter and also synchronize
> it across threads in the same operation?  If so, does this arm also need to
> cope with seccomp.mode being SECCOMP_MODE_DISABLED?
>
> [seccomp_set_mode_filter() looks to call this via seccomp_attach_filter()
> before it does seccomp_assign_mode()]

I don't entirely understand what you're asking. The threads gain the
filter and the mode before the current thread may gain the mode (if
it's the first time this has been called). Due to all the locks,
though, this isn't a problem. Is there a situation you see where there
might be a problem?

-Kees

Kees Cook July 17, 2014, 5:52 p.m. UTC | #3

On Thu, Jul 17, 2014 at 8:45 AM, Kees Cook <[e-mail]> wrote:
> On Thu, Jul 17, 2014 at 8:04 AM, David Drysdale <[e-mail]> wrote:
>> On Wed, Jul 16, 2014 at 10:50 PM, Kees Cook <[e-mail]> wrote:
>>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>>> index 9065d2c79c56..2125b83ccfd4 100644
>>> +/**
>>> + * seccomp_can_sync_threads: checks if all threads can be synchronized
>>> + *
>>> + * Expects sighand and cred_guard_mutex locks to be held.
>>> + *
>>> + * Returns 0 on success, -ve on error, or the pid of a thread which was
>>> + * either not in the correct seccomp mode or it did not have an ancestral
>>> + * seccomp filter.
>>> + */
>>> +static inline pid_t seccomp_can_sync_threads(void)
>>> +{
>>> +       struct task_struct *thread, *caller;
>>> +
>>> +       BUG_ON(!mutex_is_locked(&current->signal->cred_guard_mutex));
>>> +       BUG_ON(!spin_is_locked(&current->sighand->siglock));
>>> +
>>> +       if (current->seccomp.mode != SECCOMP_MODE_FILTER)
>>> +               return -EACCES;
>>
>> Quick question -- is it possible to apply the first filter and also synchronize
>> it across threads in the same operation?  If so, does this arm also need to
>> cope with seccomp.mode being SECCOMP_MODE_DISABLED?
>>
>> [seccomp_set_mode_filter() looks to call this via seccomp_attach_filter()
>> before it does seccomp_assign_mode()]
>
> I don't entirely understand what you're asking. The threads gain the
> filter and the mode before the current thread may gain the mode (if
> it's the first time this has been called). Due to all the locks,
> though, this isn't a problem. Is there a situation you see where there
> might be a problem?

Just to follow up for posterity on lkml: the problem was that mode was
being set in "current" _after_ sync, so the mode check in can_sync
would fail if "current" was not yet in filter mode. (i.e. the first
attached filter could not have the TSYNC flag.) This check was
redundant with the attach_filter entry point checks, and protected
nothing, so it has been removed and a new test added to the seccomp
regression test suite. :)

I sent it as a new patch on top of v11, instead of respinning
everything as v12. If that's not preferred, I can send v12 with this
fix incorporated.

Thanks!

-Kees

Patch

diff --git a/fs/exec.c b/fs/exec.c
index 0f5c272410f6..ab1f1200ce5d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1216,7 +1216,7 @@  EXPORT_SYMBOL(install_exec_creds);
 /*
  * determine how safe it is to execute the proposed program
  * - the caller must hold ->cred_guard_mutex to protect against
- *   PTRACE_ATTACH
+ *   PTRACE_ATTACH or seccomp thread-sync
  */
 static void check_unsafe_exec(struct linux_binprm *bprm)
 {
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 9ff98b4bfe2e..5d586a45a319 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -3,6 +3,8 @@ 
 
 #include <uapi/linux/seccomp.h>
 
+#define SECCOMP_FILTER_FLAG_MASK	(SECCOMP_FILTER_FLAG_TSYNC)
+
 #ifdef CONFIG_SECCOMP
 
 #include <linux/thread_info.h>
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index b258878ba754..0f238a43ff1e 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -14,6 +14,9 @@ 
 #define SECCOMP_SET_MODE_STRICT	0
 #define SECCOMP_SET_MODE_FILTER	1
 
+/* Valid flags for SECCOMP_SET_MODE_FILTER */
+#define SECCOMP_FILTER_FLAG_TSYNC	1
+
 /*
  * All BPF programs must return a 32-bit value.
  * The bottom 16-bits are for optional return data.
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 9065d2c79c56..2125b83ccfd4 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -26,6 +26,7 @@ 
 #ifdef CONFIG_SECCOMP_FILTER
 #include <asm/syscall.h>
 #include <linux/filter.h>
+#include <linux/pid.h>
 #include <linux/ptrace.h>
 #include <linux/security.h>
 #include <linux/tracehook.h>
@@ -225,6 +226,109 @@  static inline void seccomp_assign_mode(struct task_struct *task,
 }
 
 #ifdef CONFIG_SECCOMP_FILTER
+/* Returns 1 if the parent is an ancestor of the child. */
+static int is_ancestor(struct seccomp_filter *parent,
+		       struct seccomp_filter *child)
+{
+	/* NULL is the root ancestor. */
+	if (parent == NULL)
+		return 1;
+	for (; child; child = child->prev)
+		if (child == parent)
+			return 1;
+	return 0;
+}
+
+/**
+ * seccomp_can_sync_threads: checks if all threads can be synchronized
+ *
+ * Expects sighand and cred_guard_mutex locks to be held.
+ *
+ * Returns 0 on success, -ve on error, or the pid of a thread which was
+ * either not in the correct seccomp mode or it did not have an ancestral
+ * seccomp filter.
+ */
+static inline pid_t seccomp_can_sync_threads(void)
+{
+	struct task_struct *thread, *caller;
+
+	BUG_ON(!mutex_is_locked(&current->signal->cred_guard_mutex));
+	BUG_ON(!spin_is_locked(&current->sighand->siglock));
+
+	if (current->seccomp.mode != SECCOMP_MODE_FILTER)
+		return -EACCES;
+
+	/* Validate all threads being eligible for synchronization. */
+	caller = current;
+	for_each_thread(caller, thread) {
+		pid_t failed;
+
+		if (thread->seccomp.mode == SECCOMP_MODE_DISABLED ||
+		    (thread->seccomp.mode == SECCOMP_MODE_FILTER &&
+		     is_ancestor(thread->seccomp.filter,
+				 caller->seccomp.filter)))
+			continue;
+
+		/* Return the first thread that cannot be synchronized. */
+		failed = task_pid_vnr(thread);
+		/* If the pid cannot be resolved, then return -ESRCH */
+		if (unlikely(WARN_ON(failed == 0)))
+			failed = -ESRCH;
+		return failed;
+	}
+
+	return 0;
+}
+
+/**
+ * seccomp_sync_threads: sets all threads to use current's filter
+ *
+ * Expects sighand and cred_guard_mutex locks to be held, and for
+ * seccomp_can_sync_threads() to have returned success already
+ * without dropping the locks.
+ *
+ */
+static inline void seccomp_sync_threads(void)
+{
+	struct task_struct *thread, *caller;
+
+	BUG_ON(!mutex_is_locked(&current->signal->cred_guard_mutex));
+	BUG_ON(!spin_is_locked(&current->sighand->siglock));
+
+	/* Synchronize all threads. */
+	caller = current;
+	for_each_thread(caller, thread) {
+		/* Get a task reference for the new leaf node. */
+		get_seccomp_filter(caller);
+		/*
+		 * Drop the task reference to the shared ancestor since
+		 * current's path will hold a reference.  (This also
+		 * allows a put before the assignment.)
+		 */
+		put_seccomp_filter(thread);
+		smp_store_release(&thread->seccomp.filter,
+				  caller->seccomp.filter);
+		/*
+		 * Opt the other thread into seccomp if needed.
+		 * As threads are considered to be trust-realm
+		 * equivalent (see ptrace_may_access), it is safe to
+		 * allow one thread to transition the other.
+		 */
+		if (thread->seccomp.mode == SECCOMP_MODE_DISABLED) {
+			/*
+			 * Don't let an unprivileged task work around
+			 * the no_new_privs restriction by creating
+			 * a thread that sets it up, enters seccomp,
+			 * then dies.
+			 */
+			if (task_no_new_privs(caller))
+				task_set_no_new_privs(thread);
+
+			seccomp_assign_mode(thread, SECCOMP_MODE_FILTER);
+		}
+	}
+}
+
 /**
  * seccomp_prepare_filter: Prepares a seccomp filter for use.
  * @fprog: BPF program to install
@@ -364,6 +468,15 @@  static long seccomp_attach_filter(unsigned int flags,
 	if (total_insns > MAX_INSNS_PER_PATH)
 		return -ENOMEM;
 
+	/* If thread sync has been requested, check that it is possible. */
+	if (flags & SECCOMP_FILTER_FLAG_TSYNC) {
+		int ret;
+
+		ret = seccomp_can_sync_threads();
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * If there is an existing filter, make it the prev and don't drop its
 	 * task reference.
@@ -371,6 +484,10 @@  static long seccomp_attach_filter(unsigned int flags,
 	filter->prev = current->seccomp.filter;
 	current->seccomp.filter = filter;
 
+	/* Now that the new filter is in place, synchronize to all threads. */
+	if (flags & SECCOMP_FILTER_FLAG_TSYNC)
+		seccomp_sync_threads();
+
 	return 0;
 }
 
@@ -590,7 +707,7 @@  static long seccomp_set_mode_filter(unsigned int flags,
 	long ret = -EINVAL;
 
 	/* Validate flags. */
-	if (flags != 0)
+	if (flags & ~SECCOMP_FILTER_FLAG_MASK)
 		return -EINVAL;
 
 	/* Prepare the new filter before holding any locks. */
@@ -598,6 +715,14 @@  static long seccomp_set_mode_filter(unsigned int flags,
 	if (IS_ERR(prepared))
 		return PTR_ERR(prepared);
 
+	/*
+	 * Make sure we cannot change seccomp or nnp state via TSYNC
+	 * while another thread is in the middle of calling exec.
+	 */
+	if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
+	    mutex_lock_killable(&current->signal->cred_guard_mutex))
+		goto out_free;
+
 	spin_lock_irq(&current->sighand->siglock);
 
 	if (!seccomp_may_assign_mode(seccomp_mode))
@@ -612,6 +737,9 @@  static long seccomp_set_mode_filter(unsigned int flags,
 	seccomp_assign_mode(current, seccomp_mode);
 out:
 	spin_unlock_irq(&current->sighand->siglock);
+	if (flags & SECCOMP_FILTER_FLAG_TSYNC)
+		mutex_unlock(&current->signal->cred_guard_mutex);
+out_free:
 	seccomp_filter_free(prepared);
 	return ret;
 }