[PATCH V4] evm: Allow non-SHA1 digital signatures

[PATCH V4] evm: Allow non-SHA1 digital signatures

[Matthew Garrett]

SHA1 is reasonable in HMAC constructs, but it's desirable to be able to
use stronger hashes in digital signatures. Modify the EVM crypto code so
the hash type is imported from the digital signature and passed down to
the hash calculation code, and return the digest size to higher layers
for validation.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 security/integrity/evm/evm.h        | 10 ++++-
 security/integrity/evm/evm_crypto.c | 59 +++++++++++++++--------------
 security/integrity/evm/evm_main.c   | 19 ++++++----
 3 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index 45c4a89c02ff..1ed117f360af 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -42,6 +42,11 @@ extern struct crypto_shash *hash_tfm;
 /* List of EVM protected security xattrs */
 extern char *evm_config_xattrnames[];
 
+struct evm_digest {
+	struct ima_digest_data hdr;
+	char digest[IMA_MAX_DIGEST_SIZE];
+} __packed__;
+
 int evm_init_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
 			const char *req_xattr_name,
@@ -49,10 +54,11 @@ int evm_update_evmxattr(struct dentry *dentry,
 			size_t req_xattr_value_len);
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
-		  size_t req_xattr_value_len, char *digest);
+		  size_t req_xattr_value_len, struct evm_digest *data);
 int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
-		  size_t req_xattr_value_len, char type, char *digest);
+		  size_t req_xattr_value_len, char type,
+		  struct evm_digest *data);
 int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
 		  char *hmac_val);
 int evm_init_secfs(void);
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a46fba322340..e266a3f0ec33 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -21,6 +21,7 @@
 #include <linux/evm.h>
 #include <keys/encrypted-type.h>
 #include <crypto/hash.h>
+#include <crypto/hash_info.h>
 #include "evm.h"
 
 #define EVMKEY "evm-key"
@@ -28,8 +29,7 @@
 static unsigned char evmkey[MAX_KEY_SIZE];
 static int evmkey_len = MAX_KEY_SIZE;
 
-struct crypto_shash *hmac_tfm;
-struct crypto_shash *hash_tfm;
+static struct crypto_shash *evm_tfm[HASH_ALGO__LAST];
 
 static DEFINE_MUTEX(mutex);
 
@@ -37,9 +37,6 @@ static DEFINE_MUTEX(mutex);
 
 static unsigned long evm_set_key_flags;
 
-static char * const evm_hmac = "hmac(sha1)";
-static char * const evm_hash = "sha1";
-
 /**
  * evm_set_key() - set EVM HMAC key from the kernel
  * @key: pointer to a buffer with the key data
@@ -74,10 +71,10 @@ int evm_set_key(void *key, size_t keylen)
 }
 EXPORT_SYMBOL_GPL(evm_set_key);
 
-static struct shash_desc *init_desc(char type)
+static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 {
 	long rc;
-	char *algo;
+	const char *algo;
 	struct crypto_shash **tfm;
 	struct shash_desc *desc;
 
@@ -86,13 +83,11 @@ static struct shash_desc *init_desc(char type)
 			pr_err_once("HMAC key is not set\n");
 			return ERR_PTR(-ENOKEY);
 		}
-		tfm = &hmac_tfm;
-		algo = evm_hmac;
-	} else {
-		tfm = &hash_tfm;
-		algo = evm_hash;
 	}
 
+	tfm = &evm_tfm[hash_algo];
+	algo = hash_algo_name[hash_algo];
+
 	if (*tfm == NULL) {
 		mutex_lock(&mutex);
 		if (*tfm)
@@ -186,10 +181,10 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
  * each xattr, but attempt to re-use the previously allocated memory.
  */
 static int evm_calc_hmac_or_hash(struct dentry *dentry,
-				const char *req_xattr_name,
-				const char *req_xattr_value,
-				size_t req_xattr_value_len,
-				char type, char *digest)
+				 const char *req_xattr_name,
+				 const char *req_xattr_value,
+				 size_t req_xattr_value_len,
+				 uint8_t type, struct evm_digest *data)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	struct shash_desc *desc;
@@ -203,10 +198,17 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 	if (!(inode->i_opflags & IOP_XATTR))
 		return -EOPNOTSUPP;
 
-	desc = init_desc(type);
+	desc = init_desc(type, data->hdr.algo);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
+	/*
+	 * HMACs are always SHA1, but signatures may be of varying sizes -
+	 * make sure the caller knows how long the returned data is
+	 */
+	if (type != EVM_XATTR_HMAC)
+		data->hdr.length = crypto_shash_digestsize(desc->tfm);
+
 	error = -ENODATA;
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		bool is_ima = false;
@@ -238,7 +240,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		if (is_ima)
 			ima_present = true;
 	}
-	hmac_add_misc(desc, inode, type, digest);
+	hmac_add_misc(desc, inode, type, data->digest);
 
 	/* Portable EVM signatures must include an IMA hash */
 	if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
@@ -251,18 +253,18 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value, size_t req_xattr_value_len,
-		  char *digest)
+		  struct evm_digest *data)
 {
 	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
-			       req_xattr_value_len, EVM_XATTR_HMAC, digest);
+		    req_xattr_value_len, EVM_XATTR_HMAC, data);
 }
 
 int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value, size_t req_xattr_value_len,
-		  char type, char *digest)
+		  char type, struct evm_digest *data)
 {
 	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
-				     req_xattr_value_len, type, digest);
+		      req_xattr_value_len, type, data);
 }
 
 static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
