]>
Commit | Line | Data |
---|---|---|
e03f04f8 GKH |
1 | From 7a0df7fbc14505e2e2be19ed08654a09e1ed5bf6 Mon Sep 17 00:00:00 2001 |
2 | From: Tycho Andersen <tycho@tycho.ws> | |
3 | Date: Wed, 6 Mar 2019 13:14:13 -0700 | |
4 | Subject: seccomp: Make NEW_LISTENER and TSYNC flags exclusive | |
5 | ||
6 | From: Tycho Andersen <tycho@tycho.ws> | |
7 | ||
8 | commit 7a0df7fbc14505e2e2be19ed08654a09e1ed5bf6 upstream. | |
9 | ||
10 | As the comment notes, the return codes for TSYNC and NEW_LISTENER | |
11 | conflict, because they both return positive values, one in the case of | |
12 | success and one in the case of error. So, let's disallow both of these | |
13 | flags together. | |
14 | ||
15 | While this is technically a userspace break, all the users I know | |
16 | of are still waiting on me to land this feature in libseccomp, so I | |
17 | think it'll be safe. Also, at present my use case doesn't require | |
18 | TSYNC at all, so this isn't a big deal to disallow. If someone | |
19 | wanted to support this, a path forward would be to add a new flag like | |
20 | TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, | |
21 | but the use cases are so different I don't see it really happening. | |
22 | ||
23 | Finally, it's worth noting that this does actually fix a UAF issue: at the | |
24 | end of seccomp_set_mode_filter(), we have: | |
25 | ||
26 | if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { | |
27 | if (ret < 0) { | |
28 | listener_f->private_data = NULL; | |
29 | fput(listener_f); | |
30 | put_unused_fd(listener); | |
31 | } else { | |
32 | fd_install(listener, listener_f); | |
33 | ret = listener; | |
34 | } | |
35 | } | |
36 | out_free: | |
37 | seccomp_filter_free(prepared); | |
38 | ||
39 | But if ret > 0 because TSYNC raced, we'll install the listener fd and then | |
40 | free the filter out from underneath it, causing a UAF when the task closes | |
41 | it or dies. This patch also switches the condition to be simply if (ret), | |
42 | so that if someone does add the flag mentioned above, they won't have to | |
43 | remember to fix this too. | |
44 | ||
45 | Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com | |
46 | Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") | |
47 | CC: stable@vger.kernel.org # v5.0+ | |
48 | Signed-off-by: Tycho Andersen <tycho@tycho.ws> | |
49 | Signed-off-by: Kees Cook <keescook@chromium.org> | |
50 | Acked-by: James Morris <jamorris@linux.microsoft.com> | |
51 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
52 | ||
53 | --- | |
54 | kernel/seccomp.c | 17 +++++++++++++++-- | |
55 | 1 file changed, 15 insertions(+), 2 deletions(-) | |
56 | ||
57 | --- a/kernel/seccomp.c | |
58 | +++ b/kernel/seccomp.c | |
59 | @@ -500,7 +500,10 @@ out: | |
60 | * | |
61 | * Caller must be holding current->sighand->siglock lock. | |
62 | * | |
63 | - * Returns 0 on success, -ve on error. | |
64 | + * Returns 0 on success, -ve on error, or | |
65 | + * - in TSYNC mode: the pid of a thread which was either not in the correct | |
66 | + * seccomp mode or did not have an ancestral seccomp filter | |
67 | + * - in NEW_LISTENER mode: the fd of the new listener | |
68 | */ | |
69 | static long seccomp_attach_filter(unsigned int flags, | |
70 | struct seccomp_filter *filter) | |
71 | @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsi | |
72 | if (flags & ~SECCOMP_FILTER_FLAG_MASK) | |
73 | return -EINVAL; | |
74 | ||
75 | + /* | |
76 | + * In the successful case, NEW_LISTENER returns the new listener fd. | |
77 | + * But in the failure case, TSYNC returns the thread that died. If you | |
78 | + * combine these two flags, there's no way to tell whether something | |
79 | + * succeeded or failed. So, let's disallow this combination. | |
80 | + */ | |
81 | + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && | |
82 | + (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER)) | |
83 | + return -EINVAL; | |
84 | + | |
85 | /* Prepare the new filter before holding any locks. */ | |
86 | prepared = seccomp_prepare_user_filter(filter); | |
87 | if (IS_ERR(prepared)) | |
88 | @@ -1302,7 +1315,7 @@ out: | |
89 | mutex_unlock(¤t->signal->cred_guard_mutex); | |
90 | out_put_fd: | |
91 | if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { | |
92 | - if (ret < 0) { | |
93 | + if (ret) { | |
94 | listener_f->private_data = NULL; | |
95 | fput(listener_f); | |
96 | put_unused_fd(listener); |