Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface

[Lokesh Gidra <lokeshgidra@google.com>]

On Wed, Jan 6, 2021 at 6:10 PM Paul Moore <paul@paul-moore.com> wrote:
>
> 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 ...]".

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

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