@@ -302,7 +304,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct evm_ima_xattr_data xattr_data;
+	struct evm_digest data;
 	int rc = 0;
 
 	/*
@@ -315,13 +317,14 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 	if (rc)
 		return -EPERM;
 
+	data.hdr.algo = HASH_ALGO_SHA1;
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
-			   xattr_value_len, xattr_data.digest);
+			   xattr_value_len, &data);
 	if (rc == 0) {
-		xattr_data.type = EVM_XATTR_HMAC;
+		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
-					   &xattr_data,
-					   sizeof(xattr_data), 0);
+					   &data.hdr.xattr.data[1],
+					   SHA1_DIGEST_SIZE + 1, 0);
 	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
 		rc = __vfs_removexattr(dentry, XATTR_NAME_EVM);
 	}
@@ -333,7 +336,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 {
 	struct shash_desc *desc;
 
-	desc = init_desc(EVM_XATTR_HMAC);
+	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
 	if (IS_ERR(desc)) {
 		pr_info("init_desc failed\n");
 		return PTR_ERR(desc);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 9ea9c19a545c..88ea9c1962d6 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -25,6 +25,7 @@
 #include <linux/magic.h>
 
 #include <crypto/hash.h>
+#include <crypto/hash_info.h>
 #include <crypto/algapi.h>
 #include "evm.h"
 
@@ -122,8 +123,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 					     struct integrity_iint_cache *iint)
 {
 	struct evm_ima_xattr_data *xattr_data = NULL;
-	struct evm_ima_xattr_data calc;
+	struct signature_v2_hdr *hdr;
 	enum integrity_status evm_status = INTEGRITY_PASS;
+	struct evm_digest digest;
 	struct inode *inode;
 	int rc, xattr_len;
 
@@ -159,25 +161,28 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 			evm_status = INTEGRITY_FAIL;
 			goto out;
 		}
+
+		digest.hdr.algo = HASH_ALGO_SHA1;
 		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
-				   xattr_value_len, calc.digest);
+				   xattr_value_len, &digest);
 		if (rc)
 			break;
-		rc = crypto_memneq(xattr_data->digest, calc.digest,
-			    sizeof(calc.digest));
+		rc = crypto_memneq(xattr_data->digest, digest.digest,
+				   SHA1_DIGEST_SIZE);
 		if (rc)
 			rc = -EINVAL;
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
 	case EVM_XATTR_PORTABLE_DIGSIG:
+		hdr = (struct signature_v2_hdr *)xattr_data;
+		digest.hdr.algo = hdr->hash_algo;
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
-				   xattr_value_len, xattr_data->type,
-				   calc.digest);
+				   xattr_value_len, xattr_data->type, &digest);
 		if (rc)
 			break;
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
 					(const char *)xattr_data, xattr_len,
-					calc.digest, sizeof(calc.digest));
+					digest.digest, digest.hdr.length);
 		if (!rc) {
 			inode = d_backing_inode(dentry);
 
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH V4] evm: Allow non-SHA1 digital signatures

[Mimi Zohar]

On Tue, 2018-05-15 at 10:53 -0700, Matthew Garrett wrote:
> SHA1 is reasonable in HMAC constructs, but it's desirable to be able to
> use stronger hashes in digital signatures. Modify the EVM crypto code so
> the hash type is imported from the digital signature and passed down to
> the hash calculation code, and return the digest size to higher layers
> for validation.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>

> ---
>  security/integrity/evm/evm.h        | 10 ++++-
>  security/integrity/evm/evm_crypto.c | 59 +++++++++++++++--------------
>  security/integrity/evm/evm_main.c   | 19 ++++++----
>  3 files changed, 51 insertions(+), 37 deletions(-)
> 
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 45c4a89c02ff..1ed117f360af 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -42,6 +42,11 @@ extern struct crypto_shash *hash_tfm;
>  /* List of EVM protected security xattrs */
>  extern char *evm_config_xattrnames[];
> 
> +struct evm_digest {
> +	struct ima_digest_data hdr;
> +	char digest[IMA_MAX_DIGEST_SIZE];
> +} __packed__;


security/integrity/evm/evm_crypto.o:(.bss+0x0): multiple definition of
`__packed__'
security/integrity/evm/evm_main.o:(.bss+0x20): first defined here
security/integrity/evm/evm_secfs.o:(.bss+0x0): multiple definition of
`__packed__'
security/integrity/evm/evm_main.o:/home/mimi/src/kernel/linux-
integrity/security/integrity/evm/evm_main.c:236: first defined here
Makefile:1036: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

After removing it, the security.evm hmac's are not properly
validating, preventing the EVM/IMA keys from loading.  You can imagine
how far my system boots without the public keys.

Mimi


