Re: [PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded

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

On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote:
> In order to enable EVM before starting 'init' process,
> evm_initialized needs to be non-zero. Before it was
> indicating that HMAC key is loaded. When EVM loads
> X509 before calling 'init', it is possible to enable
> EVM to start signature based verification.
> 
> This patch defines bits to enable EVM if key of any type
> is loaded.

Thanks, Dmitry.  There's one comment inline.

> Changes in v2:
> * EVM_STATE_KEY_SET replaced by EVM_INIT_HMAC
> * EVM_STATE_X509_SET replaced by EVM_INIT_X509
> 
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
> ---
>  security/integrity/evm/evm.h        | 3 +++
>  security/integrity/evm/evm_crypto.c | 2 ++
>  security/integrity/evm/evm_main.c   | 6 +++++-
>  security/integrity/evm/evm_secfs.c  | 4 ++--
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 88bfe77..f5f1272 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -21,6 +21,9 @@
> 
>  #include "../integrity.h"
> 
> +#define EVM_INIT_HMAC	0x0001
> +#define EVM_INIT_X509	0x0002
> +
>  extern int evm_initialized;
>  extern char *evm_hmac;
>  extern char *evm_hash;
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 159ef3e..34e1a6f 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -40,6 +40,8 @@ static struct shash_desc *init_desc(char type)
>  	struct shash_desc *desc;
> 
>  	if (type == EVM_XATTR_HMAC) {
> +		if (!(evm_initialized & EVM_INIT_HMAC))
> +			return ERR_PTR(-ENOKEY);

init_desc() is called from a couple of different places.  In some
instances, like when converting from a signature to an hmac, if
init_desc() fails, the xattr isn't converted to an HMAC.  No big deal.
But there are other cases, like when a protected xattr is modified,
failing the write will make the file inaccessible.  Does there need to
be an error msg of some sort or an audit msg?

Mimi

>  		tfm = &hmac_tfm;
>  		algo = evm_hmac;
>  	} else {
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 519de0a..420d94d 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -475,7 +475,11 @@ EXPORT_SYMBOL_GPL(evm_inode_init_security);
>  #ifdef CONFIG_EVM_LOAD_X509
>  void __init evm_load_x509(void)
>  {
> -	integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH);
> +	int rc;
> +
> +	rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH);
> +	if (!rc)
> +		evm_initialized |= EVM_INIT_X509;
>  }
>  #endif
> 
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index cf12a04..3f775df 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -64,7 +64,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
>  	char temp[80];
>  	int i, error;
> 
> -	if (!capable(CAP_SYS_ADMIN) || evm_initialized)
> +	if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC))
>  		return -EPERM;
> 
>  	if (count >= sizeof(temp) || count == 0)
> @@ -80,7 +80,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
> 
>  	error = evm_init_key();
>  	if (!error) {
> -		evm_initialized = 1;
> +		evm_initialized |= EVM_INIT_HMAC;
>  		pr_info("initialized\n");
>  	} else
>  		pr_err("initialization failed\n");