[PATCH 0/8] ima: support stronger algorithms for attestation

[PATCH 0/8] ima: support stronger algorithms for attestation

[Roberto Sassu]

IMA extends Platform Configuration Registers (PCRs) of the TPM to give a
proof to a remote verifier that the measurement list contains all
measurements done by the kernel and that the list was not maliciously
modified by an attacker.

IMA was originally designed to extend PCRs with a SHA1 digest, provided
with the measurement list, and was subsequently updated to extend all PCR
banks in case a TPM 2.0 is used. Non-SHA1 PCR banks are not supposed to be
used for remote attestation, as they are extended with a SHA1 digest padded
with zeros, which does not increase the strength.

This patch set addresses this issue by extending PCRs with the digest of
the measurement entry calculated with the crypto subsystem. The list of
algorithms used to calculate the digest are taken from
ima_tpm_chip->allocated_banks, returned by the TPM driver. The SHA1 digest
is always calculated, as SHA1 still remains the default algorithm for the
template digest in the measurement list.

This patch set also makes two additional modifications related to the usage
of hash algorithms. First, since now the template digest for the default
IMA algorithm is always calculated, this is used for hash collision
detection, to check if there are duplicate measurement entries.

Second, it uses the default IMA hash algorithm to calculate the boot
aggregate, assuming that the corresponding PCR bank is currently allocated.
Otherwise, it finds the first PCR bank for which the crypto ID is known.
IMA initialization fails only if no algorithm known to the crypto subsystem
is found.

This patch set does not yet modify the format of the measurement list to
provide the digests passed to the TPM. However, reconstructing the value of
the quoted PCR is still possible for the verifier by calculating the digest
on measurement data found in binary_runtime_measurements.

The attest-tools library [1] has been updated to verify non-SHA1 PCR
banks [2] and to handle non-SHA1 boot aggregate [3].

[1] https://github.com/euleros/attest-tools/tree/0.2-devel
[2] https://github.com/euleros/attest-tools/commit/282a0b1a5e6d9c87adf21561018528d7bbdc7f38
[3] https://github.com/euleros/attest-tools/commit/3a4c8e250fde7661257aba022d677bf0af5399da

Roberto Sassu (8):
  tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  ima: evaluate error in init_ima()
  ima: store template digest directly in ima_template_entry
  ima: switch to dynamically allocated buffer for template digests
  ima: allocate and initialize tfm for each PCR bank
  ima: calculate and extend PCR with digests in ima_template_entry
  ima: use ima_hash_algo for collision detection in the measurement list
  ima: switch to ima_hash_algo for boot aggregate

 drivers/char/tpm/tpm2-cmd.c           |   2 +
 security/integrity/ima/ima.h          |   7 +-
 security/integrity/ima/ima_api.c      |  20 ++-
 security/integrity/ima/ima_crypto.c   | 219 ++++++++++++++++++++------
 security/integrity/ima/ima_fs.c       |   4 +-
 security/integrity/ima/ima_init.c     |   6 +-
 security/integrity/ima/ima_main.c     |   6 +
 security/integrity/ima/ima_queue.c    |  36 +++--
 security/integrity/ima/ima_template.c |  22 ++-
 9 files changed, 241 insertions(+), 81 deletions(-)

-- 
2.17.1

[PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST

[Roberto Sassu]

chip->allocated_banks contains the list of TPM algorithm IDs of allocated
PCR banks. It also contains the corresponding ID of the crypto subsystem,
so that users of the TPM driver can calculate a digest for a PCR extend
operation.

However, if there is no mapping between TPM algorithm ID and crypto ID, the
crypto_id field in chip->allocated_banks remains set to zero (the array is
allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
Zero should not be used as value for unknown mappings, as it is a valid
crypto ID (HASH_ALGO_MD4).

This patch initializes crypto_id to HASH_ALGO__LAST.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 drivers/char/tpm/tpm2-cmd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 13696deceae8..760329598b99 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -525,6 +525,8 @@ static int tpm2_init_bank_info(struct tpm_chip *chip, u32 bank_index)
 		return 0;
 	}
 
+	bank->crypto_id = HASH_ALGO__LAST;
+
 	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
 }
 
-- 
2.17.1

[PATCH 2/8] ima: evaluate error in init_ima()

[Roberto Sassu]

Evaluate error in init_ima() before register_blocking_lsm_notifier() and
return if not zero.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9fe949c6a530..2f95b133f246 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -791,6 +791,9 @@ static int __init init_ima(void)
 		error = ima_init();
 	}
 
+	if (error)
+		return error;
+
 	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
 	if (error)
 		pr_warn("Couldn't register LSM notifier, error %d\n", error);
-- 
2.17.1

[PATCH 3/8] ima: store template digest directly in ima_template_entry

[Roberto Sassu]

In preparation for the patch that calculates a digest for each allocated
PCR bank, this patch passes to ima_calc_field_array_hash() the
ima_template_entry structure, so that digests can be directly stored in
that structure instead of ima_digest_data.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h        |  3 +--
 security/integrity/ima/ima_api.c    | 12 +-----------
 security/integrity/ima/ima_crypto.c | 18 +++++++-----------
 3 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6bb3152b3e24..6238c50ae44e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -138,8 +138,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
 int ima_calc_buffer_hash(const void *buf, loff_t len,
 			 struct ima_digest_data *hash);
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
-			      struct ima_template_desc *desc, int num_fields,
-			      struct ima_digest_data *hash);
+			      struct ima_template_entry *entry);
 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash);
 void ima_add_violation(struct file *file, const unsigned char *filename,
 		       struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index f6bc00914aa5..2ef5a40c7ca5 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -96,26 +96,16 @@ int ima_store_template(struct ima_template_entry *entry,
 	static const char audit_cause[] = "hashing_error";
 	char *template_name = entry->template_desc->name;
 	int result;
-	struct {
-		struct ima_digest_data hdr;
-		char digest[TPM_DIGEST_SIZE];
-	} hash;
 
 	if (!violation) {
-		int num_fields = entry->template_desc->num_fields;
-
-		/* this function uses default algo */
-		hash.hdr.algo = HASH_ALGO_SHA1;
 		result = ima_calc_field_array_hash(&entry->template_data[0],
-						   entry->template_desc,
-						   num_fields, &hash.hdr);
+						   entry);
 		if (result < 0) {
 			integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
 					    template_name, op,
 					    audit_cause, result, 0);
 			return result;
 		}
-		memcpy(entry->digest, hash.hdr.digest, hash.hdr.length);
 	}
 	entry->pcr = pcr;
 	result = ima_add_template_entry(entry, violation, op, inode, filename);
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7967a6904851..f2d9d4b18499 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -466,18 +466,16 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
  * Calculate the hash of template data
  */
 static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
-					 struct ima_template_desc *td,
-					 int num_fields,
-					 struct ima_digest_data *hash,
+					 struct ima_template_entry *entry,
 					 struct crypto_shash *tfm)
 {
 	SHASH_DESC_ON_STACK(shash, tfm);
+	struct ima_template_desc *td = entry->template_desc;
+	int num_fields = entry->template_desc->num_fields;
 	int rc, i;
 
 	shash->tfm = tfm;
 
-	hash->length = crypto_shash_digestsize(tfm);
-
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;
@@ -506,24 +504,22 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 	}
 
 	if (!rc)
