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=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 81738C433E0 for ; Thu, 4 Jun 2020 05:20:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 582282072E for ; Thu, 4 Jun 2020 05:20:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=sargun.me header.i=@sargun.me header.b="054Uqkhv" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726657AbgFDFUp (ORCPT ); Thu, 4 Jun 2020 01:20:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53076 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726592AbgFDFUo (ORCPT ); Thu, 4 Jun 2020 01:20:44 -0400 Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8D256C03E96D for ; Wed, 3 Jun 2020 22:20:44 -0700 (PDT) Received: by mail-io1-xd44.google.com with SMTP id d7so4966425ioq.5 for ; Wed, 03 Jun 2020 22:20:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sargun.me; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=oKNXmPZh2jB30BHUyHRPpl5a8wx5yENQJM5HVKTmIUg=; b=054UqkhvVty3ow+lMQbK9j4Udt3zohY4MtWydjS8v+qaL1KEzL/lBvin5kq+w6wouX hZI4wLQja9EIX1nva9plQnfwHl8FzoXwywvdWospv8P8d7E04VgKiRV9X8gRx2J9T1CC JEXiaCtFU/kNPlAOilsNcnuQhrVealltviXyQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=oKNXmPZh2jB30BHUyHRPpl5a8wx5yENQJM5HVKTmIUg=; b=pXPHDujGD6wabh3uH+3NP1Pum+k7PdljMrxZXDgx4C6jW4cUBNCwcPehsj+wQLycOP a0nTl6Tt0ovNgUXzTGY2h2ziCWYGjLkHfZWL5jFDIyBCjEs344WOMa0UtRDJoHS0cKZr 4T67H3Tmr96GAEIln0txt7eMux0gkmMiG7WpKHQIDuBhMVRznKywiYgt6T7HPy/zPylB SdmNeSZobYbxwdbHunYNqfMeQdrgvJagTwlHAjI2jDTJlds6JLTE++4IEBL6kejCIKTa 91Uax1MbqLPuCz3iJCywO3XaWiyOX6gNqTHsGnCxP60bi1aWvv3tGXhA8NKObTxzCB0K J1tA== X-Gm-Message-State: AOAM531H9hDnll34buFUEVueH+qB7JoeficjWj+34Uts7PJXdEmw58AE g1eaAv+RBDxFNubICN0SV6PQ5A== X-Google-Smtp-Source: ABdhPJz1JJFpDvHfIzAqgbnGUeuraT6RkK3cM76pgob/tqzC+lFOJzt//SUhOOU6QHvDtvw+nG2caw== X-Received: by 2002:a5d:9e51:: with SMTP id i17mr2607501ioi.8.1591248043464; Wed, 03 Jun 2020 22:20:43 -0700 (PDT) Received: from ircssh-2.c.rugged-nimbus-611.internal (80.60.198.104.bc.googleusercontent.com. [104.198.60.80]) by smtp.gmail.com with ESMTPSA id r17sm900698ilc.33.2020.06.03.22.20.42 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Wed, 03 Jun 2020 22:20:42 -0700 (PDT) Date: Thu, 4 Jun 2020 05:20:41 +0000 From: Sargun Dhillon To: Kees Cook Cc: Christian Brauner , 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 , "David S . Miller" , 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: <20200604052040.GA16501@ircssh-2.c.rugged-nimbus-611.internal> References: <20200603011044.7972-1-sargun@sargun.me> <20200603011044.7972-2-sargun@sargun.me> <20200604012452.vh33nufblowuxfed@wittgenstein> <202006031845.F587F85A@keescook> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <202006031845.F587F85A@keescook> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org 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 > > > 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 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); }