Change in ...coreboot[master]: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33252
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 65 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/1
diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c index 4698a4d..df1ca3b 100644 --- a/src/security/tpm/tspi/tspi.c +++ b/src/security/tpm/tspi/tspi.c @@ -4,6 +4,7 @@ * Copyright (c) 2013 The Chromium OS Authors. All rights reserved. * Copyright 2017 Facebook Inc. * Copyright 2018 Siemens AG + * Copyright 2019 Eltan B.V. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -209,7 +210,28 @@ if (!digest) return TPM_E_IOERROR;
- result = tlcl_extend(pcr, digest, NULL); + if (CONFIG(TPM2)) { + TPML_DIGEST_VALUES tpml_digests; + + tpml_digests.count = 1; + switch (digest_algo) { + case VB2_HASH_SHA1: + tpml_digests.digests[0].hashAlg = TPM_ALG_SHA1; + memcpy(tpml_digests.digests[0].digest.sha1, digest, + sizeof(TPMU_HA)); + break; + case VB2_HASH_SHA256: + tpml_digests.digests[0].hashAlg = TPM_ALG_SHA256; + memcpy(tpml_digests.digests[0].digest.sha256, digest, + sizeof(TPMU_HA)); + break; + default: + return TPM_E_IOERROR; + } + result = tlcl_extend(pcr, (uint8_t *)&tpml_digests, NULL); + } else { + result = tlcl_extend(pcr, digest, NULL); + } if (result != TPM_SUCCESS) return result;
diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c index e579bff..4574b4f 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -126,24 +126,53 @@ }
/* - * The caller will provide the digest in a 32 byte buffer, let's consider it a - * sha256 digest. + * The caller will provide the digest in a 32 byte buffer */ uint32_t tlcl_extend(int pcr_num, const uint8_t *in_digest, uint8_t *out_digest) { struct tpm2_pcr_extend_cmd pcr_ext_cmd; struct tpm2_response *response; + int i; + TPML_DIGEST_VALUES *tpml_digests;
pcr_ext_cmd.pcrHandle = HR_PCR + pcr_num; - pcr_ext_cmd.digests.count = 1; - pcr_ext_cmd.digests.digests[0].hashAlg = TPM_ALG_SHA256; - memcpy(pcr_ext_cmd.digests.digests[0].digest.sha256, in_digest, - sizeof(pcr_ext_cmd.digests.digests[0].digest.sha256)); + tpml_digests = (TPML_DIGEST_VALUES *)in_digest; + pcr_ext_cmd.digests.count = tpml_digests->count; + + for (i = 0; i < tpml_digests->count ; i++) { + pcr_ext_cmd.digests.digests[i].hashAlg = + tpml_digests->digests[i].hashAlg; + switch (tpml_digests->digests[i].hashAlg) { + case TPM_ALG_SHA1: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sha1, + tpml_digests->digests[i].digest.sha1, + sizeof(TPMU_HA)); + break; + case TPM_ALG_SHA256: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sha256, + tpml_digests->digests[i].digest.sha256, + sizeof(TPMU_HA)); + } + }
response = tpm_process_command(TPM2_PCR_Extend, &pcr_ext_cmd);
- printk(BIOS_INFO, "%s: response is %x\n", + /* + * Check if we are invalidating the pcrs, ignore the error if this is + * the case + */ + if ((tpml_digests->count == 1) && + (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) && + (tpml_digests->digests[0].digest.invalidate_pcrs == 1) && + response && (response->hdr.tpm_code & ~TPM_RC_N_MASK) == + (TPM_RC_P | TPM_RC_HASH)) { + printk(BIOS_SPEW, "%s: TPM_RC_HASH returned this is" + " expected\n", __func__); + return TPM_SUCCESS; + } + + printk(BIOS_INFO, "%s: response is 0x%x\n", __func__, response ? response->hdr.tpm_code : -1); if (!response || response->hdr.tpm_code) return TPM_E_IOERROR; diff --git a/src/security/tpm/tss/tcg-2.0/tss_structures.h b/src/security/tpm/tss/tcg-2.0/tss_structures.h index 991cbcf..b11a9f4 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -79,6 +79,12 @@ space is defined by the lower 16 bits. */ #define TPM_CC_VENDOR_BIT_MASK 0x20000000
+/* Table 15 - TPM_RC Constants (Actions) */ +#define RC_FMT1 (TPM_RC)(0x080) +#define TPM_RC_HASH (TPM_RC)(RC_FMT1 + 0x003) +#define TPM_RC_P (TPM_RC)(0x040) +#define TPM_RC_N_MASK (TPM_RC)(0xF00) + /* Startup values. */ #define TPM_SU_CLEAR 0 #define TPM_SU_STATE 1
Hello Patrick Rudolph, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33252
to look at the new patch set (#2).
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 76 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/2
Hello Patrick Rudolph, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33252
to look at the new patch set (#3).
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 78 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/3
Hello Patrick Rudolph, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33252
to look at the new patch set (#4).
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 75 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/33252/4/src/security/tpm/tspi/tspi.c File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/#/c/33252/4/src/security/tpm/tspi/tspi.c@220 PS4, Line 220: memcpy(tpml_digests.digests[0].digest.sha1, digest, sizeof(TPMU_HA)); line over 80 characters
https://review.coreboot.org/#/c/33252/4/src/security/tpm/tspi/tspi.c@224 PS4, Line 224: memcpy(tpml_digests.digests[0].digest.sha256, digest, sizeof(TPMU_HA)); line over 80 characters
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_structures.h:
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tss/tcg-2.... PS4, Line 288: /* : * This is a union, but as of now we support just one digest - sha256, so : * there is just one element. : */ The comment need to be updated.
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tss/tcg-2.... PS4, Line 294: invalidate_pcrs; That's not part of spec right? Do we really need that?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 4:
(4 comments)
Will upload new patchste
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tspi/tspi.... PS4, Line 220: memcpy(tpml_digests.digests[0].digest.sha1, digest, sizeof(TPMU_HA));
line over 80 characters
Done
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tspi/tspi.... PS4, Line 224: memcpy(tpml_digests.digests[0].digest.sha256, digest, sizeof(TPMU_HA));
line over 80 characters
Done
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_structures.h:
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tss/tcg-2.... PS4, Line 288: /* : * This is a union, but as of now we support just one digest - sha256, so : * there is just one element. : */
The comment need to be updated.
Done
https://review.coreboot.org/c/coreboot/+/33252/4/src/security/tpm/tss/tcg-2.... PS4, Line 294: invalidate_pcrs;
That's not part of spec right? Do we really need that?
Used for compare, but not needed anymore.
Hello Patrick Rudolph, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33252
to look at the new patch set (#5).
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 76 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/5
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 5: Code-Review+1
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/5/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/5/src/security/tpm/tss/tcg-2.... PS5, Line 156: } please implement all hashes here that are supported in the other patch i just merged
Hello Patrick Rudolph, David Hendricks, Lance Zhao, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33252
to look at the new patch set (#6).
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 85 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/6
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 6:
(1 comment)
Created new patch solving the comment
https://review.coreboot.org/c/coreboot/+/33252/5/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/5/src/security/tpm/tss/tcg-2.... PS5, Line 156: }
please implement all hashes here that are supported in the other patch i just merged
Done
Hello Patrick Rudolph, David Hendricks, Lance Zhao, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33252
to look at the new patch set (#7).
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 88 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33252/7
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()
To support multi digists the tlcl_extend() for TPM2 expects TPML_DIGEST_VALUE pointer as input argument.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I8d86c41c23e4e93a84e0527d7cddcfd30d5d8394 Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33252 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Lance Zhao lance.zhao@gmail.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 3 files changed, 88 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Lance Zhao: Looks good to me, approved
diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c index 4698a4d..4cf3711 100644 --- a/src/security/tpm/tspi/tspi.c +++ b/src/security/tpm/tspi/tspi.c @@ -4,6 +4,7 @@ * Copyright (c) 2013 The Chromium OS Authors. All rights reserved. * Copyright 2017 Facebook Inc. * Copyright 2018 Siemens AG + * Copyright 2019 Eltan B.V. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -20,6 +21,7 @@ #include <security/tpm/tspi.h> #include <security/tpm/tss.h> #include <stdlib.h> +#include <string.h> #if CONFIG(VBOOT) #include <vb2_api.h> #include <vb2_sha.h> @@ -209,7 +211,28 @@ if (!digest) return TPM_E_IOERROR;
+#if CONFIG(TPM2) + TPML_DIGEST_VALUES tpml_digests; + + tpml_digests.count = 1; + switch (digest_algo) { + case VB2_HASH_SHA1: + tpml_digests.digests[0].hashAlg = TPM_ALG_SHA1; + memcpy(tpml_digests.digests[0].digest.sha1, + digest, sizeof(TPMU_HA)); + break; + case VB2_HASH_SHA256: + tpml_digests.digests[0].hashAlg = TPM_ALG_SHA256; + memcpy(tpml_digests.digests[0].digest.sha256, + digest, sizeof(TPMU_HA)); + break; + default: + return TPM_E_IOERROR; + } + result = tlcl_extend(pcr, (uint8_t *)&tpml_digests, NULL); +#else result = tlcl_extend(pcr, digest, NULL); +#endif if (result != TPM_SUCCESS) return result;
diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c index 16e40fe..fab334f 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -127,24 +127,68 @@ }
/* - * The caller will provide the digest in a 32 byte buffer, let's consider it a - * sha256 digest. + * The caller will provide the digest in a 32 byte buffer */ uint32_t tlcl_extend(int pcr_num, const uint8_t *in_digest, uint8_t *out_digest) { struct tpm2_pcr_extend_cmd pcr_ext_cmd; struct tpm2_response *response; + int i; + TPML_DIGEST_VALUES *tpml_digests;
pcr_ext_cmd.pcrHandle = HR_PCR + pcr_num; - pcr_ext_cmd.digests.count = 1; - pcr_ext_cmd.digests.digests[0].hashAlg = TPM_ALG_SHA256; - memcpy(pcr_ext_cmd.digests.digests[0].digest.sha256, in_digest, - sizeof(pcr_ext_cmd.digests.digests[0].digest.sha256)); + tpml_digests = (TPML_DIGEST_VALUES *)in_digest; + pcr_ext_cmd.digests.count = tpml_digests->count; + + for (i = 0; i < tpml_digests->count ; i++) { + pcr_ext_cmd.digests.digests[i].hashAlg = + tpml_digests->digests[i].hashAlg; + switch (tpml_digests->digests[i].hashAlg) { + case TPM_ALG_SHA1: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sha1, + tpml_digests->digests[i].digest.sha1, + sizeof(TPMU_HA)); + break; + case TPM_ALG_SHA256: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sha256, + tpml_digests->digests[i].digest.sha256, + sizeof(TPMU_HA)); + break; + case TPM_ALG_SHA384: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sha384, + tpml_digests->digests[i].digest.sha384, + sizeof(TPMU_HA)); + break; + case TPM_ALG_SHA512: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sha512, + tpml_digests->digests[i].digest.sha512, + sizeof(TPMU_HA)); + break; + case TPM_ALG_SM3_256: + memcpy(pcr_ext_cmd.digests.digests[i].digest.sm3_256, + tpml_digests->digests[i].digest.sm3_256, + sizeof(TPMU_HA)); + break; + } + }
response = tpm_process_command(TPM2_PCR_Extend, &pcr_ext_cmd);
- printk(BIOS_INFO, "%s: response is %x\n", + /* + * Check if we are invalidating the pcrs, ignore the error if this is + * the case + */ + if ((tpml_digests->count == 1) && + (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) && + response && (response->hdr.tpm_code & ~TPM_RC_N_MASK) == + (TPM_RC_P | TPM_RC_HASH)) { + printk(BIOS_SPEW, "%s: TPM_RC_HASH returned this is" + " expected\n", __func__); + return TPM_SUCCESS; + } + + printk(BIOS_INFO, "%s: response is 0x%x\n", __func__, response ? response->hdr.tpm_code : -1); if (!response || response->hdr.tpm_code) return TPM_E_IOERROR; diff --git a/src/security/tpm/tss/tcg-2.0/tss_structures.h b/src/security/tpm/tss/tcg-2.0/tss_structures.h index 6a017bbb..1530613 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -97,6 +97,12 @@ space is defined by the lower 16 bits. */ #define TPM_CC_VENDOR_BIT_MASK 0x20000000
+/* Table 15 - TPM_RC Constants (Actions) */ +#define RC_FMT1 (TPM_RC)(0x080) +#define TPM_RC_HASH (TPM_RC)(RC_FMT1 + 0x003) +#define TPM_RC_P (TPM_RC)(0x040) +#define TPM_RC_N_MASK (TPM_RC)(0xF00) + /* Startup values. */ #define TPM_SU_CLEAR 0 #define TPM_SU_STATE 1 @@ -311,12 +317,13 @@ TPM2B b; } TPM2B_MAX_NV_BUFFER;
-/* - * This is a union, but as of now we support just one digest - sha256, so - * there is just one element. - */ +/* Table 66 - TPMU_HA Union */ typedef union { - uint8_t sha256[SHA256_DIGEST_SIZE]; + uint8_t sha1[SHA1_DIGEST_SIZE]; + uint8_t sha256[SHA256_DIGEST_SIZE]; + uint8_t sm3_256[SM3_256_DIGEST_SIZE]; + uint8_t sha384[SHA384_DIGEST_SIZE]; + uint8_t sha512[SHA512_DIGEST_SIZE]; } TPMU_HA;
typedef struct { @@ -324,9 +331,10 @@ TPMU_HA digest; } TPMT_HA;
+/* Table 96 -- TPML_DIGEST_VALUES Structure <I/O> */ typedef struct { uint32_t count; - TPMT_HA digests[1]; /* Limit max number of hashes to 1. */ + TPMT_HA digests[HASH_COUNT]; } TPML_DIGEST_VALUES;
struct nv_read_response {
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(5 comments)
Sorry, not sure why I didn't catch this patch before it went in. But now it triggers a load of Coverity warnings from buffer overflows, and some of the design choices also look very wrong. Please provide a fixup quickly, or revert the patch until you have time to do that.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 217: tpml_digests.count = 1; Why do you need to pass a TPML_DIGEST_VALUES struct here when you only ever have a single digest? Just pass the raw byte buffer as before and add a new digest_type argument to tlcl_extend(). If we wanted to be able to extend multiple algorithms with one command we'd need that, but that's not what you're doing here.
Using TPML_DIGEST_VALUES here break the whole purpose of separating TSPI and TSS layers. The whole point of the TSPI layer is that it does *not* know about the implementation details of the specific TPM protocols, so even if you wanted to do something like this you should be using a generic interface rather than a TPM 2.0 specific protocol structure in the TSPI<->TSS interface.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 222: sizeof(TPMU_HA) This was flagged by Coverity, you're using the wrong size. This should be SHA1_DIGEST_SIZE, not the size of the whole union. You're overrunning the input buffer.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 232: result = tlcl_extend(pcr, (uint8_t *)&tpml_digests, NULL); You also shouldn't completely change the meaning of the arguments to a TSS function based on the TPM version (again, the TSPI layer should ideally not care about the TPM version at all). If you want to pass a TPML_DIGEST_VALUE pointer the argument type should be TPML_DIGEST_VALUE*, not u8* (but again, you shouldn't be doing that in the first place).
I think you should keep the existing arguments to tlcl_extend() the way they are and just add a new digest_type argument (for tss-1.2 it's fine to just return an error if the digest_type is not SHA1).
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 151: sizeof(TPMU_HA) These memcpy()s also use the wrong size.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 183: (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) && I don't understand what this is. Why would you put ALG_ERROR in there? That use doesn't look valid according to the spec.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
Hi Frans,
Due to the issues pointed out above, I've uploaded a revert of this patch for now (CB:35351). If you really need this to stay in the tree please let me know within 24h, but then please also commit to fixing the issues I pointed out in a timely manner. Otherwise, I'd like to land the revert and you're welcome to reupload your patch at any time with those issues resolved.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
Patch Set 8:
Hi Frans,
Due to the issues pointed out above, I've uploaded a revert of this patch for now (CB:35351). If you really need this to stay in the tree please let me know within 24h, but then please also commit to fixing the issues I pointed out in a timely manner. Otherwise, I'd like to land the revert and you're welcome to reupload your patch at any time with those issues resolved.
I did not noticed your comments before. Will look into this issue.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 222: sizeof(TPMU_HA)
This was flagged by Coverity, you're using the wrong size. […]
This has been correct in CB:35287
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 151: sizeof(TPMU_HA)
These memcpy()s also use the wrong size.
This has been corrected in CB:35287
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 183: (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) &&
I don't understand what this is. […]
To support invalidate of the PCR the hashAlg will be TPM_ALG_ERROR
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 183: (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) &&
To support invalidate of the PCR the hashAlg will be TPM_ALG_ERROR
I don't really understand what you mean by that. Can you give an example of how your code would call tpm_extend_pcr() in this case, and what you're trying to achieve with that? According to my understanding of the TPM 2.0 spec, passing hashAlg == TPM_ALG_ERROR is just illegal and will do nothing other than return an error. Why would you want to do that?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 183: (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) &&
I don't really understand what you mean by that. […]
Example is in mboot_hash_extend_log() of patch set CB:30833
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 183: (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) && Well... I'm pretty sure you're misunderstanding the TPM spec there? That "invalidate" thing you're trying to do doesn't work. It doesn't do anything. You cannot do a TPM2_PCR_Extend with TPM_ALG_ERROR, that has no meaning, that's just an error. See the description of TPM2_PCR_Extend in https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Comm... (section 22.2), there's nothing about passing TPM_ALG_ERROR there. It says
If the TPM unmarshals the hashAlg of a list entry and the unmarshaled value is not a hash algorithm implemented on the TPM, the TPM shall return TPM_RC_HASH.
That's the case you're hitting, because TPM_ALG_ERROR is not a hash algorithm implemented in the TPM, that's why you're getting TPM_RC_HASH back. That doesn't "invalidate" anything, it just doesn't change the PCRs at all.
The comment on your invalidate_pcrs() says " * Invalidate PCRs 0-7 with extending 1 after tpm failure." -- if that's what you want to do (extend the PCR with "1" so it can no longer match any real hash chain), you can do that, but you need to use the normal hash algorithm you used in your PCRs for that (e.g. SHA256 or whatever), not TPM_ALG_ERROR.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 206: enum vb2_hash_algorithm digest_algo We just discovered another subtle but serious problem with this CL.
Quick recap of how PCR extensions work to explain:
1. Machine boots, PCRs are initialized to 0. 2. CPU wants to measure a blob of data. 3. CPU hashes blob of data with a hash algorithm of its choice (e.g. SHA1). 4. CPU sends this hash to TPM with TPM2_Extend command. 5. TPM sets PCR to (roughly): HASH(PCR + <TPM2_Extend data>)
The problem is that the hash algorithm used in 3 and the one used in 5 are not necessarily the same thing! The algorithm used in 3 (which is the one denoted by the digest_algo parameter here) is completely hidden within the CPU, it doesn't even need to be an algorithm supported by the TPM. From the TPM's point of view, the data sent with TPM2_Extend is really just a (small) random blob of data that could be anything. The algorithm used on the TPM side (in step 5) is selected via the hashAlg parameter in the TPM2_Extend command and must be supported by the TPM. On top of that, the TPM actually maintains a completely separate PCR for every algorithm type it supports (for each PCR number)... so when you send a TPM2_Extend(PCR0, data, SHA1) and a TPM2_Extend(PCR0, data, SHA256), those two command actually extend two completely separate PCRs.
Frans, can you explain *why* you need to use the SHA1 bank of your TPM at all? It generally doesn't matter much which PCR bank you use as long you make sure you always use the same one. SHA256 is generally considered secure (and SHA1 isn't), so unless there's a real need to allow coreboot code to access different PCR banks, I think the current policy of hardcoding a single PCR hashAlg per TPM version makes a lot of sense and avoids confusing issues like this. (You can still measure your data with SHA1 hashes and extend those into the SHA256 PCR bank if you want, that's perfectly fine!)
If you really need to use a different PCR bank for some reason, I'd suggest we create a Kconfig for that (e.g. CONFIG_TPM2_PCR_BANK_ALG). Then you can use SHA1 on your platform and we can keep SHA256 as the default, and we still make sure that you can only use one PCR bank at a time. Using multiple PCR banks at once is really just a huge potential for confusion and I see no realistic use case why we should need to support that.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 206: enum vb2_hash_algorithm digest_algo
We just discovered another subtle but serious problem with this CL. […]
edit: Well... Andrey just told me that I may have still misunderstood some parts of this. It essentially works the way I described, but the data you send with TPM2_Extend still has to be exactly of the size of the hash algorithm used by the TPM, and there's a strong expectation by the spec that you use the same algorithm for both. We were using the wrong type for one of the vboot PCRs which was exposed by this patch and we need to fix that separately.
Still, I really don't think it's a good idea to allow the caller to specify the algorithm used on the TPM for every tpm_extend_pcr() call, because it completely subverts the expectations of how people think PCRs work. When you extend two things into a PCR you think they're both tied together, but if you used different algorithms for them they actually end up being completely separate and orthogonal in the measurement chain. That seems very dangerous to me and I don't think you'd ever really want to use it intentionally that way (if you want to measure things orthogonally, you'd use different PCR numbers instead).
So I would still propose to get rid of the digest_algo parameter here completely and instead either hardcode to SHA256, or if you really need control over it provide a Kconfig to select the algorithm.
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Frans Hendriks (Code Review)
-
Julius Werner (Code Review)
-
Lance Zhao (Code Review)
-
Patrick Georgi (Code Review)