Re: [PATCH v7 5/6] seccomp: add a way to pass FDs via a notification fd

On Thu, Sep 27, 2018 at 03:09:06PM -0700, Kees Cook wrote:

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

quoted

This patch adds a way to insert FDs into the tracee's process (also
close/overwrite fds for the tracee). This functionality is necessary to
mock things like socketpair() or dup2() or similar, but since it depends on
external (vfs) patches, I've left it as a separate patch as before so the
core functionality can still be merged while we argue about this. Except
this time it doesn't add any ugliness to the API :)

v7: new in v7

Signed-off-by: Tycho Andersen <redacted>
CC: Kees Cook <redacted>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Eric W. Biederman <redacted>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Christian Brauner <redacted>
CC: Tyler Hicks <redacted>
CC: Akihiro Suda <redacted>
---
 .../userspace-api/seccomp_filter.rst          |  16 +++
 include/uapi/linux/seccomp.h                  |   9 ++
 kernel/seccomp.c                              |  54 ++++++++
 tools/testing/selftests/seccomp/seccomp_bpf.c | 126 ++++++++++++++++++
 4 files changed, 205 insertions(+)

diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index d2e61f1c0a0b..383a8dbae304 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst

@@ -237,6 +237,13 @@ The interface for a seccomp notification fd consists of two structures:
         __s64 val;
     };

+    struct seccomp_notif_put_fd {
+        __u64 id;
+        __s32 fd;
+        __u32 fd_flags;
+        __s32 to_replace;
+    };
+
 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

@@ -256,6 +263,15 @@ 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.

+Userspace can also insert (or overwrite) file descriptors of the tracee using
+``ioctl(SECCOMP_NOTIF_PUT_FD)``. The ``id`` member is the request/pid to insert
+the fd into. The ``fd`` is the fd in the listener's table to send or ``-1`` if
+an fd should be closed instead. The ``to_replace`` fd is the fd in the tracee's
+table that should be overwritten, or -1 if a new fd is installed. ``fd_flags``
+should be the flags that the fd in the tracee's table is opened with (e.g.
+``O_CLOEXEC`` or similar). The return value from this ioctl is the fd number
+that was installed.
+
 Sysctls
 =======

diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index d4ccb32fe089..91d77f041fbb 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h

@@ -77,6 +77,13 @@ struct seccomp_notif_resp {
        __s64 val;
 };

+struct seccomp_notif_put_fd {
+       __u64 id;
+       __s32 fd;
+       __u32 fd_flags;
+       __s32 to_replace;
+};
+
 #define SECCOMP_IOC_MAGIC              0xF7

 /* Flags for seccomp notification fd ioctl. */

@@ -86,5 +93,7 @@ struct seccomp_notif_resp {
                                        struct seccomp_notif_resp)
 #define SECCOMP_NOTIF_ID_VALID _IOR(SECCOMP_IOC_MAGIC, 2,      \
                                        __u64)
+#define SECCOMP_NOTIF_PUT_FD   _IOR(SECCOMP_IOC_MAGIC, 3,      \
+                                       struct seccomp_notif_put_fd)

 #endif /* _UAPI_LINUX_SECCOMP_H */

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 17685803a2af..07a05ad59731 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c

@@ -41,6 +41,8 @@
 #include <linux/tracehook.h>
 #include <linux/uaccess.h>
 #include <linux/anon_inodes.h>
+#include <linux/fdtable.h>
+#include <net/cls_cgroup.h>

 enum notify_state {
        SECCOMP_NOTIFY_INIT,

@@ -1684,6 +1686,56 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
        return ret;
 }

+static long seccomp_notify_put_fd(struct seccomp_filter *filter,
+                                 unsigned long arg)
+{
+       struct seccomp_notif_put_fd req;
+       void __user *buf = (void __user *)arg;
+       struct seccomp_knotif *knotif = NULL;
+       long ret;
+
+       if (copy_from_user(&req, buf, sizeof(req)))
+               return -EFAULT;
+
+       if (req.fd < 0 && req.to_replace < 0)
+               return -EINVAL;
+
+       ret = mutex_lock_interruptible(&filter->notify_lock);
+       if (ret < 0)
+               return ret;
+
+       ret = -ENOENT;
+       list_for_each_entry(knotif, &filter->notif->notifications, list) {
+               struct file *file = NULL;
+
+               if (knotif->id != req.id)
+                       continue;
+
+               if (req.fd >= 0)
+                       file = fget(req.fd);

Shouldn't we test for !file here?

Yes. Derp.

Tycho