Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
vboot: Fix wrong algorithm in TCPA log for BOOT_MODE
The hash algorithm for VBOOTs BOOT_MODE is fixed to sha1 but TCPA log uses sha256 as the name for the algorithm. This leads to an log entry with 20 bytes (sha1) while the algorithm is set to sha256 (which needs 32 bytes of hash). Fix it by using the matching algorithm name for BOOT_MODE.
Change-Id: Ia25938ac5f6c29f60a4819023b99f7796849f574 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/security/vboot/tpm_common.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/35645/1
diff --git a/src/security/vboot/tpm_common.c b/src/security/vboot/tpm_common.c index 0a211c5..1db7189 100644 --- a/src/security/vboot/tpm_common.c +++ b/src/security/vboot/tpm_common.c @@ -46,7 +46,7 @@ switch (which_digest) { /* SHA1 of (devmode|recmode|keyblock) bits */ case BOOT_MODE_PCR: - return tpm_extend_pcr(pcr, VB2_HASH_SHA256, buffer, size, + return tpm_extend_pcr(pcr, VB2_HASH_SHA1, buffer, size, TPM_PCR_BOOT_MODE); /* SHA256 of HWID */ case HWID_DIGEST_PCR:
Hello Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35645
to look at the new patch set (#2).
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
vboot: Fix wrong algorithm in TCPA log for BOOT_MODE
The hash algorithm for VBOOTs BOOT_MODE is fixed to sha1 but TCPA log uses sha256 as the name for the algorithm. This leads to an log entry with 20 bytes (sha1) while the algorithm is set to sha256 (which needs 32 bytes of hash). Fix it by using the matching algorithm name for BOOT_MODE.
Change-Id: Ia25938ac5f6c29f60a4819023b99f7796849f574 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/security/vboot/tpm_common.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/35645/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35645/2/src/security/vboot/tpm_comm... File src/security/vboot/tpm_common.c:
https://review.coreboot.org/c/coreboot/+/35645/2/src/security/vboot/tpm_comm... PS2, Line 50: TPM_PCR_BOOT_MODE); This looks to be a revert of https://review.coreboot.org/c/coreboot/+/35476
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@9 PS2, Line 9: The hash algorithm for VBOOTs BOOT_MODE is fixed to sha1 but TCPA log I don't think this is the case any more. Does the coreboot.org vboot submodule need to be updated? Or is it the fact that tlcl_extend assumes a specific digest algo? Is that TCG defined?
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@13 PS2, Line 13: BOOT_MODE. I'm confused. TCPA log uses sha256 as the name, but how is it getting the size? In tpm_extend_pcr the digest_algo is passed into tcpa_log_add_table_entry().
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@9 PS2, Line 9: The hash algorithm for VBOOTs BOOT_MODE is fixed to sha1 but TCPA log
I don't think this is the case any more. Does the coreboot. […]
No, TCPA log uses fixed SHA2 while vb2api_get_pcr_digest() returns a size of 20 bytes for the digest_type.
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@13 PS2, Line 13: BOOT_MODE.
I'm confused. […]
Have a look at 3rdparty/vboot/firmware/2lib/2api.c:
vb2_error_t vb2api_get_pcr_digest(struct vb2_context *ctx, enum vb2_pcr_digest which_digest, uint8_t *dest, uint32_t *dest_size) { const uint8_t *digest; uint32_t digest_size;
switch (which_digest) { case BOOT_MODE_PCR: digest = vb2_get_boot_state_digest(ctx); digest_size = VB2_SHA1_DIGEST_SIZE; break; case HWID_DIGEST_PCR: digest = vb2_get_gbb(ctx)->hwid_digest; digest_size = VB2_GBB_HWID_DIGEST_SIZE; break; default: return VB2_ERROR_API_PCR_DIGEST; }
For BOOT_MODE_PCR, digest size is set to VB2_SHA1_DIGEST_SIZE which is defined in 3rdparty/vboot/firmware/2lib/include/2sha.h as
#define VB2_SHA1_DIGEST_SIZE 20
So it still uses SHA1 for BOOT_MODE_PCR.
My current vboot commit is e6700f4c70fe72850ae4f3f5df19c9281ebcefc8
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2: Code-Review-1
Sorry, we needed to change this for correctness (and can't let you change it back). See my comments in CB:33252 for explanation.
We need both vboot PCRs to use the same PCR bank (SHA256 for TPM 2.0). It's true that the data you get is only a SHA1 hash, but it is zero extended and you need to treat it like a SHA256 hash. This is how vboot has always done it and how all our userspace tools expect it, so we don't want to change it now.
The whole implementation in tpm_extend_pcr() is still a mess in that it gets passed an algorithm and a length and uses that for the TCPA log but not for actual TPM communication. That needs to be cleaned up so they at least both work on the same data. Like I mentioned in CB:33252, I think the best way to clean it up would be to remove the algorithm type (and therefore maybe also the digest length) parameter completely and just use a hardcoded hash algorithm (possibly selected by Kconfig) for both TPM communication and TCPA log. Using multiple algorithm banks at once on TPM 2.0 is just super confusing with little practical benefit in my opinion, so I think it would be better if we just don't allow that at all.
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
(1 comment)
CIL
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@13 PS2, Line 13: BOOT_MODE.
Have a look at 3rdparty/vboot/firmware/2lib/2api.c: […]
as it is now: for TPM2.0, it first uses SHA1 to calculate a digest of mode bits, then it extends it to the size of SHA256 by appending zeroes, and then it extends it to SHA256 BOOT_MODE_PCR register. and this is the value and the bank that the OS expects. for TPM1.2 it just does SHA1 digest of the mode bits and extends to BOOT_MODE_PCR (no banks for 1.2).
look what tpm_extend_pcr() -> tlcl_extend() is doing when getting the value provided by vb2_api_get_pcr_digest:
for 2.0 - always using SHA256 bank and SHA256 digest size: 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));
for 1.2 - always using SHA1 digest size (kPcrDigestLength): memcpy(&cmd, &tpm_extend_cmd, sizeof(cmd)); to_tpm_uint32(cmd.buffer + tpm_extend_cmd.pcrNum, pcr_num); memcpy(cmd.buffer + cmd.inDigest, in_digest, kPcrDigestLength);
in both cases, digest_algo is ignored for tlcl_extend (until recent CLs, which started using it to indicate what bank to use for 2.0, they were eventually reverted, but triggered this SHA1 -> SHA256 change). I haven't noticed that it is actually used in tcpa_log_add_table_entry().
Setting the bank to SHA1 for 2.0 would break OS expectations, which already has objects with policies tied to SHA256-BOOT_MODE_PCR set to a particular value.
Bottomline: having a fixed SHA1 or SHA256 for BOOT_MODE_PCR won't work, at least if it is used both to indicate the digest in the log and the bank for 2.0 case. We need different things for different TPM models.
Question: For TPM2.0 case, given that we extend a padded value to SHA256 bank, what shall be logged with tcpa_log_add_table_entry()? (A) A SHA1 digest or (B) the padded 32-byte value?
If (A), we need to separate the algo and value for tcpa_log_add_table_entry() from the value and algo(bank) for the TPM. The fix is to have two sets of algos passed to tpm_extend_pcr() and let it do the padding as needed.
If (B), then we need #if CONFIG(TPM2) when deciding a) what value to return from vb2api_get_pcr_digest() b) which algo to use when calling tpm_extend_pcr(BOOT_MODE_PCR) in vboot_extend_pcr().
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@13 PS2, Line 13: BOOT_MODE.
For TPM2.0 case, given that we extend a padded value to SHA256 bank, what shall be logged with tcpa_log_add_table_entry()? (A) A SHA1 digest or (B) the padded 32-byte value?
The TCPA log should always log the thing that was actually sent to the TPM, and the algorithm in the log should match the actual TPM bank it was written to. So SHA256 for this.
If (B), then we need #if CONFIG(TPM2) when deciding a) what value to return from vb2api_get_pcr_digest() b) which algo to use when calling tpm_extend_pcr(BOOT_MODE_PCR) in vboot_extend_pcr().
I would prefer not to pull knowledge of which TPM version we're using into vboot (for the firmware part at least). We should treat the values returned by vb2api_get_pcr_digest() not as hashes but as opaque blobs that are written into whatever PCR bank is correct for the platform. Maybe we should change the API such that dest_size is only a value and not a pointer, so that vboot doesn't pass a size back out again (because the caller can't do anything useful with that anyway, the PCR size is fixed and vboot always zero-extends the buffer if necessary).
Julius Werner has removed Julius Werner from this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Removed reviewer Julius Werner.
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35645/2//COMMIT_MSG@13 PS2, Line 13: BOOT_MODE.
The TCPA log should always log the thing that was actually sent to the TPM, and the algorithm in the log should match the actual TPM bank it was written to. So SHA256 for this.
I support this approach for TPM2.0. It looks though that with always-SHA256 we break TPM1.2 case. There TCPA log should contain SHA1 (there are no banks, and what we extend is a 20-byte SHA1 digest).
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2: Code-Review-1
The reason for this change is the following entry in the TCPA log when VBOOT is enabled on mc_bdx1 (fsp_broadwell_de) with TPM2:
PCR-0 62571891215b4efc1ceab744ce59dd0b66ea6f73 SHA256 [VBOOT: boot mode] PCR-1 a66c8c2cda246d332d0c2025b6266e1e23c89410051002f46bfad1c9265f43d0 SHA256 [VBOOT: GBB HWID]
This two PCRs claim to have the same algorithm used for hashing (SHA256) but for boot mode the entry is clear too short as it just has 20 bytes (which would be SHA1 instead of SHA256). So in this case it is just wrong that SHA256 is reported.
I do not want to break the expectations that OS has on VBOOT, just had to less background. But still it is wrong. We need to find a better way to fix it.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Patch Set 2:
The reason for this change is the following entry in the TCPA log when VBOOT is enabled on mc_bdx1 (fsp_broadwell_de) with TPM2:
PCR-0 62571891215b4efc1ceab744ce59dd0b66ea6f73 SHA256 [VBOOT: boot mode] PCR-1 a66c8c2cda246d332d0c2025b6266e1e23c89410051002f46bfad1c9265f43d0 SHA256 [VBOOT: GBB HWID]
This two PCRs claim to have the same algorithm used for hashing (SHA256) but for boot mode the entry is clear too short as it just has 20 bytes (which would be SHA1 instead of SHA256). So in this case it is just wrong that SHA256 is reported.
Yes, that needs to be fixed. That's a consequence of us passing both digest length and algorithm type as separate parameters to tpm_extend_pcr(). That doesn't make sense because those two always need to be in sync.
I'd again like to campaign for my suggestion to get rid of both of them and have the algorithm type hardcoded in Kconfig for all PCRs. That would cause the right (full 32-byte) value to be logged for PCR-0 here and would make future misconfiguration like this and others impossible.
Werner Zeh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35645 )
Change subject: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE ......................................................................
Abandoned
Addressed in CB:51720
-
Aaron Durbin (Code Review) -
Andrey Pronin (Code Review)
-
Julius Werner (Code Review)
-
Werner Zeh (Code Review)