Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
security/vboot: Provide stub functions for TPM in bootblock
With commit c79e96b4eb310db9d44e36e2dff072c01469c380 (security/vboot: Decouple measured boot from verified boot) the measured boot logic was decoupled from VBOOT and the TSS layer of the TPM driver was added to the bootblock. If now TPM_MEASURED_BOOT and VBOOT are selected at the same time, the TPM functions called by the TSS layer are missing during link time of the bootblock as at least the LPC TPM driver is not enabled for bootblcok. On the other side enabling the LPC TPM driver in bootblock leads to an increased bootblock which on Apollo Lake does not fit anymore.
Since the intention of the mentioned patch was to decouple the both features MEASURED_BOOT and VBOOT while still keep it possible to have both enabled at the same time the missing functions need to be available in bootblock though they are not needed (just to be able to link).
For this reason this patch provides a new Kconfig switch called 'TPM_IN_BOOTBLOCK' which, when set, will enable the full TPM functionality in bootblock. As it is disabled per default just empty stubs will be compiled for bootblock. If a platform needs the full TPM driver in bootblock it can simply select 'TPM_IN_BOOTBLOCK'.
Change-Id: I888928a6577f4b920b5ee8b65daf23aa46e3aff8 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/crb/tis.c M src/drivers/i2c/tpm/tis.c M src/drivers/i2c/tpm/tis_atmel.c M src/drivers/pc80/tpm/Makefile.inc M src/drivers/pc80/tpm/tis.c M src/drivers/spi/tpm/tis.c M src/security/tpm/Kconfig 7 files changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/39971/1
diff --git a/src/drivers/crb/tis.c b/src/drivers/crb/tis.c index b1fbad0..ef10002 100644 --- a/src/drivers/crb/tis.c +++ b/src/drivers/crb/tis.c @@ -44,6 +44,10 @@
int tis_open(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + if (tpm_is_open) { printk(BIOS_ERR, "%s called twice.\n", __func__); return -1; @@ -62,6 +66,10 @@
int tis_close(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + if (tpm_is_open) {
/* @@ -78,6 +86,10 @@ { struct tpm2_info info;
+ if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + // Wake TPM up (if necessary) if (tpm2_init() != 0) return -1; @@ -93,6 +105,10 @@
int tis_sendrecv(const uint8_t *sendbuf, size_t sbuf_size, uint8_t *recvbuf, size_t *rbuf_len) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + int len = tpm2_process_command(sendbuf, sbuf_size, recvbuf, *rbuf_len);
if (len == 0) diff --git a/src/drivers/i2c/tpm/tis.c b/src/drivers/i2c/tpm/tis.c index 8b07bb7..b7b9009 100644 --- a/src/drivers/i2c/tpm/tis.c +++ b/src/drivers/i2c/tpm/tis.c @@ -35,6 +35,10 @@ { int rc;
+ if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + if (chip.is_open) { printk(BIOS_DEBUG, "tis_open() called twice.\n"); return -1; @@ -53,6 +57,10 @@
int tis_close(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + if (chip.is_open) { tpm_vendor_cleanup(&chip); chip.is_open = 0; @@ -63,6 +71,10 @@
int tis_init(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + return tpm_vendor_probe(CONFIG_DRIVER_TPM_I2C_BUS, CONFIG_DRIVER_TPM_I2C_ADDR); } @@ -133,6 +145,10 @@ int tis_sendrecv(const uint8_t *sendbuf, size_t sbuf_size, uint8_t *recvbuf, size_t *rbuf_len) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + ASSERT(sbuf_size >= 10);
/* Display the TPM command */ diff --git a/src/drivers/i2c/tpm/tis_atmel.c b/src/drivers/i2c/tpm/tis_atmel.c index 74b4830..497e4fe 100644 --- a/src/drivers/i2c/tpm/tis_atmel.c +++ b/src/drivers/i2c/tpm/tis_atmel.c @@ -59,6 +59,10 @@ int status; struct stopwatch sw;
+ if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + ASSERT(sbuf_size >= 10); if (CONFIG(DRIVER_TPM_DISPLAY_TIS_BYTES)) { /* Display the TPM command */ diff --git a/src/drivers/pc80/tpm/Makefile.inc b/src/drivers/pc80/tpm/Makefile.inc index 0de1b76..cba3e43 100644 --- a/src/drivers/pc80/tpm/Makefile.inc +++ b/src/drivers/pc80/tpm/Makefile.inc @@ -1,3 +1,4 @@ +bootblock-$(CONFIG_LPC_TPM) += tis.c verstage-$(CONFIG_LPC_TPM) += tis.c romstage-$(CONFIG_LPC_TPM) += tis.c ramstage-$(CONFIG_LPC_TPM) += tis.c diff --git a/src/drivers/pc80/tpm/tis.c b/src/drivers/pc80/tpm/tis.c index a35ef83..000abe1 100644 --- a/src/drivers/pc80/tpm/tis.c +++ b/src/drivers/pc80/tpm/tis.c @@ -635,6 +635,10 @@ */ int tis_init(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return TPM_DRIVER_ERR; + } + if (tis_probe()) return TPM_DRIVER_ERR; return 0; @@ -652,6 +656,10 @@ { u8 locality = 0; /* we use locality zero for everything */
+ if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return TPM_DRIVER_ERR; + } + if (tis_close()) return TPM_DRIVER_ERR;
@@ -684,6 +692,11 @@ int tis_close(void) { u8 locality = 0; + + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return TPM_DRIVER_ERR; + } + if (tis_has_access(locality)) { tis_drop_access(locality); if (tis_wait_dropped_access(locality)) { @@ -711,6 +724,10 @@ int tis_sendrecv(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf, size_t *recv_len) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return TPM_DRIVER_ERR; + } + if (tis_senddata(sendbuf, send_size)) { printf("%s:%d failed sending data to TPM\n", __FILE__, __LINE__); diff --git a/src/drivers/spi/tpm/tis.c b/src/drivers/spi/tpm/tis.c index 6230751..2f2fe4a 100644 --- a/src/drivers/spi/tpm/tis.c +++ b/src/drivers/spi/tpm/tis.c @@ -33,6 +33,9 @@
int tis_open(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } if (tpm_is_open) { printk(BIOS_ERR, "tis_open() called twice.\n"); return -1; @@ -42,6 +45,10 @@
int tis_close(void) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + if (tpm_is_open) {
/* @@ -59,6 +66,10 @@ struct spi_slave spi; struct tpm2_info info;
+ if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + if (spi_setup_slave(CONFIG_DRIVER_TPM_SPI_BUS, CONFIG_DRIVER_TPM_SPI_CHIP, &spi)) { printk(BIOS_ERR, "Failed to setup TPM SPI slave\n"); @@ -82,6 +93,10 @@ int tis_sendrecv(const uint8_t *sendbuf, size_t sbuf_size, uint8_t *recvbuf, size_t *rbuf_len) { + if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { + return -1; + } + int len = tpm2_process_command(sendbuf, sbuf_size, recvbuf, *rbuf_len);
if (len == 0) diff --git a/src/security/tpm/Kconfig b/src/security/tpm/Kconfig index d8652b2..5c6f1aa 100644 --- a/src/security/tpm/Kconfig +++ b/src/security/tpm/Kconfig @@ -117,4 +117,12 @@ Runtime data whitelist of cbfs filenames. Needs to be a comma separated list
+config TPM_IN_BOOTBLOCK + bool "Enable TPM driver in bootblock" + default n + depends on TPM_MEASURED_BOOT + help + Enable the TPM driver in bootblock. Select this option if the bootblock + has enough space to include the TPM driver. + endmenu # Trusted Platform Module (tpm)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/crb/tis.c File src/drivers/crb/tis.c:
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/crb/tis.c@89 PS1, Line 89: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/crb/tis.c@108 PS1, Line 108: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/i2c/tpm/tis.c File src/drivers/i2c/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/i2c/tpm/tis.c@7... PS1, Line 74: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/i2c/tpm/tis.c@1... PS1, Line 148: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/i2c/tpm/tis_atm... File src/drivers/i2c/tpm/tis_atmel.c:
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/i2c/tpm/tis_atm... PS1, Line 62: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/pc80/tpm/tis.c File src/drivers/pc80/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/pc80/tpm/tis.c@... PS1, Line 638: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/pc80/tpm/tis.c@... PS1, Line 659: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/spi/tpm/tis.c File src/drivers/spi/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/39971/1/src/drivers/spi/tpm/tis.c@9... PS1, Line 96: if (ENV_BOOTBLOCK && !CONFIG(TPM_IN_BOOTBLOCK)) { braces {} are not necessary for single statement blocks
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39971/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39971/1//COMMIT_MSG@15 PS1, Line 15: co letters change
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1: Code-Review+2
Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1: Code-Review+1
It seems your 'TPM_IN_BOOTBLOCK' has similar requirement with (VBOOT_STARTS_IN_BOOTBLOCK && !VBOOT_SEPARATE_VERSTAGE). Is it possible to unify these flags?
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
It seems your 'TPM_IN_BOOTBLOCK' has similar requirement with (VBOOT_STARTS_IN_BOOTBLOCK && !VBOOT_SEPARATE_VERSTAGE). Is it possible to unify these flags?
The issue here is that it really depends on the platform the code is compiled for if the bootblock is big enough to host the full tpm driver or not. Even if VBOOT_STARTS_IN_BOOTBLOCK has worked out so far after the decouple patch it does not fit anymore on Apollo Lake. The Ide with the separate switch was to be able to decide it per platform. So if there is enough space then it can be enabled. If one platform suffers with bootblock size constraints one can just use the empty stubs.
An alternative would be to add __weak functions and get rid of the TPM driver layer completely.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1: Code-Review-1
Sorry, I don't think this is a good way to fix this. The TPM routines shouldn't get linked into the bootblock in the first place if they're not needed. We probably just need to inline the right function to help the compiler along, please give me a couple of hours to try to come up with an alternative fix.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Patch Set 1:
Here, I think this should be all we need: https://review.coreboot.org/c/coreboot/+/39993
Werner Zeh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39971 )
Change subject: security/vboot: Provide stub functions for TPM in bootblock ......................................................................
Abandoned
Addressed better in CB:39993