-		rc = crypto_shash_final(shash, hash->digest);
+		rc = crypto_shash_final(shash, entry->digest);
 
 	return rc;
 }
 
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
-			      struct ima_template_desc *desc, int num_fields,
-			      struct ima_digest_data *hash)
+			      struct ima_template_entry *entry)
 {
 	struct crypto_shash *tfm;
 	int rc;
 
-	tfm = ima_alloc_tfm(hash->algo);
+	tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 
-	rc = ima_calc_field_array_hash_tfm(field_data, desc, num_fields,
-					   hash, tfm);
+	rc = ima_calc_field_array_hash_tfm(field_data, entry, tfm);
 
 	ima_free_tfm(tfm);
 
-- 
2.17.1

[PATCH 4/8] ima: switch to dynamically allocated buffer for template digests

[Roberto Sassu]

This patch dynamically allocates the array of tpm_digest structures in
ima_alloc_init_template() and ima_restore_template_data(). It also
allocates an item in addition to the number of allocated PCR banks, to
store a SHA1 digest when the SHA1 PCR bank is not allocated.

Calculating the SHA1 digest is mandatory, as SHA1 still remains the default
hash algorithm for the measurement list. When IMA will support the Crypto
Agile format, remaining digests will be also provided.

The position in the array of the SHA1 digest is stored in the ima_sha1_idx
global variable and it is determined at IMA initialization time.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h          |  3 ++-
 security/integrity/ima/ima_api.c      |  8 ++++++++
 security/integrity/ima/ima_crypto.c   |  3 ++-
 security/integrity/ima/ima_fs.c       |  4 ++--
 security/integrity/ima/ima_main.c     |  2 ++
 security/integrity/ima/ima_queue.c    | 10 ++++++----
 security/integrity/ima/ima_template.c | 12 ++++++++++--
 7 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6238c50ae44e..d959dab5bcce 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -50,6 +50,7 @@ extern int ima_policy_flag;
 
 /* set during initialization */
 extern int ima_hash_algo;
+extern int ima_sha1_idx;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
 
@@ -92,7 +93,7 @@ struct ima_template_desc {
 
 struct ima_template_entry {
 	int pcr;
-	u8 digest[TPM_DIGEST_SIZE];	/* sha1 or md5 measurement hash */
+	struct tpm_digest *digests;
 	struct ima_template_desc *template_desc; /* template descriptor */
 	u32 template_data_len;
 	struct ima_field_data template_data[0];	/* template related data */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 2ef5a40c7ca5..ebaf0056735c 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -27,6 +27,7 @@ void ima_free_template_entry(struct ima_template_entry *entry)
 	for (i = 0; i < entry->template_desc->num_fields; i++)
 		kfree(entry->template_data[i].data);
 
+	kfree(entry->digests);
 	kfree(entry);
 }
 
@@ -50,6 +51,13 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
 	if (!*entry)
 		return -ENOMEM;
 
+	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+				    sizeof(*(*entry)->digests), GFP_NOFS);
+	if (!(*entry)->digests) {
+		result = -ENOMEM;
+		goto out;
+	}
+
 	(*entry)->template_desc = template_desc;
 	for (i = 0; i < template_desc->num_fields; i++) {
 		const struct ima_template_field *field =
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f2d9d4b18499..57b06321d5c8 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -504,7 +504,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 	}
 
 	if (!rc)
-		rc = crypto_shash_final(shash, entry->digest);
+		rc = crypto_shash_final(shash,
+					entry->digests[ima_sha1_idx].digest);
 
 	return rc;
 }
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 2000e8df0301..2b79581d0e25 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -152,7 +152,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
 	ima_putc(m, &pcr, sizeof(e->pcr));
 
 	/* 2nd: template digest */
