From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DCDB7C433DF for ; Thu, 4 Jun 2020 01:25:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B76F5207D5 for ; Thu, 4 Jun 2020 01:25:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726970AbgFDBY7 (ORCPT ); Wed, 3 Jun 2020 21:24:59 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:42656 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725946AbgFDBY7 (ORCPT ); Wed, 3 Jun 2020 21:24:59 -0400 Received: from ip5f5af183.dynamic.kabel-deutschland.de ([95.90.241.131] helo=wittgenstein) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jgecn-0006qD-L0; Thu, 04 Jun 2020 01:24:53 +0000 Date: Thu, 4 Jun 2020 03:24:52 +0200 From: Christian Brauner To: Sargun Dhillon Cc: Kees Cook , 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 Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Message-ID: <20200604012452.vh33nufblowuxfed@wittgenstein> References: <20200603011044.7972-1-sargun@sargun.me> <20200603011044.7972-2-sargun@sargun.me> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200603011044.7972-2-sargun@sargun.me> Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org 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? > +{ > + struct socket *sock; > + int err; > + > + err = security_file_receive(file); > + if (err) > + return err; > + > + fd_install(fd, get_file(file)); > + > + sock = sock_from_file(file, &err); > + if (sock) { > + sock_update_netprioidx(&sock->sk->sk_cgrp_data); > + sock_update_classid(&sock->sk->sk_cgrp_data); > + } > + > + return 0; > +} > + > static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags) > { > int err = -EBADF; > diff --git a/include/linux/file.h b/include/linux/file.h > index 142d102f285e..7b56dc23e560 100644 > --- a/include/linux/file.h > +++ b/include/linux/file.h > @@ -94,4 +94,5 @@ extern void fd_install(unsigned int fd, struct file *file); > extern void flush_delayed_fput(void); > extern void __fput_sync(struct file *); > > +extern int file_receive(int fd, struct file *file); > #endif /* __LINUX_FILE_H */ > diff --git a/net/compat.c b/net/compat.c > index 4bed96e84d9a..8ac0e7e09208 100644 > --- a/net/compat.c > +++ b/net/compat.c > @@ -293,9 +293,6 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm) > > for (i = 0, cmfptr = (int __user *) CMSG_COMPAT_DATA(cm); i < fdmax; i++, cmfptr++) { > int new_fd; > - err = security_file_receive(fp[i]); > - if (err) > - break; > err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & kmsg->msg_flags > ? O_CLOEXEC : 0); > if (err < 0) > @@ -306,8 +303,11 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm) > put_unused_fd(new_fd); > break; > } > - /* Bump the usage count and install the file. */ > - fd_install(new_fd, get_file(fp[i])); > + err = file_receive(new_fd, fp[i]); > + if (err) { > + put_unused_fd(new_fd); > + break; > + } > } > > if (i > 0) { > diff --git a/net/core/scm.c b/net/core/scm.c > index dc6fed1f221c..ba93abf2881b 100644 > --- a/net/core/scm.c > +++ b/net/core/scm.c > @@ -303,11 +303,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm) > for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i i++, cmfptr++) > { > - struct socket *sock; > int new_fd; > - err = security_file_receive(fp[i]); > - if (err) > - break; > err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags > ? O_CLOEXEC : 0); > if (err < 0) > @@ -318,13 +314,11 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm) > put_unused_fd(new_fd); > break; > } > - /* Bump the usage count and install the file. */ > - sock = sock_from_file(fp[i], &err); > - if (sock) { > - sock_update_netprioidx(&sock->sk->sk_cgrp_data); > - sock_update_classid(&sock->sk->sk_cgrp_data); > + err = file_receive(new_fd, fp[i]); > + if (err) { > + put_unused_fd(new_fd); > + break; > } > - fd_install(new_fd, get_file(fp[i])); > } > > if (i > 0) > -- > 2.25.1 >