Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace

On Thu, Sep 27, 2018 at 11:51:40PM +0200, Jann Horn wrote:

+Christoph Hellwig, Al Viro, fsdevel: For two questions about the poll
interface (search for "seccomp_notify_poll" and
"seccomp_notify_release" in the patch)

@Tycho: FYI, I've gone through all of v7 now, apart from the
test/sample code. So don't wait for more comments from me before
sending out v8.

(assuming you meant v8 -> v9) yes thanks for your reviews! Much
appreciated.

On Thu, Sep 27, 2018 at 5:11 PM Tycho Andersen [off-list ref] wrote:

quoted

This patch introduces a means for syscalls matched in seccomp to notify
some other task that a particular filter has been triggered.

The motivation for this is primarily for use with containers. For example,
if a container does an init_module(), we obviously don't want to load this
untrusted code, which may be compiled for the wrong version of the kernel
anyway. Instead, we could parse the module image, figure out which module
the container is trying to load and load it on the host.

As another example, containers cannot mknod(), since this checks
capable(CAP_SYS_ADMIN). However, harmless devices like /dev/null or
/dev/zero should be ok for containers to mknod, but we'd like to avoid hard
coding some whitelist in the kernel. Another example is mount(), which has
many security restrictions for good reason, but configuration or runtime
knowledge could potentially be used to relax these restrictions.

Note that in that case, the trusted runtime needs to be in the same
mount namespace as the container. mount() doesn't work on the mount
structure of a foreign mount namespace; check_mnt() specifically
checks for this case, and I think pretty much everything in
sys_mount() uses that check. So you'd have to join the container's
mount namespace before forwarding a mount syscall.

Yep, Serge came up with a pretty neat trick that we used in LXD to
accomplish sending mounts to containers, but it requires some
coordination up front.

quoted

This patch adds functionality that is already possible via at least two
other means that I know about, both of which involve ptrace(): first, one
could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
Unfortunately this is slow, so a faster version would be to install a
filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
Since ptrace allows only one tracer, if the container runtime is that
tracer, users inside the container (or outside) trying to debug it will not
be able to use ptrace, which is annoying. It also means that older
distributions based on Upstart cannot boot inside containers using ptrace,
since upstart itself uses ptrace to start services.

The actual implementation of this is fairly small, although getting the
synchronization right was/is slightly complex.

Finally, it's worth noting that the classic seccomp TOCTOU of reading
memory data from the task still applies here,

Actually, it doesn't, right? It would apply if you told the kernel "go
ahead, that syscall is fine", but that's not how the API works - you
always intercept the syscall, copy argument data to a trusted tracer,
and then the tracer can make a replacement syscall. Sounds fine to me.

Right, I guess the point here is just "you need to copy all the data
to the tracer *before* making a policy decision".

quoted

but can be avoided with
careful design of the userspace handler: if the userspace handler reads all
of the task memory that is necessary before applying its security policy,
the tracee's subsequent memory edits will not be read by the tracer.

[...]

quoted

diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst

[...]

quoted

+which (on success) will return a listener fd for the filter, which can then be
+passed around via ``SCM_RIGHTS`` or similar. Alternatively, a filter fd can be
+acquired via:
+
+.. code-block::
+
+    fd = ptrace(PTRACE_SECCOMP_NEW_LISTENER, pid, 0);

The manpage documents ptrace() as taking four arguments, not three. I
know that the header defines it with varargs, but it would probably be
more useful to require passing in zero as the fourth argument so that
we have a place to stick flags if necessary in the future.

Yep, I'll fix this, thanks. But also this documentation should really
live in the seccomp patch; some rebase got screwed up somewhere.

quoted

+which grabs the 0th filter for some task which the tracer has privilege over.
+Note that filter fds correspond to a particular filter, and not a particular
+task. So if this task then forks, notifications from both tasks will appear on
+the same filter fd. Reads and writes to/from a filter fd are also synchronized,
+so a filter fd can safely have many readers.

Add a note about needing CAP_SYS_ADMIN here? Also, might be useful to
clarify in which direction "nth filter" counts.

Will do.

quoted

