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

Sat May 19 17:07:33 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 19:

(16 comments)

https://review.coreboot.org/#/c/24903/18/src/cpu/intel/haswell/romstage.c
File src/cpu/intel/haswell/romstage.c:

https://review.coreboot.org/#/c/24903/18/src/cpu/intel/haswell/romstage.c@248
PS18, Line 248: 	if (IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2))
> Actually, how are all these tpm_setup()s in chipset code going to interact with vboot? CONFIG_VBOOT  […]
I fixed this issue in a later commit by introducing a tpm ramstage driver. People told me to split it up

https://review.coreboot.org/#/c/24903/18/src/drivers/i2c/tpm/Makefile.inc
File src/drivers/i2c/tpm/Makefile.inc:

https://review.coreboot.org/#/c/24903/18/src/drivers/i2c/tpm/Makefile.inc@1
PS18, Line 1: ifneq ($(I2C_TPM),)
> Shouldn't this just check for I2C_TPM? (Not sure why I didn't say that before... […]
Done

https://review.coreboot.org/#/c/24903/18/src/drivers/pc80/tpm/Makefile.inc
File src/drivers/pc80/tpm/Makefile.inc:

https://review.coreboot.org/#/c/24903/18/src/drivers/pc80/tpm/Makefile.inc@1
PS18, Line 1: ifneq ($(LPC_TPM),)
> Same here. […]
Done

https://review.coreboot.org/#/c/24903/18/src/drivers/spi/tpm/Makefile.inc
File src/drivers/spi/tpm/Makefile.inc:

https://review.coreboot.org/#/c/24903/18/src/drivers/spi/tpm/Makefile.inc@1
PS18, Line 1: ifneq ($(SPI_TPM),)
> Same here
Done

https://review.coreboot.org/#/c/24903/18/src/drivers/spi/tpm/Makefile.inc@7
PS18, Line 7: 
> ...also, I'm not sure where this came from, but this could be cleaned up while you're here.
Done

https://review.coreboot.org/#/c/24903/18/src/mainboard/gigabyte/ga-b75m-d3h/Kconfig
File src/mainboard/gigabyte/ga-b75m-d3h/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/gigabyte/ga-b75m-d3h/Kconfig@20
PS18, Line 20: 	select MAINBOARD_HAS_LPC_TPM
> Don't you need MAINBOARD_HAS_TPM1 if this board has TPM1. […]
No it's like before. You can select that via kconfig

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/kahlee/Kconfig
File src/mainboard/google/kahlee/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/kahlee/Kconfig@128
PS18, Line 128: config KAHLEE_TPM
> I don't think this is the best way to deal with this, with two separate options. […]
Done

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/oak/Kconfig
File src/mainboard/google/oak/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/oak/Kconfig@26
PS18, Line 26: 
> This is not enough. […]
Done

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/octopus/Kconfig
File src/mainboard/google/octopus/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/octopus/Kconfig@19
PS18, Line 19: 	select MAINBOARD_HAS_SPI_TPM_CR50
> Also needs MAINBOARD_HAS_TPM2, right?
Done

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/poppy/Kconfig
File src/mainboard/google/poppy/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/poppy/Kconfig@141
PS18, Line 141: 	select EXCLUDE_NATIVE_SD_INTERFACE
> This could just go to the BASEBOARD option, I think.
Done

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/stout/Kconfig
File src/mainboard/google/stout/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/google/stout/Kconfig@17
PS18, Line 17: 	select MAINBOARD_HAS_TPM1
> You're still missing a lot of Google boards in this patch. Daisy, Storm, Veyron, etc... […]
Done

https://review.coreboot.org/#/c/24903/18/src/mainboard/intel/glkrvp/Kconfig
File src/mainboard/intel/glkrvp/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/intel/glkrvp/Kconfig@12
PS18, Line 12: 	select MAINBOARD_HAS_LPC_TPM
> Wouldn't you need MAINBOARD_HAS_TPM2 here though? You still need to select the used version of the T […]
see comment before

https://review.coreboot.org/#/c/24903/18/src/mainboard/intel/kblrvp/Kconfig
File src/mainboard/intel/kblrvp/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/mainboard/intel/kblrvp/Kconfig@17
PS18, Line 17: 	select MAINBOARD_HAS_LPC_TPM
> I think this should select both MAINBOARD_HAS_TPM1 and MAINBOARD_HAS_TPM2, so that the user can sele […]
already done by not setting it.

https://review.coreboot.org/#/c/24903/18/src/security/tpm/tss/vendor/cr50/tss_structures.h
File src/security/tpm/tss/vendor/cr50/tss_structures.h:

https://review.coreboot.org/#/c/24903/18/src/security/tpm/tss/vendor/cr50/tss_structures.h@23
PS18, Line 23:     knowledge of all commands. */
> Is this a comment you added or did you copy it from somewhere? Because I don't really understand wha […]
It's google stuff. Ask Vadim..

https://review.coreboot.org/#/c/24903/18/src/security/vboot/Kconfig
File src/security/vboot/Kconfig:

https://review.coreboot.org/#/c/24903/18/src/security/vboot/Kconfig@20
PS18, Line 20: 	select VBOOT_MOCK_SECDATA if !TPM1 && !TPM2
> This is still not right...
To be honest if you don't have a tpm you can use a verified boot. Maybe it's not secure against replay attacks but that's how it is.

https://review.coreboot.org/#/c/24903/18/src/security/vboot/secdata_mock.c
File src/security/vboot/secdata_mock.c:

https://review.coreboot.org/#/c/24903/18/src/security/vboot/secdata_mock.c@43
PS18, Line 43: 	return VB2_SUCCESS;
> Still disagreeing with the return code change.
Let's ask someone else

-- 
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: 19
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
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: Sat, 19 May 2018 15:07:33 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180519/31ae347c/attachment.html>