[PATCH v2 16/30] acl: add vfs_get_acl()

[Paul Moore]

On Wed, Sep 28, 2022 at 3:40 AM Christian Brauner <brauner at kernel.org> wrote:
>
> On Tue, Sep 27, 2022 at 06:55:25PM -0400, Paul Moore wrote:
> > On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner at kernel.org> wrote:
> > >
> > > In previous patches we implemented get and set inode operations for all
> > > non-stacking filesystems that support posix acls but didn't yet
> > > implement get and/or set acl inode operations. This specifically
> > > affected cifs and 9p.
> > >
> > > Now we can build a posix acl api based solely on get and set inode
> > > operations. We add a new vfs_get_acl() api that can be used to get posix
> > > acls. This finally removes all type unsafety and type conversion issues
> > > explained in detail in [1] that we aim to get rid of.
> > >
> > > After we finished building the vfs api we can switch stacking
> > > filesystems to rely on the new posix api and then finally switch the
> > > xattr system calls themselves to rely on the posix acl api.
> > >
> > > Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> > > Signed-off-by: Christian Brauner (Microsoft) <brauner at kernel.org>
> > > ---
> > >
> > > Notes:
> > >     /* v2 */
> > >     unchanged
> > >
> > >  fs/posix_acl.c                  | 131 ++++++++++++++++++++++++++++++--
> > >  include/linux/posix_acl.h       |   9 +++
> > >  include/linux/posix_acl_xattr.h |  10 +++
> > >  3 files changed, 142 insertions(+), 8 deletions(-)
> >
> > ...
> >
> > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> > > index ef0908a4bc46..18873be583a9 100644
> > > --- a/fs/posix_acl.c
> > > +++ b/fs/posix_acl.c
> > > @@ -1369,3 +1439,48 @@ int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
> > >         return error;
> > >  }
> > >  EXPORT_SYMBOL(vfs_set_acl);
> > > +
> > > +/**
> > > + * vfs_get_acl - get posix acls
> > > + * @mnt_userns: user namespace of the mount
> > > + * @dentry: the dentry based on which to retrieve the posix acls
> > > + * @acl_name: the name of the posix acl
> > > + *
> > > + * This function retrieves @kacl from the filesystem. The caller must all
> > > + * posix_acl_release() on @kacl.
> > > + *
> > > + * Return: On success POSIX ACLs in VFS format, on error negative errno.
> > > + */
> > > +struct posix_acl *vfs_get_acl(struct user_namespace *mnt_userns,
> > > +                             struct dentry *dentry, const char *acl_name)
> > > +{
> > > +       struct inode *inode = d_inode(dentry);
> > > +       struct posix_acl *acl;
> > > +       int acl_type, error;
> > > +
> > > +       acl_type = posix_acl_type(acl_name);
> > > +       if (acl_type < 0)
> > > +               return ERR_PTR(-EINVAL);
> > > +
> > > +       /*
> > > +        * The VFS has no restrictions on reading POSIX ACLs so calling
> > > +        * something like xattr_permission() isn't needed. Only LSMs get a say.
> > > +        */
> > > +       error = security_inode_getxattr(dentry, acl_name);
> > > +       if (error)
> > > +               return ERR_PTR(error);
> >
> > I understand the desire to reuse the security_inode_getxattr() hook
> > here, it makes perfect sense, but given that this patchset introduces
> > an ACL specific setter hook I think it makes sense to have a matching
> > getter hook.  It's arguably a little silly given the current crop of
> > LSMs and their approach to ACLs, but if we are going to differentiate
> > on the write side I think we might as well be consistent and
> > differentiate on the read side as well.
>
> Sure, I don't mind doing that. I'll add the infrastructure and then the
> individual LSMs can add their own hooks.

Adding the ACL hook infrastructure, including the call in
vfs_get_acl(), without the LSM implementations would result in an
access control regression for both SELinux and Smack.  Similar issues
with the removexattr hook, although that looks to have IMA/EVM calls
too (which may be noops in the case of an ACL, I haven't checked).

The good news is that the individual LSM implementations should be
trivial, and if you wanted to just have the new ACL hook
implementations call into the existing xattr implementations inside
each LSM I think that would be okay to start.

-- 
paul-moore.com