Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp1279816ybg; Thu, 4 Jun 2020 05:57:38 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz7YFANwe83fLa/klV8qxYnTW8TlhBfZlnD1eDSN9SHn1wHBwhYpTdYIBV6Z3M4rzh8wP4o X-Received: by 2002:a17:906:e05:: with SMTP id l5mr4044374eji.318.1591275458676; Thu, 04 Jun 2020 05:57:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591275458; cv=none; d=google.com; s=arc-20160816; b=rMpub6HxawjY9wObX7Vf5U1VdAqpt9RIZG04DRlXf8hg7Hiz+cmQK1kym5E/Z0i44z TIambcdNefdxBHB1kTOf7m5sbQ3X8QB3DqMT2sEQEpKiUtTaGve4SqdWC9MLcFiKoDsD rqx6iml/I+dQwXcReir5SpyV8cEYoUeKXuCXVkuykY44uagIMPpB29sQalZPhRDWQkYA yJhduvrZuDt+vDPDgMsut45LWqh/Txpq/jZyhTva+wHzHp7SNg3mFnx9oF4/WtzuMre7 qjENA3oxckE1joFN7qYyr25hQZ5Do+Xd28lIbFLJsJTNwuPeMTUsslNLuvbhlXIV4UbD pWqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=V/OjkUTbGPOMndlEhqbZiZ2SIdZp0z45CGuHb2VxHOA=; b=VMgotVgq8izEi4VzSE0Yj188QxiLjCdttdS/KZoYyYU1EZPT+bSK6YDN0zf+oXSyMD VshxfrfPpTf7BghrFLzNV+dTmmLGZSyFYx2UeKGu6ZRFRbO9GvrIJiWsYLyPNwRBeNud 0SVw+tDRf6fHXR/cZV/RqVr2OTVNSEXDcdPRA+vslw7aGxdFajomMSVDSGEL2LMyHQhH NLLp3krTMlC4xi21QL9899FHOHhHfXyLNUIa2rp+bZ4SI+mIkLbmOcBZ/8/TQI0AI41U xjYSEWjsYtdmGK885PDuTy7PyaC/BagnBOIWWeab+lH/IHt753O3KmXXtzXMpPaBpHV4 8AFA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id a5si1672175ejv.78.2020.06.04.05.57.14; Thu, 04 Jun 2020 05:57:38 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728033AbgFDMwg (ORCPT + 99 others); Thu, 4 Jun 2020 08:52:36 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:35727 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725926AbgFDMwg (ORCPT ); Thu, 4 Jun 2020 08:52:36 -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 1jgpMC-0007l3-4H; Thu, 04 Jun 2020 12:52:28 +0000 Date: Thu, 4 Jun 2020 14:52:26 +0200 From: Christian Brauner To: Kees Cook 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 Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Message-ID: <20200604125226.eztfrpvvuji7cbb2@wittgenstein> 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=utf-8 Content-Disposition: inline In-Reply-To: <202006031845.F587F85A@keescook> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@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) I still fail to see what this whole put_user() handling buys us at all and why this function needs to be anymore complicated then simply: fd_install_received(int fd, struct file *file) { security_file_receive(file); sock = sock_from_file(fd, &err); if (sock) { sock_update_netprioidx(&sock->sk->sk_cgrp_data); sock_update_classid(&sock->sk->sk_cgrp_data); } fd_install(); return; } exactly like fd_install() but for received files. For scm you can fail somewhere in the middle of putting any number of file descriptors so you're left in a state with only a subset of requested file descriptors installed so it's not really useful there. And if you manage to install an fd but then fail to put_user() it userspace can simply check it's fds via proc and has to anyway on any scm message error. If you fail an scm message userspace better check their fds. For seccomp maybe but even there I doubt it and I still maintain that userspace screwing this up is on them which is how we do this most of the time. And for pidfd_getfd() this whole put_user() thing doesn't matter at all. It's much easier and clearer if we simply have a fd_install() - fd_install_received() parallelism where we follow an established convention. _But_ if that blocks you from making this generic enough then at least the replace_fd() vs fd_install() logic seems it shouldn't be in there. And the function name really needs to drive home the point that it installs an fd into the tasks fdtable no matter what version you go with. file_receive() is really not accurate enough for this at all. > { > 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