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.9 required=3.0 tests=DKIMWL_WL_HIGH,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 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 1489BC433E2 for ; Thu, 4 Jun 2020 02:23:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E50B0208A7 for ; Thu, 4 Jun 2020 02:23:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Qn2BQw97" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726853AbgFDCXA (ORCPT ); Wed, 3 Jun 2020 22:23:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53750 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726093AbgFDCXA (ORCPT ); Wed, 3 Jun 2020 22:23:00 -0400 Received: from mail-pj1-x1043.google.com (mail-pj1-x1043.google.com [IPv6:2607:f8b0:4864:20::1043]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DB9B8C08C5C0 for ; Wed, 3 Jun 2020 19:22:59 -0700 (PDT) Received: by mail-pj1-x1043.google.com with SMTP id q24so426925pjd.1 for ; Wed, 03 Jun 2020 19:22:59 -0700 (PDT) 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= 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; bh=Yipjb/nJ+PoPYGY3bQlr0vEC5GDtsZwp9TVgxD0rnGk=; b=Y5Na+ZSPg+nVayvw4ZjGD9Y+T6wQE+zZYX74UU1WKFCYOgUxcaPCES0bsXhQ+mZoMK 1I8wVJoM757g+BScSqBRGjM13ZtSA14mArwXPNi02sa0S5dvXJ+/+WnauhIBye+YmMyT IAG1OXn5IerGPDTz+bREnvjEsDdoDJXOLjVAmPhsVhe6gTHgSgDOKRvYy1ODWieEL/+E CbgVKcH2BPgPUeED3SDHPv3j7Jox08FzI5ni1uByeLd8Xn3MxP/Yu3NuW+cX3I0czfhu JFs7ROqUv38iLnCRUj3FxUkdBwVV6DmxoqmQTJJlePNL9HXTq8TkNp2atHgeqlOsykNh oB1A== X-Gm-Message-State: AOAM533A+UzxjC+ZnDKfiMzbwmsyNXg1BzBeJ8dWq59uE/SDIkXWPAfl gIY1dt0HDoGCjuIJE2JJC4Ts3w== X-Google-Smtp-Source: ABdhPJyecMU8uoaQFlZIapME+iRHy6MQjiZHyGdWTsN1FsR15QCal3RaNi3fnZYfnzICd0tiI8+uGQ== X-Received: by 2002:a17:90a:4d4e:: with SMTP id l14mr3326764pjh.10.1591237379271; Wed, 03 Jun 2020 19:22:59 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id y6sm3106283pfp.144.2020.06.03.19.22.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 Jun 2020 19:22:58 -0700 (PDT) Date: Wed, 3 Jun 2020 19:22:57 -0700 From: Kees Cook 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 Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes 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 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200604012452.vh33nufblowuxfed@wittgenstein> Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: 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