Change in coreboot[master]: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_ex...
Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35351 )
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()"
This reverts commit fdb9805d6884090fd7bf62dbdf9c858692e55fb4.
CB:33252 wasn't reviewed by a TPM maintainer and breaks abstraction layers (pulling TSS-details into TSPI, completely changing interpretation of the arguments to tlcl_extend() based on TSS version). It's also not clear why it was implemented the way it was (should have been much easier and cleaner ways to achieve the same thing).
Since the author is not reacting, let's revert it for now. It can be cleaned up and resubmitted later.
Change-Id: Ice44f55c75a0acc07794fe41c757a7bca75406eb --- 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, 13 insertions(+), 88 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/35351/1
diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c index e64e04f..4698a4d 100644 --- a/src/security/tpm/tspi/tspi.c +++ b/src/security/tpm/tspi/tspi.c @@ -4,7 +4,6 @@ * 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 @@ -21,7 +20,6 @@ #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> @@ -211,28 +209,7 @@ 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, SHA1_DIGEST_SIZE); - break; - case VB2_HASH_SHA256: - tpml_digests.digests[0].hashAlg = TPM_ALG_SHA256; - memcpy(tpml_digests.digests[0].digest.sha256, - digest, SHA256_DIGEST_SIZE); - 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 d9deba5..16e40fe 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -127,68 +127,24 @@ }
/* - * The caller will provide the digest in a 32 byte buffer + * The caller will provide the digest in a 32 byte buffer, let's consider it a + * sha256 digest. */ 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; - 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, - SHA1_DIGEST_SIZE); - break; - case TPM_ALG_SHA256: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sha256, - tpml_digests->digests[i].digest.sha256, - SHA256_DIGEST_SIZE); - break; - case TPM_ALG_SHA384: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sha384, - tpml_digests->digests[i].digest.sha384, - SHA384_DIGEST_SIZE); - break; - case TPM_ALG_SHA512: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sha512, - tpml_digests->digests[i].digest.sha512, - SHA512_DIGEST_SIZE); - break; - case TPM_ALG_SM3_256: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sm3_256, - tpml_digests->digests[i].digest.sm3_256, - SM3_256_DIGEST_SIZE); - break; - } - } + 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));
response = tpm_process_command(TPM2_PCR_Extend, &pcr_ext_cmd);
- /* - * 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", + printk(BIOS_INFO, "%s: response is %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 1530613..6a017bbb 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -97,12 +97,6 @@ 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 @@ -317,13 +311,12 @@ TPM2B b; } TPM2B_MAX_NV_BUFFER;
-/* Table 66 - TPMU_HA Union */ +/* + * This is a union, but as of now we support just one digest - sha256, so + * there is just one element. + */ typedef union { - 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]; + uint8_t sha256[SHA256_DIGEST_SIZE]; } TPMU_HA;
typedef struct { @@ -331,10 +324,9 @@ TPMU_HA digest; } TPMT_HA;
-/* Table 96 -- TPML_DIGEST_VALUES Structure <I/O> */ typedef struct { uint32_t count; - TPMT_HA digests[HASH_COUNT]; + TPMT_HA digests[1]; /* Limit max number of hashes to 1. */ } TPML_DIGEST_VALUES;
struct nv_read_response {
Hello Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35351
to look at the new patch set (#2).
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()"
This reverts commit fdb9805d6884090fd7bf62dbdf9c858692e55fb4.
CB:33252 wasn't reviewed by a TPM maintainer and breaks abstraction layers (pulling TSS-details into TSPI, completely changing interpretation of the arguments to tlcl_extend() based on TSS version). It's also not clear why it was implemented the way it was (should have been much easier and cleaner ways to achieve the same thing).
Since the author is not reacting, let's revert it for now. It can be cleaned up and resubmitted later.
Change-Id: Ice44f55c75a0acc07794fe41c757a7bca75406eb Signed-off-by: Julius Werner jwerner@chromium.org --- 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, 13 insertions(+), 88 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/35351/2
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35351 )
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Patch Set 2: Code-Review+2
Hello Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35351
to look at the new patch set (#3).
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()"
This reverts commit fdb9805d6884090fd7bf62dbdf9c858692e55fb4.
CB:33252 wasn't reviewed by a TPM maintainer and breaks abstraction layers (pulling TSS-details into TSPI, completely changing interpretation of the arguments to tlcl_extend() based on TSS version). It's also not clear why it was implemented the way it was (should have been much easier and cleaner ways to achieve the same thing).
Since the author is not reacting, let's revert it for now. It can be cleaned up and resubmitted later. (Not reverting the header changes since those are not objectionable, and there are later patches dependent on it.)
Change-Id: Ice44f55c75a0acc07794fe41c757a7bca75406eb Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c 2 files changed, 7 insertions(+), 74 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/35351/3
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35351 )
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Patch Set 3: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35351 )
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()"
This reverts commit fdb9805d6884090fd7bf62dbdf9c858692e55fb4.
CB:33252 wasn't reviewed by a TPM maintainer and breaks abstraction layers (pulling TSS-details into TSPI, completely changing interpretation of the arguments to tlcl_extend() based on TSS version). It's also not clear why it was implemented the way it was (should have been much easier and cleaner ways to achieve the same thing).
Since the author is not reacting, let's revert it for now. It can be cleaned up and resubmitted later. (Not reverting the header changes since those are not objectionable, and there are later patches dependent on it.)
Change-Id: Ice44f55c75a0acc07794fe41c757a7bca75406eb Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/35351 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/security/tpm/tspi/tspi.c M src/security/tpm/tss/tcg-2.0/tss.c 2 files changed, 7 insertions(+), 74 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved
diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c index e64e04f..4698a4d 100644 --- a/src/security/tpm/tspi/tspi.c +++ b/src/security/tpm/tspi/tspi.c @@ -4,7 +4,6 @@ * 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 @@ -21,7 +20,6 @@ #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> @@ -211,28 +209,7 @@ 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, SHA1_DIGEST_SIZE); - break; - case VB2_HASH_SHA256: - tpml_digests.digests[0].hashAlg = TPM_ALG_SHA256; - memcpy(tpml_digests.digests[0].digest.sha256, - digest, SHA256_DIGEST_SIZE); - 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 d9deba5..16e40fe 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -127,68 +127,24 @@ }
/* - * The caller will provide the digest in a 32 byte buffer + * The caller will provide the digest in a 32 byte buffer, let's consider it a + * sha256 digest. */ 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; - 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, - SHA1_DIGEST_SIZE); - break; - case TPM_ALG_SHA256: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sha256, - tpml_digests->digests[i].digest.sha256, - SHA256_DIGEST_SIZE); - break; - case TPM_ALG_SHA384: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sha384, - tpml_digests->digests[i].digest.sha384, - SHA384_DIGEST_SIZE); - break; - case TPM_ALG_SHA512: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sha512, - tpml_digests->digests[i].digest.sha512, - SHA512_DIGEST_SIZE); - break; - case TPM_ALG_SM3_256: - memcpy(pcr_ext_cmd.digests.digests[i].digest.sm3_256, - tpml_digests->digests[i].digest.sm3_256, - SM3_256_DIGEST_SIZE); - break; - } - } + 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));
response = tpm_process_command(TPM2_PCR_Extend, &pcr_ext_cmd);
- /* - * 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", + printk(BIOS_INFO, "%s: response is %x\n", __func__, response ? response->hdr.tpm_code : -1); if (!response || response->hdr.tpm_code) return TPM_E_IOERROR;
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35351 )
Change subject: Revert "security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend()" ......................................................................
Patch Set 4:
Change has been successfully cherry-picked as b3426c03b4cf84af871c6d4c32afed2086f3fd1a by Philipp Deppenwiese
Well, I actually meant to give Frans 24h to respond before merging this... ^^
Anyway, as I said on the other CL, feel free to resubmit this feature at any time Frans, just please address the issues I pointed out there.
-
Julius Werner (Code Review) -
Philipp Deppenwiese (Code Review)