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=-17.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, 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_IN_DEF_DKIM_WL autolearn=ham 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 8F4FEC43461 for ; Mon, 7 Sep 2020 07:46:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4F5832145D for ; Mon, 7 Sep 2020 07:46:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="kQgmoMef" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727876AbgIGHqP (ORCPT ); Mon, 7 Sep 2020 03:46:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37254 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727847AbgIGHqJ (ORCPT ); Mon, 7 Sep 2020 03:46:09 -0400 Received: from mail-il1-x143.google.com (mail-il1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5458CC061755 for ; Mon, 7 Sep 2020 00:46:09 -0700 (PDT) Received: by mail-il1-x143.google.com with SMTP id h11so11682618ilj.11 for ; Mon, 07 Sep 2020 00:46:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=eOcu8D5wTw2HeDSoosOfCg4t0DhAHSO+wPplJiiP8zs=; b=kQgmoMefVTk+IjUZ91VOxE5uOnj6dN3OhFswHjPA8Sohyz/hprwyvf/fQn6Qoci4Jn +QAj/8ckPZAQfT/hBgM6aPTrkQSyXVpGrDiRLjaZY14xaKKjo0wiD7ecNm1TgEJRaABC vdH6n3ctU+AsvpY/VVWtT6KEOOFZJ44rlwDnYO9E1niHmbd3YWirTDC+aF7X//AzhGdJ OvAXENpZMHKtvIaHfLrCEL4c1vDSyqIbLf70DiGnPXLWmUXx0/eVXIK/5pe/APtq6toh c2/eaIZmh1uCL+bJd+1HrEKfAUazohnDWZbQUoWRz1c0K33JNlTT0fHRyZL2L42pERMP ynZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=eOcu8D5wTw2HeDSoosOfCg4t0DhAHSO+wPplJiiP8zs=; b=syv4CtzDulJGKftZktsYaHXEVbaC8zquu24yfi6fU17lEOHiFSt3s/D8trfY3Y+ZCx vbj/GimlaEUbzM6ga9LsVmMV7Uuker9igt8srdqm6ORQWphcf/qt7rYYMKHXYuxWFOaA 0O9qeVLyCI8ORkl3iEqqm8Q8NgY139LnWQxYpb0ZEGa0it8seZjTmdvOl+GKdX6LZneL +0Y3ESCu2XDnRArQpv9JQWugZrg7OHUPGQnIKvv855uaPUwWfG873+w5ZcrJ40wkbdP2 rwOEQGN94hnjQAMa4fxq3W0L86up7SOHy3DsWkik0dsbEoi1PJgEkGJ9E/e3wT66AQCo Fpqg== X-Gm-Message-State: AOAM5331gWkVQo83IWWKS2uLIHq2MiC2odcv836fNU7Lyg8qrdjnBTUV qKwIB32M8NuEDt+myY1EIYb64FMzkrU8/LIAmg1Opw== X-Google-Smtp-Source: ABdhPJziDuXiwGvW9UM22wNMoxrqrL0azfO21D5OHGUhZejQeMz0dhIUigYLyuSuoA9rIgKn3f2lHV90i9Kjf0GwjC8= X-Received: by 2002:a92:c5ac:: with SMTP id r12mr17119968ilt.274.1599464767126; Mon, 07 Sep 2020 00:46:07 -0700 (PDT) MIME-Version: 1.0 References: <20200827063522.2563293-1-lokeshgidra@google.com> <20200827063522.2563293-2-lokeshgidra@google.com> <20200901124136.r3krb2p23343licq@wittgenstein> In-Reply-To: <20200901124136.r3krb2p23343licq@wittgenstein> From: Lokesh Gidra Date: Mon, 7 Sep 2020 00:45:56 -0700 Message-ID: Subject: Re: [PATCH v8 1/3] Add a new LSM-supporting anonymous inode interface To: Christian Brauner Cc: Alexander Viro , James Morris , Stephen Smalley , Casey Schaufler , Eric Biggers , "Serge E. Hallyn" , Paul Moore , Eric Paris , Daniel Colascione , Kees Cook , "Eric W. Biederman" , KP Singh , David Howells , Thomas Cedeno , Anders Roxell , Sami Tolvanen , Matthew Garrett , Aaron Goidel , Randy Dunlap , "Joel Fernandes (Google)" , YueHaibing , Alexei Starovoitov , Alexey Budankov , Adrian Reber , Aleksa Sarai , Linux FS Devel , linux-kernel , LSM List , SElinux list , Kalesh Singh , Calin Juravle , Suren Baghdasaryan , Nick Kralevich , Jeffrey Vander Stoep , kernel-team@android.com, Jann Horn Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 1, 2020 at 5:41 AM Christian Brauner wrote: > > On Wed, Aug 26, 2020 at 11:35:20PM -0700, Lokesh Gidra wrote: > > From: Daniel Colascione > > > > This change adds a new function, anon_inode_getfd_secure, that creates > > anonymous-node file with individual non-S_PRIVATE inode to which security > > modules can apply policy. Existing callers continue using the original > > singleton-inode kind of anonymous-inode file. We can transition anonymous > > inode users to the new kind of anonymous inode in individual patches for > > the sake of bisection and review. > > > > The new function accepts an optional context_inode parameter that > > callers can use to provide additional contextual information to > > security modules for granting/denying permission to create an anon inode > > of the same type. > > > > For example, in case of userfaultfd, the created inode is a > > 'logical child' of the context_inode (userfaultfd inode of the > > parent process) in the sense that it provides the security context > > required during creation of the child process' userfaultfd inode. > > > > Signed-off-by: Daniel Colascione > > > > [Fix comment documenting return values of inode_init_security_anon()] > > [Add context_inode description in comments to anon_inode_getfd_secure()] > > [Remove definition of anon_inode_getfile_secure() as there are no callers] > > [Make _anon_inode_getfile() static] > > [Use correct error cast in _anon_inode_getfile()] > > [Fix error handling in _anon_inode_getfile()] > > > > Signed-off-by: Lokesh Gidra > > --- > > fs/anon_inodes.c | 147 +++++++++++++++++++++++++--------- > > include/linux/anon_inodes.h | 8 ++ > > include/linux/lsm_hook_defs.h | 2 + > > include/linux/lsm_hooks.h | 9 +++ > > include/linux/security.h | 10 +++ > > security/security.c | 8 ++ > > 6 files changed, 144 insertions(+), 40 deletions(-) > > > > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c > > index 89714308c25b..c3f16deda211 100644 > > --- a/fs/anon_inodes.c > > +++ b/fs/anon_inodes.c > > @@ -55,61 +55,79 @@ static struct file_system_type anon_inode_fs_type = { > > .kill_sb = kill_anon_super, > > }; > > > > -/** > > - * anon_inode_getfile - creates a new file instance by hooking it up to an > > - * anonymous inode, and a dentry that describe the "class" > > - * of the file > > - * > > - * @name: [in] name of the "class" of the new file > > - * @fops: [in] file operations for the new file > > - * @priv: [in] private data for the new file (will be file's private_data) > > - * @flags: [in] flags > > - * > > - * Creates a new file by hooking it on a single inode. This is useful for files > > - * that do not need to have a full-fledged inode in order to operate correctly. > > - * All the files created with anon_inode_getfile() will share a single inode, > > - * hence saving memory and avoiding code duplication for the file/inode/dentry > > - * setup. Returns the newly created file* or an error pointer. > > - */ > > -struct file *anon_inode_getfile(const char *name, > > - const struct file_operations *fops, > > - void *priv, int flags) > > +static struct inode *anon_inode_make_secure_inode( > > + const char *name, > > + const struct inode *context_inode) > > { > > - struct file *file; > > + struct inode *inode; > > + const struct qstr qname = QSTR_INIT(name, strlen(name)); > > + int error; > > + > > + inode = alloc_anon_inode(anon_inode_mnt->mnt_sb); > > + if (IS_ERR(inode)) > > + return inode; > > + inode->i_flags &= ~S_PRIVATE; > > + error = security_inode_init_security_anon(inode, &qname, context_inode); > > + if (error) { > > + iput(inode); > > + return ERR_PTR(error); > > + } > > + return inode; > > +} > > Hey, > > Iiuc, this makes each newly created anon inode fd correspond to a unique > file and to a unique inode: > > fd1 -> file1 -> inode1 > fd2 -> file2 -> inode2 > Not every anon inode. Just the ones created through anon_inode_getfd_secure() API. > Whereas before we had every anon inode fd correspond to a unique file > but all files map to the _same_ inode: > > fd1 -> file1 -> inode > fd2 -> file2 -> inode > Thils is still the case if anon_inode_getfile() and/or anon_inode_getfd() APIs are used. > The old behavior of hooking up a each anon inode fd to the same inode > prevented having an evict method attached to the inode. Because it was > shared that wasn't possible but also simply because that inode never got > evicted anyway. That surely was intended but it's a bummer to some > extent. > With the new model you also can't have an evict method because now you > have a separate inode for each file. > > I'm probably going to get killed for suggesting this but: > If we're going to expand the anonymous inode infrastructure anyway is > there a way we can make it so that we have a way to allocate a single > inode for multiple anonymous inode fds and have callers opt-in to this > behavior. We'd need a way to find this inode again, obviously. > > This would allow for some features on top of anonymous inode fds that > can refer to the same object, i.e. anonymous inode fds that currently > stash away the same object in f->private_data. > In such a model we could allow such anonymous inode fds to stash away > objects in inode->i_private instead of f->private_data and attach an > evict method to it. This would e.g. allow a process to be killed when > the last pidfd to it is closed or a seccomp notifier fd to notify when > the filter is released without having to do separate reference counting. > I didn't fully understand the example you gave and the role that evict method will play in it. Can you please elaborate a bit more. But, I'd like to point you to a previous discussion between Daniel Colascione (the original contributor of this patch series) and Stephan Smalley on the topic of inodes https://lore.kernel.org/lkml/CAKOZuesUVSYJ6EjHFL3QyiWKVmyhm1fLp5Bm_SHjB3_s1gn08A@mail.gmail.com/ I agree with Daniel (see his replies in the thread link above) that a separate inode per anon inode fd keeps the design simple, particularly from the security context perspective. > This would need a way to lookup that inode by the object that is stashed > away in it of course which could probably be done by an idr or an > xarray or something cleverer. It would obviously only affect a subset of > anonymous inode fds so any other anonymous inode fds wouldn't be > impacted since they can still use the single-anon-inode interface. > > Christian