+The interface for a seccomp notification fd consists of two structures:
+
+.. code-block::
+
+    struct seccomp_notif {
+        __u16 len;
+        __u64 id;
+        pid_t pid;
+        __u8 signalled;
+        struct seccomp_data data;
+    };
+
+    struct seccomp_notif_resp {
+        __u16 len;
+        __u64 id;
+        __s32 error;
+        __s64 val;
+    };
+
+Users can read via ``ioctl(SECCOMP_NOTIF_RECV)``  (or ``poll()``) on a seccomp
+notification fd to receive a ``struct seccomp_notif``, which contains five
+members: the input length of the structure, a unique-per-filter ``id``, the
+``pid`` of the task which triggered this request (which may be 0 if the task is
+in a pid ns not visible from the listener's pid namespace), a flag representing
+whether or not the notification is a result of a non-fatal signal, and the
+``data`` passed to seccomp. Userspace can then make a decision based on this
+information about what to do, and ``ioctl(SECCOMP_NOTIF_SEND)`` a response,
+indicating what should be returned to userspace. The ``id`` member of ``struct
+seccomp_notif_resp`` should be the same ``id`` as in ``struct seccomp_notif``.
+
+It is worth noting that ``struct seccomp_data`` contains the values of register
+arguments to the syscall, but does not contain pointers to memory. The task's
+memory is accessible to suitably privileged traces via ``ptrace()`` or
+``/proc/pid/map_files/``.

You probably don't actually want to use /proc/pid/map_files here; you
can't use that to access anonymous memory, and it needs CAP_SYS_ADMIN.
And while reading memory via ptrace() is possible, the interface is
really ugly (e.g. you can only read data in 4-byte chunks), and your
caveat about locking out other ptracers (or getting locked out by
them) applies. I'm not even sure if you could read memory via ptrace
while a process is stopped in the seccomp logic? PTRACE_PEEKDATA
requires the target to be in a __TASK_TRACED state.
The two interfaces you might want to use instead are /proc/$pid/mem
and process_vm_{readv,writev}, which allow you to do nice,
arbitrarily-sized, vectored IO on the memory of another process.

Yes, in fact the sample code does use /proc/$pid/mem, but the docs
should be correct :)

quoted

+       /*
+        * Here it's possible we got a signal and then had to wait on the mutex
+        * while the reply was sent, so let's be sure there wasn't a response
+        * in the meantime.
+        */
+       if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
+               /*
+                * We got a signal. Let's tell userspace about it (potentially
+                * again, if we had already notified them about the first one).
+                */
+               n.signaled = true;
+               if (n.state == SECCOMP_NOTIFY_SENT) {
+                       n.state = SECCOMP_NOTIFY_INIT;
+                       up(&match->notif->request);
+               }

Do you need another wake_up_poll() here?

Yes! Good point.

quoted

+               mutex_unlock(&match->notify_lock);
+               err = wait_for_completion_killable(&n.ready);
+               mutex_lock(&match->notify_lock);
+               if (err < 0)
+                       goto remove_list;

Add a comment here explaining that we intentionally leave the
semaphore count too high (because otherwise we'd have to block), and
seccomp_notify_recv() compensates for that?

Will do.

quoted

+       }
+
+       ret = n.val;
+       err = n.error;
+
+remove_list:
+       list_del(&n.list);
+out:
+       mutex_unlock(&match->notify_lock);
+       syscall_set_return_value(current, task_pt_regs(current),
+                                err, ret);
+}
+
 static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
                            const bool recheck_after_trace)
 {

[...]

quoted

 #ifdef CONFIG_SECCOMP_FILTER
+static struct file *init_listener(struct task_struct *,
+                                 struct seccomp_filter *);
+
 /**
  * seccomp_set_mode_filter: internal function for setting seccomp filter
  * @flags:  flags to change filter behavior

@@ -853,6 +1000,8 @@ static long seccomp_set_mode_filter(unsigned int flags,
        const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
        struct seccomp_filter *prepared = NULL;
        long ret = -EINVAL;
+       int listener = 0;
+       struct file *listener_f = NULL;

        /* Validate flags. */
        if (flags & ~SECCOMP_FILTER_FLAG_MASK)

@@ -863,13 +1012,28 @@ static long seccomp_set_mode_filter(unsigned int flags,
        if (IS_ERR(prepared))
                return PTR_ERR(prepared);

+       if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
+               listener = get_unused_fd_flags(0);
+               if (listener < 0) {
+                       ret = listener;
+                       goto out_free;
+               }
+
+               listener_f = init_listener(current, prepared);
+               if (IS_ERR(listener_f)) {
+                       put_unused_fd(listener);
+                       ret = PTR_ERR(listener_f);
+                       goto out_free;
+               }
+       }
+
        /*
         * Make sure we cannot change seccomp or nnp state via TSYNC
         * while another thread is in the middle of calling exec.
         */
        if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
            mutex_lock_killable(&current->signal->cred_guard_mutex))
