Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/22374 )
Change subject: security/tpm: Refactor TSS 1.2 and 2.0 implementation ......................................................................
Patch Set 32:
(14 comments)
https://review.coreboot.org/#/c/22374/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/22374/25//COMMIT_MSG@12 PS25, Line 12: * Fix TPM1 missing kconfig option for boards with TPM 1.2.
This wasn't missing, it's intentional. […]
okay got it.
https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/Kconfig File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/Kconfig@11... PS25, Line 117: depends on TPM2
Can also be used with TPM1, I think. […]
Done
https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/memory_ini... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/memory_ini... PS25, Line 38: #if IS_ENABLED(CONFIG_FSP2_0_USES_TPM_MRC_HASH)
Do not #ifdef out code just because it won't be used. Just do […]
Done
https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig@70 PS25, Line 70:
This Kconfig doesn't do anything anymore after your previous patch. Either implement it or drop it.
Done
https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig@78 PS25, Line 78: Power off machine while waiting for CR50 update to take effect.
ditto
Done
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h File src/security/tpm/tss.h:
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h@25 PS25, Line 25: #if (IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2))
You should not #ifdef out header definitions unless there's a specific reason. […]
Done
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h@86 PS25, Line 86: #endif
Should be fixed in the previous CL already.
Done
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/cr50/tss.c File src/security/tpm/tss/cr50/tss.c:
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/cr50/tss.c@1 PS25, Line 1: /*
I wonder if this is the best name/location for it... […]
good point, let me fix that
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-1.2/tss_st... File src/security/tpm/tss/tcg-1.2/tss_structures.h:
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-1.2/tss_st... PS25, Line 17: #define TPM_PCR_DIGEST 20
... […]
Fixed see secdata_tpm.c comment
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-2.0/tss.c File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-2.0/tss.c@... PS25, Line 19: void *tpm_process_command(TPM_CC command, void *command_body);
Let's put this in a real header somewhere. […]
Done
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss_error_messages... File src/security/tpm/tss_error_messages.h:
https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss_error_messages... PS25, Line 12: TSS_ERROR_MESSAGES_H_
nit: error_*messages* doesn't really fit for this file anymore. Maybe just tss_errors. […]
Ack
https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_mock.c File src/security/vboot/secdata_mock.c:
https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_mock.c@7... PS25, Line 75: VB2_SUCCESS
These changes aren't really correct, most of these functions in the non-mock implementation still us […]
This code belongs to vboot2 IMHO it should return vboot error codes instead of tpm error codes. Especially if we talk about interfaces..
https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c@56 PS25, Line 56: #define RETURN_ON_FAILURE(tpm_cmd) do { \
I think it would be better to just move this to the existing #if...TPM2 block below. […]
Done
https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c@10... PS25, Line 104: B2_SUCCESS)
Why did this get renamed? tlcl_extend() in it's current form can only take digests of exactly this l […]
we have TPM 1.2 and 2.0 support. Both have a min size of 20 bytes because of SHA-1. I moved TPM_PCR_MIN_DIGEST to the common area