-	ima_putc(m, e->digest, TPM_DIGEST_SIZE);
+	ima_putc(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
 
 	/* 3rd: template name size */
 	namelen = !ima_canonical_fmt ? strlen(template_name) :
@@ -235,7 +235,7 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
 	seq_printf(m, "%2d ", e->pcr);
 
 	/* 2nd: SHA1 template hash */
-	ima_print_digest(m, e->digest, TPM_DIGEST_SIZE);
+	ima_print_digest(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
 
 	/* 3th:  template name */
 	seq_printf(m, " %s", template_name);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2f95b133f246..c068067a0c47 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -30,6 +30,8 @@
 
 #include "ima.h"
 
+int ima_sha1_idx;
+
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise = IMA_APPRAISE_ENFORCE;
 #else
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..bcd99db9722c 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -57,7 +57,8 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 	key = ima_hash_key(digest_value);
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
-		rc = memcmp(qe->entry->digest, digest_value, TPM_DIGEST_SIZE);
+		rc = memcmp(qe->entry->digests[ima_sha1_idx].digest,
+			    digest_value, TPM_DIGEST_SIZE);
 		if ((rc == 0) && (qe->entry->pcr == pcr)) {
 			ret = qe;
 			break;
@@ -77,7 +78,7 @@ static int get_binary_runtime_size(struct ima_template_entry *entry)
 	int size = 0;
 
 	size += sizeof(u32);	/* pcr */
-	size += sizeof(entry->digest);
+	size += TPM_DIGEST_SIZE;
 	size += sizeof(int);	/* template name size field */
 	size += strlen(entry->template_desc->name);
 	size += sizeof(entry->template_data_len);
@@ -109,7 +110,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 
 	atomic_long_inc(&ima_htable.len);
 	if (update_htable) {
-		key = ima_hash_key(entry->digest);
+		key = ima_hash_key(entry->digests[ima_sha1_idx].digest);
 		hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
 	}
 
@@ -173,7 +174,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 
 	mutex_lock(&ima_extend_list_mutex);
 	if (!violation) {
-		memcpy(digest, entry->digest, sizeof(digest));
+		memcpy(digest, entry->digests[ima_sha1_idx].digest,
+		       sizeof(digest));
 		if (ima_lookup_digest_entry(digest, entry->pcr)) {
 			audit_cause = "hash_exists";
 			result = -EEXIST;
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 6aa6408603e3..2e9c5d99e70e 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -311,11 +311,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 	if (!*entry)
 		return -ENOMEM;
 
+	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+				    sizeof(*(*entry)->digests), GFP_NOFS);
+	if (!(*entry)->digests) {
+		kfree(*entry);
+		return -ENOMEM;
+	}
+
 	ret = ima_parse_buf(template_data, template_data + template_data_size,
 			    NULL, template_desc->num_fields,
 			    (*entry)->template_data, NULL, NULL,
 			    ENFORCE_FIELDS | ENFORCE_BUFEND, "template data");
 	if (ret < 0) {
+		kfree((*entry)->digests);
 		kfree(*entry);
 		return ret;
 	}
@@ -447,8 +455,8 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 		if (ret < 0)
 			break;
 
-		memcpy(entry->digest, hdr[HDR_DIGEST].data,
-		       hdr[HDR_DIGEST].len);
+		memcpy(entry->digests[ima_sha1_idx].digest,
+		       hdr[HDR_DIGEST].data, hdr[HDR_DIGEST].len);
 		entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
 			     le32_to_cpu(*(hdr[HDR_PCR].data));
 		ret = ima_restore_measurement_entry(entry);
-- 
2.17.1

[PATCH 5/8] ima: allocate and initialize tfm for each PCR bank

[Roberto Sassu]

This patch creates a crypto_shash structure for each allocated PCR bank and
for SHA1 if a bank with that algorithm is not currently allocated.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 137 +++++++++++++++++++++++-----
 1 file changed, 113 insertions(+), 24 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 57b06321d5c8..63fb4bdf80b0 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -59,7 +59,14 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
 static struct crypto_shash *ima_shash_tfm;
 static struct crypto_ahash *ima_ahash_tfm;
 
-int __init ima_init_crypto(void)
+struct ima_algo_desc {
+	struct crypto_shash *tfm;
+	enum hash_algo algo;
+};
+
+static struct ima_algo_desc *ima_algo_array;
+
+int __init ima_init_ima_crypto(void)
 {
 	long rc;
 
@@ -78,26 +85,116 @@ int __init ima_init_crypto(void)
 static struct crypto_shash *ima_alloc_tfm(enum hash_algo algo)
 {
 	struct crypto_shash *tfm = ima_shash_tfm;
-	int rc;
+	int rc, i;
 
 	if (algo < 0 || algo >= HASH_ALGO__LAST)
 		algo = ima_hash_algo;
 
-	if (algo != ima_hash_algo) {
-		tfm = crypto_alloc_shash(hash_algo_name[algo], 0, 0);
-		if (IS_ERR(tfm)) {
-			rc = PTR_ERR(tfm);
-			pr_err("Can not allocate %s (reason: %d)\n",
-			       hash_algo_name[algo], rc);
-		}
+	if (algo == ima_hash_algo)
+		return tfm;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+		if (ima_algo_array[i].algo == algo && ima_algo_array[i].tfm)
+			return ima_algo_array[i].tfm;
+
+	tfm = crypto_alloc_shash(hash_algo_name[algo], 0, 0);
+	if (IS_ERR(tfm)) {
+		rc = PTR_ERR(tfm);
+		pr_err("Can not allocate %s (reason: %d)\n",
+		       hash_algo_name[algo], rc);
 	}
 	return tfm;
 }
 
+int __init ima_init_crypto(void)
+{
+	enum hash_algo algo;
+	long rc;
+	int i;
+
+	rc = ima_init_ima_crypto();
+	if (rc)
+		return rc;
+
+	ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
+				       sizeof(*ima_algo_array), GFP_KERNEL);
+	if (!ima_algo_array) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+		ima_algo_array[i].tfm = NULL;
+		ima_algo_array[i].algo = HASH_ALGO__LAST;
+	}
+
+	ima_sha1_idx = -1;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
+		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
+		ima_algo_array[i].algo = algo;
+
+		/* unknown TPM algorithm */
+		if (algo == HASH_ALGO__LAST)
+			continue;
+
+		if (algo == HASH_ALGO_SHA1)
+			ima_sha1_idx = i;
+
+		if (algo == ima_hash_algo) {
+			ima_algo_array[i].tfm = ima_shash_tfm;
+			continue;
+		}
+
+		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
+		if (IS_ERR(ima_algo_array[i].tfm)) {
+			if (algo == HASH_ALGO_SHA1) {
+				rc = PTR_ERR(ima_algo_array[i].tfm);
+				ima_algo_array[i].tfm = NULL;
+				goto out_array;
+			}
+
+			ima_algo_array[i].tfm = NULL;
+		}
+	}
+
+	if (ima_sha1_idx < 0) {
+		ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
+		if (IS_ERR(ima_algo_array[i].tfm)) {
+			rc = PTR_ERR(ima_algo_array[i].tfm);
+			goto out_array;
+		}
+
+		ima_sha1_idx = i;
+		ima_algo_array[i].algo = HASH_ALGO_SHA1;
+	}
+
+	return 0;
+out_array:
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+		if (!ima_algo_array[i].tfm ||
+		    ima_algo_array[i].tfm == ima_shash_tfm)
+			continue;
+
+		crypto_free_shash(ima_algo_array[i].tfm);
+	}
+out:
+	crypto_free_shash(ima_shash_tfm);
+	return rc;
+}
+
 static void ima_free_tfm(struct crypto_shash *tfm)
 {
-	if (tfm != ima_shash_tfm)
-		crypto_free_shash(tfm);
+	int i;
+
+	if (tfm == ima_shash_tfm)
+		return;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+		if (ima_algo_array[i].tfm == tfm)
+			return;
+
+	crypto_free_shash(tfm);
 }
 
 /**
@@ -467,14 +564,14 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
  */
 static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 					 struct ima_template_entry *entry,
-					 struct crypto_shash *tfm)
+					 int tfm_idx)
 {
-	SHASH_DESC_ON_STACK(shash, tfm);
+	SHASH_DESC_ON_STACK(shash, ima_algo_array[tfm_idx].tfm);
 	struct ima_template_desc *td = entry->template_desc;
 	int num_fields = entry->template_desc->num_fields;
 	int rc, i;
 
-	shash->tfm = tfm;
+	shash->tfm = ima_algo_array[tfm_idx].tfm;
 
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
@@ -505,7 +602,7 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 
 	if (!rc)
 		rc = crypto_shash_final(shash,
-					entry->digests[ima_sha1_idx].digest);
+					entry->digests[tfm_idx].digest);
 
 	return rc;
 }
@@ -513,17 +610,9 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
 			      struct ima_template_entry *entry)
 {
-	struct crypto_shash *tfm;
 	int rc;
 
-	tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
-	if (IS_ERR(tfm))
-		return PTR_ERR(tfm);
-
-	rc = ima_calc_field_array_hash_tfm(field_data, entry, tfm);
-
-	ima_free_tfm(tfm);
-
+	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
 	return rc;
 }
 
-- 
2.17.1

[PATCH 6/8] ima: calculate and extend PCR with digests in ima_template_entry

[Roberto Sassu]

This patch modifies ima_calc_field_array_hash() to calculate a template
digest for each allocated PCR bank and SHA1. It also passes the tpm_digest
array of the template entry to ima_pcr_extend() or in case of a violation,
the pre-initialized digests array filled with 0xff.

Padding with zeros is still done if the mapping between TPM algorithm ID
and crypto ID is unknown.

This patch calculates again the template digest when a measurement list is
restored. Copying only the SHA1 digest (due to the limitation of the
current measurement list format) is not sufficient, as hash collision
detection will be done on the digest calculated with the default IMA hash
algorithm.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c   | 26 ++++++++++++++++++++++-
 security/integrity/ima/ima_queue.c    | 30 ++++++++++++++++-----------
 security/integrity/ima/ima_template.c | 14 +++++++++++--
 3 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 63fb4bdf80b0..786340feebbb 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -610,9 +610,33 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
 			      struct ima_template_entry *entry)
 {
-	int rc;
+	u16 alg_id;
+	int rc, i;
 
 	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
+	if (rc)
+		return rc;
+
+	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+		if (i == ima_sha1_idx)
+			continue;
+
+		alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+		entry->digests[i].alg_id = alg_id;
+
+		if (!ima_algo_array[i].tfm) {
+			memcpy(entry->digests[i].digest,
+			       entry->digests[ima_sha1_idx].digest,
+			       TPM_DIGEST_SIZE);
+			continue;
+		}
+
+		rc = ima_calc_field_array_hash_tfm(field_data, entry, i);
+		if (rc)
+			return rc;
+	}
 	return rc;
 }
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index bcd99db9722c..7f7509774b85 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -137,18 +137,14 @@ unsigned long ima_get_binary_runtime_size(void)
 		return binary_runtime_size + sizeof(struct ima_kexec_hdr);
 };
 
-static int ima_pcr_extend(const u8 *hash, int pcr)
+static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
 	int result = 0;
