-----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
[off-list ref]; 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:

quoted

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

quoted

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.

quoted

This patch tries to use ima_hash_algo as hash algorithm for

boot_aggregate.

quoted

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 [off-list ref]

quoted

Suggested-by: James Bottomley

[off-list ref]

quoted

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
 security/integrity/ima/ima_crypto.c | 45

+++++++++++++++++++++++++----

quoted

 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

quoted

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)

quoted

 }

 /*
- * Calculate the boot aggregate hash
+ * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.

With

quoted

+ * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but

with

quoted

+ * 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

quoted

+ * 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",

quoted

+		 d.alg_id);
+

Good

quoted

 	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;

@@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char

*digest,

quoted

 	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);

quoted

+		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,

quoted

 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
 {

< snip >

quoted

 	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

quoted

 	ima_free_tfm(tfm);