> +
>  int evm_init_key(void);
>  int evm_update_evmxattr(struct dentry *dentry,
>  			const char *req_xattr_name,
> @@ -49,10 +54,11 @@ int evm_update_evmxattr(struct dentry *dentry,
>  			size_t req_xattr_value_len);
>  int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value,
> -		  size_t req_xattr_value_len, char *digest);
> +		  size_t req_xattr_value_len, struct evm_digest *data);
>  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value,
> -		  size_t req_xattr_value_len, char type, char *digest);
> +		  size_t req_xattr_value_len, char type,
> +		  struct evm_digest *data);
>  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>  		  char *hmac_val);
>  int evm_init_secfs(void);
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a46fba322340..e266a3f0ec33 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -21,6 +21,7 @@
>  #include <linux/evm.h>
>  #include <keys/encrypted-type.h>
>  #include <crypto/hash.h>
> +#include <crypto/hash_info.h>
>  #include "evm.h"
> 
>  #define EVMKEY "evm-key"
> @@ -28,8 +29,7 @@
>  static unsigned char evmkey[MAX_KEY_SIZE];
>  static int evmkey_len = MAX_KEY_SIZE;
> 
> -struct crypto_shash *hmac_tfm;
> -struct crypto_shash *hash_tfm;
> +static struct crypto_shash *evm_tfm[HASH_ALGO__LAST];
> 
>  static DEFINE_MUTEX(mutex);
> 
> @@ -37,9 +37,6 @@ static DEFINE_MUTEX(mutex);
> 
>  static unsigned long evm_set_key_flags;
> 
> -static char * const evm_hmac = "hmac(sha1)";
> -static char * const evm_hash = "sha1";
> -
>  /**
>   * evm_set_key() - set EVM HMAC key from the kernel
>   * @key: pointer to a buffer with the key data
> @@ -74,10 +71,10 @@ int evm_set_key(void *key, size_t keylen)
>  }
>  EXPORT_SYMBOL_GPL(evm_set_key);
> 
> -static struct shash_desc *init_desc(char type)
> +static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>  {
>  	long rc;
> -	char *algo;
> +	const char *algo;
>  	struct crypto_shash **tfm;
>  	struct shash_desc *desc;
> 
> @@ -86,13 +83,11 @@ static struct shash_desc *init_desc(char type)
>  			pr_err_once("HMAC key is not set\n");
>  			return ERR_PTR(-ENOKEY);
>  		}
> -		tfm = &hmac_tfm;
> -		algo = evm_hmac;
> -	} else {
> -		tfm = &hash_tfm;
> -		algo = evm_hash;
>  	}
> 
> +	tfm = &evm_tfm[hash_algo];
> +	algo = hash_algo_name[hash_algo];
> +
>  	if (*tfm == NULL) {
>  		mutex_lock(&mutex);
>  		if (*tfm)
> @@ -186,10 +181,10 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>   * each xattr, but attempt to re-use the previously allocated memory.
>   */
>  static int evm_calc_hmac_or_hash(struct dentry *dentry,
> -				const char *req_xattr_name,
> -				const char *req_xattr_value,
> -				size_t req_xattr_value_len,
> -				char type, char *digest)
> +				 const char *req_xattr_name,
> +				 const char *req_xattr_value,
> +				 size_t req_xattr_value_len,
> +				 uint8_t type, struct evm_digest *data)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
>  	struct shash_desc *desc;
> @@ -203,10 +198,17 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  	if (!(inode->i_opflags & IOP_XATTR))
>  		return -EOPNOTSUPP;
> 
> -	desc = init_desc(type);
> +	desc = init_desc(type, data->hdr.algo);
>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);
> 
> +	/*
> +	 * HMACs are always SHA1, but signatures may be of varying sizes -
> +	 * make sure the caller knows how long the returned data is
> +	 */
> +	if (type != EVM_XATTR_HMAC)
> +		data->hdr.length = crypto_shash_digestsize(desc->tfm);
> +
>  	error = -ENODATA;
>  	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
>  		bool is_ima = false;
> @@ -238,7 +240,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  		if (is_ima)
>  			ima_present = true;
>  	}
> -	hmac_add_misc(desc, inode, type, digest);
> +	hmac_add_misc(desc, inode, type, data->digest);
> 
>  	/* Portable EVM signatures must include an IMA hash */
>  	if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
> @@ -251,18 +253,18 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> 
>  int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value, size_t req_xattr_value_len,
> -		  char *digest)
> +		  struct evm_digest *data)
>  {
>  	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> -			       req_xattr_value_len, EVM_XATTR_HMAC, digest);
> +		    req_xattr_value_len, EVM_XATTR_HMAC, data);
>  }
> 
>  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value, size_t req_xattr_value_len,
> -		  char type, char *digest)
> +		  char type, struct evm_digest *data)
>  {
>  	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> -				     req_xattr_value_len, type, digest);
> +		      req_xattr_value_len, type, data);
>  }
> 
>  static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
> @@ -302,7 +304,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  			const char *xattr_value, size_t xattr_value_len)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
> -	struct evm_ima_xattr_data xattr_data;
> +	struct evm_digest data;
>  	int rc = 0;
> 
>  	/*
> @@ -315,13 +317,14 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  	if (rc)
>  		return -EPERM;
> 
> +	data.hdr.algo = HASH_ALGO_SHA1;
>  	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -			   xattr_value_len, xattr_data.digest);
> +			   xattr_value_len, &data);
>  	if (rc == 0) {
> -		xattr_data.type = EVM_XATTR_HMAC;
> +		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>  		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> -					   &xattr_data,
> -					   sizeof(xattr_data), 0);
> +					   &data.hdr.xattr.data[1],
> +					   SHA1_DIGEST_SIZE + 1, 0);
>  	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>  		rc = __vfs_removexattr(dentry, XATTR_NAME_EVM);
>  	}
> @@ -333,7 +336,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
>  {
>  	struct shash_desc *desc;
> 
> -	desc = init_desc(EVM_XATTR_HMAC);
> +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
>  	if (IS_ERR(desc)) {
>  		pr_info("init_desc failed\n");
>  		return PTR_ERR(desc);
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 9ea9c19a545c..88ea9c1962d6 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -25,6 +25,7 @@
>  #include <linux/magic.h>
> 
>  #include <crypto/hash.h>
> +#include <crypto/hash_info.h>
>  #include <crypto/algapi.h>
>  #include "evm.h"
> 
> @@ -122,8 +123,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  					     struct integrity_iint_cache *iint)
>  {
>  	struct evm_ima_xattr_data *xattr_data = NULL;
> -	struct evm_ima_xattr_data calc;
> +	struct signature_v2_hdr *hdr;
>  	enum integrity_status evm_status = INTEGRITY_PASS;
> +	struct evm_digest digest;
>  	struct inode *inode;
>  	int rc, xattr_len;
> 
> @@ -159,25 +161,28 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  			evm_status = INTEGRITY_FAIL;
>  			goto out;
>  		}
> +
> +		digest.hdr.algo = HASH_ALGO_SHA1;
>  		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, calc.digest);
> +				   xattr_value_len, &digest);
>  		if (rc)
>  			break;
> -		rc = crypto_memneq(xattr_data->digest, calc.digest,
> -			    sizeof(calc.digest));
> +		rc = crypto_memneq(xattr_data->digest, digest.digest,
> +				   SHA1_DIGEST_SIZE);
>  		if (rc)
>  			rc = -EINVAL;
>  		break;
>  	case EVM_IMA_XATTR_DIGSIG:
>  	case EVM_XATTR_PORTABLE_DIGSIG:
> +		hdr = (struct signature_v2_hdr *)xattr_data;
> +		digest.hdr.algo = hdr->hash_algo;
>  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, xattr_data->type,
> -				   calc.digest);
> +				   xattr_value_len, xattr_data->type, &digest);
>  		if (rc)
>  			break;
>  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
>  					(const char *)xattr_data, xattr_len,
> -					calc.digest, sizeof(calc.digest));
> +					digest.digest, digest.hdr.length);
>  		if (!rc) {
>  			inode = d_backing_inode(dentry);
> 

Re: [PATCH V4] evm: Allow non-SHA1 digital signatures

[Matthew Garrett]

Oh bother - I think I see what's wrong. Does this version work better?
I'm afraid I only tested against signatures rather than HMACs, and I was
generating a raw SHA1 rather than an HMAC :(

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index 45c4a89c02ff..a8289fbf2f0c 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -42,6 +42,11 @@ extern struct crypto_shash *hash_tfm;
 /* List of EVM protected security xattrs */
 extern char *evm_config_xattrnames[];
 
