From: Tycho Andersen <tycho@tycho.ws>
To: Kees Cook <keescook@chromium.org>
Cc: Jann Horn <jannh@google.com>, Christoph Hellwig <hch@lst.de>,
Al Viro <viro@zeniv.linux.org.uk>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
kernel list <linux-kernel@vger.kernel.org>,
Linux Containers <containers@lists.linux-foundation.org>,
Linux API <linux-api@vger.kernel.org>,
Andy Lutomirski <luto@amacapital.net>,
Oleg Nesterov <oleg@redhat.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
Christian Brauner <christian.brauner@ubuntu.com>,
Tyler Hicks <tyhicks@canonical.com>,
Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Subject: Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
Date: Thu, 27 Sep 2018 17:08:06 -0600 [thread overview]
Message-ID: <20180927230806.GI15491@cisco.cisco.com> (raw)
In-Reply-To: <CAGXu5j+uqgCkxaEPoOa_+XSTJBe==bNATCewsqqzwiyKhbLGVQ@mail.gmail.com>
On Thu, Sep 27, 2018 at 03:45:11PM -0700, Kees Cook wrote:
> On Thu, Sep 27, 2018 at 2:51 PM, Jann Horn <jannh@google.com> wrote:
> > On Thu, Sep 27, 2018 at 5:11 PM Tycho Andersen <tycho@tycho.ws> wrote:
> >> However, care should be taken to avoid the TOCTOU
> >> +mentioned above in this document: all arguments being read from the tracee's
> >> +memory should be read into the tracer's memory before any policy decisions are
> >> +made. This allows for an atomic decision on syscall arguments.
> >
> > Again, I don't really see how you could get this wrong.
>
> Doesn't hurt to mention it, IMO.
>
> >> +static long seccomp_notify_send(struct seccomp_filter *filter,
> >> + unsigned long arg)
> >> +{
> >> + struct seccomp_notif_resp resp = {};
> >> + struct seccomp_knotif *knotif = NULL;
> >> + long ret;
> >> + u16 size;
> >> + void __user *buf = (void __user *)arg;
> >> +
> >> + if (copy_from_user(&size, buf, sizeof(size)))
> >> + return -EFAULT;
> >> + size = min_t(size_t, size, sizeof(resp));
> >> + if (copy_from_user(&resp, buf, size))
> >> + return -EFAULT;
> >> +
> >> + ret = mutex_lock_interruptible(&filter->notify_lock);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + list_for_each_entry(knotif, &filter->notif->notifications, list) {
> >> + if (knotif->id == resp.id)
> >> + break;
> >> + }
> >> +
> >> + if (!knotif || knotif->id != resp.id) {
> >
> > Uuuh, this looks unsafe and wrong. I don't think `knotif` can ever be
> > NULL here. If `filter->notif->notifications` is empty, I think
> > `knotif` will be `container_of(&filter->notif->notifications, struct
> > seccom_knotif, list)` - in other words, you'll have a type confusion,
> > and `knotif` probably points into some random memory in front of
> > `filter->notif`.
> >
> > Am I missing something?
>
> Oh, good catch. This just needs to be fixed like it's done in
> seccomp_notif_recv (separate cur and knotif).
>
> >> +static struct file *init_listener(struct task_struct *task,
> >> + struct seccomp_filter *filter)
> >> +{
> >
> > Why does this function take a `task` pointer instead of always
> > accessing `current`? If `task` actually wasn't `current`, I would have
> > concurrency concerns. A comment in seccomp.h even explains:
> >
> > * @filter must only be accessed from the context of current as there
> > * is no read locking.
> >
> > Unless there's a good reason for it, I would prefer it if this
> > function didn't take a `task` pointer.
>
> This is to support PTRACE_SECCOMP_NEW_LISTENER.
>
> But you make an excellent point. Even TSYNC expects to operate only on
> the current thread group. Hmm.
>
> While the process is stopped by ptrace, we could, in theory, update
> task->seccomp.filter via something like TSYNC.
>
> So perhaps use:
>
> mutex_lock_killable(&task->signal->cred_guard_mutex);
>
> before walking the notify_locks?
This means that all the seccomp/ptrace code probably needs to be
updated for this? I'll try to send patches for this as well as the
return code thing Jann pointed out.
> >
> >> + struct file *ret = ERR_PTR(-EBUSY);
> >> + struct seccomp_filter *cur, *last_locked = NULL;
> >> + int filter_nesting = 0;
> >> +
> >> + for (cur = task->seccomp.filter; cur; cur = cur->prev) {
> >> + mutex_lock_nested(&cur->notify_lock, filter_nesting);
> >> + filter_nesting++;
> >> + last_locked = cur;
> >> + if (cur->notif)
> >> + goto out;
> >> + }
> >> +
> >> + ret = ERR_PTR(-ENOMEM);
> >> + filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL);
> >
> > sizeof(struct notification) instead, to make the code clearer?
>
> I prefer what Tycho has: I want to allocate an instances of whatever
> filter->notif is.
>
> Though, let's do the kzalloc outside of the locking, instead?
Yep, sounds good.
> >> + ret = anon_inode_getfile("seccomp notify", &seccomp_notify_ops,
> >> + filter, O_RDWR);
> >> + if (IS_ERR(ret))
> >> + goto out;
> >> +
> >> +
> >> + /* The file has a reference to it now */
> >> + __get_seccomp_filter(filter);
> >
> > __get_seccomp_filter() has a comment in it that claims "/* Reference
> > count is bounded by the number of total processes. */". I think this
> > change invalidates that comment. I think it should be fine to just
> > remove the comment.
>
> Update it to "bounded by total processes and notification listeners"?
Will do.
> >> +out:
> >> + for (cur = task->seccomp.filter; cur; cur = cur->prev) {
> >
> > s/; cur;/; 1;/, or use a while loop instead? If the NULL check fires
> > here, something went very wrong.
>
> Hm? This is correct. This is how seccomp_run_filters() walks the list too:
>
> struct seccomp_filter *f =
> READ_ONCE(current->seccomp.filter);
> ...
> for (; f; f = f->prev) {
>
> Especially if we'll be holding the cred_guard_mutex.
There is a last_locked local here though, I think that's what Jann is
pointing out.
Cheers,
Tycho
next prev parent reply other threads:[~2018-09-27 23:08 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-27 15:11 [PATCH v7 0/6] seccomp trap to userspace Tycho Andersen
2018-09-27 15:11 ` [PATCH v7 1/6] seccomp: add a return code to " Tycho Andersen
2018-09-27 21:31 ` Kees Cook
2018-09-27 22:48 ` Tycho Andersen
2018-09-27 23:10 ` Kees Cook
2018-09-28 14:39 ` Tycho Andersen
2018-10-08 14:58 ` Christian Brauner
2018-10-09 14:28 ` Tycho Andersen
2018-10-09 16:24 ` Christian Brauner
2018-10-09 16:29 ` Tycho Andersen
2018-10-17 20:29 ` Tycho Andersen
2018-10-17 22:21 ` Kees Cook
2018-10-17 22:33 ` Tycho Andersen
2018-10-21 16:04 ` Tycho Andersen
2018-10-22 9:42 ` Christian Brauner
2018-09-27 21:51 ` Jann Horn
2018-09-27 22:45 ` Kees Cook
2018-09-27 23:08 ` Tycho Andersen [this message]
2018-09-27 23:04 ` Tycho Andersen
2018-09-27 23:37 ` Jann Horn
2018-09-29 0:28 ` Aleksa Sarai
2018-09-27 15:11 ` [PATCH v7 2/6] seccomp: make get_nth_filter available outside of CHECKPOINT_RESTORE Tycho Andersen
2018-09-27 16:51 ` Jann Horn
2018-09-27 21:42 ` Kees Cook
2018-10-08 13:55 ` Christian Brauner
2018-09-27 15:11 ` [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace Tycho Andersen
2018-09-27 16:20 ` Jann Horn
2018-09-27 16:34 ` Tycho Andersen
2018-09-27 17:35 ` Jann Horn
2018-09-27 18:09 ` Tycho Andersen
2018-09-27 21:53 ` Kees Cook
2018-10-08 15:16 ` Christian Brauner
2018-10-08 15:33 ` Jann Horn
2018-10-08 16:21 ` Christian Brauner
2018-10-08 16:42 ` Jann Horn
2018-10-08 18:18 ` Christian Brauner
2018-10-09 12:39 ` Jann Horn
2018-10-09 13:28 ` Christian Brauner
2018-10-09 13:36 ` Jann Horn
2018-10-09 13:49 ` Christian Brauner
2018-10-09 13:50 ` Jann Horn
2018-10-09 14:09 ` Christian Brauner
2018-10-09 15:26 ` Jann Horn
2018-10-09 16:20 ` Christian Brauner
2018-10-09 16:26 ` Jann Horn
2018-10-10 12:54 ` Christian Brauner
2018-10-10 13:09 ` Christian Brauner
2018-10-10 13:10 ` Jann Horn
2018-10-10 13:18 ` Christian Brauner
2018-10-10 15:31 ` Paul Moore
2018-10-10 15:33 ` Jann Horn
2018-10-10 15:39 ` Christian Brauner
2018-10-10 16:54 ` Tycho Andersen
2018-10-10 17:15 ` Christian Brauner
2018-10-10 17:26 ` Tycho Andersen
2018-10-10 18:28 ` Christian Brauner
2018-10-11 7:24 ` Paul Moore
2018-10-11 13:39 ` Jann Horn
2018-10-11 23:10 ` Paul Moore
2018-10-12 1:02 ` Andy Lutomirski
2018-10-12 20:02 ` Tycho Andersen
2018-10-12 20:06 ` Jann Horn
2018-10-12 20:11 ` Christian Brauner
2018-10-08 18:00 ` Tycho Andersen
2018-10-08 18:41 ` Christian Brauner
2018-10-10 17:45 ` Andy Lutomirski
2018-10-10 18:26 ` Christian Brauner
2018-09-27 15:11 ` [PATCH v7 4/6] files: add a replace_fd_files() function Tycho Andersen
2018-09-27 16:49 ` Jann Horn
2018-09-27 18:04 ` Tycho Andersen
2018-09-27 21:59 ` Kees Cook
2018-09-28 2:20 ` Kees Cook
2018-09-28 2:46 ` Jann Horn
2018-09-28 5:23 ` Tycho Andersen
2018-09-27 15:11 ` [PATCH v7 5/6] seccomp: add a way to pass FDs via a notification fd Tycho Andersen
2018-09-27 16:39 ` Jann Horn
2018-09-27 22:13 ` Tycho Andersen
2018-09-27 19:28 ` Jann Horn
2018-09-27 22:14 ` Tycho Andersen
2018-09-27 22:17 ` Jann Horn
2018-09-27 22:49 ` Tycho Andersen
2018-09-27 22:09 ` Kees Cook
2018-09-27 22:15 ` Tycho Andersen
2018-09-27 15:11 ` [PATCH v7 6/6] samples: add an example of seccomp user trap Tycho Andersen
2018-09-27 22:11 ` Kees Cook
2018-09-28 21:57 ` [PATCH v7 0/6] seccomp trap to userspace Michael Kerrisk (man-opages)
2018-09-28 22:03 ` Tycho Andersen
2018-09-28 22:16 ` Michael Kerrisk (man-pages)
2018-09-28 22:34 ` Kees Cook
2018-09-28 22:46 ` Michael Kerrisk (man-pages)
2018-09-28 22:48 ` Jann Horn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180927230806.GI15491@cisco.cisco.com \
--to=tycho@tycho.ws \
--cc=christian.brauner@ubuntu.com \
--cc=containers@lists.linux-foundation.org \
--cc=ebiederm@xmission.com \
--cc=hch@lst.de \
--cc=jannh@google.com \
--cc=keescook@chromium.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=oleg@redhat.com \
--cc=serge@hallyn.com \
--cc=suda.akihiro@lab.ntt.co.jp \
--cc=tyhicks@canonical.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).