Change in ...coreboot[master]: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for has...
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33251
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size
mashal_TPMT_HA() uses size of SHA-256 hash. Use tlcll_get_hash_size_from_algo(0) to determince the hash size.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I739260e13e9cd10a61d52e13e8741b12ec868d7f Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tss.h M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 4 files changed, 52 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/33251/1
diff --git a/src/security/tpm/tss.h b/src/security/tpm/tss.h index 548a39a..2ff9eed 100644 --- a/src/security/tpm/tss.h +++ b/src/security/tpm/tss.h @@ -70,6 +70,14 @@ * custom tpm standards like cr50 */ void *tpm_process_command(TPM_CC command, void *command_body); +/* + * Return size of digest. + * + * @param[in] HashAlgo Hash algorithm + * + * @return size of digest + */ +uint16_t tlcl_get_hash_size_from_algo(TPMI_ALG_HASH hashAlgo);
#endif
diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c index e579bff..24d7726 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -361,3 +361,33 @@
return TPM_SUCCESS; } +/** + Return size of digest. + + @param[in] HashAlgo Hash algorithm + + @return size of digest +**/ + +uint16_t tlcl_get_hash_size_from_algo(TPMI_ALG_HASH hashAlgo) +{ + uint16_t value; + + switch (hashAlgo) { + case TPM_ALG_ERROR: + value = 1; + break; + case TPM_ALG_SHA1: + value = SHA1_DIGEST_SIZE; + break; + case TPM_ALG_SHA256: + value = SHA256_DIGEST_SIZE; + break; + default: + printk(BIOS_SPEW, "%s: unknown hash algorithm %d\n", __func__, + hashAlgo); + value = 0; + }; + + return value; +} diff --git a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c index 21da73a..a74eb09 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c +++ b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c @@ -12,6 +12,7 @@
#include "tss_marshaling.h" #include <security/tpm/tss/vendor/cr50/cr50.h> +#include <security/tpm/tss.h>
static uint16_t tpm_tag CAR_GLOBAL; /* Depends on the command type. */
@@ -82,7 +83,7 @@
rc |= marshal_TPMI_ALG_HASH(ob, tpmtha->hashAlg); rc |= obuf_write(ob, tpmtha->digest.sha256, - sizeof(tpmtha->digest.sha256)); + tlcl_get_hash_size_from_algo(tpmtha->hashAlg));
return rc; } 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..dcd4468 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -36,12 +36,24 @@ typedef TPM_HANDLE TPM_RH;
/* Some hardcoded algorithm values. */ +/* Table 7 - TPM_ALG_ID Constants */ +#define TPM_ALG_ERROR ((TPM_ALG_ID)0x0000) #define TPM_ALG_HMAC ((TPM_ALG_ID)0x0005) #define TPM_ALG_NULL ((TPM_ALG_ID)0x0010) #define TPM_ALG_SHA1 ((TPM_ALG_ID)0x0004) #define TPM_ALG_SHA256 ((TPM_ALG_ID)0x000b) +#define TPM_ALG_SHA384 ((TPM_ALG_ID)0x000C) +#define TPM_ALG_SHA512 ((TPM_ALG_ID)0x000D) +#define TPM_ALG_SM3_256 ((TPM_ALG_ID)0x0012)
+/* Annex A Algorithm Constants */ + +/* Table 205 - Defines for SHA1 Hash Values */ +#define SHA1_DIGEST_SIZE 20 +/* Table 206 - Defines for SHA256 Hash Values */ #define SHA256_DIGEST_SIZE 32 +/* Table 208 - Defines for SHA512 Hash Values */ +#define SHA512_DIGEST_SIZE 64
/* Some hardcoded hierarchies. */ #define TPM_RH_NULL 0x40000007
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/+/33251
to look at the new patch set (#2).
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size
mashal_TPMT_HA() uses size of SHA-256 hash. Use tlcll_get_hash_size_from_algo() to determince the hash size.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I739260e13e9cd10a61d52e13e8741b12ec868d7f Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tss.h M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 4 files changed, 40 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/33251/2
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33251/2/src/security/tpm/tss/tcg-2.0/tss.c File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/#/c/33251/2/src/security/tpm/tss/tcg-2.0/tss.c@3... PS2, Line 356: break; Can we add all TCG specified algorithms here?
Hello Patrick Rudolph, Julius Werner, 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/+/33251
to look at the new patch set (#3).
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size
mashal_TPMT_HA() uses size of SHA-256 hash. Use tlcll_get_hash_size_from_algo() to determince the hash size.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I739260e13e9cd10a61d52e13e8741b12ec868d7f Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/security/tpm/tss.h M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 4 files changed, 58 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/33251/3
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 3:
(1 comment)
Have upload a new patchset.
https://review.coreboot.org/#/c/33251/2/src/security/tpm/tss/tcg-2.0/tss.c File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/#/c/33251/2/src/security/tpm/tss/tcg-2.0/tss.c@3... PS2, Line 356: break;
Can we add all TCG specified algorithms here?
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 3: Code-Review+1
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33251/3/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/33251/3/src/security/tpm/tss/tcg-2.... PS3, Line 84: Will that be conflict with CB:33252?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33251/3/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/33251/3/src/security/tpm/tss/tcg-2.... PS3, Line 84:
Will that be conflict with CB:33252?
No. hash size will updated form algorithm size in CB:332.
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 3: Code-Review+1
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 3: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size
mashal_TPMT_HA() uses size of SHA-256 hash. Use tlcll_get_hash_size_from_algo() to determince the hash size.
BUG=N/A TEST=Build binary and verified logging on Facebook FBG-1701
Change-Id: I739260e13e9cd10a61d52e13e8741b12ec868d7f Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33251 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Lance Zhao lance.zhao@gmail.com Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/security/tpm/tss.h M src/security/tpm/tss/tcg-2.0/tss.c M src/security/tpm/tss/tcg-2.0/tss_marshaling.c M src/security/tpm/tss/tcg-2.0/tss_structures.h 4 files changed, 57 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Lance Zhao: Looks good to me, but someone else must approve
diff --git a/src/security/tpm/tss.h b/src/security/tpm/tss.h index 30e2a7b..336935d 100644 --- a/src/security/tpm/tss.h +++ b/src/security/tpm/tss.h @@ -79,6 +79,9 @@ */ void *tpm_process_command(TPM_CC command, void *command_body);
+/* Return digest size of hash algorithm */ +uint16_t tlcl_get_hash_size_from_algo(TPMI_ALG_HASH hash_algo); + #endif
/*****************************************************************************/ diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c index 08a7caa..16e40fe 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -352,6 +352,38 @@ } }
+uint16_t tlcl_get_hash_size_from_algo(TPMI_ALG_HASH hash_algo) +{ + uint16_t value; + + switch (hash_algo) { + case TPM_ALG_ERROR: + value = 1; + break; + case TPM_ALG_SHA1: + value = SHA1_DIGEST_SIZE; + break; + case TPM_ALG_SHA256: + value = SHA256_DIGEST_SIZE; + break; + case TPM_ALG_SHA384: + value = SHA384_DIGEST_SIZE; + break; + case TPM_ALG_SHA512: + value = SHA512_DIGEST_SIZE; + break; + case TPM_ALG_SM3_256: + value = SM3_256_DIGEST_SIZE; + break; + default: + printk(BIOS_SPEW, "%s: unknown hash algorithm %d\n", __func__, + hash_algo); + value = 0; + }; + + return value; +} + uint32_t tlcl_disable_platform_hierarchy(void) { struct tpm2_response *response; diff --git a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c index 345aec5..ec3cd8b 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_marshaling.c +++ b/src/security/tpm/tss/tcg-2.0/tss_marshaling.c @@ -84,7 +84,7 @@
rc |= marshal_TPMI_ALG_HASH(ob, tpmtha->hashAlg); rc |= obuf_write(ob, tpmtha->digest.sha256, - sizeof(tpmtha->digest.sha256)); + tlcl_get_hash_size_from_algo(tpmtha->hashAlg));
return rc; } 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 7332739..6a017bbb 100644 --- a/src/security/tpm/tss/tcg-2.0/tss_structures.h +++ b/src/security/tpm/tss/tcg-2.0/tss_structures.h @@ -38,12 +38,28 @@ typedef TPM_HANDLE TPM_RH;
/* Some hardcoded algorithm values. */ -#define TPM_ALG_HMAC ((TPM_ALG_ID)0x0005) -#define TPM_ALG_NULL ((TPM_ALG_ID)0x0010) -#define TPM_ALG_SHA1 ((TPM_ALG_ID)0x0004) -#define TPM_ALG_SHA256 ((TPM_ALG_ID)0x000b) +/* Table 7 - TPM_ALG_ID Constants */ +#define TPM_ALG_ERROR ((TPM_ALG_ID)0x0000) +#define TPM_ALG_HMAC ((TPM_ALG_ID)0x0005) +#define TPM_ALG_NULL ((TPM_ALG_ID)0x0010) +#define TPM_ALG_SHA1 ((TPM_ALG_ID)0x0004) +#define TPM_ALG_SHA256 ((TPM_ALG_ID)0x000b) +#define TPM_ALG_SHA384 ((TPM_ALG_ID)0x000C) +#define TPM_ALG_SHA512 ((TPM_ALG_ID)0x000D) +#define TPM_ALG_SM3_256 ((TPM_ALG_ID)0x0012)
-#define SHA256_DIGEST_SIZE 32 +/* Annex A Algorithm Constants */ + +/* Table 205 - Defines for SHA1 Hash Values */ +#define SHA1_DIGEST_SIZE 20 +/* Table 206 - Defines for SHA256 Hash Values */ +#define SHA256_DIGEST_SIZE 32 +/* Table 207 - Defines for SHA384 Hash Values */ +#define SHA384_DIGEST_SIZE 48 +/* Table 208 - Defines for SHA512 Hash Values */ +#define SHA512_DIGEST_SIZE 64 +/* Table 209 - Defines for SM3_256 Hash Values */ +#define SM3_256_DIGEST_SIZE 32
/* Some hardcoded hierarchies. */ #define TPM_RH_NULL 0x40000007
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33251 )
Change subject: security/tpm/tss/tcg-2.0: Use tlcl_get_hash_size_from_algo() for hash size ......................................................................
Patch Set 4:
(1 comment)
oh, just found a possible problem right after merging this patch... it shouldn't hit that code path at the moment, but I'd really appreciate this being fixed before it will cause problems (only out of bounds read and not an out of bounds write; if it was the latter, I'd have reverted this patch)
https://review.coreboot.org/c/coreboot/+/33251/4/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/33251/4/src/security/tpm/tss/tcg-2.... PS4, Line 86: tpmtha->digest.sha256 this should also be replaced, so you don't get an out-of-bounds access when selecting e.q. sha512
-
Felix Held (Code Review) -
Frans Hendriks (Code Review)
-
Lance Zhao (Code Review)
-
Philipp Deppenwiese (Code Review)