linux-kernel - Re: [PATCH v9 3/4] 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:   Mon, 3 Dec 2018 08:52:11 -0700
From:   Tycho Andersen <tycho@...ho.ws>
To:     "Serge E. Hallyn" <serge@...lyn.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        Oleg Nesterov <oleg@...hat.com>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Christian Brauner <christian@...uner.io>,
        Tyler Hicks <tyhicks@...onical.com>,
        Akihiro Suda <suda.akihiro@....ntt.co.jp>,
        Aleksa Sarai <asarai@...e.de>, Jann Horn <jannh@...gle.com>,
        linux-kernel@...r.kernel.org,
        containers@...ts.linux-foundation.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v9 3/4] seccomp: add a return code to trap to userspace

On Sun, Dec 02, 2018 at 11:26:50PM -0600, Serge E. Hallyn wrote:
> On Sun, Dec 02, 2018 at 08:28:26PM -0700, Tycho Andersen wrote:
> > +struct seccomp_knotif {
> > +	/* The struct pid of the task whose filter triggered the notification */
> > +	struct task_struct *task;
> > +
> > +	/* The "cookie" for this request; this is unique for this filter. */
> > +	u64 id;
> > +
> > +	/*
> > +	 * The seccomp data. This pointer is valid the entire time this
> > +	 * notification is active, since it comes from __seccomp_filter which
> 
> define 'active' - is a notification in state REPLIED still active?

Yes,

> Actually while looking at that, I came to wondering - when are knotifs
> freed?  Seems like only during seccomp_notify_release(), i.e. when the
> tracing task stops polling for events?  Is that going to be a problem?
> Or am I misreading this?

they're stack allocated in do_user_notification(). So "active" in this
sense really means "somewhere in do_user_notification()".

> > +	if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
> > +		ret = -EFAULT;
> > +
> > +		/*
> > +		 * Userspace screwed up. To make sure that we keep this
> > +		 * notification alive, let's reset it back to INIT. It
> 
> Is keeping the notification alive the right thing to do?
> 
> If userspace has messed up in something this touchy, it seems unlikely
> that it'll to better if we give it a do-over...  I'm not sure whether
> killing the whole thing (victim and tracer) is the right thing or not.

I suppose we could do that too. I just didn't want to get into a
situation where the notification is lost and the task is stuck because
userspace screwed up here.

Tycho