Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/24903 )
Change subject: security/tpm: Unify the coreboot TPM software stack ......................................................................
Patch Set 36:
(11 comments)
https://review.coreboot.org/#/c/24903/33/src/cpu/intel/haswell/romstage.c File src/cpu/intel/haswell/romstage.c:
https://review.coreboot.org/#/c/24903/33/src/cpu/intel/haswell/romstage.c@24... PS33, Line 248: if (IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2))
Follow up: add helper tpm_enabled_in_build() which encapsulates this?
I wrote a generic driver which runs in ramstage. So it's already done in a follow up. :)
https://review.coreboot.org/#/c/24903/33/src/mainboard/google/auron/Kconfig File src/mainboard/google/auron/Kconfig:
https://review.coreboot.org/#/c/24903/33/src/mainboard/google/auron/Kconfig@... PS33, Line 13: select MAINBOARD_HAS_TPM1
I'm a little confused by this. […]
Yeah that's normal. If you don't select it you will get a menu in kconfig for selecting the tpm version. This is needed for desktop and server boards having a TPM header
https://review.coreboot.org/#/c/24903/33/src/mainboard/google/cheza/Kconfig File src/mainboard/google/cheza/Kconfig:
https://review.coreboot.org/#/c/24903/33/src/mainboard/google/cheza/Kconfig@... PS33, Line 15: select MAINBOARD_HAS_TPM1
Where'd you come up with this one? It's actually using cr50 over spi. […]
Okay will fix it.
https://review.coreboot.org/#/c/24903/33/src/mainboard/google/eve/Kconfig File src/mainboard/google/eve/Kconfig:
https://review.coreboot.org/#/c/24903/33/src/mainboard/google/eve/Kconfig@19 PS33, Line 19: select MAINBOARD_HAS_I2C_TPM_CR50
I think TPM_CR50 should auto select HAS_TPM2... […]
CR50 is just an addition and get in through MAINBOARD_HAS_I2C_TPM_CR50. We could also a MAINBOARD_HAS_CR50
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tspi.h File src/security/tpm/tspi.h:
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tspi.h@20 PS33, Line 20: #include <security/tpm/tss.h>
We should document the return values here so it's understood what the meaning of the values are.
Okay got it. Will also add the function parms as well
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss.h File src/security/tpm/tss.h:
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss.h@57 PS33, Line 57:
Not necessarily true any longer as one can specify the policy? Or is the nv_policy different than th […]
Yep good catch will fix it
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss.h@172 PS33, Line 172: */
Still need to get through the rest of the patch, but why does this declaration need to be here if it […]
It's used if you have a tpm variant based on TPM2 by googles CR50 So let's just rename the comment
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss/tcg-2.0/tss_ma... File src/security/tpm/tss/tcg-2.0/tss_marshaling.h:
https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss/tcg-2.0/tss_ma... PS33, Line 11: #include <security/tpm/tss/vendor/cr50/cr50.h>
why is this header needed in here?
Okay I will fix it
https://review.coreboot.org/#/c/24903/33/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/24903/33/src/security/vboot/Kconfig@20 PS33, Line 20: select VBOOT_MOCK_SECDATA if !TPM1 && !TPM2
I was hoping we could get away with having this override everything. But it is what it is. […]
I did that in order to support verified boot without TPM support even if it is more insecure. There is no other solution
https://review.coreboot.org/#/c/24903/33/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/24903/33/src/security/vboot/secdata_tpm.c@16... PS33, Line 166: const TPMA_NV ro_space_attributes = {
Were these extern'd in a header somewhere? If no one is using these outside of this compilation unit […]
gotcha
https://review.coreboot.org/#/c/24903/33/src/soc/intel/apollolake/Kconfig File src/soc/intel/apollolake/Kconfig:
https://review.coreboot.org/#/c/24903/33/src/soc/intel/apollolake/Kconfig@12... PS33, Line 126: default n
why get rid of the auto-selection here?
maybe depends on MAINBOARD_HAS_LPC_TPM makes more sense. Will fix it