Attention is currently required from: Jason Glenesk, Raul Rangel, Michał Żygowski, Frans Hendriks, Matt DeVillier, Christian Walter, Julius Werner, Krystian Hebel, Fred Reitberger, Yu-Ping Wu, Felix Held.
Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69160 )
Change subject: security/tpm: resolve conflicts in TSS implementations ......................................................................
Patch Set 5:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/69160/comment/ac60b42d_2bd6dd5f PS4, Line 8:
I suppose the goal is compile both TPM1 and TPM2 code at the same time (CB:69162). […]
Sure, I'll update the message. Same platform can be used with different TPMs and hard-coding TPM version at compile-time currently requires updating firmware in order to switch a TPM device.
File src/security/tpm/tss.h:
https://review.coreboot.org/c/coreboot/+/69160/comment/393b17c6_20539b53 PS4, Line 26: * tlcl_lib_init().
I think you should use tlcl1_ and tlcl2_ as prefixes... mixing tlcl12_ and tlcl2_ is just confusing.
Done
https://review.coreboot.org/c/coreboot/+/69160/comment/30fdb622_5f6259cd PS4, Line 30: * TPM1.2-specific
I think this would be a good opportunity to factor this out into more files, since this one is getti […]
Done
File src/security/tpm/tss/tss.c:
https://review.coreboot.org/c/coreboot/+/69160/comment/65054ba0_5ab06a4f PS4, Line 32: #if CONFIG(TPM1)
These don't need to be preprocessor #if, you can just write `if (CONFIG(TPM1) && tpm_family == 1)`. […]
I remember getting undefined symbol errors with that. Maybe something was off with the build at that point. Updated.
https://review.coreboot.org/c/coreboot/+/69160/comment/c1582b0e_da076f54 PS4, Line 35: return VB2_SUCCESS;
Why don't you just make tis_sendrecv an exported global in this file that can be directly accessed f […]
Didn't want to use a global variable. Added one.
https://review.coreboot.org/c/coreboot/+/69160/comment/99940a6e_bf667ce2 PS4, Line 62: die("%s: can't be used with TPM %d\n", __func__, tpm_family);
also, die() here and elsewhere in the file seems inappropriate. […]
I thought that using a `__noreturn` function is better than returning some semi-random `TPM_E_*` error precisely because this shouldn't happen. Changed to `return TPM_E_INTERNAL_INCONSISTENCY;`.
-
Sergii Dmytruk (Code Review)