Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Mimi Zohar <zohar@linux.ibm.com>]

[Cc'ing John Johansen]

Hi Roberto,

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
> 
> However, IMA is not notified about metadata changes and, after the first
> successful appraisal, always allows access to the files without checking
> metadata again.
> 
> This patch introduces the new atomic flag EVM_RESET_STATUS in
> integrity_iint_cache that is set in the EVM post hooks and cleared in
> evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
> it is set, it clears the appraisal flags.
> 
> Although the flag could be cleared also by evm_inode_setxattr() and
> evm_inode_setattr() before IMA sees it, this does not happen if
> EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> is cleared.
> 
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
> 
> Cc: stable@vger.kernel.org # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
>  security/integrity/ima/ima_main.c |  8 ++++++--
>  security/integrity/integrity.h    |  1 +
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 4e9f5e8b21d5..05be1ad3e6f3 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  		evm_status = (rc == -ENODATA) ?
>  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
>  out:
> -	if (iint)
> +	if (iint) {
> +		/*
> +		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
> +		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
> +		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
> +		 */
> +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
>  		iint->evm_status = evm_status;

True IMA is currently the only caller of evm_verifyxattr() in the
upstreamed kernel, but it is an exported function, which may be called
from elsewhere.  The previous version crossed the boundary between EVM
& IMA with EVM modifying the IMA flag directly.  This version assumes
that IMA will be the only caller.  Otherwise, I like this version.

Mimi

> +	}
>  	kfree(xattr_data);
>  	return evm_status;
>  }
> @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
>  	struct integrity_iint_cache *iint;
>  
>  	iint = integrity_iint_find(inode);
> -	if (iint)
> +	if (iint) {
> +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> +
>  		iint->evm_status = INTEGRITY_UNKNOWN;
> +	}
>  }
>  
>  /**
> @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  	if (!evm_key_loaded())
>  		return;
>  
> +	evm_reset_status(dentry->d_inode);
> +
>  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>  		evm_update_evmxattr(dentry, NULL, NULL, 0);
>  }