Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/8] ima: support stronger algorithms for attestation
@ 2020-02-05 10:33 Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
                   ` (7 more replies)
  0 siblings, 8 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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 SHA256 PCR bank (mandatory for TPM 2.0 in TCG PC
Client specification). Lastly, if that bank was not found, it selects the
first PCR bank. If the TPM algorithm ID of the first PCR bank is not mapped
to a crypto ID, boot_aggregate is set to zero.

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.

attest-tools (https://github.com/euleros/attest-tools, branch 0.2-devel)
has the ability to parse the BIOS and IMA event logs, and to compare
boot_aggregate with the digest of final PCR values obtained by performing
in software the PCR extend operation with digests in the BIOS event log.

To perform the test, it is necessary to have a complete BIOS event log and
to apply the boot_aggregate patches. It would be possible to use qemu and
swtpm from Stefan Berger, but at the moment it is necessary to change the
ACPI parser in drivers/char/tpm/event_log/acpi.c to accept TPM 2.0 and to
return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2.

Create req.json with this content:
---
{
  "reqs":{
    "dummy|verify":"",
    "ima_boot_aggregate|verify":""
  }
}
---

With the requirements above, attest-tools verifies boot_aggregate and
accepts any other entry in the event logs.

On server side run:
# attest_ra_server -p 10 -r req.json -s -i

-s disables TPM signature verification
-i allows IMA violations

To enable TPM signature verification it is necessary to have a valid AK
certificate. It can be obtained by following the instructions at:

https://github.com/euleros/attest-tools/blob/0.2-devel/README

On client side run:
# echo test > aik_cert.pem
# echo aik_cert.pem > list_privacy_ca
# attest_ra_client -A

The commands above generate an AK and tell attest-tools to use a dummy AK
certificate.

# attest_ra_client -s <server IP> -q -p 10 -P <PCR algo> -b -i

The command above sends the TPM quote and the event logs to the RA server
and gets the response (successful/failed verification).

-b includes the BIOS event log from securityfs
-i includes the IMA event log from securityfs

To check that IMA extends non-SHA1 PCR banks with an appropriate digest,
use -P sha256, so that attest_ra_client selects the SHA256 PCR bank. To
check that boot_aggregate is calculated properly, set ima_hash=sha256 in
the kernel command line.

Changelog

v1:
- move ima_sha1_idx and ima_hash_algo_idx to ima_crypto.c
- introduce ima_num_template_digests (suggested by Mimi)
- determine ima_num_template_digests before allocating ima_algo_array
  (suggested by Mimi)
- replace kmalloc_array() with kcalloc() in ima_init_crypto() (suggested by
  Mimi)
- check if ima_tpm_chip is NULL

Roberto Sassu (8):
  tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  ima: Switch to ima_hash_algo for boot aggregate
  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

 drivers/char/tpm/tpm2-cmd.c           |   2 +
 security/integrity/ima/ima.h          |   8 +-
 security/integrity/ima/ima_api.c      |  20 +--
 security/integrity/ima/ima_crypto.c   | 244 ++++++++++++++++++++++----
 security/integrity/ima/ima_fs.c       |   4 +-
 security/integrity/ima/ima_init.c     |  22 ++-
 security/integrity/ima/ima_main.c     |   3 +
 security/integrity/ima/ima_queue.c    |  36 ++--
 security/integrity/ima/ima_template.c |  22 ++-
 9 files changed, 288 insertions(+), 73 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  2020-02-05 21:57   ` Jarkko Sakkinen
  2020-02-05 10:33 ` [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu, stable

chip->allocated_banks, an array of tpm_bank_info structures, 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 of tpm_bank_info 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).

Thus, initialize crypto_id to HASH_ALGO__LAST.

Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Cc: stable@vger.kernel.org
---
 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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
       [not found]   ` <20200205144515.1DDE4217F4@mail.kernel.org>
                     ` (2 more replies)
  2020-02-05 10:33 ` [PATCH v2 3/8] ima: Evaluate error in init_ima() Roberto Sassu
                   ` (5 subsequent siblings)
  7 siblings, 3 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu, stable

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 tries to find the SHA256 PCR
bank (which is mandatory in the TCG PC Client specification). If also this
bank is not found, the patch selects the first one. If the TPM algorithm
of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.

Changelog

v1:
- add Mimi's comments
- if there is no PCR bank for the IMA default algorithm use SHA256 or the
  first bank (suggested by James Bottomley)

Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
 security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++----
 security/integrity/ima/ima_init.c   | 22 ++++++++++----
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 73044fc6a952..f2f41a2bc3d4 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
 }
 
 /*
- * Calculate the boot aggregate hash
+ * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.  With
+ * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with
+ * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
+ * allowing firmware to configure and enable different banks.
+ *
+ * Knowing which TPM bank is read to calculate the boot_aggregate digest
+ * needs to be conveyed to a verifier.  For this reason, use the same
+ * hash algorithm for reading the TPM PCRs as for calculating the boot
+ * aggregate digest as stored in the measurement list.
  */
-static int __init ima_calc_boot_aggregate_tfm(char *digest,
+static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 					      struct crypto_shash *tfm)
 {
-	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
+	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
 	int rc;
 	u32 i;
 	SHASH_DESC_ON_STACK(shash, tfm);
 
 	shash->tfm = tfm;
 
+	pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n",
+		 d.alg_id);
+
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;
@@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 	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,
+					 crypto_shash_digestsize(tfm));
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
@@ -685,14 +697,35 @@ 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;
+	u16 crypto_id, alg_id;
+	int rc, i, bank_idx = 0;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
+		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
+		if (crypto_id == hash->algo) {
+			bank_idx = i;
+			break;
+		}
+
+		if (crypto_id == HASH_ALGO_SHA256)
+			bank_idx = i;
+	}
+
+	if (bank_idx == 0 &&
+	    ima_tpm_chip->allocated_banks[0].crypto_id == HASH_ALGO__LAST) {
+		pr_err("No suitable TPM algorithm for boot aggregate\n");
+		return 0;
+	}
+
+	hash->algo = ima_tpm_chip->allocated_banks[bank_idx].crypto_id;
 
 	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);
+	alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
+	rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
 
 	ima_free_tfm(tfm);
 
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5d55ade5f3b9..fbd7a8e28a6b 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -27,7 +27,7 @@ struct tpm_chip *ima_tpm_chip;
 /* Add the boot aggregate to the IMA measurement list and extend
  * the PCR register.
  *
- * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
+ * Calculate the boot aggregate, a hash over tpm registers 0-7,
  * assuming a TPM chip exists, and zeroes if the TPM chip does not
  * exist.  Add the boot aggregate measurement to the measurement
  * list and extend the PCR register.
@@ -51,15 +51,27 @@ static int __init ima_add_boot_aggregate(void)
 	int violation = 0;
 	struct {
 		struct ima_digest_data hdr;
-		char digest[TPM_DIGEST_SIZE];
+		char digest[TPM_MAX_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];
+
+	/*
+	 * With TPM 2.0 hash agility, TPM chips could support multiple TPM
+	 * PCR banks, allowing firmware to configure and enable different
+	 * banks.  The SHA1 bank is not necessarily enabled.
+	 *
+	 * Use the same hash algorithm for reading the TPM PCRs as for
+	 * calculating the boot aggregate digest.  Preference is given to
+	 * the configured IMA default hash algorithm.  Otherwise, use the
+	 * TPM required banks - SHA256 for TPM 2.0, SHA1 for TPM 1.2.  If
+	 * SHA256 is not available, use the first PCR bank and if that is
+	 * not mapped to a crypto ID, set boot_aggregate to zero.
+	 */
 	if (ima_tpm_chip) {
 		result = ima_calc_boot_aggregate(&hash.hdr);
 		if (result < 0) {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 3/8] ima: Evaluate error in init_ima()
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  2020-02-05 10:39   ` Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 4/8] ima: Store template digest directly in ima_template_entry Roberto Sassu
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

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

