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