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=-23.6 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, 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 1FED5C433DB for ; Thu, 7 Jan 2021 02:44:07 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 870C12246B for ; Thu, 7 Jan 2021 02:44:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 870C12246B Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id DA4CE8D0114; Wed, 6 Jan 2021 21:44:05 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D54AC8D0090; Wed, 6 Jan 2021 21:44:05 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BA69E8D0114; Wed, 6 Jan 2021 21:44:05 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0127.hostedemail.com [216.40.44.127]) by kanga.kvack.org (Postfix) with ESMTP id 93FD88D0090 for ; Wed, 6 Jan 2021 21:44:05 -0500 (EST) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 4EDEA1EF3 for ; Thu, 7 Jan 2021 02:44:05 +0000 (UTC) X-FDA: 77677434450.15.side92_28058ea274e6 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin15.hostedemail.com (Postfix) with ESMTP id 246AC1814B0C1 for ; Thu, 7 Jan 2021 02:44:05 +0000 (UTC) X-HE-Tag: side92_28058ea274e6 X-Filterd-Recvd-Size: 11478 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by imf33.hostedemail.com (Postfix) with ESMTP for ; Thu, 7 Jan 2021 02:44:04 +0000 (UTC) Received: by mail-ed1-f47.google.com with SMTP id c7so6300044edv.6 for ; Wed, 06 Jan 2021 18:44:04 -0800 (PST) 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=OMOKHTGqmc8H/goueItvedsRqkw1V7cr/6m0zsCokxw=; b=gyKgWQMwx/fTejAGvT1hbN4rzR75Q1DifGuW0ra4ZEh5ftPL8zEY4t7NeUB5EqUJIP V0blI2KlAUL8JdQVW1/c+YtmoKvSecARmdppSAdJYwjqwe/NAn3+4wgklIhNTeyRgS2R w05YjLwZpGCyHcKUIJ/Q9oRznw0AcM1r6qF2WK8yj2Gz4MdsT0ZzuGaOFI76AfGJc2vB fRfb9WoD4qo932KS+okUkD744Ux7YW9MuYc+QPYgL2COSkrIEsAppNS2ztQqRuSfYtsG h2dl0xVbrXhvaYsJ5Q0MN+c2J+kcXRHWrwA8MSZaUbxeWccd8go9LR8zrFLnrFjcMQZ6 aBhQ== 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=OMOKHTGqmc8H/goueItvedsRqkw1V7cr/6m0zsCokxw=; b=uavoEkKiXFLOgYoVP0yraDUZFkd5eL57NeJ/Cu+PIcZCdcif/ESzFkOnA/1fcxqLGt mIZQb/4yWFSlisunGCoqPW40q46woFLQpYlwYhzYMa0Xq7wctgk4iwt/SL7WGlE9trnb 7PLK02MeYXMx0vtnLqI6ebviy3AJFOWbTQVLq8WkugZzXC+SdDg+MeQaeYRS1DVqZGkX /NwSkPVZdB9ofJgFqFQ6LLATtR+7psX6K+//RKJDjwGW1uDMFyxVnsEmXXfpnQbSFBlV BZpbxV2KOujuFaPssgB56UW9MP3K6f593mdq27mBaFrH3oORCBpWkx71kRtBsgpfCB59 cy0w== X-Gm-Message-State: AOAM532wJxbCWwQFgt/NUY93Ulw3z9GilbZ+RodV+IC5XzJYFAzbWlFZ P/cHYt+gCdgg8tmozd1TtnX1GCDZAnp6NfFMUMmeww== X-Google-Smtp-Source: ABdhPJxBb+RdnrsFN59ka23PplstQ6jdggXOAEPVHXXH8Ohp0tDlQq8TsnXKFSOgQicIgRKCKgy9RZ1egRVgU/h/DdQ= X-Received: by 2002:a05:6402:1ad1:: with SMTP id ba17mr29013edb.51.1609987443025; Wed, 06 Jan 2021 18:44:03 -0800 (PST) MIME-Version: 1.0 References: <20201112015359.1103333-1-lokeshgidra@google.com> <20201112015359.1103333-3-lokeshgidra@google.com> In-Reply-To: From: Lokesh Gidra Date: Wed, 6 Jan 2021 18:43:52 -0800 Message-ID: Subject: Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface To: Paul Moore Cc: Andrea Arcangeli , Alexander Viro , James Morris , Stephen Smalley , Casey Schaufler , Eric Biggers , "Serge E. Hallyn" , Eric Paris , Daniel Colascione , Kees Cook , "Eric W. Biederman" , KP Singh , David Howells , Anders Roxell , Sami Tolvanen , Matthew Garrett , Aaron Goidel , Randy Dunlap , "Joel Fernandes (Google)" , YueHaibing , Christian Brauner , Alexei Starovoitov , Alexey Budankov , Adrian Reber , Aleksa Sarai , Linux FS Devel , linux-kernel , LSM List , SElinux list , Kalesh Singh , Calin Juravle , Suren Baghdasaryan , Jeffrey Vander Stoep , "Cc: Android Kernel" , "open list:MEMORY MANAGEMENT" , Andrew Morton , hch@infradead.org, Daniel Colascione , Eric Biggers Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Jan 6, 2021 at 6:10 PM Paul Moore wrote: > > On Wed, Nov 11, 2020 at 8:54 PM 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 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 > > > > [Delete obsolete comments to alloc_anon_inode()] > > [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()] > > Lokesh, I'm assuming you made the changes in the brackets above? If > so they should include your initials or some other means of > attributing them to you, e.g. "[LG: Fix error ...]". Thanks for reviewing the patch. Sorry for missing this. If it's critical then I can upload another version of the patches to fix this. Kindly let me know. > > > Signed-off-by: Lokesh Gidra > > Reviewed-by: Eric Biggers > > --- > > fs/anon_inodes.c | 150 ++++++++++++++++++++++++++---------- > > fs/libfs.c | 5 -- > > include/linux/anon_inodes.h | 5 ++ > > 3 files changed, 115 insertions(+), 45 deletions(-) > > > > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c > > index 89714308c25b..023337d65a03 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; > > +} > > > > - if (IS_ERR(anon_inode_inode)) > > - return ERR_PTR(-ENODEV); > > +static struct file *__anon_inode_getfile(const char *name, > > + const struct file_operations *fops, > > + void *priv, int flags, > > + const struct inode *context_inode, > > + bool secure) > > Is it necessary to pass both the context_inode pointer and the secure > boolean? It seems like if context_inode is non-NULL then one could > assume that a secure anonymous inode was requested; is there ever > going to be a case where this is not true? Yes, it is necessary as there are scenarios where a secure anon-inode is to be created but there is no context_inode available. For instance, in patch 4/4 of this series you'll see that when a secure anon-inode is created in the userfaultfd syscall, context_inode isn't available. > > > +{ > > + struct inode *inode; > > + struct file *file; > > > > if (fops->owner && !try_module_get(fops->owner)) > > return ERR_PTR(-ENOENT); > > > > - /* > > - * We know the anon_inode inode count is always greater than zero, > > - * so ihold() is safe. > > - */ > > - ihold(anon_inode_inode); > > - file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name, > > + if (secure) { > > + inode = anon_inode_make_secure_inode(name, context_inode); > > + if (IS_ERR(inode)) { > > + file = ERR_CAST(inode); > > + goto err; > > + } > > + } else { > > + inode = anon_inode_inode; > > + if (IS_ERR(inode)) { > > + file = ERR_PTR(-ENODEV); > > + goto err; > > + } > > + /* > > + * We know the anon_inode inode count is always > > + * greater than zero, so ihold() is safe. > > + */ > > + ihold(inode); > > + } > > + > > + file = alloc_file_pseudo(inode, anon_inode_mnt, name, > > flags & (O_ACCMODE | O_NONBLOCK), fops); > > if (IS_ERR(file)) > > - goto err; > > + goto err_iput; > > > > - file->f_mapping = anon_inode_inode->i_mapping; > > + file->f_mapping = inode->i_mapping; > > > > file->private_data = priv; > > > > return file; > > > > +err_iput: > > + iput(inode); > > err: > > - iput(anon_inode_inode); > > module_put(fops->owner); > > return file; > > } > > -EXPORT_SYMBOL_GPL(anon_inode_getfile); > > -- > paul moore > www.paul-moore.com