| Message ID | 20200210100048.21448-3-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: support stronger algorithms for attestation | expand |
On Mon, 2020-02-10 at 11:00 +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). Up to here, the patch description matches the code. > 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. This comment and the one inline are left over from previous version. > > Changelog > > v1: > - add Mimi's comments > - if there is no PCR bank for the IMA default algorithm use SHA256 > (suggested by James Bottomley) > > Cc: stable@vger.kernel.org # 5.1.x > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read") > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> > Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Thanks, Roberto. This patch is dependent on 1/8. As soon as there's a topic branch, I'll queue this, removing the extraneous comments. Mimi > --- > 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. The last line of the above comment is left over from the previous version. > + */ > if (ima_tpm_chip) { > result = ima_calc_boot_aggregate(&hash.hdr); > if (result < 0) {
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Monday, February 10, 2020 11:24 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 v3 2/8] ima: Switch to ima_hash_algo for boot > aggregate > > On Mon, 2020-02-10 at 11:00 +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). > > Up to here, the patch description matches the code. > > 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. > > This comment and the one inline are left over from previous version. Hi Mimi actually the code does what is described above. bank_idx is initially set to zero and remains as it is if there is no PCR bank for the default IMA algorithm or SHA256. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > > Changelog > > > > v1: > > - add Mimi's comments > > - if there is no PCR bank for the IMA default algorithm use SHA256 > > (suggested by James Bottomley) > > > > Cc: stable@vger.kernel.org # 5.1.x > > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms > with PCR read") > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Suggested-by: James Bottomley > <James.Bottomley@HansenPartnership.com> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Thanks, Roberto. This patch is dependent on 1/8. As soon as there's > a topic branch, I'll queue this, removing the extraneous comments. > > Mimi > > > --- > > 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. > > The last line of the above comment is left over from the previous > version. > > > + */ > > if (ima_tpm_chip) { > > result = ima_calc_boot_aggregate(&hash.hdr); > > if (result < 0) {
On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote: > > -----Original Message----- Please find/use a mailer that doesn't include this junk. > > On Mon, 2020-02-10 at 11:00 +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). > > > > Up to here, the patch description matches the code. > > > 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. > > > > This comment and the one inline are left over from previous version. > > Hi Mimi > > actually the code does what is described above. bank_idx is initially > set to zero and remains as it is if there is no PCR bank for the default > IMA algorithm or SHA256. Sorry for the delay in continuing to review this patch set. It took a while to write ima-evm-utils regression tests for it. Dmitry and you were the ones that initiated ima-evm-utils, saying there should a single package for signing files and integrity testing. The features in ima-evm-utils should reflect what is actually upstreamed in the kernel. (Currently there are a few experimental features which were never upstreamed. I'd like to remove them, but am a bit concerned that they are being used.) I'd appreciate your help in keeping ima-evm-utils up to date. It will help simplify upstreaming new kernel features. My initial patch attempted to use any common TPM and kernel hash algorithm to calculate the boot_aggregate. The discussion with James was pretty clear, which you even stated in the Changelog. Either we use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM 1.2 for the boot-aggregate. thanks, Mimi
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Monday, March 2, 2020 2:42 PM > To: Roberto Sassu <roberto.sassu@huawei.com>; > James.Bottomley@HansenPartnership.com; > jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin > <dmitry.kasatkin@gmail.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 v3 2/8] ima: Switch to ima_hash_algo for boot > aggregate > > On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote: > > > -----Original Message----- > > Please find/use a mailer that doesn't include this junk. I will do. I didn't have the time yet. > > > On Mon, 2020-02-10 at 11:00 +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). > > > > > > Up to here, the patch description matches the code. > > > > 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. > > > > > > This comment and the one inline are left over from previous version. > > > > Hi Mimi > > > > actually the code does what is described above. bank_idx is initially > > set to zero and remains as it is if there is no PCR bank for the default > > IMA algorithm or SHA256. > > Sorry for the delay in continuing to review this patch set. It took a > while to write ima-evm-utils regression tests for it. > > Dmitry and you were the ones that initiated ima-evm-utils, saying > there should a single package for signing files and integrity testing. > The features in ima-evm-utils should reflect what is actually > upstreamed in the kernel. (Currently there are a few experimental > features which were never upstreamed. I'd like to remove them, but am > a bit concerned that they are being used.) I'd appreciate your help > in keeping ima-evm-utils up to date. It will help simplify > upstreaming new kernel features. > > My initial patch attempted to use any common TPM and kernel hash > algorithm to calculate the boot_aggregate. The discussion with James > was pretty clear, which you even stated in the Changelog. Either we > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM > 1.2 for the boot-aggregate. Ok, I didn't understand fully. I thought we should use the default IMA algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no PCR bank for default algorithm. I additionally implemented the logic to select the first PCR bank if the SHA256 PCR bank is not available but I can remove it. SHA256 should be the minimum requirement for boot aggregate. The advantage of using the default IMA algorithm is that it will be possible to select stronger algorithms when they are supported by the TPM. We might introduce a new option to specify only the algorithm for boot aggregate, like James suggested to support embedded systems. Let me know which option you prefer. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
> > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote: > > My initial patch attempted to use any common TPM and kernel hash > > algorithm to calculate the boot_aggregate. The discussion with James > > was pretty clear, which you even stated in the Changelog. Either we > > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM > > 1.2 for the boot-aggregate. > > Ok, I didn't understand fully. I thought we should use the default IMA > algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no > PCR bank for default algorithm. Yes, preference is given to the IMA default algorithm, but it should fall back to using SHA256 or SHA1, based on the TPM. > I additionally implemented the logic to > select the first PCR bank if the SHA256 PCR bank is not available but I can > remove it. > > SHA256 should be the minimum requirement for boot aggregate. The > advantage of using the default IMA algorithm is that it will be possible to > select stronger algorithms when they are supported by the TPM. We might > introduce a new option to specify only the algorithm for boot aggregate, > like James suggested to support embedded systems. Let me know which > option you prefer. I don't remember James saying that, but if the community really wants that support, then it should be upstreamed independently, as a separate patch. Let's first get the basics working. thanks, Mimi
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Monday, March 2, 2020 3:47 PM > To: Roberto Sassu <roberto.sassu@huawei.com>; > James.Bottomley@HansenPartnership.com; > jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin > <dmitry.kasatkin@gmail.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 v3 2/8] ima: Switch to ima_hash_algo for boot > aggregate > > > > > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote: > > > My initial patch attempted to use any common TPM and kernel hash > > > algorithm to calculate the boot_aggregate. The discussion with James > > > was pretty clear, which you even stated in the Changelog. Either we > > > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for > TPM > > > 1.2 for the boot-aggregate. > > > > Ok, I didn't understand fully. I thought we should use the default IMA > > algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no > > PCR bank for default algorithm. > > Yes, preference is given to the IMA default algorithm, but it should > fall back to using SHA256 or SHA1, based on the TPM. Ok. The patch already does it even if the TPM version is not checked. For TPM 1.2, if the default algorithm is not SHA1 the patch will select the first PCR bank (SHA1). Should I send a new patch which explicitly checks the TPM version? > > I additionally implemented the logic to > > select the first PCR bank if the SHA256 PCR bank is not available but I can > > remove it. > > > > SHA256 should be the minimum requirement for boot aggregate. The > > advantage of using the default IMA algorithm is that it will be possible to > > select stronger algorithms when they are supported by the TPM. We > might > > introduce a new option to specify only the algorithm for boot aggregate, > > like James suggested to support embedded systems. Let me know which > > option you prefer. > > I don't remember James saying that, but if the community really wants > that support, then it should be upstreamed independently, as a > separate patch. Let's first get the basics working. Ok. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Mon, 2020-03-02 at 15:11 +0000, Roberto Sassu wrote: > > Yes, preference is given to the IMA default algorithm, but it should > > fall back to using SHA256 or SHA1, based on the TPM. > > Ok. The patch already does it even if the TPM version is not checked. > For TPM 1.2, if the default algorithm is not SHA1 the patch will select > the first PCR bank (SHA1). > > Should I send a new patch which explicitly checks the TPM version? Checking the TPM version shouldn't be necessary. The code currently sets bank_idx to the HASH_ALGO_SHA256. If instead of initializing bank_idx to 0, initialize it to the nr_allocated_banks or -1. As long as the bank_idx value is the same as the initialized value, set the bank_idx to HASH_ALGO_SHA1. The subsequent bank_idx would then be limited to testing for the initialized value. Mimi
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) {
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 (suggested by James Bottomley) Cc: stable@vger.kernel.org # 5.1.x Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read") Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++---- security/integrity/ima/ima_init.c | 22 ++++++++++---- 2 files changed, 56 insertions(+), 11 deletions(-)