+struct evm_digest {
+	struct ima_digest_data hdr;
+	char digest[IMA_MAX_DIGEST_SIZE];
+} __packed;
+
 int evm_init_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
 			const char *req_xattr_name,
@@ -49,10 +54,11 @@ int evm_update_evmxattr(struct dentry *dentry,
 			size_t req_xattr_value_len);
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
-		  size_t req_xattr_value_len, char *digest);
+		  size_t req_xattr_value_len, struct evm_digest *data);
 int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
-		  size_t req_xattr_value_len, char type, char *digest);
+		  size_t req_xattr_value_len, char type,
+		  struct evm_digest *data);
 int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
 		  char *hmac_val);
 int evm_init_secfs(void);
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a46fba322340..eb87d40c62a5 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -21,6 +21,7 @@
 #include <linux/evm.h>
 #include <keys/encrypted-type.h>
 #include <crypto/hash.h>
+#include <crypto/hash_info.h>
 #include "evm.h"
 
 #define EVMKEY "evm-key"
@@ -29,7 +30,7 @@ static unsigned char evmkey[MAX_KEY_SIZE];
 static int evmkey_len = MAX_KEY_SIZE;
 
 struct crypto_shash *hmac_tfm;
-struct crypto_shash *hash_tfm;
+static struct crypto_shash *evm_tfm[HASH_ALGO__LAST];
 
 static DEFINE_MUTEX(mutex);
 
@@ -38,7 +39,6 @@ static DEFINE_MUTEX(mutex);
 static unsigned long evm_set_key_flags;
 
 static char * const evm_hmac = "hmac(sha1)";
-static char * const evm_hash = "sha1";
 
 /**
  * evm_set_key() - set EVM HMAC key from the kernel
@@ -74,10 +74,10 @@ int evm_set_key(void *key, size_t keylen)
 }
 EXPORT_SYMBOL_GPL(evm_set_key);
 
-static struct shash_desc *init_desc(char type)
+static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 {
 	long rc;
-	char *algo;
+	const char *algo;
 	struct crypto_shash **tfm;
 	struct shash_desc *desc;
 
@@ -89,8 +89,8 @@ static struct shash_desc *init_desc(char type)
 		tfm = &hmac_tfm;
 		algo = evm_hmac;
 	} else {
-		tfm = &hash_tfm;
-		algo = evm_hash;
+		tfm = &evm_tfm[hash_algo];
+		algo = hash_algo_name[hash_algo];
 	}
 
 	if (*tfm == NULL) {
@@ -186,10 +186,10 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
  * each xattr, but attempt to re-use the previously allocated memory.
  */
 static int evm_calc_hmac_or_hash(struct dentry *dentry,
-				const char *req_xattr_name,
-				const char *req_xattr_value,
-				size_t req_xattr_value_len,
-				char type, char *digest)
+				 const char *req_xattr_name,
+				 const char *req_xattr_value,
+				 size_t req_xattr_value_len,
+				 uint8_t type, struct evm_digest *data)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	struct shash_desc *desc;
@@ -203,10 +203,12 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 	if (!(inode->i_opflags & IOP_XATTR))
 		return -EOPNOTSUPP;
 
-	desc = init_desc(type);
+	desc = init_desc(type, data->hdr.algo);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
+	data->hdr.length = crypto_shash_digestsize(desc->tfm);
+
 	error = -ENODATA;
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		bool is_ima = false;
@@ -238,7 +240,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		if (is_ima)
 			ima_present = true;
 	}
-	hmac_add_misc(desc, inode, type, digest);
+	hmac_add_misc(desc, inode, type, data->digest);
 
 	/* Portable EVM signatures must include an IMA hash */
 	if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
@@ -251,18 +253,18 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value, size_t req_xattr_value_len,
-		  char *digest)
+		  struct evm_digest *data)
 {
 	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
-			       req_xattr_value_len, EVM_XATTR_HMAC, digest);
+				    req_xattr_value_len, EVM_XATTR_HMAC, data);
 }
 
 int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value, size_t req_xattr_value_len,