-	int i;
 
 	if (!ima_tpm_chip)
 		return result;
 
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
-		memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE);
-
-	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests);
+	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
 	if (result != 0)
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 	return result;
@@ -166,7 +162,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename)
 {
-	u8 digest[TPM_DIGEST_SIZE];
+	u8 *digest = entry->digests[ima_sha1_idx].digest;
+	struct tpm_digest *digests_arg = entry->digests;
 	const char *audit_cause = "hash_added";
 	char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX];
 	int audit_info = 1;
@@ -174,8 +171,6 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 
 	mutex_lock(&ima_extend_list_mutex);
 	if (!violation) {
-		memcpy(digest, entry->digests[ima_sha1_idx].digest,
-		       sizeof(digest));
 		if (ima_lookup_digest_entry(digest, entry->pcr)) {
 			audit_cause = "hash_exists";
 			result = -EEXIST;
@@ -191,9 +186,9 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 	}
 
 	if (violation)		/* invalidate pcr */
-		memset(digest, 0xff, sizeof(digest));
+		digests_arg = digests;
 
-	tpmresult = ima_pcr_extend(digest, entry->pcr);
+	tpmresult = ima_pcr_extend(digests_arg, entry->pcr);
 	if (tpmresult != 0) {
 		snprintf(tpm_audit_cause, AUDIT_CAUSE_LEN_MAX, "TPM_error(%d)",
 			 tpmresult);
@@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry)
 
 int __init ima_init_digests(void)
 {
+	u16 digest_size;
+	u16 crypto_id;
 	int i;
 
 	if (!ima_tpm_chip)
@@ -229,8 +226,17 @@ int __init ima_init_digests(void)
 	if (!digests)
 		return -ENOMEM;
 
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
 		digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+		digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
+		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
+
+		/* for unmapped TPM algorithms digest is still a padded SHA1 */
+		if (crypto_id == HASH_ALGO__LAST)
+			digest_size = SHA1_DIGEST_SIZE;
+
+		memset(digests[i].digest, 0xff, digest_size);
+	}
 
 	return 0;
 }
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 2e9c5d99e70e..a9e1390c8e12 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -356,6 +356,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 int ima_restore_measurement_list(loff_t size, void *buf)
 {
 	char template_name[MAX_TEMPLATE_NAME_LEN];
+	unsigned char zero[TPM_DIGEST_SIZE] = { 0 };
 
 	struct ima_kexec_hdr *khdr = buf;
 	struct ima_field_data hdr[HDR__LAST] = {
@@ -455,8 +456,17 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 		if (ret < 0)
 			break;
 
-		memcpy(entry->digests[ima_sha1_idx].digest,
-		       hdr[HDR_DIGEST].data, hdr[HDR_DIGEST].len);
+		if (memcmp(hdr[HDR_DIGEST].data, zero, sizeof(zero))) {
+			ret = ima_calc_field_array_hash(
+						&entry->template_data[0],
+						entry);
+			if (ret < 0) {
+				pr_err("cannot calculate template digest\n");
+				ret = -EINVAL;
+				break;
+			}
+		}
+
 		entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
 			     le32_to_cpu(*(hdr[HDR_PCR].data));
 		ret = ima_restore_measurement_entry(entry);
-- 
2.17.1

[PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list

[Roberto Sassu]

Before calculating a digest for each PCR bank, collisions were detected
with a SHA1 digest. This patch includes ima_hash_algo among the algorithms
used to calculate the template digest and checks collisions on that digest.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h          |  1 +
 security/integrity/ima/ima_api.c      |  2 +-
 security/integrity/ima/ima_crypto.c   | 25 ++++++++++++++++++-------
 security/integrity/ima/ima_main.c     |  1 +
 security/integrity/ima/ima_queue.c    |  8 ++++----
 security/integrity/ima/ima_template.c |  2 +-
 6 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d959dab5bcce..3e1afa5150bc 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -51,6 +51,7 @@ extern int ima_policy_flag;
 /* set during initialization */
 extern int ima_hash_algo;
 extern int ima_sha1_idx;
+extern int ima_hash_algo_idx;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index ebaf0056735c..a9bb45de6db9 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
 	if (!*entry)
 		return -ENOMEM;
 
-	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
 				    sizeof(*(*entry)->digests), GFP_NOFS);
 	if (!(*entry)->digests) {
 		result = -ENOMEM;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 786340feebbb..f84dfd8fc5ca 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -93,7 +93,7 @@ static struct crypto_shash *ima_alloc_tfm(enum hash_algo algo)
 	if (algo == ima_hash_algo)
 		return tfm;
 
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++)
 		if (ima_algo_array[i].algo == algo && ima_algo_array[i].tfm)
 			return ima_algo_array[i].tfm;
 
@@ -116,19 +116,20 @@ int __init ima_init_crypto(void)
 	if (rc)
 		return rc;
 
-	ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
+	ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 2,
 				       sizeof(*ima_algo_array), GFP_KERNEL);
 	if (!ima_algo_array) {
 		rc = -ENOMEM;
 		goto out;
 	}
 
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
 		ima_algo_array[i].tfm = NULL;
 		ima_algo_array[i].algo = HASH_ALGO__LAST;
 	}
 
 	ima_sha1_idx = -1;
+	ima_hash_algo_idx = -1;
 
 	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
 		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
@@ -143,6 +144,7 @@ int __init ima_init_crypto(void)
 
 		if (algo == ima_hash_algo) {
 			ima_algo_array[i].tfm = ima_shash_tfm;
+			ima_hash_algo_idx = i;
 			continue;
 		}
 
@@ -166,12 +168,21 @@ int __init ima_init_crypto(void)
 		}
 
 		ima_sha1_idx = i;
-		ima_algo_array[i].algo = HASH_ALGO_SHA1;
+		if (ima_hash_algo == HASH_ALGO_SHA1)
+			ima_hash_algo_idx = i;
+
+		ima_algo_array[i++].algo = HASH_ALGO_SHA1;
+	}
+
+	if (ima_hash_algo_idx < 0) {
+		ima_algo_array[i].tfm = ima_shash_tfm;
+		ima_algo_array[i].algo = ima_hash_algo;
+		ima_hash_algo_idx = i;
 	}
 
 	return 0;
 out_array:
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
 		if (!ima_algo_array[i].tfm ||
 		    ima_algo_array[i].tfm == ima_shash_tfm)
 			continue;
@@ -190,7 +201,7 @@ static void ima_free_tfm(struct crypto_shash *tfm)
 	if (tfm == ima_shash_tfm)
 		return;
 
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++)
 		if (ima_algo_array[i].tfm == tfm)
 			return;
 
@@ -619,7 +630,7 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
 
 	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
 
-	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
 		if (i == ima_sha1_idx)
 			continue;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c068067a0c47..6963d6c31bf1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -31,6 +31,7 @@
 #include "ima.h"
 
 int ima_sha1_idx;
