Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp1039174ybg; Wed, 3 Jun 2020 22:27:34 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzxIIxSgQsCdQFvFxnZLOxJHD5EkwaXgjo7H8ZqzJgNuQ7ljQK5c+RGqnFjjlomT9c6mjez X-Received: by 2002:a17:906:7083:: with SMTP id b3mr2300867ejk.57.1591248454122; Wed, 03 Jun 2020 22:27:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591248454; cv=none; d=google.com; s=arc-20160816; b=lT+u2Eg9zrgWTQcRjvxZhLhU4deJlU65Rn9m8QyI780tGLlMrzZboThxcClX7AcWgl TBWQ7RkH69At+JWh0SjKKHgnxovBKxcZvy59iNjlZBHMayvulk1WCF5kC3XrbcZOZH3q 6nKm8fOwJBDdRCazOv4Su9OlBu41SDo/Ujg3ezA4vAeCxVFQxHp7i/o304MqJJkdVMpm mPrkJ9TmNhmhvBeXQU94aXX2FjrikATztIunQBF4309xoTzcWgeGyAwMdGiizRvQ37bm sZci+JVTBJEu8dBgYIajEoAHvNnTNEGYsjIMR4nbLRTIP/0tielpP1XmHJ1sb5N2kyuf YAeQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=oKNXmPZh2jB30BHUyHRPpl5a8wx5yENQJM5HVKTmIUg=; b=wmggxvTVpio9qt2FgYRacuCwtnsYan6aan0NWoVJG1Tew2AoZy4QnNTcrIBnYYx8rq 5s4w12PaSj5xALVBWlvVQUOmzBW8Ryy+VSRtAAfDhCgj6QgwRHWNLNpV0wiAqeOr9PAn fygriMK/fX2b6MfOVBuMfZYS1C+ldJ+4m/ontPuqHeNIMdqQLOtA10LG8l3MASUtgENx bszqtUkwKjsSOETM5gjw35R829d19/AhqFjL/t+ie5HvaE83qGPmTMno9TmU5EEEDd4b VxVGpXYrvzzJbPggh+N28DvSrCXQxsgmusUUpHN3OvrSzl6ppUDbK6+haJt5s5Zp9ttf Z06Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sargun.me header.s=google header.b=054Uqkhv; 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 l5si1077353ede.477.2020.06.03.22.27.11; Wed, 03 Jun 2020 22:27:34 -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; dkim=pass header.i=@sargun.me header.s=google header.b=054Uqkhv; 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 S1726661AbgFDFUp (ORCPT + 99 others); Thu, 4 Jun 2020 01:20:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53074 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725959AbgFDFUo (ORCPT ); Thu, 4 Jun 2020 01:20:44 -0400 Received: from mail-io1-xd41.google.com (mail-io1-xd41.google.com [IPv6:2607:f8b0:4864:20::d41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 84F8FC05BD1E for ; Wed, 3 Jun 2020 22:20:44 -0700 (PDT) Received: by mail-io1-xd41.google.com with SMTP id h4so4913869iob.10 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=FVH3BOQj0HtXWA0nlicrMmryfbGoU0ISnRuMEowfWbi/5sV0sYlcPn6mKI/J8UxCJH uW55iYwjZ6cbnDTaVRK671OpKFJ/t600FSsOi68WYt0djrOKrHtrZjVajODvWO861XwT 8Oyre899cAytUnqooagE9LJzmwkVvk0DMYTTwa4UeRNun5FX2e6tQpnQMtxTFndZZw4q 2WopKA8lfZ5htQ+ZC1TmbEBCcPswFCJ/Yvegmjp+BLb7oAOIu/Xdt5NOqHlGVfMn4glD sUgmrcRdtZHskJ7bKsgs/5G5yCbtcq7QTFdHcyNhTFDZAwnPy33W0I/L3PPhJHhOFZMg jBXA== X-Gm-Message-State: AOAM533tQv9nd+aWtq3GbdBxCARa0IFpCic5V4ow7WRhXzAnV+W+fVWd tFASYbXkE+XtSNXy28brpw1n8Q== 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-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) > { > 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); }