-		  char type, char *digest)
+		  char type, struct evm_digest *data)
 {
 	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
-				     req_xattr_value_len, type, digest);
+				     req_xattr_value_len, type, data);
 }
 
 static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
@@ -302,7 +304,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct evm_ima_xattr_data xattr_data;
+	struct evm_digest data;
 	int rc = 0;
 
 	/*
@@ -315,13 +317,14 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 	if (rc)
 		return -EPERM;
 
+	data.hdr.algo = HASH_ALGO_SHA1;
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
-			   xattr_value_len, xattr_data.digest);
+			   xattr_value_len, &data);
 	if (rc == 0) {
-		xattr_data.type = EVM_XATTR_HMAC;
+		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
-					   &xattr_data,
-					   sizeof(xattr_data), 0);
+					   &data.hdr.xattr.data[1],
+					   SHA1_DIGEST_SIZE + 1, 0);
 	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
 		rc = __vfs_removexattr(dentry, XATTR_NAME_EVM);
 	}
@@ -333,7 +336,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 {
 	struct shash_desc *desc;
 
-	desc = init_desc(EVM_XATTR_HMAC);
+	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
 	if (IS_ERR(desc)) {
 		pr_info("init_desc failed\n");
 		return PTR_ERR(desc);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 9ea9c19a545c..88ea9c1962d6 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -25,6 +25,7 @@
 #include <linux/magic.h>
 
 #include <crypto/hash.h>
+#include <crypto/hash_info.h>
 #include <crypto/algapi.h>
 #include "evm.h"
 
@@ -122,8 +123,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 					     struct integrity_iint_cache *iint)
 {
 	struct evm_ima_xattr_data *xattr_data = NULL;
-	struct evm_ima_xattr_data calc;
+	struct signature_v2_hdr *hdr;
 	enum integrity_status evm_status = INTEGRITY_PASS;
+	struct evm_digest digest;
 	struct inode *inode;
 	int rc, xattr_len;
 
@@ -159,25 +161,28 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 			evm_status = INTEGRITY_FAIL;
 			goto out;
 		}
+
+		digest.hdr.algo = HASH_ALGO_SHA1;
 		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
-				   xattr_value_len, calc.digest);
+				   xattr_value_len, &digest);
 		if (rc)
 			break;
-		rc = crypto_memneq(xattr_data->digest, calc.digest,
-			    sizeof(calc.digest));
+		rc = crypto_memneq(xattr_data->digest, digest.digest,
+				   SHA1_DIGEST_SIZE);
 		if (rc)
 			rc = -EINVAL;
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
 	case EVM_XATTR_PORTABLE_DIGSIG:
+		hdr = (struct signature_v2_hdr *)xattr_data;
+		digest.hdr.algo = hdr->hash_algo;
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
-				   xattr_value_len, xattr_data->type,
-				   calc.digest);
+				   xattr_value_len, xattr_data->type, &digest);
 		if (rc)
 			break;
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
 					(const char *)xattr_data, xattr_len,
-					calc.digest, sizeof(calc.digest));
+					digest.digest, digest.hdr.length);
 		if (!rc) {
 			inode = d_backing_inode(dentry);
 
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH V4] evm: Allow non-SHA1 digital signatures

[Mimi Zohar]

On Thu, 2018-05-17 at 15:09 -0700, Matthew Garrett wrote:
> Oh bother - I think I see what's wrong. Does this version work better?
> I'm afraid I only tested against signatures rather than HMACs, and I was
> generating a raw SHA1 rather than an HMAC :(

That's a lot better!

FYI, Wang Junwen reported a problem with enabling EVM with just the
immutable and portable keys.  Without trusted keys enabled, SHA1 isn't
being built into the kernel.  Loading the SHA1 kernel module fails.
 Without knowing apriori which hash algorithms need to be builtin is a
problem.

Mimi

> 
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 45c4a89c02ff..a8289fbf2f0c 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -42,6 +42,11 @@ extern struct crypto_shash *hash_tfm;
>  /* List of EVM protected security xattrs */
>  extern char *evm_config_xattrnames[];
> 
> +struct evm_digest {
> +	struct ima_digest_data hdr;
> +	char digest[IMA_MAX_DIGEST_SIZE];
> +} __packed;
> +
>  int evm_init_key(void);
>  int evm_update_evmxattr(struct dentry *dentry,
>  			const char *req_xattr_name,
> @@ -49,10 +54,11 @@ int evm_update_evmxattr(struct dentry *dentry,
>  			size_t req_xattr_value_len);
>  int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value,
> -		  size_t req_xattr_value_len, char *digest);
> +		  size_t req_xattr_value_len, struct evm_digest *data);
>  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value,
> -		  size_t req_xattr_value_len, char type, char *digest);
> +		  size_t req_xattr_value_len, char type,
> +		  struct evm_digest *data);
>  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>  		  char *hmac_val);
>  int evm_init_secfs(void);
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a46fba322340..eb87d40c62a5 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -21,6 +21,7 @@
>  #include <linux/evm.h>
>  #include <keys/encrypted-type.h>
>  #include <crypto/hash.h>
> +#include <crypto/hash_info.h>
>  #include "evm.h"
> 
>  #define EVMKEY "evm-key"
> @@ -29,7 +30,7 @@ static unsigned char evmkey[MAX_KEY_SIZE];
>  static int evmkey_len = MAX_KEY_SIZE;
> 
>  struct crypto_shash *hmac_tfm;
> -struct crypto_shash *hash_tfm;
> +static struct crypto_shash *evm_tfm[HASH_ALGO__LAST];
> 
>  static DEFINE_MUTEX(mutex);
> 
> @@ -38,7 +39,6 @@ static DEFINE_MUTEX(mutex);
>  static unsigned long evm_set_key_flags;
> 
>  static char * const evm_hmac = "hmac(sha1)";
> -static char * const evm_hash = "sha1";
> 
>  /**
>   * evm_set_key() - set EVM HMAC key from the kernel
> @@ -74,10 +74,10 @@ int evm_set_key(void *key, size_t keylen)
>  }
>  EXPORT_SYMBOL_GPL(evm_set_key);
> 
> -static struct shash_desc *init_desc(char type)
> +static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>  {
>  	long rc;
> -	char *algo;
> +	const char *algo;
>  	struct crypto_shash **tfm;
>  	struct shash_desc *desc;
> 
> @@ -89,8 +89,8 @@ static struct shash_desc *init_desc(char type)
>  		tfm = &hmac_tfm;
>  		algo = evm_hmac;
>  	} else {
> -		tfm = &hash_tfm;
> -		algo = evm_hash;
> +		tfm = &evm_tfm[hash_algo];
> +		algo = hash_algo_name[hash_algo];
>  	}
> 
>  	if (*tfm == NULL) {
> @@ -186,10 +186,10 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>   * each xattr, but attempt to re-use the previously allocated memory.
>   */
>  static int evm_calc_hmac_or_hash(struct dentry *dentry,
> -				const char *req_xattr_name,
> -				const char *req_xattr_value,
> -				size_t req_xattr_value_len,
> -				char type, char *digest)
> +				 const char *req_xattr_name,
> +				 const char *req_xattr_value,
> +				 size_t req_xattr_value_len,
> +				 uint8_t type, struct evm_digest *data)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
>  	struct shash_desc *desc;
> @@ -203,10 +203,12 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  	if (!(inode->i_opflags & IOP_XATTR))
>  		return -EOPNOTSUPP;
> 
> -	desc = init_desc(type);
> +	desc = init_desc(type, data->hdr.algo);
>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);
> 
> +	data->hdr.length = crypto_shash_digestsize(desc->tfm);
> +
>  	error = -ENODATA;
>  	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
>  		bool is_ima = false;
> @@ -238,7 +240,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  		if (is_ima)
>  			ima_present = true;
>  	}
> -	hmac_add_misc(desc, inode, type, digest);
> +	hmac_add_misc(desc, inode, type, data->digest);
> 
>  	/* Portable EVM signatures must include an IMA hash */
>  	if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
> @@ -251,18 +253,18 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> 
>  int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value, size_t req_xattr_value_len,
> -		  char *digest)
> +		  struct evm_digest *data)
>  {
>  	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> -			       req_xattr_value_len, EVM_XATTR_HMAC, digest);
> +				    req_xattr_value_len, EVM_XATTR_HMAC, data);
>  }
> 
>  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value, size_t req_xattr_value_len,
> -		  char type, char *digest)
> +		  char type, struct evm_digest *data)
>  {
>  	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> -				     req_xattr_value_len, type, digest);
> +				     req_xattr_value_len, type, data);
>  }
> 
>  static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
> @@ -302,7 +304,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  			const char *xattr_value, size_t xattr_value_len)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
> -	struct evm_ima_xattr_data xattr_data;
> +	struct evm_digest data;
>  	int rc = 0;
> 
>  	/*
> @@ -315,13 +317,14 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  	if (rc)
>  		return -EPERM;
> 
> +	data.hdr.algo = HASH_ALGO_SHA1;
>  	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -			   xattr_value_len, xattr_data.digest);
> +			   xattr_value_len, &data);
>  	if (rc == 0) {
> -		xattr_data.type = EVM_XATTR_HMAC;
> +		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>  		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> -					   &xattr_data,
> -					   sizeof(xattr_data), 0);
> +					   &data.hdr.xattr.data[1],
> +					   SHA1_DIGEST_SIZE + 1, 0);
>  	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>  		rc = __vfs_removexattr(dentry, XATTR_NAME_EVM);
>  	}
> @@ -333,7 +336,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
>  {
>  	struct shash_desc *desc;
> 
> -	desc = init_desc(EVM_XATTR_HMAC);
> +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
>  	if (IS_ERR(desc)) {
>  		pr_info("init_desc failed\n");
>  		return PTR_ERR(desc);
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 9ea9c19a545c..88ea9c1962d6 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -25,6 +25,7 @@
>  #include <linux/magic.h>
> 
>  #include <crypto/hash.h>
> +#include <crypto/hash_info.h>
>  #include <crypto/algapi.h>
>  #include "evm.h"
> 
> @@ -122,8 +123,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  					     struct integrity_iint_cache *iint)
>  {
>  	struct evm_ima_xattr_data *xattr_data = NULL;
> -	struct evm_ima_xattr_data calc;
> +	struct signature_v2_hdr *hdr;
>  	enum integrity_status evm_status = INTEGRITY_PASS;
> +	struct evm_digest digest;
>  	struct inode *inode;
>  	int rc, xattr_len;
> 
> @@ -159,25 +161,28 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  			evm_status = INTEGRITY_FAIL;
>  			goto out;
>  		}
> +
> +		digest.hdr.algo = HASH_ALGO_SHA1;
>  		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, calc.digest);
> +				   xattr_value_len, &digest);
>  		if (rc)
>  			break;
> -		rc = crypto_memneq(xattr_data->digest, calc.digest,
> -			    sizeof(calc.digest));
> +		rc = crypto_memneq(xattr_data->digest, digest.digest,
> +				   SHA1_DIGEST_SIZE);
>  		if (rc)
>  			rc = -EINVAL;
>  		break;
>  	case EVM_IMA_XATTR_DIGSIG:
>  	case EVM_XATTR_PORTABLE_DIGSIG:
> +		hdr = (struct signature_v2_hdr *)xattr_data;
> +		digest.hdr.algo = hdr->hash_algo;
>  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, xattr_data->type,
> -				   calc.digest);
> +				   xattr_value_len, xattr_data->type, &digest);
>  		if (rc)
>  			break;
>  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
>  					(const char *)xattr_data, xattr_len,
> -					calc.digest, sizeof(calc.digest));
> +					digest.digest, digest.hdr.length);
>  		if (!rc) {
>  			inode = d_backing_inode(dentry);
> 

Re: [PATCH V4] evm: Allow non-SHA1 digital signatures

[Matthew Garrett]

On Fri, May 18, 2018 at 9:03 AM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Thu, 2018-05-17 at 15:09 -0700, Matthew Garrett wrote:
> > Oh bother - I think I see what's wrong. Does this version work better?
> > I'm afraid I only tested against signatures rather than HMACs, and I was
> > generating a raw SHA1 rather than an HMAC :(

> That's a lot better!

> FYI, Wang Junwen reported a problem with enabling EVM with just the
> immutable and portable keys.  Without trusted keys enabled, SHA1 isn't
> being built into the kernel.  Loading the SHA1 kernel module fails.
>   Without knowing apriori which hash algorithms need to be builtin is a
> problem.

It looks like Kconfig is selecting CRYPTO_SHA1 when EVM is enabled, and
since that's a bool it should be forcing it to be built-in? I can't see a
good way of extending that generally, unfortunately. Is the problem with
loading the module that you're enforcing an IMA policy before loading it?

Re: [PATCH V4] evm: Allow non-SHA1 digital signatures

[Matthew Garrett]

On Tue, May 29, 2018 at 11:25 PM Wang,Junwen <wangjunwen@baidu.com> wrote:
> if we need fix this problem
> 1. load the hash algorithm at initial time instead of runtime
> OR
> 2. avoid the crypto_alloc_shash try to load modules in init_desc

The outcome here is presumably going to be failure regardless - if
appraisal is required and the hash module is unavailable, failing to load
the module won't result in deadlock but will result in an unusable machine?
I think the only way this can work is to ensure the crypto modules are
available before a policy is enabled, but let me look to see if there's a
way to at least make the failure clean and more debuggable.

[PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Matthew Garrett]

Does this help?

Trying to instantiate a non-existent crypto algorithm will cause the
kernel to trigger a module load. If EVM appraisal is enabled, this will
in turn trigger appraisal of the module, which will fail because the
crypto algorithm isn't available. Add a CRYPTO_NOLOAD flag and skip
module loading if it's set, and add that flag in the EVM case.
---
 crypto/api.c                        | 2 +-
 include/linux/crypto.h              | 5 +++++
 security/integrity/evm/evm_crypto.c | 3 ++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/crypto/api.c b/crypto/api.c
index 0ee632bba064..7aca9f86c5f3 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -229,7 +229,7 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 	mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
 
 	alg = crypto_alg_lookup(name, type, mask);
-	if (!alg) {
+	if (!alg && !(mask & CRYPTO_NOLOAD)) {
 		request_module("crypto-%s", name);
 
 		if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 6eb06101089f..e8839d3a7559 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -112,6 +112,11 @@
  */
 #define CRYPTO_ALG_OPTIONAL_KEY		0x00004000
 
