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
