diff mbox series

[v3,5/8] ima: Switch to dynamically allocated buffer for template digests

Message ID 20200210100244.21728-1-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series ima: support stronger algorithms for attestation | expand

Commit Message

Roberto Sassu Feb. 10, 2020, 10:02 a.m. UTC
This patch dynamically allocates the array of tpm_digest structures in
ima_alloc_init_template() and ima_restore_template_data(). The size of the
array is equal to the number of PCR banks plus ima_extra_slots, to make
room for SHA1 and the IMA default algorithm, when PCR banks with those
algorithms are 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 measurement entry array of the SHA1 digest is stored in
the ima_sha1_idx global variable and is determined at IMA initialization
time.

Changelog

v2:
- add NR_BANKS macro to return zero if ima_tpm_chip is NULL
- replace ima_num_template_digests with
  NR_BANKS(ima_tpm_chip) + ima_extra_slots (suggested by Mimi)
- add __ro_after_init to declaration of ima_sha1_idx and ima_extra_slots
  (suggested by Mimi)

v1:
- move ima_sha1_idx to ima_crypto.c
- introduce ima_num_template_digests (suggested by Mimi)

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

Comments

Mimi Zohar March 2, 2020, 10:20 p.m. UTC | #1
On Mon, 2020-02-10 at 11:02 +0100, Roberto Sassu wrote:
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2f380fb92a7a..f04bec2ab83f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -45,11 +45,15 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
>  #define IMA_TEMPLATE_IMA_NAME "ima"
>  #define IMA_TEMPLATE_IMA_FMT "d|n"
>  
> +#define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
> +
>  /* current content of the policy */
>  extern int ima_policy_flag;
>  
>  /* set during initialization */
>  extern int ima_hash_algo;
> +extern int ima_sha1_idx __ro_after_init;
> +extern int ima_extra_slots __ro_after_init;
>  extern int ima_appraise;
>  extern struct tpm_chip *ima_tpm_chip;
>  
> @@ -92,7 +96,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 51f562111864..ea6d834a5fab 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(NR_BANKS(ima_tpm_chip) + ima_extra_slots,
> +				    sizeof(*(*entry)->digests), GFP_NOFS);

"sizeof(*(*entry)" should be simplified for readability.  Defining a
local "entry" or "digests" variable might help.  Similarly in
ima_restore_template_data().

> +	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 2d356ae8e823..ea24d2f6b513 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -59,6 +59,10 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
>  static struct crypto_shash *ima_shash_tfm;
>  static struct crypto_ahash *ima_ahash_tfm;
>  
> +int ima_sha1_idx __ro_after_init;
> +/* Additional number of slots to be reserved for SHA1 and IMA default algo */
> +int ima_extra_slots __ro_after_init = 1;

The reasoning behind needing an extra slot is clear from the patch
description, but not the reason for defining a variable.  Let's
clarify this comment a bit by inserting "reserved, as needed, for
SHA1"

> +
>  int __init ima_init_crypto(void)
>  {
>  	long rc;
> @@ -502,7 +506,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);

There's no reason to split this line.  It's less than 80 characters.

Mimi

>  
>  	return rc;
>  }
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2f380fb92a7a..f04bec2ab83f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -45,11 +45,15 @@  enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
 #define IMA_TEMPLATE_IMA_NAME "ima"
 #define IMA_TEMPLATE_IMA_FMT "d|n"
 
+#define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
+
 /* current content of the policy */
 extern int ima_policy_flag;
 
 /* set during initialization */
 extern int ima_hash_algo;
+extern int ima_sha1_idx __ro_after_init;
+extern int ima_extra_slots __ro_after_init;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
 
@@ -92,7 +96,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 51f562111864..ea6d834a5fab 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(NR_BANKS(ima_tpm_chip) + ima_extra_slots,
+				    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 2d356ae8e823..ea24d2f6b513 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -59,6 +59,10 @@  MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
 static struct crypto_shash *ima_shash_tfm;
 static struct crypto_ahash *ima_ahash_tfm;
 
+int ima_sha1_idx __ro_after_init;
+/* Additional number of slots to be reserved for SHA1 and IMA default algo */
+int ima_extra_slots __ro_after_init = 1;
+
 int __init ima_init_crypto(void)
 {
 	long rc;
@@ -502,7 +506,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_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..586e9fd9fc12 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(NR_BANKS(ima_tpm_chip) + ima_extra_slots,
+				    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);