On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <[email protected]> wrote: > As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and > the ptrace code that can inspect a filter of another process. Let's > introduce read locking into the two ptrace accesses so that we don't race.
Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds the siglock while grabbing the seccomp filter and bumping its refcount. And TSYNC happens from seccomp_set_mode_filter(), which takes the siglock. So this looks okay to me? > Signed-off-by: Tycho Andersen <[email protected]> > Reported-by: Jann Horn <[email protected]> > CC: Kees Cook <[email protected]> > CC: Andy Lutomirski <[email protected]> > --- > include/linux/seccomp.h | 4 ++-- > kernel/seccomp.c | 10 ++++++++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h > index 8429bdda947a..30b27e898162 100644 > --- a/include/linux/seccomp.h > +++ b/include/linux/seccomp.h > @@ -22,8 +22,8 @@ struct seccomp_filter; > * @filter: must always point to a valid seccomp-filter or NULL as it is > * accessed without locking during system call entry. > * > - * @filter must only be accessed from the context of current as > there > - * is no read locking. > + * @filter is read-protected by task->signal->cred_guard_mutex when > + * outside of current context. > */ > struct seccomp { > int mode; > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index ef80dd19f268..f65d47650ac1 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1042,7 +1042,12 @@ int seccomp_get_filter(struct task_struct *task, > unsigned long filter_off, > return -EACCES; > } > > + ret = mutex_lock_killable(&task->signal->cred_guard_mutex); > + if (ret < 0) > + return ret; > + > filter = get_nth_filter(task, filter_off); > + mutex_unlock(&task->signal->cred_guard_mutex); > if (IS_ERR(filter)) > return PTR_ERR(filter); > > @@ -1088,7 +1093,12 @@ int seccomp_get_metadata(struct task_struct *task, > if (copy_from_user(&kmd.filter_off, data, sizeof(kmd.filter_off))) > return -EFAULT; > > + ret = mutex_lock_killable(&task->signal->cred_guard_mutex); > + if (ret < 0) > + return ret; > + > filter = get_nth_filter(task, kmd.filter_off); > + mutex_unlock(&task->signal->cred_guard_mutex); > if (IS_ERR(filter)) > return PTR_ERR(filter); > > -- > 2.17.1 >