Fixes: b16942455193 ("ima: use the lsm policy update notifier")
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 d7e987baf127..a16c148ed90d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -738,6 +738,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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 4/8] ima: Store template digest directly in ima_template_entry
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
                   ` (2 preceding siblings ...)
  2020-02-05 10:33 ` [PATCH v2 3/8] ima: Evaluate error in init_ima() Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests Roberto Sassu
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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 df4ca482fb53..2f380fb92a7a 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 610759fe63b8..51f562111864 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 f2f41a2bc3d4..2d356ae8e823 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -464,18 +464,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;
@@ -504,24 +502,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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
                   ` (3 preceding siblings ...)
  2020-02-05 10:33 ` [PATCH v2 4/8] ima: Store template digest directly in ima_template_entry Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  2020-02-05 16:39   ` Roberto Sassu
  2020-02-06 16:08   ` Mimi Zohar
  2020-02-05 10:33 ` [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank Roberto Sassu
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

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, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
and will be determined in the upcoming patches depending on the allocated
PCR banks and the chosen default IMA algorithm.

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.

Changelog

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          |  4 +++-
 security/integrity/ima/ima_api.c      |  8 ++++++++
 security/integrity/ima/ima_crypto.c   |  6 +++++-
 security/integrity/ima/ima_fs.c       |  4 ++--
 security/integrity/ima/ima_queue.c    | 10 ++++++----
 security/integrity/ima/ima_template.c | 12 ++++++++++--
 6 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2f380fb92a7a..4843077dc9e8 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -50,6 +50,8 @@ extern int ima_policy_flag;
 
 /* set during initialization */
 extern int ima_hash_algo;
+extern int ima_sha1_idx;
+extern int ima_num_template_digests;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
 
@@ -92,7 +94,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..2ced5975c16f 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_num_template_digests,
+				    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..31f2497a3ab8 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -59,6 +59,9 @@ 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;
+int ima_num_template_digests = 1;
+
 int __init ima_init_crypto(void)
 {
 	long rc;
@@ -502,7 +505,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..751f101ae927 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_num_template_digests,
+				    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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
                   ` (4 preceding siblings ...)
  2020-02-05 10:33 ` [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  2020-02-05 23:41   ` kbuild test robot
  2020-02-05 23:41   ` [RFC PATCH] ima: ima_init_ima_crypto() can be static kbuild test robot
  2020-02-05 10:33 ` [PATCH v2 7/8] ima: Calculate and extend PCR with digests in ima_template_entry Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 8/8] ima: Use ima_hash_algo for collision detection in the measurement list Roberto Sassu
  7 siblings, 2 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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.

Changelog

v1:
- determine ima_num_template_digests before allocating ima_algo_array
  (suggested by Mimi)
- replace kmalloc_array() with kcalloc() in ima_init_crypto() (suggested by
  Mimi)
