[PATCH 2/5] seccomp: Introduce addfd ioctl to seccomp user notifier
Christian Brauner
christian.brauner at ubuntu.com
Mon May 25 13:50:36 UTC 2020
On Sun, May 24, 2020 at 04:39:39PM -0700, Sargun Dhillon wrote:
> This adds a seccomp notifier ioctl which allows for the listener to "add"
> file descriptors to a process which originated a seccomp user
> notification. This allows calls like mount, and mknod to be "implemented",
> as the return value, and the arguments are data in memory. On the other
> hand, calls like connect can be "implemented" using pidfd_getfd.
>
> Unfortunately, there are calls which return file descriptors, like
> open, which are vulnerable to TOC-TOU attacks, and require that the
> more privileged supervisor can inspect the argument, and perform the
> syscall on behalf of the process generating the notifiation. This
> allows the file descriptor generated from that open call to be
> returned to the calling process.
>
> In addition, there is funcitonality to allow for replacement of
> specific file descriptors, following dup2-like semantics.
>
> Signed-off-by: Sargun Dhillon <sargun at sargun.me>
> Suggested-by: Matt Denton <mpdenton at google.com>
> Cc: Kees Cook <keescook at google.com>,
> Cc: Jann Horn <jannh at google.com>,
> Cc: Robert Sesek <rsesek at google.com>,
> Cc: Chris Palmer <palmer at google.com>
> Cc: Christian Brauner <christian.brauner at ubuntu.com>
> Cc: Tycho Andersen <tycho at tycho.ws>
> ---
> include/uapi/linux/seccomp.h | 25 ++++++
> kernel/seccomp.c | 169 ++++++++++++++++++++++++++++++++++-
> 2 files changed, 193 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index c1735455bc53..7d450a9e4c29 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -113,6 +113,27 @@ struct seccomp_notif_resp {
> __u32 flags;
> };
>
> +/* valid flags for seccomp_notif_addfd */
> +#define SECCOMP_ADDFD_FLAG_SETFD (1UL << 0) /* Specify remote fd */
> +
> +/**
> + * struct seccomp_notif_addfd
> + * @size: The size of the seccomp_notif_addfd datastructure
> + * @fd: The local fd number
> + * @id: The ID of the seccomp notification
> + * @fd_flags: Flags the remote FD should be allocated under
> + * @remote_fd: Optional remote FD number if SETFD option is set, otherwise 0.
> + * @flags: SECCOMP_ADDFD_FLAG_*
> + */
> +struct seccomp_notif_addfd {
> + __u32 size;
> + __u32 fd;
> + __u64 id;
> + __u32 fd_flags;
> + __u32 remote_fd;
> + __u64 flags;
> +};
This was a little confusing to me at first. So fd is the fd from which
we take the struct file and remote_fd is either -1 at which point we
just allocate the next free fd number and if it is not we
allocate/replace a specific one. Maybe it would be clearer if we did:
struct seccomp_notif_addfd {
__u32 size;
__u64 id;
__u64 flags;
__u32 srcfd;
__u32 newfd;
__u32 newfd_flags;
};
No need to hide in the name that this is remote_dup2().
> +
> #define SECCOMP_IOC_MAGIC '!'
> #define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
> #define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
> @@ -124,4 +145,8 @@ struct seccomp_notif_resp {
> #define SECCOMP_IOCTL_NOTIF_SEND SECCOMP_IOWR(1, \
> struct seccomp_notif_resp)
> #define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64)
> +/* On success, the return value is the remote process's added fd number */
> +#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \
> + struct seccomp_notif_addfd)
> +
> #endif /* _UAPI_LINUX_SECCOMP_H */
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index f6ce94b7a167..88940eeabaee 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -77,10 +77,42 @@ struct seccomp_knotif {
> long val;
> u32 flags;
>
> - /* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
> + /*
> + * Signals when this has changed states, such as the listener
> + * dying, a new seccomp addfd message, or changing to REPLIED
> + */
> struct completion ready;
>
> struct list_head list;
> +
> + /* outstanding addfd requests */
> + struct list_head addfd;
> +};
> +
> +/**
> + * struct seccomp_kaddfd - contianer for seccomp_addfd ioctl messages
^^^ typo
> + *
> + * @file: A reference to the file to install in the other task
> + * @fd: The fd number to install it at. If the fd number is -1, it means the
> + * installing process should allocate the fd as normal.
> + * @flags: The flags for the new file descriptor. At the moment, only O_CLOEXEC
> + * is allowed.
> + * @ret: The return value of the installing process. It is set to the fd num
> + * upon success (>= 0).
> + * @completion: Indicates that the installing process has completed fd
> + * installation, or gone away (either due to successful
> + * reply, or signal)
> + *
> + */
> +struct seccomp_kaddfd {
> + struct file *file;
> + int fd;
> + unsigned int flags;
> +
> + /* To only be set on reply */
> + int ret;
> + struct completion completion;
> + struct list_head list;
> };
>
> /**
> @@ -735,6 +767,35 @@ static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
> return filter->notif->next_id++;
> }
>
> +static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd)
> +{
> + int ret;
> +
> + /*
> + * Remove the notification, and reset the list pointers, indicating
> + * that it has been handled.
> + */
> + list_del_init(&addfd->list);
> +
> + ret = security_file_receive(addfd->file);
> + if (ret)
> + goto out;
> +
> + if (addfd->fd >= 0) {
> + ret = replace_fd(addfd->fd, addfd->file, addfd->flags);
> + if (ret >= 0)
> + fput(addfd->file);
> + } else {
> + ret = get_unused_fd_flags(addfd->flags);
> + if (ret >= 0)
> + fd_install(ret, addfd->file);
> + }
> +
> +out:
> + addfd->ret = ret;
> + complete(&addfd->completion);
> +}
> +
> static int seccomp_do_user_notification(int this_syscall,
> struct seccomp_filter *match,
> const struct seccomp_data *sd)
> @@ -743,6 +804,7 @@ static int seccomp_do_user_notification(int this_syscall,
> u32 flags = 0;
> long ret = 0;
> struct seccomp_knotif n = {};
> + struct seccomp_kaddfd *addfd, *tmp;
>
> mutex_lock(&match->notify_lock);
> err = -ENOSYS;
> @@ -755,6 +817,7 @@ static int seccomp_do_user_notification(int this_syscall,
> n.id = seccomp_next_notify_id(match);
> init_completion(&n.ready);
> list_add(&n.list, &match->notif->notifications);
> + INIT_LIST_HEAD(&n.addfd);
>
> up(&match->notif->request);
> wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
> @@ -763,14 +826,31 @@ static int seccomp_do_user_notification(int this_syscall,
> /*
> * This is where we wait for a reply from userspace.
> */
> +wait:
> err = wait_for_completion_interruptible(&n.ready);
> mutex_lock(&match->notify_lock);
> if (err == 0) {
> + /* Check if we were woken up by a addfd message */
> + addfd = list_first_entry_or_null(&n.addfd,
> + struct seccomp_kaddfd, list);
> + if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) {
> + seccomp_handle_addfd(addfd);
> + mutex_unlock(&match->notify_lock);
> + goto wait;
> + }
> ret = n.val;
> err = n.error;
> flags = n.flags;
> }
>
> + /* If there were any pending addfd calls, clear them out */
> + list_for_each_entry_safe(addfd, tmp, &n.addfd, list) {
> + /* The process went away before we got a chance to handle it */
> + addfd->ret = -ENOENT;
Looks like it should be -ESRCH?
> + list_del_init(&addfd->list);
> + complete(&addfd->completion);
> + }
> +
> /*
> * Note that it's possible the listener died in between the time when
> * we were notified of a respons (or a signal) and when we were able to
> @@ -1179,6 +1259,91 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> return ret;
> }
>
> +static long seccomp_notify_addfd(struct seccomp_filter *filter,
> + struct seccomp_notif_addfd __user *uaddfd)
> +{
> + struct seccomp_notif_addfd addfd;
> + struct seccomp_knotif *knotif;
> + struct seccomp_kaddfd kaddfd;
> + u32 size;
> + int ret;
> +
> + ret = get_user(size, &uaddfd->size);
> + if (ret)
> + return ret;
> +
> + ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
> + if (ret)
> + return ret;
> +
> + if (addfd.fd_flags & (~O_CLOEXEC))
> + return -EINVAL;
> +
> + if (addfd.flags & ~(SECCOMP_ADDFD_FLAG_SETFD))
> + return -EINVAL;
I'd suggest to remove the brackets and just do
if (addfd.flags & ~SECCOMP_ADDFD_FLAG_SETFD)
that's more common in the kernel.
> +
> + if (addfd.remote_fd && !(addfd.flags & SECCOMP_ADDFD_FLAG_SETFD))
> + return -EINVAL;
> +
> + kaddfd.file = fget(addfd.fd);
> + if (!kaddfd.file)
> + return -EBADF;
> +
> + kaddfd.flags = addfd.fd_flags;
> + kaddfd.fd = (addfd.flags & SECCOMP_ADDFD_FLAG_SETFD) ?
> + addfd.remote_fd : -1;
> + init_completion(&kaddfd.completion);
> +
> + ret = mutex_lock_interruptible(&filter->notify_lock);
> + if (ret)
The patter in all other places in seccomp is to compare against < 0, so
if (ret < 0)
goto out;
I think.
> + goto out;
> +
> + knotif = find_notification(filter, addfd.id);
> + /*
> + * We do not want to allow for FD injection to occur before the
> + * notification has been picked up by a userspace handler, or after
> + * the notification has been replied to.
> + */
> + if (!knotif || knotif->state != SECCOMP_NOTIFY_SENT) {
> + ret = -ENOENT;
> + goto out_unlock;
> + }
if (!knotif) {
ret = -ESRCH;
goto out_unlock;
}
if (knotif->state != SECCOMP_NOTIFY_SENT) {
ret = -EBUSY/-EINVAL;
goto out_unlock;
}
so we don't overload errors?
> +
> + list_add(&kaddfd.list, &knotif->addfd);
> + complete(&knotif->ready);
> + mutex_unlock(&filter->notify_lock);
> +
> + /* Now we wait for it to be processed */
> + ret = wait_for_completion_interruptible(&kaddfd.completion);
> + if (ret == 0) {
> + /*
> + * We had a successful completion. The other side has already
> + * removed us from the addfd queue, and
> + * wait_for_completion_interruptible has a memory barrier.
> + */
> + ret = kaddfd.ret;
> + goto out;
> + }
> +
> + mutex_lock(&filter->notify_lock);
> + /*
> + * Even though we were woken up by a signal, and not a successful
> + * completion, a completion may have happened in the mean time.
> + */
> + if (list_empty(&kaddfd.list))
> + ret = kaddfd.ret;
> + else
> + list_del(&kaddfd.list);
> +
> +out_unlock:
> + mutex_unlock(&filter->notify_lock);
> +out:
> + if (ret < 0)
> + fput(kaddfd.file);
> +
> + return ret;
> +}
> +
> static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -1192,6 +1357,8 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
> return seccomp_notify_send(filter, buf);
> case SECCOMP_IOCTL_NOTIF_ID_VALID:
> return seccomp_notify_id_valid(filter, buf);
> + case SECCOMP_IOCTL_NOTIF_ADDFD:
> + return seccomp_notify_addfd(filter, buf);
> default:
> return -EINVAL;
> }
> --
> 2.25.1
>
More information about the Containers
mailing list