+/*
+ * Don't trigger module loading
+ */
+#define CRYPTO_NOLOAD			0x00008000
+
 /*
  * Transform masks and values (for crt_flags).
  */
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index eb87d40c62a5..ff8687232a1a 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -97,7 +97,8 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		mutex_lock(&mutex);
 		if (*tfm)
 			goto out;
-		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_ALG_ASYNC);
+		*tfm = crypto_alloc_shash(algo, 0,
+					  CRYPTO_ALG_ASYNC | CRYPTO_NOLOAD);
 		if (IS_ERR(*tfm)) {
 			rc = PTR_ERR(*tfm);
 			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
-- 
2.17.1.1185.g55be947832-goog

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Matthew Garrett]

Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?
If so I'll resend with a s-o-b
On Wed, May 30, 2018 at 8:14 PM Wang,Junwen <wangjunwen@baidu.com> wrote:
>
> this pretty good, now ima_appraise & evm working well on my machine
> thank you, Matthew :)
> ________________________________________
> ???: Matthew Garrett [mjg59@google.com]
> ????: 2018?5?31? 4:28
> ???: linux-integrity@vger.kernel.org
> ??: zohar@linux.vnet.ibm.com; Matthew Garrett; Wang,Junwen
> ??: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable
>
> Does this help?
>
> Trying to instantiate a non-existent crypto algorithm will cause the
> kernel to trigger a module load. If EVM appraisal is enabled, this will
> in turn trigger appraisal of the module, which will fail because the
> crypto algorithm isn't available. Add a CRYPTO_NOLOAD flag and skip
> module loading if it's set, and add that flag in the EVM case.
> ---
>  crypto/api.c                        | 2 +-
>  include/linux/crypto.h              | 5 +++++
>  security/integrity/evm/evm_crypto.c | 3 ++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/api.c b/crypto/api.c
> index 0ee632bba064..7aca9f86c5f3 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -229,7 +229,7 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
>         mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
>
>         alg = crypto_alg_lookup(name, type, mask);
> -       if (!alg) {
> +       if (!alg && !(mask & CRYPTO_NOLOAD)) {
>                 request_module("crypto-%s", name);
>
>                 if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 6eb06101089f..e8839d3a7559 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -112,6 +112,11 @@
>   */
>  #define CRYPTO_ALG_OPTIONAL_KEY                0x00004000
>
> +/*
> + * Don't trigger module loading
> + */
> +#define CRYPTO_NOLOAD                  0x00008000
> +
>  /*
>   * Transform masks and values (for crt_flags).
>   */
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index eb87d40c62a5..ff8687232a1a 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -97,7 +97,8 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>                 mutex_lock(&mutex);
>                 if (*tfm)
>                         goto out;
> -               *tfm = crypto_alloc_shash(algo, 0, CRYPTO_ALG_ASYNC);
> +               *tfm = crypto_alloc_shash(algo, 0,
> +                                         CRYPTO_ALG_ASYNC | CRYPTO_NOLOAD);
>                 if (IS_ERR(*tfm)) {
>                         rc = PTR_ERR(*tfm);
>                         pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
> --
> 2.17.1.1185.g55be947832-goog
>

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Mimi Zohar]

On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote:
> Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?

This should be posted as a separate patch, with the appropriate
"Fixes" commit info.  It requires an ack from Herbert Xu.

thanks,

Mimi

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Matthew Garrett]

On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote:
> > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?
>
> This should be posted as a separate patch, with the appropriate
> "Fixes" commit info.  It requires an ack from Herbert Xu.

The non-sha1 patch doesn't seem to be in -next at the moment - should
I wait for you to merge that so I can add the fixes tag?

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Mimi Zohar]

