On Wed, Jan 6, 2021 at 6:10 PM Paul Moore [off-list ref] wrote:

On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra [off-list ref] wrote:

quoted

From: Daniel Colascione <redacted>

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 <redacted>

[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.

quoted

Signed-off-by: Lokesh Gidra <redacted>
Reviewed-by: Eric Biggers <redacted>
---
 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.

quoted

+{
+       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