+int ima_hash_algo_idx;
 
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise = IMA_APPRAISE_ENFORCE;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 7f7509774b85..58983d0f0214 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -57,8 +57,8 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 	key = ima_hash_key(digest_value);
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
-		rc = memcmp(qe->entry->digests[ima_sha1_idx].digest,
-			    digest_value, TPM_DIGEST_SIZE);
+		rc = memcmp(qe->entry->digests[ima_hash_algo_idx].digest,
+			    digest_value, hash_digest_size[ima_hash_algo]);
 		if ((rc == 0) && (qe->entry->pcr == pcr)) {
 			ret = qe;
 			break;
@@ -110,7 +110,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 
 	atomic_long_inc(&ima_htable.len);
 	if (update_htable) {
-		key = ima_hash_key(entry->digests[ima_sha1_idx].digest);
+		key = ima_hash_key(entry->digests[ima_hash_algo_idx].digest);
 		hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
 	}
 
@@ -162,7 +162,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename)
 {
-	u8 *digest = entry->digests[ima_sha1_idx].digest;
+	u8 *digest = entry->digests[ima_hash_algo_idx].digest;
 	struct tpm_digest *digests_arg = entry->digests;
 	const char *audit_cause = "hash_added";
 	char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX];
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index a9e1390c8e12..f71b5ee44179 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -311,7 +311,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
 	if (!*entry)
 		return -ENOMEM;
 
