linux-kernel - Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Nov 2018 07:38:39 -0600
From:   Tycho Andersen <tycho@...ho.ws>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Christian Brauner <christian@...uner.io>,
        Tyler Hicks <tyhicks@...onical.com>,
        Akihiro Suda <suda.akihiro@....ntt.co.jp>,
        Aleksa Sarai <asarai@...e.de>, linux-kernel@...r.kernel.org,
        containers@...ts.linux-foundation.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace

On Fri, Nov 02, 2018 at 11:02:35AM +0100, Oleg Nesterov wrote:
> On 11/01, Tycho Andersen wrote:
> >
> > On Thu, Nov 01, 2018 at 02:40:02PM +0100, Oleg Nesterov wrote:
> > >
> > > Somehow I no longer understand why do you need to take all locks. Isn't
> > > the first filter's notify_lock enough? IOW,
> > >
> > > 		for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> > > 			if (cur->notif)
> > > 				return ERR_PTR(-EBUSY);
> > > 			first = cur;
> > > 		}
> > >
> > > 		if (first)
> > > 			mutex_lock(&first->notify_lock);
> > >
> > > 		... initialize filter->notif ...
> > >
> > > 	out:
> > > 		if (first)
> > > 			mutex_unlock(&first->notify_lock);
> > >
> > > 		return ret;
> >
> > The idea here is to prevent people from "nesting" notify filters. So
> > if any filter in the chain has a listener attached, it refuses to
> > install another filter with a listener.
> 
> Yes, I understand, so we need to check cur->notif. My point was, we do not
> need to take all the locks in the ->prev chain, we need only one:
> first->notify_lock.
> 
> But you know what? today I think that we do not need any locking at all,
> all we need is the lockless
> 
> 	for (cur = current->seccomp.filter; cur; cur = cur->prev)
> 		if (cur->notif)
> 			return ERR_PTR(-EBUSY);
> 
> at the start, nothing more.

Hmm, you're right. The locking is residual from when the old ptrace()
API was also a thing: with that, you could attach a filter at any
point in the tree, so we needed to lock to prevent that. But now
that it's only possible to do it at the bottom, we don't need to lock.

> > But it just occurred to me that we don't handle the TSYNC case
> > correctly by doing it this way,
> 
> Why? Perhaps I missed your point, but TSYNC case looks fine. I mean, if 2
> threads do seccomp_set_mode_filter(NEW_LISTENER | TSYNC) then only one can
> win the race and succeed, but this has nothing to do with init_listener(),
> we rely on ->siglock and is_ancestor() check.

Yes, agreed.

Thanks!

Tycho