- check tfm first in ima_alloc_tfm()
- check if ima_tpm_chip is NULL

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

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 31f2497a3ab8..3101c343f963 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -59,10 +59,17 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
 static struct crypto_shash *ima_shash_tfm;
 static struct crypto_ahash *ima_ahash_tfm;
 
+struct ima_algo_desc {
+	struct crypto_shash *tfm;
+	enum hash_algo algo;
+};
+
 int ima_sha1_idx;
-int ima_num_template_digests = 1;
+int ima_num_template_digests;
 
-int __init ima_init_crypto(void)
+static struct ima_algo_desc *ima_algo_array;
+
+int __init ima_init_ima_crypto(void)
 {
 	long rc;
 
@@ -81,26 +88,120 @@ 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_num_template_digests; i++)
+		if (ima_algo_array[i].tfm && ima_algo_array[i].algo == algo)
+			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, nr_allocated_banks = 0;
+
+	rc = ima_init_ima_crypto();
+	if (rc)
+		return rc;
+
+	if (ima_tpm_chip)
+		nr_allocated_banks = ima_tpm_chip->nr_allocated_banks;
+
+	ima_sha1_idx = -1;
+	ima_num_template_digests = nr_allocated_banks;
+
+	for (i = 0; i < nr_allocated_banks; i++) {
+		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
+		if (algo == HASH_ALGO_SHA1)
+			ima_sha1_idx = i;
+	}
+
+	if (ima_sha1_idx < 0)
+		ima_sha1_idx = ima_num_template_digests++;
+
+	ima_algo_array = kcalloc(ima_num_template_digests,
+				 sizeof(*ima_algo_array), GFP_KERNEL);
+	if (!ima_algo_array) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < 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 == 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 >= nr_allocated_banks) {
+		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_algo_array[i].algo = HASH_ALGO_SHA1;
+	}
+
+	return 0;
+out_array:
+	for (i = 0; i < ima_num_template_digests; 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_num_template_digests; i++)
+		if (ima_algo_array[i].tfm == tfm)
+			return;
+
+	crypto_free_shash(tfm);
 }
 
 /**
@@ -468,14 +569,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)
@@ -506,7 +607,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;
 }
@@ -514,17 +615,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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 7/8] ima: Calculate and extend PCR with digests in ima_template_entry
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
                   ` (5 preceding siblings ...)
  2020-02-05 10:33 ` [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  2020-02-05 10:33 ` [PATCH v2 8/8] ima: Use ima_hash_algo for collision detection in the measurement list Roberto Sassu
  7 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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.

Changelog

v1:
- replace ima_tpm_chip->nr_allocated_banks with ima_num_template_digests
  (suggested by Mimi)
- retrieve alg_id only if i < ima_tpm_chip->nr_allocated_banks
- check if ima_tpm_chip is NULL

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

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 3101c343f963..1ee813d33bdc 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -615,9 +615,39 @@ 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, nr_allocated_banks = 0;
 
 	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;
+
+	if (ima_tpm_chip)
+		nr_allocated_banks = ima_tpm_chip->nr_allocated_banks;
+
+	for (i = 0; i < ima_num_template_digests; i++) {
+		if (i == ima_sha1_idx)
+			continue;
+
+		if (i < nr_allocated_banks) {
+			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+			entry->digests[i].alg_id = alg_id;
+		}
+
+		/* for unmapped TPM algorithms digest is still a padded SHA1 */
+		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 751f101ae927..dae6aab8d0bd 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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 8/8] ima: Use ima_hash_algo for collision detection in the measurement list
  2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
                   ` (6 preceding siblings ...)
  2020-02-05 10:33 ` [PATCH v2 7/8] ima: Calculate and extend PCR with digests in ima_template_entry Roberto Sassu
@ 2020-02-05 10:33 ` Roberto Sassu
  7 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:33 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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.

Changelog

v1:
- increment ima_num_template_digests before kcalloc() (suggested by Mimi)
- check if ima_tpm_chip is NULL

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h        |  1 +
 security/integrity/ima/ima_crypto.c | 20 ++++++++++++++++++--
 security/integrity/ima/ima_queue.c  |  8 ++++----
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4843077dc9e8..23d63bb96d2c 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_num_template_digests;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 1ee813d33bdc..f391ee3412b9 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -65,6 +65,7 @@ struct ima_algo_desc {
 };
 
 int ima_sha1_idx;
+int ima_hash_algo_idx;
 int ima_num_template_digests;
 
 static struct ima_algo_desc *ima_algo_array;
@@ -123,16 +124,26 @@ int __init ima_init_crypto(void)
 		nr_allocated_banks = ima_tpm_chip->nr_allocated_banks;
 
 	ima_sha1_idx = -1;
+	ima_hash_algo_idx = -1;
 	ima_num_template_digests = nr_allocated_banks;
 
 	for (i = 0; i < nr_allocated_banks; i++) {
 		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
 		if (algo == HASH_ALGO_SHA1)
 			ima_sha1_idx = i;
+
+		if (algo == ima_hash_algo)
+			ima_hash_algo_idx = i;
 	}
 
-	if (ima_sha1_idx < 0)
+	if (ima_sha1_idx < 0) {
 		ima_sha1_idx = ima_num_template_digests++;
+		if (ima_hash_algo == HASH_ALGO_SHA1)
+			ima_hash_algo_idx = ima_sha1_idx;
+	}
+
+	if (ima_hash_algo_idx < 0)
+		ima_hash_algo_idx = ima_num_template_digests++;
 
 	ima_algo_array = kcalloc(ima_num_template_digests,
 				 sizeof(*ima_algo_array), GFP_KERNEL);
@@ -173,7 +184,12 @@ int __init ima_init_crypto(void)
 			goto out_array;
 		}
 
-		ima_algo_array[i].algo = HASH_ALGO_SHA1;
+		ima_algo_array[i++].algo = HASH_ALGO_SHA1;
+	}
+
+	if (ima_hash_algo_idx >= nr_allocated_banks) {
+		ima_algo_array[i].tfm = ima_shash_tfm;
+		ima_algo_array[i].algo = ima_hash_algo;
 	}
 
 	return 0;
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];
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 3/8] ima: Evaluate error in init_ima()
  2020-02-05 10:33 ` [PATCH v2 3/8] ima: Evaluate error in init_ima() Roberto Sassu
