Re: [PATCH v7 5/6] evm: Align evm_inode_init_security() definition with LSM infrastructure

[Mimi Zohar]

On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> 
> Change the evm_inode_init_security() definition to align with the LSM
> infrastructure. Keep the existing behavior of including in the HMAC
> calculation only the first xattr provided by LSMs.
> 
> Changing the evm_inode_init_security() definition requires passing only the
> xattr array allocated by security_inode_init_security(), instead of the
> first LSM xattr and the place where the EVM xattr should be filled. In lieu
> of passing the EVM xattr, EVM must position itself after the last filled
> xattr (by checking the xattr name), since only the beginning of the xattr
> array is given.
> 
> Finally, make evm_inode_init_security() return value compatible with the
> inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
> setting an xattr.
> 
> EVM is a bit tricky, because xattrs is both an input and an output. If it
> was just output, EVM should have returned zero if xattrs is NULL. But,
> since xattrs is also input, EVM is unable to do its calculations, so return
> -EOPNOTSUPP and handle this error in security_inode_init_security().
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

One comment below, otherwise,
Reviewed-by: Mimi  Zohar <zohar@xxxxxxxxxxxxx>


> ---
>  include/linux/evm.h               | 12 ++++++------
>  security/integrity/evm/evm_main.c | 20 +++++++++++++-------
>  security/security.c               |  5 ++---
>  3 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index aa63e0b3c0a2..3bb2ae9fe098 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -35,9 +35,9 @@ extern int evm_inode_removexattr(struct user_namespace *mnt_userns,
>  				 struct dentry *dentry, const char *xattr_name);
>  extern void evm_inode_post_removexattr(struct dentry *dentry,
>  				       const char *xattr_name);
> -extern int evm_inode_init_security(struct inode *inode,
> -				   const struct xattr *xattr_array,
> -				   struct xattr *evm);
> +extern int evm_inode_init_security(struct inode *inode, struct inode *dir,
> +				   const struct qstr *qstr,
> +				   struct xattr *xattrs);
>  extern bool evm_revalidate_status(const char *xattr_name);
>  extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
>  extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
> @@ -108,9 +108,9 @@ static inline void evm_inode_post_removexattr(struct dentry *dentry,
>  	return;
>  }
>  
> -static inline int evm_inode_init_security(struct inode *inode,
> -					  const struct xattr *xattr_array,
> -					  struct xattr *evm)
> +static inline int evm_inode_init_security(struct inode *inode, struct inode *dir,
> +					  const struct qstr *qstr,
> +					  struct xattr *xattrs)
>  {
>  	return 0;
>  }
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 23d484e05e6f..0a312cafb7de 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -845,23 +845,29 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  /*
>   * evm_inode_init_security - initializes security.evm HMAC value
>   */
> -int evm_inode_init_security(struct inode *inode,
> -				 const struct xattr *lsm_xattr,
> -				 struct xattr *evm_xattr)
> +int evm_inode_init_security(struct inode *inode, struct inode *dir,
> +			    const struct qstr *qstr,
> +			    struct xattr *xattrs)
>  {
>  	struct evm_xattr *xattr_data;
> +	struct xattr *xattr, *evm_xattr;
>  	int rc;
>  
> -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> -	    !evm_protected_xattr(lsm_xattr->name))
> -		return 0;
> +	if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs ||
> +	    !evm_protected_xattr(xattrs->name))
> +		return -EOPNOTSUPP;
> +
> +	for (xattr = xattrs; xattr->value != NULL; xattr++)
> +		;

security_inode_init_security() already contains a comment for
allocating +2 extra space.   Adding a similar comment here to explain
why walking the xattrs like this is safe would be nice.

> +
> +	evm_xattr = xattr;
>  
>  	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
>  	if (!xattr_data)
>  		return -ENOMEM;
>  
>  	xattr_data->data.type = EVM_XATTR_HMAC;
> -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> +	rc = evm_init_hmac(inode, xattrs, xattr_data->digest);
>  	if (rc < 0)
>  		goto out;
>  
> diff --git a/security/security.c b/security/security.c
> index 36804609caaa..44ce579daec1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1190,9 +1190,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  	if (!num_filled_xattrs)
>  		goto out;
>  
> -	ret = evm_inode_init_security(inode, new_xattrs,
> -				      new_xattrs + num_filled_xattrs);
> -	if (ret)
> +	ret = evm_inode_init_security(inode, dir, qstr, new_xattrs);
> +	if (ret && ret != -EOPNOTSUPP)
>  		goto out;
>  	ret = initxattrs(inode, new_xattrs, fs_data);
>  out: