On Thu, Oct 01, 2020 at 12:00:46AM +0200, Jann Horn wrote:
> On Wed, Sep 30, 2020 at 5:20 PM YiFei Zhu <zhuyifei1...@gmail.com> wrote:
> [...]
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> [...]
> > +int proc_pid_seccomp_cache(struct seq_file *m, struct pid_namespace *ns,
> > + struct pid *pid, struct task_struct *task)
> > +{
> > + struct seccomp_filter *f;
> > +
> > + /*
> > + * We don't want some sandboxed process know what their seccomp
> > + * filters consist of.
> > + */
> > + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> > + return -EACCES;
> > +
> > + f = READ_ONCE(task->seccomp.filter);
> > + if (!f)
> > + return 0;
>
> Hmm, this won't work, because the task could be exiting, and seccomp
> filters are detached in release_task() (using
> seccomp_filter_release()). And at the moment, seccomp_filter_release()
> just locklessly NULLs out the tsk->seccomp.filter pointer and drops
> the reference.
Oh nice catch. Yeah, this would only happen if it was the only filter
remaining on a process with no children, etc.
>
> The locking here is kind of gross, but basically I think you can
> change this code to use lock_task_sighand() / unlock_task_sighand()
> (see the other examples in fs/proc/base.c), and bail out if
> lock_task_sighand() returns NULL. And in seccomp_filter_release(), add
> something like this:
>
> /* We are effectively holding the siglock by not having any sighand. */
> WARN_ON(tsk->sighand != NULL);
Yeah, good idea.
--
Kees Cook
