Message-ID: <20200604052040.GA16501@ircssh-2.c.rugged-nimbus-611.internal>
Date:   Thu, 4 Jun 2020 05:20:41 +0000
From:   Sargun Dhillon <sargun@...gun.me>
To:     Kees Cook <keescook@...omium.org>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        linux-kernel@...r.kernel.org, Tycho Andersen <tycho@...ho.ws>,
        Matt Denton <mpdenton@...gle.com>,
        Jann Horn <jannh@...gle.com>, Chris Palmer <palmer@...gle.com>,
        Aleksa Sarai <cyphar@...har.com>,
        Robert Sesek <rsesek@...gle.com>,
        containers@...ts.linux-foundation.org,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Tejun Heo <tj@...nel.org>, stable@...r.kernel.org,
        cgroups@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to
 move fds across processes

On Wed, Jun 03, 2020 at 07:22:57PM -0700, Kees Cook wrote:
> On Thu, Jun 04, 2020 at 03:24:52AM +0200, Christian Brauner wrote:
> > On Tue, Jun 02, 2020 at 06:10:41PM -0700, Sargun Dhillon wrote:
> > > Previously there were two chunks of code where the logic to receive file
> > > descriptors was duplicated in net. The compat version of copying
> > > file descriptors via SCM_RIGHTS did not have logic to update cgroups.
> > > Logic to change the cgroup data was added in:
> > > commit 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
> > > commit d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
> > > 
> > > This was not copied to the compat path. This commit fixes that, and thus
> > > should be cherry-picked into stable.
> > > 
> > > This introduces a helper (file_receive) which encapsulates the logic for
> > > handling calling security hooks as well as manipulating cgroup information.
> > > This helper can then be used other places in the kernel where file
> > > descriptors are copied between processes
> > > 
> > > I tested cgroup classid setting on both the compat (x32) path, and the
> > > native path to ensure that when moving the file descriptor the classid
> > > is set.
> > > 
> > > Signed-off-by: Sargun Dhillon <sargun@...gun.me>
> > > Suggested-by: Kees Cook <keescook@...omium.org>
> > > Cc: Al Viro <viro@...iv.linux.org.uk>
> > > Cc: Christian Brauner <christian.brauner@...ntu.com>
> > > Cc: Daniel Wagner <daniel.wagner@...-carit.de>
> > > Cc: David S. Miller <davem@...emloft.net>
> > > Cc: Jann Horn <jannh@...gle.com>,
> > > Cc: John Fastabend <john.r.fastabend@...el.com>
> > > Cc: Tejun Heo <tj@...nel.org>
> > > Cc: Tycho Andersen <tycho@...ho.ws>
> > > Cc: stable@...r.kernel.org
> > > Cc: cgroups@...r.kernel.org
> > > Cc: linux-fsdevel@...r.kernel.org
> > > Cc: linux-kernel@...r.kernel.org
> > > ---
> > >  fs/file.c            | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/file.h |  1 +
> > >  net/compat.c         | 10 +++++-----
> > >  net/core/scm.c       | 14 ++++----------
> > >  4 files changed, 45 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/fs/file.c b/fs/file.c
> > > index abb8b7081d7a..5afd76fca8c2 100644
> > > --- a/fs/file.c
> > > +++ b/fs/file.c
> > > @@ -18,6 +18,9 @@
> > >  #include <linux/bitops.h>
> > >  #include <linux/spinlock.h>
> > >  #include <linux/rcupdate.h>
> > > +#include <net/sock.h>
> > > +#include <net/netprio_cgroup.h>
> > > +#include <net/cls_cgroup.h>
> > >  
> > >  unsigned int sysctl_nr_open __read_mostly = 1024*1024;
> > >  unsigned int sysctl_nr_open_min = BITS_PER_LONG;
> > > @@ -931,6 +934,38 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
> > >  	return err;
> > >  }
> > >  
> > > +/*
> > > + * File Receive - Receive a file from another process
> > > + *
> > > + * This function is designed to receive files from other tasks. It encapsulates
> > > + * logic around security and cgroups. The file descriptor provided must be a
> > > + * freshly allocated (unused) file descriptor.
> > > + *
> > > + * This helper does not consume a reference to the file, so the caller must put
> > > + * their reference.
> > > + *
> > > + * Returns 0 upon success.
> > > + */
> > > +int file_receive(int fd, struct file *file)
> > 
> > This is all just a remote version of fd_install(), yet it deviates from
> > fd_install()'s semantics and naming. That's not great imho. What about
> > naming this something like:
> > 
> > fd_install_received()
> > 
> > and move the get_file() out of there so it has the same semantics as
> > fd_install(). It seems rather dangerous to have a function like
> > fd_install() that consumes a reference once it returned and another
> > version of this that is basically the same thing but doesn't consume a
> > reference because it takes its own. Seems an invitation for confusion.
> > Does that make sense?
> 
> We have some competing opinions on this, I guess. What I really don't
> like is the copy/pasting of the get_unused_fd_flags() and
> put_unused_fd() needed by (nearly) all the callers. If it's a helper, it
> should help. Specifically, I'd like to see this:
> 
> int file_receive(int fd, unsigned long flags, struct file *file,
> 		 int __user *fdptr)
> {
> 	struct socket *sock;
> 	int err;
> 
> 	err = security_file_receive(file);
> 	if (err)
> 		return err;
> 
> 	if (fd < 0) {
> 		/* Install new fd. */
> 		int new_fd;
> 
> 		err = get_unused_fd_flags(flags);
> 		if (err < 0)
> 			return err;
> 		new_fd = err;
> 
> 		/* Copy fd to any waiting user memory. */
> 		if (fdptr) {
> 			err = put_user(new_fd, fdptr);
> 			if (err < 0) {
> 				put_unused_fd(new_fd);
> 				return err;
> 			}
> 		}
> 		fd_install(new_fd, get_file(file));
> 		fd = new_fd;
> 	} else {
> 		/* Replace existing fd. */
> 		err = replace_fd(fd, file, flags);
> 		if (err)
> 			return err;
> 	}
> 
> 	/* Bump the cgroup usage counts. */
> 	sock = sock_from_file(fd, &err);
> 	if (sock) {
> 		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> 		sock_update_classid(&sock->sk->sk_cgrp_data);
> 	}
> 
> 	return fd;
> }
> 
> If everyone else *really* prefers keeping the get_unused_fd_flags() /
> put_unused_fd() stuff outside the helper, then I guess I'll give up,
> but I think it is MUCH cleaner this way -- all 4 users trim down lots
> of code duplication.
> 
> -- 
> Kees Cook
This seems weird that the function has two different return mechanisms
depending on the value of fdptr, especially given that behaviour is
only invoked by SCM, whereas the other callers (addfd, and pidfd_getfd)
just want the FD value returned.

Won't this produce a "bad" result, if the user does:

struct msghdr msg = {};
struct cmsghdr *cmsg;
struct iovec io = {
	.iov_base = &c,
	.iov_len = 1,
};

msg.msg_iov = &io;
msg.msg_iovlen = 1;
msg.msg_control = NULL;
msg.msg_controllen = sizeof(buf);

recvmsg(sock, &msg, 0);
----

This will end up installing the FD, but it will efault, when
scm_detach_fds tries to fill out the rest of the info. 

I mean, we can easily solve this with a null pointer check
in scm_detach_fds, but my fear is that user n will forget
to do this, and make a mistake.

Maybe it would be nice to have:

/* Receives file descriptor and installs it in userspace at uptr. */
static inline intfile_receive_user(struct file *file, unsigned long flags,
				   int __user *fdptr)
{
	if (fdptr == NULL)
		return -EFAULT;

	return __file_receive(-1, flags, file, uptr);
}

And then just let pidfd_getfd, and seccomp_addfd call __file_receive
directly, or offer a different helper like:

static inline file_receive(long fd, struct *file, unsigned long flags)
{
	return __file_receive(fd, flags, file, NULL);
}

Powered by blists - more mailing lists