From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Date: Wed, 3 Jun 2020 19:22:57 -0700 Message-ID: <202006031845.F587F85A@keescook> References: <20200603011044.7972-1-sargun@sargun.me> <20200603011044.7972-2-sargun@sargun.me> <20200604012452.vh33nufblowuxfed@wittgenstein> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Yipjb/nJ+PoPYGY3bQlr0vEC5GDtsZwp9TVgxD0rnGk=; b=Qn2BQw97F7bIZrtjDlecVedaFxpvLQXfHqx5ZIkhrdMftq1Uzxuvb3gxbs04al8f6z dUNJTwOCuq0f5AzUIWaq/boxSUbsE7CQq5poZsErcM9jbnlKvwg3ugwzZmw6UY051thR aeypVbCcCPa33vgHBqM+4kc/3K8/7P1pLFmH0= Content-Disposition: inline In-Reply-To: <20200604012452.vh33nufblowuxfed@wittgenstein> Sender: linux-kernel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Christian Brauner Cc: Sargun Dhillon , linux-kernel@vger.kernel.org, Tycho Andersen , Matt Denton , Jann Horn , Chris Palmer , Aleksa Sarai , Robert Sesek , containers@lists.linux-foundation.org, Giuseppe Scrivano , Greg Kroah-Hartman , Al Viro , Daniel Wagner , "David S . Miller" , John Fastabend , Tejun Heo , stable@vger.kernel.org, cgroups@vger.kernel.org, linux-fsdevel@vger.kernel.org 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 > > Suggested-by: Kees Cook > > Cc: Al Viro > > Cc: Christian Brauner > > Cc: Daniel Wagner > > Cc: David S. Miller > > Cc: Jann Horn , > > Cc: John Fastabend > > Cc: Tejun Heo > > Cc: Tycho Andersen > > Cc: stable@vger.kernel.org > > Cc: cgroups@vger.kernel.org > > Cc: linux-fsdevel@vger.kernel.org > > Cc: linux-kernel@vger.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 > > #include > > #include > > +#include > > +#include > > +#include > > > > 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