@ 2020-02-05 10:39   ` Roberto Sassu
  0 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 10:39 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: Roberto Sassu
> Sent: Wednesday, February 5, 2020 11:33 AM
> To: zohar@linux.ibm.com; James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; 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 v2 3/8] ima: Evaluate error in init_ima()
> 
> Evaluate error in init_ima() before register_blocking_lsm_notifier() and
> return if not zero.
> 
> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Add in CC stable@vger.kernel.org.

Roberto

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

> ---
>  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 d7e987baf127..a16c148ed90d 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -738,6 +738,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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
       [not found]   ` <20200205144515.1DDE4217F4@mail.kernel.org>
@ 2020-02-05 15:36     ` Roberto Sassu
  0 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 15:36 UTC (permalink / raw)
  To: Sasha Levin, zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, stable, stable, Silviu Vlasceanu

> -----Original Message-----
> From: Sasha Levin [mailto:sashal@kernel.org]
> Sent: Wednesday, February 5, 2020 3:45 PM
> To: Sasha Levin <sashal@kernel.org>; Roberto Sassu
> <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> James.Bottomley@HansenPartnership.com
> Cc: linux-integrity@vger.kernel.org; stable@vger.kernel.org;
> stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
> 
> The bot has tested the following trees: v5.5.1, v5.4.17, v4.19.101, v4.14.169,
> v4.9.212, v4.4.212.
> 
> v5.5.1: Build OK!
> v5.4.17: Build OK!
> v4.19.101: Failed to apply! Possible dependencies:
>     100b16a6f290 ("tpm: sort objects in the Makefile")
>     1ad6640cd614 ("tpm: move tpm1_pcr_extend to tpm1-cmd.c")
>     70a3199a7101 ("tpm: factor out tpm_get_timeouts()")
>     879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR
> read")

Hi Sasha

this patch is necessary. However, backporting it won't be that easy
as it was part of a patch set. Before this patch, users of the TPM driver
could only read the SHA1 PCR bank. The IMA patch needs to read also
other PCR banks.

> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?

This question should be for Jarkko (added in CC), as some patches for the
TPM driver must be backported to apply the IMA patch.

Roberto

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
  2020-02-05 10:33 ` [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests Roberto Sassu
@ 2020-02-05 16:39   ` Roberto Sassu
  2020-02-06 16:08   ` Mimi Zohar
  1 sibling, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-05 16:39 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel, Silviu Vlasceanu

> -----Original Message-----
> From: Roberto Sassu
> Sent: Wednesday, February 5, 2020 11:33 AM
> To: zohar@linux.ibm.com; James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; 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 v2 5/8] ima: Switch to dynamically allocated buffer for
> template digests
> 
> 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, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> and will be determined in the upcoming patches depending on the allocated
> PCR banks and the chosen default IMA algorithm.
> 
> 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.
> 
> Changelog
> 
> 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          |  4 +++-
>  security/integrity/ima/ima_api.c      |  8 ++++++++
>  security/integrity/ima/ima_crypto.c   |  6 +++++-
>  security/integrity/ima/ima_fs.c       |  4 ++--
>  security/integrity/ima/ima_queue.c    | 10 ++++++----
>  security/integrity/ima/ima_template.c | 12 ++++++++++--
>  6 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2f380fb92a7a..4843077dc9e8 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -50,6 +50,8 @@ extern int ima_policy_flag;
> 
>  /* set during initialization */
>  extern int ima_hash_algo;
> +extern int ima_sha1_idx;
> +extern int ima_num_template_digests;
>  extern int ima_appraise;
>  extern struct tpm_chip *ima_tpm_chip;
> 
> @@ -92,7 +94,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..2ced5975c16f 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_num_template_digests,
> +				    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..31f2497a3ab8 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -59,6 +59,9 @@ 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;
> +int ima_num_template_digests = 1;

I forgot to add __ro_after_init.

Will do in the next version.

Roberto

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