On Thu, 2018-05-31 at 13:07 -0700, Matthew Garrett wrote:
> On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote:
> > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?
> >
> > This should be posted as a separate patch, with the appropriate
> > "Fixes" commit info.  It requires an ack from Herbert Xu.
> 
> The non-sha1 patch doesn't seem to be in -next at the moment - should
> I wait for you to merge that so I can add the fixes tag?

The problem exists prior to the non-sha1 patch, but could have been
resolved differently for only sha1 (eg. Kconfig requiring sha1).  The
non-sha1 patch requires a different solution.

Junwen, is this a regression?  Did a particular patch introduce this
problem?

Mimi

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Matthew Garrett]

On Thu, May 31, 2018 at 1:32 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> On Thu, 2018-05-31 at 13:07 -0700, Matthew Garrett wrote:
> > On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > >
> > > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote:
> > > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?
> > >
> > > This should be posted as a separate patch, with the appropriate
> > > "Fixes" commit info.  It requires an ack from Herbert Xu.
> >
> > The non-sha1 patch doesn't seem to be in -next at the moment - should
> > I wait for you to merge that so I can add the fixes tag?
>
> The problem exists prior to the non-sha1 patch, but could have been
> resolved differently for only sha1 (eg. Kconfig requiring sha1).  The
> non-sha1 patch requires a different solution.

EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok
before that patch?

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Mimi Zohar]

On Thu, 2018-05-31 at 14:06 -0700, Matthew Garrett wrote:
> On Thu, May 31, 2018 at 1:32 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > On Thu, 2018-05-31 at 13:07 -0700, Matthew Garrett wrote:
> > > On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > >
> > > > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote:
> > > > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?
> > > >
> > > > This should be posted as a separate patch, with the appropriate
> > > > "Fixes" commit info.  It requires an ack from Herbert Xu.
> > >
> > > The non-sha1 patch doesn't seem to be in -next at the moment - should
> > > I wait for you to merge that so I can add the fixes tag?
> >
> > The problem exists prior to the non-sha1 patch, but could have been
> > resolved differently for only sha1 (eg. Kconfig requiring sha1).  The
> > non-sha1 patch requires a different solution.
> 
> EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok
> before that patch?

According to Junwen, with CONFIG_TRUSTED_KEYS enabled the HMAC and
SHA1 are allocated at __init.  The locking problem occurs when
CONFIG_TRUSTED_KEYS is not enabled.  His solution would have been to
move the crypto_alloc_shash() in EVM to an __init function.

Mimi

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Matthew Garrett]

On Fri, Jun 1, 2018 at 4:21 AM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Thu, 2018-05-31 at 14:06 -0700, Matthew Garrett wrote:
> > EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok
> > before that patch?
>
> According to Junwen, with CONFIG_TRUSTED_KEYS enabled the HMAC and
> SHA1 are allocated at __init.  The locking problem occurs when
> CONFIG_TRUSTED_KEYS is not enabled.  His solution would have been to
> move the crypto_alloc_shash() in EVM to an __init function.

Ok - I think just allowing it to be deferred is preferable, since
otherwise we'd have to build in every hash algorithm that could be
used for the signatures (which wasn't a problem before the non-sha1
patch). How would you prefer me to send these two? The non-sha1 patch
isn't in -next, so I can't add a fixes: for it at this point.

Re: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable

[Mimi Zohar]

On Fri, 2018-06-01 at 13:52 -0700, Matthew Garrett wrote:
> On Fri, Jun 1, 2018 at 4:21 AM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Thu, 2018-05-31 at 14:06 -0700, Matthew Garrett wrote:
> > > EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok
> > > before that patch?
> >
> > According to Junwen, with CONFIG_TRUSTED_KEYS enabled the HMAC and
> > SHA1 are allocated at __init.  The locking problem occurs when
> > CONFIG_TRUSTED_KEYS is not enabled.  His solution would have been to
> > move the crypto_alloc_shash() in EVM to an __init function.
> 
> Ok - I think just allowing it to be deferred is preferable, since
> otherwise we'd have to build in every hash algorithm that could be
> used for the signatures (which wasn't a problem before the non-sha1
> patch). How would you prefer me to send these two? The non-sha1 patch
> isn't in -next, so I can't add a fixes: for it at this point.

Switch the order of the two patches.  The bug fix goes first, then the
non-sha1 patch.  Remember we need an Ack from Herbert Xu for
introducing CRYPTO_NOLOAD.

Mimi