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

[Tycho Andersen]

On Thu, Sep 27, 2018 at 03:09:06PM -0700, Kees Cook wrote:
> On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@xxxxxxxx> wrote:
> > 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 <tycho@xxxxxxxx>
> > CC: Kees Cook <keescook@xxxxxxxxxxxx>
> > CC: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> > CC: Oleg Nesterov <oleg@xxxxxxxxxx>
> > CC: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> > CC: "Serge E. Hallyn" <serge@xxxxxxxxxx>
> > CC: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > CC: Tyler Hicks <tyhicks@xxxxxxxxxxxxx>
> > CC: Akihiro Suda <suda.akihiro@xxxxxxxxxxxxx>
> > ---
> >  .../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