-	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
 				    sizeof(*(*entry)->digests), GFP_NOFS);
 	if (!(*entry)->digests) {
 		kfree(*entry);
-- 
2.17.1

[PATCH 8/8] ima: switch to ima_hash_algo for boot aggregate

[Roberto Sassu]

boot_aggregate is the first entry of IMA measurement list. Its purpose is
to link pre-boot measurements to IMA measurements. As IMA was designed to
work with a TPM 1.2, the SHA1 PCR bank was always selected.

Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
However, the assumption that the SHA1 PCR bank is always available is not
correct, as PCR banks can be selected with the PCR_Allocate() TPM command.

This patch tries to use ima_hash_algo as hash algorithm for boot_aggregate.
If no PCR bank uses that algorithm, the patch scans the allocated PCR banks
and selects the first for which the mapping between TPM algorithm ID and
crypto algorithm ID is known. If no suitable algorithm is found, an error
is returned.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 38 +++++++++++++++++------------
 security/integrity/ima/ima_init.c   |  6 ++---
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f84dfd8fc5ca..9bf5e69945b7 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -780,25 +780,27 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
 /*
  * Calculate the boot aggregate hash
  */
-static int __init ima_calc_boot_aggregate_tfm(char *digest,
-					      struct crypto_shash *tfm)
+static int __init ima_calc_boot_aggregate_tfm(char *digest, int bank_idx)
 {
-	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
+	struct tpm_digest d = { .digest = {0} };
 	int rc;
 	u32 i;
-	SHASH_DESC_ON_STACK(shash, tfm);
+	SHASH_DESC_ON_STACK(shash, ima_algo_array[bank_idx].tfm);
 
-	shash->tfm = tfm;
+	shash->tfm = ima_algo_array[bank_idx].tfm;
 
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;
 
+	d.alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
+
 	/* cumulative sha1 over tpm registers 0-7 */
 	for (i = TPM_PCR0; i < TPM_PCR8; i++) {
 		ima_pcrread(i, &d);
 		/* now accumulate with current aggregate */
-		rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
+		rc = crypto_shash_update(shash, d.digest,
+			ima_tpm_chip->allocated_banks[bank_idx].digest_size);
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
@@ -807,17 +809,21 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 
 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
 {
-	struct crypto_shash *tfm;
-	int rc;
+	int bank_idx = ima_hash_algo_idx;
 
-	tfm = ima_alloc_tfm(hash->algo);
-	if (IS_ERR(tfm))
-		return PTR_ERR(tfm);
-
-	hash->length = crypto_shash_digestsize(tfm);
-	rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
+	if (bank_idx >= ima_tpm_chip->nr_allocated_banks) {
+		for (bank_idx = 0; bank_idx < ima_tpm_chip->nr_allocated_banks;
+		     bank_idx++)
+			if (ima_algo_array[bank_idx].tfm)
+				break;
 
-	ima_free_tfm(tfm);
+		if (bank_idx == ima_tpm_chip->nr_allocated_banks) {
+			pr_err("No suitable algo found for boot aggregate\n");
+			return -ENOENT;
+		}
+	}
 
-	return rc;
+	hash->algo = ima_algo_array[bank_idx].algo;
+	hash->length = crypto_shash_digestsize(ima_algo_array[bank_idx].tfm);
+	return ima_calc_boot_aggregate_tfm(hash->digest, bank_idx);
 }
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 195cb4079b2b..b4da190a33ba 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -51,14 +51,14 @@ static int __init ima_add_boot_aggregate(void)
 	int violation = 0;
 	struct {
 		struct ima_digest_data hdr;
-		char digest[TPM_DIGEST_SIZE];
+		char digest[SHA512_DIGEST_SIZE];
 	} hash;
 
 	memset(iint, 0, sizeof(*iint));
 	memset(&hash, 0, sizeof(hash));
 	iint->ima_hash = &hash.hdr;
-	iint->ima_hash->algo = HASH_ALGO_SHA1;
-	iint->ima_hash->length = SHA1_DIGEST_SIZE;
+	iint->ima_hash->algo = ima_hash_algo;
+	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
 
 	if (ima_tpm_chip) {
 		result = ima_calc_boot_aggregate(&hash.hdr);
-- 
2.17.1

RE: [PATCH 6/8] ima: calculate and extend PCR with digests in ima_template_entry

[Roberto Sassu]

> -----Original Message-----
> From: Roberto Sassu
> Sent: Monday, January 27, 2020 6:05 PM
> To: zohar@linux.ibm.com; jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>; Roberto Sassu
> <roberto.sassu@huawei.com>
> Subject: [PATCH 6/8] ima: calculate and extend PCR with digests in
> ima_template_entry
> 
> This patch modifies ima_calc_field_array_hash() to calculate a template
> digest for each allocated PCR bank and SHA1. It also passes the tpm_digest
> array of the template entry to ima_pcr_extend() or in case of a violation,
> the pre-initialized digests array filled with 0xff.
> 
> Padding with zeros is still done if the mapping between TPM algorithm ID
> and crypto ID is unknown.
> 
> This patch calculates again the template digest when a measurement list is
> restored. Copying only the SHA1 digest (due to the limitation of the
> current measurement list format) is not sufficient, as hash collision
> detection will be done on the digest calculated with the default IMA hash
> algorithm.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_crypto.c   | 26 ++++++++++++++++++++++-
>  security/integrity/ima/ima_queue.c    | 30 ++++++++++++++++-----------
>  security/integrity/ima/ima_template.c | 14 +++++++++++--
>  3 files changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> index 63fb4bdf80b0..786340feebbb 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -610,9 +610,33 @@ static int ima_calc_field_array_hash_tfm(struct
> ima_field_data *field_data,
>  int ima_calc_field_array_hash(struct ima_field_data *field_data,
>  			      struct ima_template_entry *entry)
>  {
> -	int rc;
> +	u16 alg_id;
> +	int rc, i;
> 
>  	rc = ima_calc_field_array_hash_tfm(field_data, entry,
> ima_sha1_idx);
> +	if (rc)
> +		return rc;
> +
> +	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
> +
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> +		if (i == ima_sha1_idx)
> +			continue;
> +
> +		alg_id = ima_tpm_chip->allocated_banks[i].alg_id;

The line above should be executed for i < ima_tpm_chip->nr_allocated_banks.

I will fix in the next version of the patch set.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST

[Petr Vorel]

Hi Roberto,

> chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> PCR banks. It also contains the corresponding ID of the crypto subsystem,
> so that users of the TPM driver can calculate a digest for a PCR extend
> operation.

> However, if there is no mapping between TPM algorithm ID and crypto ID, the
> crypto_id field in chip->allocated_banks remains set to zero (the array is
> allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> Zero should not be used as value for unknown mappings, as it is a valid
> crypto ID (HASH_ALGO_MD4).

> This patch initializes crypto_id to HASH_ALGO__LAST.

Make sense.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST

[Jarkko Sakkinen]

On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> PCR banks. It also contains the corresponding ID of the crypto subsystem,
> so that users of the TPM driver can calculate a digest for a PCR extend
> operation.
> 
> However, if there is no mapping between TPM algorithm ID and crypto ID, the
> crypto_id field in chip->allocated_banks remains set to zero (the array is
> allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> Zero should not be used as value for unknown mappings, as it is a valid
> crypto ID (HASH_ALGO_MD4).
> 
> This patch initializes crypto_id to HASH_ALGO__LAST.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---

Remarks:

* After the subsystem tag, short summary starts with a capital lettter.
* Missing fixes tag and cc tag to stable.
* A struct called allocated_banks does not exist.
* Please prefer using an imperative sentence when describing the action
  to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".

/Jarkko

RE: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST

[Roberto Sassu]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, January 30, 2020 9:48 AM
> To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> HASH_ALGO__LAST
> 
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > so that users of the TPM driver can calculate a digest for a PCR extend
> > operation.
> >
> > However, if there is no mapping between TPM algorithm ID and crypto ID,
> the
> > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > Zero should not be used as value for unknown mappings, as it is a valid
> > crypto ID (HASH_ALGO_MD4).
> >
> > This patch initializes crypto_id to HASH_ALGO__LAST.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
> 
> Remarks:
> 
> * After the subsystem tag, short summary starts with a capital lettter.
> * Missing fixes tag and cc tag to stable.
> * A struct called allocated_banks does not exist.
> * Please prefer using an imperative sentence when describing the action
>   to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".

Thanks. I will fix these issues in the next version of the patch set.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list

[Mimi Zohar]

On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> Before calculating a digest for each PCR bank, collisions were detected
> with a SHA1 digest. This patch includes ima_hash_algo among the algorithms
> used to calculate the template digest and checks collisions on that digest.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Definitely needed to protect against a sha1 collision attack.

<snip>

>  
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index ebaf0056735c..a9bb45de6db9 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
>  	if (!*entry)
>  		return -ENOMEM;
>  
> -	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> +	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
>  				    sizeof(*(*entry)->digests), GFP_NOFS);
>  	if (!(*entry)->digests) {
>  		result = -ENOMEM;

I would prefer not having to allocate and use "nr_allocated_banks + 1"
everywhere, but I understand the need for it.  I'm not sure this patch
warrants allocating +2.  Perhaps, if a TPM bank doesn't exist for the
IMA default hash algorithm, use a different algorithm or, worst case,
continue using the ima_sha1_idx.

Mimi

Re: [PATCH 0/8] ima: support stronger algorithms for attestation

[Mimi Zohar]

Hi Roberto,

On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> IMA extends Platform Configuration Registers (PCRs) of the TPM to give a
> proof to a remote verifier that the measurement list contains all
> measurements done by the kernel and that the list was not maliciously
> modified by an attacker.
> 
> IMA was originally designed to extend PCRs with a SHA1 digest, provided
> with the measurement list, and was subsequently updated to extend all PCR
> banks in case a TPM 2.0 is used. Non-SHA1 PCR banks are not supposed to be
> used for remote attestation, as they are extended with a SHA1 digest padded
> with zeros, which does not increase the strength.
> 
> This patch set addresses this issue by extending PCRs with the digest of
> the measurement entry calculated with the crypto subsystem. The list of
> algorithms used to calculate the digest are taken from
> ima_tpm_chip->allocated_banks, returned by the TPM driver. The SHA1 digest
> is always calculated, as SHA1 still remains the default algorithm for the
> template digest in the measurement list.
> 
> This patch set also makes two additional modifications related to the usage
> of hash algorithms. First, since now the template digest for the default
> IMA algorithm is always calculated, this is used for hash collision
> detection, to check if there are duplicate measurement entries.
> 
> Second, it uses the default IMA hash algorithm to calculate the boot
> aggregate, assuming that the corresponding PCR bank is currently allocated.
> Otherwise, it finds the first PCR bank for which the crypto ID is known.
> IMA initialization fails only if no algorithm known to the crypto subsystem
> is found.
> 
> This patch set does not yet modify the format of the measurement list to
> provide the digests passed to the TPM. However, reconstructing the value of
> the quoted PCR is still possible for the verifier by calculating the digest
> on measurement data found in binary_runtime_measurements.

Thank you!  I'm still reviewing and testing the patches, but it is
really nicely written.

Mimi 

Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank

[Mimi Zohar]

On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> This patch creates a crypto_shash structure for each allocated PCR bank and
> for SHA1 if a bank with that algorithm is not currently allocated.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

<snip>

> +int __init ima_init_crypto(void)
> +{
> +	enum hash_algo algo;
> +	long rc;
> +	int i;
> +
> +	rc = ima_init_ima_crypto();
> +	if (rc)
> +		return rc;
> +
> +	ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
> +				       sizeof(*ima_algo_array), GFP_KERNEL);

Perhaps instead of hard coding "nr_allocated_banks + 1", create a
variable for storing the number of "extra" banks.  Walk the banks
setting ima_sha1_idx and, later, in a subsequent patch set
ima_hash_algo_idx.

> +	if (!ima_algo_array) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> +		ima_algo_array[i].tfm = NULL;
> +		ima_algo_array[i].algo = HASH_ALGO__LAST;
> +	}

ima_init_crypto() is executed once on initialization.  I'm not sure if
it makes a difference, but if you're really concerned about an
additional loop, the initialization, here, could be limited to the
"extra" banks.  The other banks could be initialized at the beginning
of the next loop.

thanks,

Mimi

> +	ima_sha1_idx = -1;
> +
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> +		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> +		ima_algo_array[i].algo = algo;
> +
> +		/* unknown TPM algorithm */
> +		if (algo == HASH_ALGO__LAST)
> +			continue;
> +
> +		if (algo == HASH_ALGO_SHA1)
> +			ima_sha1_idx = i;
> +
> +		if (algo == ima_hash_algo) {
> +			ima_algo_array[i].tfm = ima_shash_tfm;
> +			continue;
> +		}
> +
> +		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
> +		if (IS_ERR(ima_algo_array[i].tfm)) {
> +			if (algo == HASH_ALGO_SHA1) {
> +				rc = PTR_ERR(ima_algo_array[i].tfm);
> +				ima_algo_array[i].tfm = NULL;
> +				goto out_array;
> +			}
> +
> +			ima_algo_array[i].tfm = NULL;
> +		}
> +	}
> +
> +	if (ima_sha1_idx < 0) {
> +		ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
> +		if (IS_ERR(ima_algo_array[i].tfm)) {
> +			rc = PTR_ERR(ima_algo_array[i].tfm);
> +			goto out_array;
> +		}
> +
> +		ima_sha1_idx = i;
> +		ima_algo_array[i].algo = HASH_ALGO_SHA1;
> +	}
> +
> +	return 0;
> +out_array:
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> +		if (!ima_algo_array[i].tfm ||
> +		    ima_algo_array[i].tfm == ima_shash_tfm)
> +			continue;
> +
> +		crypto_free_shash(ima_algo_array[i].tfm);
> +	}
> +out:
> +	crypto_free_shash(ima_shash_tfm);
> +	return rc;
> +}

Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST

[Mimi Zohar]

On Thu, 2020-01-30 at 16:11 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > Sent: Thursday, January 30, 2020 9:48 AM
> > To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> > HASH_ALGO__LAST
> > 
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > > so that users of the TPM driver can calculate a digest for a PCR extend
> > > operation.
> > >
> > > However, if there is no mapping between TPM algorithm ID and crypto ID,
> > the
> > > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > > Zero should not be used as value for unknown mappings, as it is a valid
> > > crypto ID (HASH_ALGO_MD4).
> > >
> > > This patch initializes crypto_id to HASH_ALGO__LAST.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
> > 
> > Remarks:
> > 
> > * After the subsystem tag, short summary starts with a capital lettter.
> > * Missing fixes tag and cc tag to stable.
> > * A struct called allocated_banks does not exist.
> > * Please prefer using an imperative sentence when describing the action
> >   to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
> 
> Thanks. I will fix these issues in the next version of the patch set.

Jarkko, I realize this is a TPM patch, but this patch set is dependent
on it.  When this patch is ready, could you create a topic branch,
which both of us could merge?

thanks,

Mimi

Re: [PATCH 2/8] ima: evaluate error in init_ima()

[Mimi Zohar]

On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> Evaluate error in init_ima() before register_blocking_lsm_notifier() and
> return if not zero.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks.  Please include a "Fixes" tag.

Mimi

RE: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank

[Roberto Sassu]

> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Friday, January 31, 2020 1:19 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
> 
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > This patch creates a crypto_shash structure for each allocated PCR bank
> and
> > for SHA1 if a bank with that algorithm is not currently allocated.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> <snip>
> 
> > +int __init ima_init_crypto(void)
> > +{
> > +	enum hash_algo algo;
> > +	long rc;
> > +	int i;
> > +
> > +	rc = ima_init_ima_crypto();
> > +	if (rc)
> > +		return rc;
> > +
> > +	ima_algo_array = kmalloc_array(ima_tpm_chip-
> >nr_allocated_banks + 1,
> > +				       sizeof(*ima_algo_array), GFP_KERNEL);
> 
> Perhaps instead of hard coding "nr_allocated_banks + 1", create a
> variable for storing the number of "extra" banks.  Walk the banks
> setting ima_sha1_idx and, later, in a subsequent patch set
> ima_hash_algo_idx.

I could store the indexes in an array and add ARRAY_SIZE of that array.

> > +	if (!ima_algo_array) {
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> > +		ima_algo_array[i].tfm = NULL;
> > +		ima_algo_array[i].algo = HASH_ALGO__LAST;
> > +	}
> 
> ima_init_crypto() is executed once on initialization.  I'm not sure if
> it makes a difference, but if you're really concerned about an
> additional loop, the initialization, here, could be limited to the
> "extra" banks.  The other banks could be initialized at the beginning
> of the next loop.

I set algo to HASH_ALGO__LAST to mark the ima_algo_array entries as
uninitialized. ima_alloc_tfm() uses ima_algo_array in the next loop.
Replacing kmalloc_array() with kcalloc() would cause algo to be set to
HASH_ALGO_MD4.

I could check tfm first, which ensures that also algo was initialized.

Roberto

> > +	ima_sha1_idx = -1;
> > +
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > +		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> > +		ima_algo_array[i].algo = algo;
> > +
> > +		/* unknown TPM algorithm */
> > +		if (algo == HASH_ALGO__LAST)
> > +			continue;
> > +
> > +		if (algo == HASH_ALGO_SHA1)
> > +			ima_sha1_idx = i;
> > +
> > +		if (algo == ima_hash_algo) {
> > +			ima_algo_array[i].tfm = ima_shash_tfm;
> > +			continue;
> > +		}
> > +
> > +		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
> > +		if (IS_ERR(ima_algo_array[i].tfm)) {
> > +			if (algo == HASH_ALGO_SHA1) {
> > +				rc = PTR_ERR(ima_algo_array[i].tfm);
> > +				ima_algo_array[i].tfm = NULL;
> > +				goto out_array;
> > +			}
> > +
> > +			ima_algo_array[i].tfm = NULL;
> > +		}
> > +	}
> > +
> > +	if (ima_sha1_idx < 0) {
> > +		ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
> > +		if (IS_ERR(ima_algo_array[i].tfm)) {
> > +			rc = PTR_ERR(ima_algo_array[i].tfm);
> > +			goto out_array;
> > +		}
> > +
> > +		ima_sha1_idx = i;
> > +		ima_algo_array[i].algo = HASH_ALGO_SHA1;
> > +	}
> > +
> > +	return 0;
> > +out_array:
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> > +		if (!ima_algo_array[i].tfm ||
> > +		    ima_algo_array[i].tfm == ima_shash_tfm)
> > +			continue;
> > +
> > +		crypto_free_shash(ima_algo_array[i].tfm);
> > +	}
> > +out:
> > +	crypto_free_shash(ima_shash_tfm);
> > +	return rc;
> > +}

Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank

[Mimi Zohar]

On Fri, 2020-01-31 at 13:42 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Friday, January 31, 2020 1:19 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > jarkko.sakkinen@linux.intel.com;
> > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
> > 
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > This patch creates a crypto_shash structure for each allocated PCR bank
> > and
> > > for SHA1 if a bank with that algorithm is not currently allocated.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > <snip>
> > 
> > > +int __init ima_init_crypto(void)
> > > +{
> > > +	enum hash_algo algo;
> > > +	long rc;
> > > +	int i;
> > > +
> > > +	rc = ima_init_ima_crypto();
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	ima_algo_array = kmalloc_array(ima_tpm_chip-
> > >nr_allocated_banks + 1,
> > > +				       sizeof(*ima_algo_array), GFP_KERNEL);
> > 
> > Perhaps instead of hard coding "nr_allocated_banks + 1", create a
> > variable for storing the number of "extra" banks.  Walk the banks
> > setting ima_sha1_idx and, later, in a subsequent patch set
> > ima_hash_algo_idx.
> 
> I could store the indexes in an array and add ARRAY_SIZE of that array.

Your current patches are straight forward and easy to review.  I
really like that.  What I'm proposing is a minor change, at least I
think it is a minor change.  I trust whatever change you decide to
make will be just as easy to review.

Before re-posting, please define ima_sha1_idx and ima_hash_algo_idx as
__ro_after_init.

thanks,

Mimi

RE: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list

[Roberto Sassu]

> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Thursday, January 30, 2020 11:26 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> the measurement list
> 
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > Before calculating a digest for each PCR bank, collisions were detected
> > with a SHA1 digest. This patch includes ima_hash_algo among the
> algorithms
> > used to calculate the template digest and checks collisions on that digest.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Definitely needed to protect against a sha1 collision attack.
> 
> <snip>
> 
> >
> > diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> > index ebaf0056735c..a9bb45de6db9 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data
> *event_data,
> >  	if (!*entry)
> >  		return -ENOMEM;
> >
> > -	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > +	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> >  				    sizeof(*(*entry)->digests), GFP_NOFS);
> >  	if (!(*entry)->digests) {
> >  		result = -ENOMEM;
> 
> I would prefer not having to allocate and use "nr_allocated_banks + 1"
> everywhere, but I understand the need for it.  I'm not sure this patch
> warrants allocating +2.  Perhaps, if a TPM bank doesn't exist for the
> IMA default hash algorithm, use a different algorithm or, worst case,
> continue using the ima_sha1_idx.

We could introduce a new option called ima_hash_algo_tpm to specify
the algorithm of an allocated bank. We can use this for boot_aggregate
and hash collision detection.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list

[Mimi Zohar]

On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Thursday, January 30, 2020 11:26 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > jarkko.sakkinen@linux.intel.com;
> > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> > the measurement list
> > 
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > Before calculating a digest for each PCR bank, collisions were detected
> > > with a SHA1 digest. This patch includes ima_hash_algo among the
> > algorithms
> > > used to calculate the template digest and checks collisions on that digest.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Definitely needed to protect against a sha1 collision attack.
> > 
> > <snip>
> > 
> > >
> > > diff --git a/security/integrity/ima/ima_api.c
> > b/security/integrity/ima/ima_api.c
> > > index ebaf0056735c..a9bb45de6db9 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data
> > *event_data,
> > >  	if (!*entry)
> > >  		return -ENOMEM;
> > >
> > > -	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > > +	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > >  				    sizeof(*(*entry)->digests), GFP_NOFS);
> > >  	if (!(*entry)->digests) {
> > >  		result = -ENOMEM;
> > 
> > I would prefer not having to allocate and use "nr_allocated_banks + 1"
> > everywhere, but I understand the need for it.  I'm not sure this patch
> > warrants allocating +2.  Perhaps, if a TPM bank doesn't exist for the
> > IMA default hash algorithm, use a different algorithm or, worst case,
> > continue using the ima_sha1_idx.
> 
> We could introduce a new option called ima_hash_algo_tpm to specify
> the algorithm of an allocated bank. We can use this for boot_aggregate
> and hash collision detection.

I don't think that would work in the case where the IMA default hash
is set to sha256, but the system has a TPM 1.2 chip.  We would be left
using SHA1 for the file hash collision detection.

With my suggestion of defining an "extra" variable, I kind of back
tracked here.  There are two problems that I'm trying to address -
hard coding the number of additional "banks" and unnecessarily
allocating more memory than necessary.  By pre-walking the list,
calculating the "extra" banks, you'll resolve both issues.

Mimi

RE: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list

[Roberto Sassu]

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, January 31, 2020 3:22 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> the measurement list
> 
> On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > > Sent: Thursday, January 30, 2020 11:26 PM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > > jarkko.sakkinen@linux.intel.com;
> > > james.bottomley@hansenpartnership.com; linux-
> integrity@vger.kernel.org
> > > Cc: linux-security-module@vger.kernel.org; linux-
> kernel@vger.kernel.org;
> > > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection
> in
> > > the measurement list
> > >
> > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > > Before calculating a digest for each PCR bank, collisions were detected
> > > > with a SHA1 digest. This patch includes ima_hash_algo among the
> > > algorithms
> > > > used to calculate the template digest and checks collisions on that
> digest.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Definitely needed to protect against a sha1 collision attack.
> > >
> > > <snip>
> > >
> > > >
> > > > diff --git a/security/integrity/ima/ima_api.c
> > > b/security/integrity/ima/ima_api.c
> > > > index ebaf0056735c..a9bb45de6db9 100644
> > > > --- a/security/integrity/ima/ima_api.c
> > > > +++ b/security/integrity/ima/ima_api.c
> > > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct
> ima_event_data
> > > *event_data,
> > > >  	if (!*entry)
> > > >  		return -ENOMEM;
> > > >
> > > > -	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > > > +	(*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > > >  				    sizeof(*(*entry)->digests), GFP_NOFS);
> > > >  	if (!(*entry)->digests) {
> > > >  		result = -ENOMEM;
> > >
> > > I would prefer not having to allocate and use "nr_allocated_banks + 1"
> > > everywhere, but I understand the need for it.  I'm not sure this patch
> > > warrants allocating +2.  Perhaps, if a TPM bank doesn't exist for the
> > > IMA default hash algorithm, use a different algorithm or, worst case,
> > > continue using the ima_sha1_idx.
> >
> > We could introduce a new option called ima_hash_algo_tpm to specify
> > the algorithm of an allocated bank. We can use this for boot_aggregate
> > and hash collision detection.
> 
> I don't think that would work in the case where the IMA default hash
> is set to sha256, but the system has a TPM 1.2 chip.  We would be left
> using SHA1 for the file hash collision detection.

I thought that using a stronger algorithm for hash collision detection but
doing remote attestation with the weaker would not bring additional value.

If there is a hash collision on SHA1, an attacker can still replace the data of
one of the two entries in the measurement list with the data of the other
without being detected (without additional countermeasures).

If the verifier additionally checks for duplicate template digests, he could
detect the attack (IMA would not add a new measurement entry with the
same template digest of previous entries).

Ok, I will use ima_hash_algo for hash collision detection.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list

[Mimi Zohar]

On Fri, 2020-01-31 at 14:41 +0000, Roberto Sassu wrote:
> I thought that using a stronger algorithm for hash collision detection but
> doing remote attestation with the weaker would not bring additional value.
> 
> If there is a hash collision on SHA1, an attacker can still replace the data of
> one of the two entries in the measurement list with the data of the other
> without being detected (without additional countermeasures).
> 
> If the verifier additionally checks for duplicate template digests, he could
> detect the attack (IMA would not add a new measurement entry with the
> same template digest of previous entries).
> 
> Ok, I will use ima_hash_algo for hash collision detection.

Thanks!

Mimi

RE: [PATCH 8/8] ima: switch to ima_hash_algo for boot aggregate

[Roberto Sassu]

> -----Original Message-----
> From: Roberto Sassu
> Sent: Monday, January 27, 2020 6:05 PM
> To: zohar@linux.ibm.com; jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>; Roberto Sassu
> <roberto.sassu@huawei.com>
> Subject: [PATCH 8/8] ima: switch to ima_hash_algo for boot aggregate

I will remove this patch from the patch set.

[PATCH v3 1/2] ima: support calculating the boot aggregate based on non-SHA1 algorithms
[PATCH v3 2/2] ima: support calculating the boot_aggregate based on different TPM banks

by Mimi will provide similar functionality.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST

[Jarkko Sakkinen]

On Fri, Jan 31, 2020 at 08:33:10AM -0500, Mimi Zohar wrote:
> On Thu, 2020-01-30 at 16:11 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > > Sent: Thursday, January 30, 2020 9:48 AM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> > > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> > > HASH_ALGO__LAST
> > > 
> > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > > > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > > > so that users of the TPM driver can calculate a digest for a PCR extend
> > > > operation.
> > > >
> > > > However, if there is no mapping between TPM algorithm ID and crypto ID,
> > > the
> > > > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > > > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > > > Zero should not be used as value for unknown mappings, as it is a valid
> > > > crypto ID (HASH_ALGO_MD4).
> > > >
> > > > This patch initializes crypto_id to HASH_ALGO__LAST.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
> > > 
> > > Remarks:
> > > 
> > > * After the subsystem tag, short summary starts with a capital lettter.
> > > * Missing fixes tag and cc tag to stable.
> > > * A struct called allocated_banks does not exist.
> > > * Please prefer using an imperative sentence when describing the action
> > >   to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
> > 
> > Thanks. I will fix these issues in the next version of the patch set.
> 
> Jarkko, I realize this is a TPM patch, but this patch set is dependent
> on it.  When this patch is ready, could you create a topic branch,
> which both of us could merge?

WFM.

/Jarkko