>  int __init ima_init_crypto(void)
>  {
>  	long rc;
> @@ -502,7 +505,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..751f101ae927 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_num_template_digests,
> +				    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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-05 10:33 ` [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
       [not found]   ` <20200205144515.1DDE4217F4@mail.kernel.org>
@ 2020-02-05 20:41   ` Mimi Zohar
  2020-02-06  9:33     ` Roberto Sassu
  2020-02-05 21:00   ` Mimi Zohar
  2 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2020-02-05 20:41 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, stable

On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> 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 tries to find the SHA256 PCR
> bank (which is mandatory in the TCG PC Client specification). If also this
> bank is not found, the patch selects the first one. If the TPM algorithm
> of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
> 
> Changelog
> 
> v1:
> - add Mimi's comments
> - if there is no PCR bank for the IMA default algorithm use SHA256 or the
>   first bank (suggested by James Bottomley)

If the IMA default hash algorithm is not enabled, James' comment was
to use SHA256 for TPM 2.0 and SHA1 for TPM 1.2.  I don't remember him
saying anything about using the first bank, that was in v1 of my
patch.  Please refer to v2 of my patch, based on James' comments.

> 
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>  security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++----
>  security/integrity/ima/ima_init.c   | 22 ++++++++++----
>  2 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 73044fc6a952..f2f41a2bc3d4 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
>  }
>  
>  /*
> - * Calculate the boot aggregate hash
> + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.  With
> + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with
> + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> + * allowing firmware to configure and enable different banks.
> + *
> + * Knowing which TPM bank is read to calculate the boot_aggregate digest
> + * needs to be conveyed to a verifier.  For this reason, use the same
> + * hash algorithm for reading the TPM PCRs as for calculating the boot
> + * aggregate digest as stored in the measurement list.
>   */
> -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
>  					      struct crypto_shash *tfm)
>  {
> -	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> +	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
>  	int rc;
>  	u32 i;
>  	SHASH_DESC_ON_STACK(shash, tfm);
>  
>  	shash->tfm = tfm;
>  
> +	pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n",
> +		 d.alg_id);
> +

Good

>  	rc = crypto_shash_init(shash);
>  	if (rc != 0)
>  		return rc;
> @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
>  	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,
> +					 crypto_shash_digestsize(tfm));
>  	}
>  	if (!rc)
>  		crypto_shash_final(shash, digest);
> @@ -685,14 +697,35 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
>  int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
>  {
> 
< snip >
>  
>  	hash->length = crypto_shash_digestsize(tfm);
> -	rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
> +	alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> +	rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);

Sure, backporting this change to ima_calc_boot_aggregate_tfm() should
be fine.

Mimi

>  	ima_free_tfm(tfm);
>  


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-05 10:33 ` [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
       [not found]   ` <20200205144515.1DDE4217F4@mail.kernel.org>
  2020-02-05 20:41   ` Mimi Zohar
@ 2020-02-05 21:00   ` Mimi Zohar
  2020-02-06  9:36     ` Roberto Sassu
  2 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2020-02-05 21:00 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, stable

Hi Roberto,

On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:

<snip>

> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org

Cc'ing stable resulted in Sasha's automated message.  If you're going
to Cc stable, then please include the stable kernel release (e.g. Cc: 
stable@vger.kernel.org # v5.3).  Also please include a "Fixes" tag.
 Normally only bug fixes are backported.

Mimi


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  2020-02-05 10:33 ` [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
@ 2020-02-05 21:57   ` Jarkko Sakkinen
  0 siblings, 0 replies; 26+ messages in thread
From: Jarkko Sakkinen @ 2020-02-05 21:57 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, James.Bottomley, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, stable

On Wed, Feb 05, 2020 at 11:33:10AM +0100, Roberto Sassu wrote:
> chip->allocated_banks, an array of tpm_bank_info structures, 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 of tpm_bank_info 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).
> 
> Thus, initialize crypto_id to HASH_ALGO__LAST.
> 
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Cc: stable@vger.kernel.org

Cc should be first.

/Jarkko

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank
  2020-02-05 10:33 ` [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank Roberto Sassu
@ 2020-02-05 23:41   ` kbuild test robot
  2020-02-05 23:41   ` [RFC PATCH] ima: ima_init_ima_crypto() can be static kbuild test robot
  1 sibling, 0 replies; 26+ messages in thread
From: kbuild test robot @ 2020-02-05 23:41 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: kbuild-all, zohar, James.Bottomley, jarkko.sakkinen,
	linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

Hi Roberto,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on integrity/next-integrity]
[also build test WARNING on jss-tpmdd/next v5.5 next-20200205]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-support-stronger-algorithms-for-attestation/20200205-233901
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-154-g1dc00f87-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> security/integrity/ima/ima_crypto.c:72:12: sparse: sparse: symbol 'ima_init_ima_crypto' was not declared. Should it be static?
   security/integrity/ima/ima_crypto.c:545:36: sparse: sparse: invalid assignment: |=
   security/integrity/ima/ima_crypto.c:545:36: sparse:    left side has type unsigned int
   security/integrity/ima/ima_crypto.c:545:36: sparse:    right side has type restricted fmode_t
   security/integrity/ima/ima_crypto.c:565:28: sparse: sparse: invalid assignment: &=
   security/integrity/ima/ima_crypto.c:565:28: sparse:    left side has type unsigned int
   security/integrity/ima/ima_crypto.c:565:28: sparse:    right side has type restricted fmode_t
   security/integrity/ima/ima_crypto.c:592:52: sparse: sparse: restricted __le32 degrades to integer

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH] ima: ima_init_ima_crypto() can be static
  2020-02-05 10:33 ` [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank Roberto Sassu
  2020-02-05 23:41   ` kbuild test robot
@ 2020-02-05 23:41   ` kbuild test robot
  1 sibling, 0 replies; 26+ messages in thread
From: kbuild test robot @ 2020-02-05 23:41 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: kbuild-all, zohar, James.Bottomley, jarkko.sakkinen,
	linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu


Fixes: d49f85a89d4e ("ima: Allocate and initialize tfm for each PCR bank")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 ima_crypto.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7b63a32eefea17..d865e26839ef6d 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -69,7 +69,7 @@ int ima_num_template_digests;
 
 static struct ima_algo_desc *ima_algo_array;
 
-int __init ima_init_ima_crypto(void)
+static int __init ima_init_ima_crypto(void)
 {
 	long rc;
 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-05 20:41   ` Mimi Zohar
@ 2020-02-06  9:33     ` Roberto Sassu
  0 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-06  9:33 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, February 5, 2020 9:41 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > 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 tries to find the SHA256 PCR
> > bank (which is mandatory in the TCG PC Client specification). If also this
> > bank is not found, the patch selects the first one. If the TPM algorithm
> > of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
> >
> > Changelog
> >
> > v1:
> > - add Mimi's comments
> > - if there is no PCR bank for the IMA default algorithm use SHA256 or the
> >   first bank (suggested by James Bottomley)
> 
> If the IMA default hash algorithm is not enabled, James' comment was
> to use SHA256 for TPM 2.0 and SHA1 for TPM 1.2.  I don't remember him
> saying anything about using the first bank, that was in v1 of my
> patch.  Please refer to v2 of my patch, based on James' comments.

Right. Will fix in the next version.

Thanks

Roberto

 > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Suggested-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  security/integrity/ima/ima_crypto.c | 45
> +++++++++++++++++++++++++----
> >  security/integrity/ima/ima_init.c   | 22 ++++++++++----
> >  2 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> > index 73044fc6a952..f2f41a2bc3d4 100644
> > --- a/security/integrity/ima/ima_crypto.c
> > +++ b/security/integrity/ima/ima_crypto.c
> > @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct
> tpm_digest *d)
> >  }
> >
> >  /*
> > - * Calculate the boot aggregate hash
> > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.
> With
> > + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but
> with
> > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> > + * allowing firmware to configure and enable different banks.
> > + *
> > + * Knowing which TPM bank is read to calculate the boot_aggregate
> digest
> > + * needs to be conveyed to a verifier.  For this reason, use the same
> > + * hash algorithm for reading the TPM PCRs as for calculating the boot
> > + * aggregate digest as stored in the measurement list.
> >   */
> > -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> > +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
> >  					      struct crypto_shash *tfm)
> >  {
> > -	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> > +	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
> >  	int rc;
> >  	u32 i;
> >  	SHASH_DESC_ON_STACK(shash, tfm);
> >
> >  	shash->tfm = tfm;
> >
> > +	pr_devel("calculating the boot-aggregate based on TPM
> bank: %04x\n",
> > +		 d.alg_id);
> > +
> 
> Good
> 
> >  	rc = crypto_shash_init(shash);
> >  	if (rc != 0)
> >  		return rc;
> > @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char
> *digest,
> >  	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,
> > +					 crypto_shash_digestsize(tfm));
> >  	}
> >  	if (!rc)
> >  		crypto_shash_final(shash, digest);
> > @@ -685,14 +697,35 @@ static int __init
> ima_calc_boot_aggregate_tfm(char *digest,
> >  int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
> >  {
> >
> < snip >
> >
> >  	hash->length = crypto_shash_digestsize(tfm);
> > -	rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
> > +	alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> > +	rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
> 
> Sure, backporting this change to ima_calc_boot_aggregate_tfm() should
> be fine.
> 
> Mimi
> 
> >  	ima_free_tfm(tfm);
> >


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-05 21:00   ` Mimi Zohar
@ 2020-02-06  9:36     ` Roberto Sassu
  2020-02-06 12:17       ` Mimi Zohar
  0 siblings, 1 reply; 26+ messages in thread
From: Roberto Sassu @ 2020-02-06  9:36 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Wednesday, February 5, 2020 10:01 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> Hi Roberto,
> 
> On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> 
> <snip>
> 
> > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Suggested-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Cc: stable@vger.kernel.org
> 
> Cc'ing stable resulted in Sasha's automated message.  If you're going
> to Cc stable, then please include the stable kernel release (e.g. Cc:
> stable@vger.kernel.org # v5.3).  Also please include a "Fixes" tag.
>  Normally only bug fixes are backported.

Ok, will add the kernel version. I also thought which commit I should
mention in the Fixes tag. IMA always read the SHA1 bank from the
beginning. I could mention the patch that introduces the new API
to read other banks, but I'm not sure. What do you think?

Thanks

Roberto

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-06  9:36     ` Roberto Sassu
@ 2020-02-06 12:17       ` Mimi Zohar
  2020-02-06 12:28         ` Roberto Sassu
  0 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2020-02-06 12:17 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

On Thu, 2020-02-06 at 09:36 +0000, Roberto Sassu wrote:
> > Hi Roberto,
> > 
> > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > 
> > <snip>
> > 
> > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Suggested-by: James Bottomley
> > <James.Bottomley@HansenPartnership.com>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Cc: stable@vger.kernel.org
> > 
> > Cc'ing stable resulted in Sasha's automated message.  If you're going
> > to Cc stable, then please include the stable kernel release (e.g. Cc:
> > stable@vger.kernel.org # v5.3).  Also please include a "Fixes" tag.
> >  Normally only bug fixes are backported.
> 
> Ok, will add the kernel version. I also thought which commit I should
> mention in the Fixes tag. IMA always read the SHA1 bank from the
> beginning. I could mention the patch that introduces the new API
> to read other banks, but I'm not sure. What do you think?

This patch is dependent on nr_allocated_banks.  Please try applying
this patch to the earliest stable kernel with the commit that
introduces nr_allocated_banks and test to make sure it works properly.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-06 12:17       ` Mimi Zohar
@ 2020-02-06 12:28         ` Roberto Sassu
  2020-02-06 12:31           ` Mimi Zohar
  0 siblings, 1 reply; 26+ messages in thread
From: Roberto Sassu @ 2020-02-06 12:28 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, February 6, 2020 1:17 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> On Thu, 2020-02-06 at 09:36 +0000, Roberto Sassu wrote:
> > > Hi Roberto,
> > >
> > > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > >
> > > <snip>
> > >
> > > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > Suggested-by: James Bottomley
> > > <James.Bottomley@HansenPartnership.com>
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Cc: stable@vger.kernel.org
> > >
> > > Cc'ing stable resulted in Sasha's automated message.  If you're going
> > > to Cc stable, then please include the stable kernel release (e.g. Cc:
> > > stable@vger.kernel.org # v5.3).  Also please include a "Fixes" tag.
> > >  Normally only bug fixes are backported.
> >
> > Ok, will add the kernel version. I also thought which commit I should
> > mention in the Fixes tag. IMA always read the SHA1 bank from the
> > beginning. I could mention the patch that introduces the new API
> > to read other banks, but I'm not sure. What do you think?
> 
> This patch is dependent on nr_allocated_banks.  Please try applying
> this patch to the earliest stable kernel with the commit that
> introduces nr_allocated_banks and test to make sure it works properly.

It also depends on 879b589210a9 ("tpm: retrieve digest size of unknown"
algorithms with PCR read") which exported the mapping between TPM
algorithm ID and crypto ID, and changed the definition of tpm_pcr_read()
to read non-SHA1 PCR banks. It requires many patches, so backporting it
is not a trivial task. I think the earliest kernel this patch can be backported to
is 5.1.

Roberto

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-06 12:28         ` Roberto Sassu
@ 2020-02-06 12:31           ` Mimi Zohar
  0 siblings, 0 replies; 26+ messages in thread
From: Mimi Zohar @ 2020-02-06 12:31 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

On Thu, 2020-02-06 at 12:28 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Thursday, February 6, 2020 1:17 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > James.Bottomley@HansenPartnership.com;
> > jarkko.sakkinen@linux.intel.com
> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> > aggregate
> > 
> > On Thu, 2020-02-06 at 09:36 +0000, Roberto Sassu wrote:
> > > > Hi Roberto,
> > > >
> > > > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > > >
> > > > <snip>
> > > >
> > > > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > Suggested-by: James Bottomley
> > > > <James.Bottomley@HansenPartnership.com>
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Cc: stable@vger.kernel.org
> > > >
> > > > Cc'ing stable resulted in Sasha's automated message.  If you're going
> > > > to Cc stable, then please include the stable kernel release (e.g. Cc:
> > > > stable@vger.kernel.org # v5.3).  Also please include a "Fixes" tag.
> > > >  Normally only bug fixes are backported.
> > >
> > > Ok, will add the kernel version. I also thought which commit I should
> > > mention in the Fixes tag. IMA always read the SHA1 bank from the
> > > beginning. I could mention the patch that introduces the new API
> > > to read other banks, but I'm not sure. What do you think?
> > 
> > This patch is dependent on nr_allocated_banks.  Please try applying
> > this patch to the earliest stable kernel with the commit that
> > introduces nr_allocated_banks and test to make sure it works properly.
> 
> It also depends on 879b589210a9 ("tpm: retrieve digest size of unknown"
> algorithms with PCR read") which exported the mapping between TPM
> algorithm ID and crypto ID, and changed the definition of tpm_pcr_read()
> to read non-SHA1 PCR banks. It requires many patches, so backporting it
> is not a trivial task. I think the earliest kernel this patch can be backported to
> is 5.1.

Agreed.  Thank you for checking.

Mimi


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
  2020-02-05 10:33 ` [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests Roberto Sassu
  2020-02-05 16:39   ` Roberto Sassu
@ 2020-02-06 16:08   ` Mimi Zohar
  2020-02-06 16:27     ` Roberto Sassu
  1 sibling, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2020-02-06 16:08 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel, silviu.vlasceanu

Hi Roberto,

On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> 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, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> and will be determined in the upcoming patches depending on the allocated
> PCR banks and the chosen default IMA algorithm.
> 
> 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.
> 
> Changelog
> 
> v1:
> - move ima_sha1_idx to ima_crypto.c
> - introduce ima_num_template_digests (suggested by Mimi)

Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
2", I suggested defining "nr_allocated_banks + extra", where "extra"
could be 0, 1, or 2.

The rest of the code would remain exactly the same as you had.

Mimi


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
  2020-02-06 16:08   ` Mimi Zohar
@ 2020-02-06 16:27     ` Roberto Sassu
  2020-02-06 16:33       ` Mimi Zohar
  0 siblings, 1 reply; 26+ messages in thread
From: Roberto Sassu @ 2020-02-06 16:27 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel, Silviu Vlasceanu

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, February 6, 2020 5:08 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for
> template digests
> 
> Hi Roberto,
> 
> On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > 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, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> > and will be determined in the upcoming patches depending on the
> allocated
> > PCR banks and the chosen default IMA algorithm.
> >
> > 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.
> >
> > Changelog
> >
> > v1:
> > - move ima_sha1_idx to ima_crypto.c
> > - introduce ima_num_template_digests (suggested by Mimi)
> 
> Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
> 2", I suggested defining "nr_allocated_banks + extra", where "extra"
> could be 0, 1, or 2.
> 
> The rest of the code would remain exactly the same as you had.

Ok. I did a small improvement. Since we determine the number of
required elements of ima_algo_array before kmalloc() I thought it
was ok to directly set that number of elements in a single variable.

If you think that having two variables is better, I will change it.

Thanks

Roberto

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
  2020-02-06 16:27     ` Roberto Sassu
@ 2020-02-06 16:33       ` Mimi Zohar
  2020-02-06 16:36         ` Roberto Sassu
  0 siblings, 1 reply; 26+ messages in thread
From: Mimi Zohar @ 2020-02-06 16:33 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel, Silviu Vlasceanu

On Thu, 2020-02-06 at 16:27 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Thursday, February 6, 2020 5:08 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > James.Bottomley@HansenPartnership.com;
> > jarkko.sakkinen@linux.intel.com
> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for
> > template digests
> > 
> > Hi Roberto,
> > 
> > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > > 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, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> > > and will be determined in the upcoming patches depending on the
> > allocated
> > > PCR banks and the chosen default IMA algorithm.
> > >
> > > 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.
> > >
> > > Changelog
> > >
> > > v1:
> > > - move ima_sha1_idx to ima_crypto.c
> > > - introduce ima_num_template_digests (suggested by Mimi)
> > 
> > Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
> > 2", I suggested defining "nr_allocated_banks + extra", where "extra"
> > could be 0, 1, or 2.
> > 
> > The rest of the code would remain exactly the same as you had.
> 
> Ok. I did a small improvement. Since we determine the number of
> required elements of ima_algo_array before kmalloc() I thought it
> was ok to directly set that number of elements in a single variable.
> 
> If you think that having two variables is better, I will change it.

The connection to nr_allocated_banks is then not as visible.  Using
two variables is clearer.

Mimi


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
  2020-02-06 16:33       ` Mimi Zohar
@ 2020-02-06 16:36         ` Roberto Sassu
  0 siblings, 0 replies; 26+ messages in thread
From: Roberto Sassu @ 2020-02-06 16:36 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel, Silviu Vlasceanu

> -----Original Message-----
> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Thursday, February 6, 2020 5:34 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for
> template digests
> 
> On Thu, 2020-02-06 at 16:27 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Thursday, February 6, 2020 5:08 PM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > > James.Bottomley@HansenPartnership.com;
> > > jarkko.sakkinen@linux.intel.com
> > > Cc: linux-integrity@vger.kernel.org; linux-security-
> module@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > > <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer
> for
> > > template digests
> > >
> > > Hi Roberto,
> > >
> > > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > > > 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, stored in ima_num_template_digests, is initially equal to 1
> (SHA1)
> > > > and will be determined in the upcoming patches depending on the
> > > allocated
> > > > PCR banks and the chosen default IMA algorithm.
> > > >
> > > > 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.
> > > >
> > > > Changelog
> > > >
> > > > v1:
> > > > - move ima_sha1_idx to ima_crypto.c
> > > > - introduce ima_num_template_digests (suggested by Mimi)
> > >
> > > Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
> > > 2", I suggested defining "nr_allocated_banks + extra", where "extra"
> > > could be 0, 1, or 2.
> > >
> > > The rest of the code would remain exactly the same as you had.
> >
> > Ok. I did a small improvement. Since we determine the number of
> > required elements of ima_algo_array before kmalloc() I thought it
> > was ok to directly set that number of elements in a single variable.
> >
> > If you think that having two variables is better, I will change it.
> 
> The connection to nr_allocated_banks is then not as visible.  Using
> two variables is clearer.

Ok, no problem. I will change it in the next version.

Roberto

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 10:33 [PATCH v2 0/8] ima: support stronger algorithms for attestation Roberto Sassu
2020-02-05 10:33 ` [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
2020-02-05 21:57   ` Jarkko Sakkinen
2020-02-05 10:33 ` [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
     [not found]   ` <20200205144515.1DDE4217F4@mail.kernel.org>
2020-02-05 15:36     ` Roberto Sassu
2020-02-05 20:41   ` Mimi Zohar
2020-02-06  9:33     ` Roberto Sassu
2020-02-05 21:00   ` Mimi Zohar
2020-02-06  9:36     ` Roberto Sassu
2020-02-06 12:17       ` Mimi Zohar
2020-02-06 12:28         ` Roberto Sassu
2020-02-06 12:31           ` Mimi Zohar
2020-02-05 10:33 ` [PATCH v2 3/8] ima: Evaluate error in init_ima() Roberto Sassu
2020-02-05 10:39   ` Roberto Sassu
2020-02-05 10:33 ` [PATCH v2 4/8] ima: Store template digest directly in ima_template_entry Roberto Sassu
2020-02-05 10:33 ` [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests Roberto Sassu
2020-02-05 16:39   ` Roberto Sassu
2020-02-06 16:08   ` Mimi Zohar
2020-02-06 16:27     ` Roberto Sassu
2020-02-06 16:33       ` Mimi Zohar
2020-02-06 16:36         ` Roberto Sassu
2020-02-05 10:33 ` [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank Roberto Sassu
2020-02-05 23:41   ` kbuild test robot
2020-02-05 23:41   ` [RFC PATCH] ima: ima_init_ima_crypto() can be static kbuild test robot
2020-02-05 10:33 ` [PATCH v2 7/8] ima: Calculate and extend PCR with digests in ima_template_entry Roberto Sassu
2020-02-05 10:33 ` [PATCH v2 8/8] ima: Use ima_hash_algo for collision detection in the measurement list Roberto Sassu

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git