[coreboot-gerrit] Change in coreboot[master]: vboot: Fix wrong algorithm in TCPA log for BOOT_MODE

[Andrey Pronin (Code Review)]

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().