[PATCH v2 18/30] evm: simplify evm_xattr_acl_change()

[Paul Moore]

On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner at kernel.org> 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 at kernel.org>
> ---
>
> 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?

--
paul-moore.com