From: Paul Moore <paul@paul-moore.com>
To: Lokesh Gidra <lokeshgidra@google.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>,
Alexander Viro <viro@zeniv.linux.org.uk>,
James Morris <jmorris@namei.org>,
Stephen Smalley <stephen.smalley.work@gmail.com>,
Casey Schaufler <casey@schaufler-ca.com>,
Eric Biggers <ebiggers@kernel.org>,
"Serge E. Hallyn" <serge@hallyn.com>,
Eric Paris <eparis@parisplace.org>,
Daniel Colascione <dancol@dancol.org>,
Kees Cook <keescook@chromium.org>,
"Eric W. Biederman" <ebiederm@xmission.com>,
KP Singh <kpsingh@google.com>,
David Howells <dhowells@redhat.com>,
Anders Roxell <anders.roxell@linaro.org>,
Sami Tolvanen <samitolvanen@google.com>,
Matthew Garrett <matthewgarrett@google.com>,
Aaron Goidel <acgoide@tycho.nsa.gov>,
Randy Dunlap <rdunlap@infradead.org>,
"Joel Fernandes (Google)" <joel@joelfernandes.org>,
YueHaibing <yuehaibing@huawei.com>,
Christian Brauner <christian.brauner@ubuntu.com>,
Alexei Starovoitov <ast@kernel.org>,
Alexey Budankov <alexey.budankov@linux.intel.com>,
Adrian Reber <areber@redhat.com>,
Aleksa Sarai <cyphar@cyphar.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
kaleshsingh@google.com, calin@google.com, surenb@google.com,
jeffv@google.com, kernel-team@android.com, linux-mm@kvack.org,
Andrew Morton <akpm@linux-foundation.org>,
hch@infradead.org, Daniel Colascione <dancol@google.com>,
Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface
Date: Wed, 6 Jan 2021 21:09:49 -0500 [thread overview]
Message-ID: <CAHC9VhScpFVtxzU_nUDUc4zGT7+EZKFRpYAm+Ps5vd2AjKkaMQ@mail.gmail.com> (raw)
In-Reply-To: <20201112015359.1103333-3-lokeshgidra@google.com>
On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> From: Daniel Colascione <dancol@google.com>
>
> 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 <dancol@google.com>
>
> [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 ...]".
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> Reviewed-by: Eric Biggers <ebiggers@google.com>
> ---
> 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?
> +{
> + 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
next prev parent reply other threads:[~2021-01-07 2:10 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 1:53 [PATCH v13 0/4] SELinux support for anonymous inodes and UFFD Lokesh Gidra
2020-11-12 1:53 ` [PATCH v13 1/4] security: add inode_init_security_anon() LSM hook Lokesh Gidra
2020-11-12 1:53 ` [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface Lokesh Gidra
2021-01-07 2:09 ` Paul Moore [this message]
2021-01-07 2:42 ` dancol
2021-01-07 3:05 ` Paul Moore
2021-01-07 2:43 ` Lokesh Gidra
2021-01-07 3:08 ` Paul Moore
2020-11-12 1:53 ` [PATCH v13 3/4] selinux: teach SELinux about anonymous inodes Lokesh Gidra
2021-01-07 3:03 ` Paul Moore
2021-01-07 3:55 ` Lokesh Gidra
2021-01-07 22:30 ` Paul Moore
2021-01-07 22:40 ` Lokesh Gidra
2021-01-08 19:35 ` Stephen Smalley
2021-01-08 20:17 ` Lokesh Gidra
2021-01-08 21:23 ` Stephen Smalley
2021-01-08 21:31 ` Lokesh Gidra
2021-01-08 20:58 ` Paul Moore
2020-11-12 1:53 ` [PATCH v13 4/4] userfaultfd: use secure anon inodes for userfaultfd Lokesh Gidra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHC9VhScpFVtxzU_nUDUc4zGT7+EZKFRpYAm+Ps5vd2AjKkaMQ@mail.gmail.com \
--to=paul@paul-moore.com \
--cc=aarcange@redhat.com \
--cc=acgoide@tycho.nsa.gov \
--cc=akpm@linux-foundation.org \
--cc=alexey.budankov@linux.intel.com \
--cc=anders.roxell@linaro.org \
--cc=areber@redhat.com \
--cc=ast@kernel.org \
--cc=calin@google.com \
--cc=casey@schaufler-ca.com \
--cc=christian.brauner@ubuntu.com \
--cc=cyphar@cyphar.com \
--cc=dancol@dancol.org \
--cc=dancol@google.com \
--cc=dhowells@redhat.com \
--cc=ebiederm@xmission.com \
--cc=ebiggers@google.com \
--cc=ebiggers@kernel.org \
--cc=eparis@parisplace.org \
--cc=hch@infradead.org \
--cc=jeffv@google.com \
--cc=jmorris@namei.org \
--cc=joel@joelfernandes.org \
--cc=kaleshsingh@google.com \
--cc=keescook@chromium.org \
--cc=kernel-team@android.com \
--cc=kpsingh@google.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-security-module@vger.kernel.org \
--cc=lokeshgidra@google.com \
--cc=matthewgarrett@google.com \
--cc=rdunlap@infradead.org \
--cc=samitolvanen@google.com \
--cc=selinux@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=stephen.smalley.work@gmail.com \
--cc=surenb@google.com \
--cc=viro@zeniv.linux.org.uk \
--cc=yuehaibing@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).