On Tue, Sep 27, 2022 at 06:56:44PM -0400, Paul Moore wrote:
> On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > The posix acl api provides a dedicated security and integrity hook for
> > setting posix acls. This means that
> >
> > evm_protect_xattr()
> > -> evm_xattr_change()
> > -> evm_xattr_acl_change()
> >
> > is now only hit during vfs_remove_acl() at which point we are guaranteed
> > that xattr_value and xattr_value_len are NULL and 0. In this case evm
> > always used to return 1. Simplify this function to do just that.
> >
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> > ---
> >
> > Notes:
> > /* v2 */
> > unchanged
> >
> > security/integrity/evm/evm_main.c | 62 +++++++------------------------
> > 1 file changed, 14 insertions(+), 48 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 15aa5995fff4..1fbe1b8d0364 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -436,62 +436,29 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
> >
> > /*
> > * evm_xattr_acl_change - check if passed ACL changes the inode mode
> > - * @mnt_userns: user namespace of the idmapped mount
> > - * @dentry: pointer to the affected dentry
> > * @xattr_name: requested xattr
> > * @xattr_value: requested xattr value
> > * @xattr_value_len: requested xattr value length
> > *
> > - * Check if passed ACL changes the inode mode, which is protected by EVM.
> > + * This is only hit during xattr removal at which point we always return 1.
> > + * Splat a warning in case someone managed to pass data to this function. That
> > + * should never happen.
> > *
> > * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> > */
> > -static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
> > - struct dentry *dentry, const char *xattr_name,
> > - const void *xattr_value, size_t xattr_value_len)
> > +static int evm_xattr_acl_change(const void *xattr_value, size_t xattr_value_len)
> > {
> > -#ifdef CONFIG_FS_POSIX_ACL
> > - umode_t mode;
> > - struct posix_acl *acl = NULL, *acl_res;
> > - struct inode *inode = d_backing_inode(dentry);
> > - int rc;
> > -
> > - /*
> > - * An earlier comment here mentioned that the idmappings for
> > - * ACL_{GROUP,USER} don't matter since EVM is only interested in the
> > - * mode stored as part of POSIX ACLs. Nonetheless, if it must translate
> > - * from the uapi POSIX ACL representation to the VFS internal POSIX ACL
> > - * representation it should do so correctly. There's no guarantee that
> > - * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters
> > - * for the mode at some point and it's difficult to keep track of all
> > - * the LSM and integrity modules and what they do to POSIX ACLs.
> > - *
> > - * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX
> > - * ACLs it received. It requires knowledge that only the VFS is
> > - * guaranteed to have.
> > - */
> > - acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
> > - xattr_value, xattr_value_len);
> > - if (IS_ERR_OR_NULL(acl))
> > - return 1;
> > -
> > - acl_res = acl;
> > - /*
> > - * Passing mnt_userns is necessary to correctly determine the GID in
> > - * an idmapped mount, as the GID is used to clear the setgid bit in
> > - * the inode mode.
> > - */
> > - rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
> > -
> > - posix_acl_release(acl);
> > -
> > - if (rc)
> > - return 1;
> > + int rc = 0;
> >
> > - if (inode->i_mode != mode)
> > - return 1;
> > +#ifdef CONFIG_FS_POSIX_ACL
> > + WARN_ONCE(xattr_value != NULL,
> > + "Passing xattr value for POSIX ACLs not supported\n");
> > + WARN_ONCE(xattr_value_len != 0,
> > + "Passing non-zero length for POSIX ACLs not supported\n");
> > + rc = 1;
> > #endif
> > - return 0;
> > +
> > + return rc;
> > }
>
> This is another case where I'll leave the final say up to Mimi, but
> why not just get rid of evm_xattr_acl_change() entirely? Unless I'm
> missing something, it's only reason for existing now is to check that
> it is passed the proper (empty) parameters which seems pointless ...
> no?
Yeah, I think we can remove it. evm_inode_remove_acl() is just
evm_inode_set_acl(NULL, 0) so if we add evm_inode_remove_acl() as a
wrapper around it instead of simply abusing the existing
evm_inode_removexattr() we can delete all that code indeed as it won't
be reachable from generic xattr code anymore.