-               goto out_free;
+               goto out_put_fd;

        spin_lock_irq(&current->sighand->siglock);

@@ -887,6 +1051,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
        spin_unlock_irq(&current->sighand->siglock);
        if (flags & SECCOMP_FILTER_FLAG_TSYNC)
                mutex_unlock(&current->signal->cred_guard_mutex);
+out_put_fd:
+       if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
+               if (ret < 0) {
+                       fput(listener_f);
+                       put_unused_fd(listener);
+               } else {
+                       fd_install(listener, listener_f);
+                       ret = listener;
+               }
+       }
 out_free:
        seccomp_filter_free(prepared);
        return ret;

[...]

quoted

+
+#ifdef CONFIG_SECCOMP_FILTER
+static int seccomp_notify_release(struct inode *inode, struct file *file)
+{
+       struct seccomp_filter *filter = file->private_data;
+       struct seccomp_knotif *knotif;
+
+       mutex_lock(&filter->notify_lock);
+
+       /*
+        * If this file is being closed because e.g. the task who owned it
+        * died, let's wake everyone up who was waiting on us.
+        */
+       list_for_each_entry(knotif, &filter->notif->notifications, list) {
+               if (knotif->state == SECCOMP_NOTIFY_REPLIED)
+                       continue;
+
+               knotif->state = SECCOMP_NOTIFY_REPLIED;
+               knotif->error = -ENOSYS;
+               knotif->val = 0;
+
+               complete(&knotif->ready);
+       }
+
+       wake_up_all(&filter->notif->wqh);

If select() is polling us, a reference to the open file is being held,
and this can't be reached; and I think if epoll is polling us,
eventpoll_release() will remove itself from the wait queue, right? So
can this wake_up_all() actually ever notify anyone?

I don't know actually, I just thought better safe than sorry. I can
drop it, though.

quoted

+       kfree(filter->notif);
+       filter->notif = NULL;
+       mutex_unlock(&filter->notify_lock);
+       __put_seccomp_filter(filter);
+       return 0;
+}
+
+static long seccomp_notify_recv(struct seccomp_filter *filter,
+                               unsigned long arg)
+{
+       struct seccomp_knotif *knotif = NULL, *cur;
+       struct seccomp_notif unotif = {};
+       ssize_t ret;
+       u16 size;
+       void __user *buf = (void __user *)arg;
+
+       if (copy_from_user(&size, buf, sizeof(size)))
+               return -EFAULT;
+
+       ret = down_interruptible(&filter->notif->request);
+       if (ret < 0)
+               return ret;
+
+       mutex_lock(&filter->notify_lock);
+       list_for_each_entry(cur, &filter->notif->notifications, list) {
+               if (cur->state == SECCOMP_NOTIFY_INIT) {
+                       knotif = cur;
+                       break;
+               }
+       }
+
+       /*
+        * If we didn't find a notification, it could be that the task was
+        * interrupted between the time we were woken and when we were able to

s/interrupted/interrupted by a fatal signal/ ?

quoted

+        * acquire the rw lock.

State more explicitly here that we are compensating for an incorrectly
high semaphore count?

Will do, thanks.

quoted

+        */
+       if (!knotif) {
+               ret = -ENOENT;
+               goto out;
+       }
+
+       size = min_t(size_t, size, sizeof(unotif));
+
+       unotif.len = size;
+       unotif.id = knotif->id;
+       unotif.pid = task_pid_vnr(knotif->task);
+       unotif.signaled = knotif->signaled;
+       unotif.data = *(knotif->data);
+
+       if (copy_to_user(buf, &unotif, size)) {
+               ret = -EFAULT;
+               goto out;
+       }
+
+       ret = size;
+       knotif->state = SECCOMP_NOTIFY_SENT;
+       wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
+
+
+out:
+       mutex_unlock(&filter->notify_lock);
+       return ret;
+}
+
+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?

No, I just flubbed the list API.

quoted

