Messages in this thread | | | From | Kees Cook <> | Date | Sat, 12 Jan 2019 10:36:33 -0800 | Subject | Re: [PATCH] seccomp: fix UAF in user-trap code |
| |
On Sat, Jan 12, 2019 at 10:24 AM Tycho Andersen <tycho@tycho.ws> wrote: > > On the failure path, we do an fput() of the listener fd if the filter fails > to install (e.g. because of a TSYNC race that's lost, or if the thread is > killed, etc.). fput() doesn't actually release the fd, it just ads it to a > work queue. Then the thread proceeds to free the filter, even though the > listener struct file has a reference to it. > > To fix this, on the failure path let's set the private data to null, so we > know in ->release() to ignore the filter. > > Reported-by: syzbot+981c26489b2d1c6316ba@syzkaller.appspotmail.com > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > --- > This is a little ugly, but I can't really think of a better way (other than > force freeing, but the fput function that does the actual work is declared > static with four underscores :).
This makes sense to me. Thanks for fixing it!
Acked-by: Kees Cook <keescook@chromium.org>
James, can you add this to your fixes tree for sending to Linus?
-Kees
> --- > kernel/seccomp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index d7f538847b84..e815781ed751 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -976,6 +976,9 @@ static int seccomp_notify_release(struct inode *inode, struct file *file) > struct seccomp_filter *filter = file->private_data; > struct seccomp_knotif *knotif; > > + if (!filter) > + return 0; > + > mutex_lock(&filter->notify_lock); > > /* > @@ -1300,6 +1303,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > out_put_fd: > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > if (ret < 0) { > + listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > } else { > -- > 2.19.1 >
-- Kees Cook
| |