[coreboot-gerrit] Change in coreboot[master]: security/tpm: Unify the coreboot TPM software stack

Fri Jun 1 16:56:41 CEST 2018

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@248
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@13
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@15
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_marshaling.h
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_marshaling.h@11
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@166
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@126
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

-- 
To view, visit https://review.coreboot.org/24903
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ec277e82a3c20c62a0548a1a2b013e6ce8f5b3f
Gerrit-Change-Number: 24903
Gerrit-PatchSet: 36
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph at 9elements.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Ronald G. Minnich <rminnich at gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer at coreboot.org>
Gerrit-Reviewer: Trammell Hudson <hudson at trmm.net>
Gerrit-Reviewer: Vadim Bendebury <vbendeb at chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Julius Werner <jwerner at chromium.org>
Gerrit-CC: Patrick Rudolph <siro at das-labor.org>
Gerrit-CC: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Comment-Date: Fri, 01 Jun 2018 14:56:41 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180601/4f284e9d/attachment.html>