| From dfe719fef03d752f1682fa8aeddf30ba501c8555 Mon Sep 17 00:00:00 2001 |
| From: Jann Horn <jannh@google.com> |
| Date: Mon, 5 Oct 2020 03:44:01 +0200 |
| Subject: seccomp: Make duplicate listener detection non-racy |
| |
| From: Jann Horn <jannh@google.com> |
| |
| commit dfe719fef03d752f1682fa8aeddf30ba501c8555 upstream. |
| |
| Currently, init_listener() tries to prevent adding a filter with |
| SECCOMP_FILTER_FLAG_NEW_LISTENER if one of the existing filters already |
| has a listener. However, this check happens without holding any lock that |
| would prevent another thread from concurrently installing a new filter |
| (potentially with a listener) on top of the ones we already have. |
| |
| Theoretically, this is also a data race: The plain load from |
| current->seccomp.filter can race with concurrent writes to the same |
| location. |
| |
| Fix it by moving the check into the region that holds the siglock to guard |
| against concurrent TSYNC. |
| |
| (The "Fixes" tag points to the commit that introduced the theoretical |
| data race; concurrent installation of another filter with TSYNC only |
| became possible later, in commit 51891498f2da ("seccomp: allow TSYNC and |
| USER_NOTIF together").) |
| |
| Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") |
| Reviewed-by: Tycho Andersen <tycho@tycho.pizza> |
| Signed-off-by: Jann Horn <jannh@google.com> |
| Signed-off-by: Kees Cook <keescook@chromium.org> |
| Cc: stable@vger.kernel.org |
| Link: https://lore.kernel.org/r/20201005014401.490175-1-jannh@google.com |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| kernel/seccomp.c | 38 +++++++++++++++++++++++++++++++------- |
| 1 file changed, 31 insertions(+), 7 deletions(-) |
| |
| --- a/kernel/seccomp.c |
| +++ b/kernel/seccomp.c |
| @@ -1472,13 +1472,7 @@ static const struct file_operations secc |
| |
| static struct file *init_listener(struct seccomp_filter *filter) |
| { |
| - struct file *ret = ERR_PTR(-EBUSY); |
| - struct seccomp_filter *cur; |
| - |
| - for (cur = current->seccomp.filter; cur; cur = cur->prev) { |
| - if (cur->notif) |
| - goto out; |
| - } |
| + struct file *ret; |
| |
| ret = ERR_PTR(-ENOMEM); |
| filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL); |
| @@ -1504,6 +1498,31 @@ out: |
| return ret; |
| } |
| |
| +/* |
| + * Does @new_child have a listener while an ancestor also has a listener? |
| + * If so, we'll want to reject this filter. |
| + * This only has to be tested for the current process, even in the TSYNC case, |
| + * because TSYNC installs @child with the same parent on all threads. |
| + * Note that @new_child is not hooked up to its parent at this point yet, so |
| + * we use current->seccomp.filter. |
| + */ |
| +static bool has_duplicate_listener(struct seccomp_filter *new_child) |
| +{ |
| + struct seccomp_filter *cur; |
| + |
| + /* must be protected against concurrent TSYNC */ |
| + lockdep_assert_held(¤t->sighand->siglock); |
| + |
| + if (!new_child->notif) |
| + return false; |
| + for (cur = current->seccomp.filter; cur; cur = cur->prev) { |
| + if (cur->notif) |
| + return true; |
| + } |
| + |
| + return false; |
| +} |
| + |
| /** |
| * seccomp_set_mode_filter: internal function for setting seccomp filter |
| * @flags: flags to change filter behavior |
| @@ -1575,6 +1594,11 @@ static long seccomp_set_mode_filter(unsi |
| if (!seccomp_may_assign_mode(seccomp_mode)) |
| goto out; |
| |
| + if (has_duplicate_listener(prepared)) { |
| + ret = -EBUSY; |
| + goto out; |
| + } |
| + |
| ret = seccomp_attach_filter(flags, prepared); |
| if (ret) |
| goto out; |