[coreboot-gerrit] Change in coreboot[master]: security/vboot: Add boot mode selection

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/23756 )

Change subject: security/vboot: Add boot mode selection
......................................................................


Patch Set 14:

(9 comments)

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

https://review.coreboot.org/#/c/23756/14/src/drivers/i2c/tpm/Makefile.inc@5
PS14, Line 5: postcar-$(CONFIG_DRIVER_TIS_DEFAULT) += tis.c
> you shouldn't replace bootblock with postcar. you should add postcar.
Done


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

https://review.coreboot.org/#/c/23756/14/src/drivers/spi/tpm/Makefile.inc@4
PS14, Line 4: postcar-$(CONFIG_SPI_TPM) += tis.c tpm.c
> need bootblock here too
Done


https://review.coreboot.org/#/c/23756/14/src/mainboard/google/poppy/Kconfig
File src/mainboard/google/poppy/Kconfig:

https://review.coreboot.org/#/c/23756/14/src/mainboard/google/poppy/Kconfig@123
PS14, Line 123: 	select VARIANT_HAS_I2C_TPM
> What's this one for? What's the interaction when VBOOT_MOCK_SECDATA is enabled?
Only enable the tpm if antirollback protection isn't disabled. Does not make sense because can be set through defconfig


https://review.coreboot.org/#/c/23756/14/src/security/vboot/Makefile.inc
File src/security/vboot/Makefile.inc:

https://review.coreboot.org/#/c/23756/14/src/security/vboot/Makefile.inc@75
PS14, Line 75: verstage-y += secdata_measurements.c
> This should be based on a Kconfig.
Done


https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurements.c
File src/security/vboot/secdata_measurements.c:

https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurements.c@21
PS14, Line 21: uint32_t vboot_measure_self(void)
> 'self' is not really self. It's only bootblock. Sounds like you want vboot_measure_bootblock().
vboot_measure_crtm() -> core root of trust for measurement


https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurements.c@24
PS14, Line 24: PROG_UNKNOWN
> Why not have PROG_BOOTBLOCK? You could just call measured_prog_run() in that case instead of hardcod […]
Done


https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurements.c@30
PS14, Line 30: 	if (cbfs_boot_locate(&file1, prog_name(&bootblock), NULL))
> So this is assuming SEPARATE_VERSTAGE it seems?
Done


https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_tpm.c
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_tpm.c@a448
PS14, Line 448: 
> Where is this being done now?
vboot_logic at the beginning of verstage_main()


https://review.coreboot.org/#/c/23756/14/src/security/vboot/vboot_logic.c
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/23756/14/src/security/vboot/vboot_logic.c@315
PS14, Line 315: #if IS_ENABLED(CONFIG_VBOOT_MODE_VERIFIED_AND_MEASURED_BOOT)
> no #ifs
Done



-- 
To view, visit https://review.coreboot.org/23756
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: I43d233d5a8766af2dd7f07cc0b64293a80d5d7d2
Gerrit-Change-Number: 23756
Gerrit-PatchSet: 14
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Thu, 15 Feb 2018 01:15:57 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180215/10efb627/attachment.html>