+               ret = -ENOENT;
+               goto out;
+       }
+
+       /* Allow exactly one reply. */
+       if (knotif->state != SECCOMP_NOTIFY_SENT) {
+               ret = -EINPROGRESS;
+               goto out;
+       }

This means that if seccomp_do_user_notification() has in the meantime
received a signal and transitioned from SENT back to INIT, this will
fail, right? So we fail here, then we read the new notification, and
then we can retry SECCOMP_NOTIF_SEND? Is that intended?

I think so, the idea being that you might want to do something
different if a signal was sent. But Andy seemed to think that we might
not actually do anything different.

Either way, for the case you describe, EINPROGRESS is a little weird.
Perhaps it should be:

if (knotif->state == SECCOMP_NOTIFY_INIT) {
        ret = -EBUSY; /* or something? */
        goto out;
} else if (knotif->state == SECCOMP_NOTIFY_REPLIED) {
        ret = -EINPROGRESS;
        goto out;
}

?

quoted

+       ret = size;
+       knotif->state = SECCOMP_NOTIFY_REPLIED;
+       knotif->error = resp.error;
+       knotif->val = resp.val;
+       complete(&knotif->ready);
+out:
+       mutex_unlock(&filter->notify_lock);
+       return ret;
+}
+
+static long seccomp_notify_id_valid(struct seccomp_filter *filter,
+                                   unsigned long arg)
+{
+       struct seccomp_knotif *knotif = NULL;
+       void __user *buf = (void __user *)arg;
+       u64 id;
+       long ret;
+
+       if (copy_from_user(&id, buf, sizeof(id)))
+               return -EFAULT;
+
+       ret = mutex_lock_interruptible(&filter->notify_lock);
+       if (ret < 0)
+               return ret;
+
+       ret = -1;

In strace, this is going to show up as EPERM. Maybe use something like
-ENOENT instead? Or whatever you think resembles a fitting error
number.

Yep, will do.

quoted

+       list_for_each_entry(knotif, &filter->notif->notifications, list) {
+               if (knotif->id == id) {
+                       ret = 0;

Would it make sense to treat notifications that have already been
replied to as invalid?

I suppose so, since we aren't going to let you reply to them anyway.

quoted

+                       goto out;
+               }
+       }
+
+out:
+       mutex_unlock(&filter->notify_lock);
+       return ret;
+}
+

[...]

quoted

+static __poll_t seccomp_notify_poll(struct file *file,
+                                   struct poll_table_struct *poll_tab)
+{
+       struct seccomp_filter *filter = file->private_data;
+       __poll_t ret = 0;
+       struct seccomp_knotif *cur;
+
+       poll_wait(file, &filter->notif->wqh, poll_tab);
+
+       ret = mutex_lock_interruptible(&filter->notify_lock);
+       if (ret < 0)
+               return ret;

Looking at the callers of vfs_poll(), as far as I can tell, a poll
handler is not allowed to return error codes. Perhaps someone who
knows the poll interface better can weigh in here. I've CCed some
people who should hopefully know better how this stuff works.

Thanks.

quoted

+       list_for_each_entry(cur, &filter->notif->notifications, list) {
+               if (cur->state == SECCOMP_NOTIFY_INIT)
+                       ret |= EPOLLIN | EPOLLRDNORM;
+               if (cur->state == SECCOMP_NOTIFY_SENT)
+                       ret |= EPOLLOUT | EPOLLWRNORM;
+               if (ret & EPOLLIN && ret & EPOLLOUT)
+                       break;
+       }
+
+       mutex_unlock(&filter->notify_lock);
+
+       return ret;
+}
+
+static const struct file_operations seccomp_notify_ops = {
+       .poll = seccomp_notify_poll,
+       .release = seccomp_notify_release,
+       .unlocked_ioctl = seccomp_notify_ioctl,
+};
+
+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.

I think Kees replied already, but yes, this is a good point :(. We can
continue in his thread.

quoted

+       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?

quoted

+       if (!filter->notif)
+               goto out;
+
+       sema_init(&filter->notif->request, 0);
+       INIT_LIST_HEAD(&filter->notif->notifications);
+       filter->notif->next_id = get_random_u64();
+       init_waitqueue_head(&filter->notif->wqh);

Nit: next_id and notifications are declared in reverse order in the
struct. Could you flip them around here?

Sure, will do.

quoted

+       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.

Will do, thanks.

quoted

+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.

I suppose so, given that we have last_locked.

Tycho