> -----Original Message----- > From: Mimi Zohar [mailto:zohar@xxxxxxxxxxxxx] > Sent: Wednesday, February 5, 2020 9:41 PM > To: Roberto Sassu <roberto.sassu@xxxxxxxxxx>; > James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; > jarkko.sakkinen@xxxxxxxxxxxxxxx > Cc: linux-integrity@xxxxxxxxxxxxxxx; linux-security-module@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; Silviu Vlasceanu > <Silviu.Vlasceanu@xxxxxxxxxx>; stable@xxxxxxxxxxxxxxx > 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@xxxxxxxxxx> > > Suggested-by: James Bottomley > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > Cc: stable@xxxxxxxxxxxxxxx > > --- > > 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); > >
