[M] Change in coreboot[master]: security/tpm: replace CONFIG(TPMx) checks with runtime check

[Sergii Dmytruk (Code Review)]

Attention is currently required from: Lance Zhao, Michał Żygowski, Christian Walter, Tim Wawrzynczak, Julius Werner, Krystian Hebel, Yu-Ping Wu.

Hello Michał Żygowski, Julius Werner, Krystian Hebel,

I'd like you to do a code review.
Please visit

https://review.coreboot.org/c/coreboot/+/69161

to review the following change.

Change subject: security/tpm: replace CONFIG(TPMx) checks with runtime check
......................................................................

security/tpm: replace CONFIG(TPMx) checks with runtime check

Change-Id: Id9cc25aad8d1d7bfad12b7a92059b1b3641bbfa9
Ticket: https://ticket.coreboot.org/issues/433
Signed-off-by: Sergii Dmytruk 
---
M src/acpi/acpi.c
M src/drivers/crb/tis.c
M src/drivers/pc80/tpm/tis.c
M src/drivers/tpm/ppi.c
M src/security/tpm/tspi/crtm.h
M src/security/tpm/tspi/tspi.c
M src/security/tpm/tss.h
M src/security/tpm/tss/tss.c
M src/security/vboot/secdata_tpm.c
M src/security/vboot/tpm_common.c
M src/vendorcode/google/chromeos/cse_board_reset.c
M src/vendorcode/google/chromeos/tpm2.c
12 files changed, 108 insertions(+), 65 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/69161/1

diff --git a/src/acpi/acpi.c b/src/acpi/acpi.c
index e211558..8f6019c 100644
--- a/src/acpi/acpi.c
+++ b/src/acpi/acpi.c
@@ -26,6 +26,7 @@
 #include <device/mmio.h>
 #include <device/pci.h>
 #include <pc80/mc146818rtc.h>
+#include <security/tpm/tss.h>
 #include <string.h>
 #include <types.h>
 #include <version.h>
@@ -1781,7 +1782,7 @@
    	acpi_add_table(rsdp, mcfg);
    }

-	if (CONFIG(TPM1)) {
+	if (tlcl_get_family() == 1) {
    	printk(BIOS_DEBUG, "ACPI:    * TCPA\n");
    	tcpa = (acpi_tcpa_t *) current;
    	acpi_create_tcpa(tcpa);
@@ -1792,7 +1793,7 @@
    	}
    }

-	if (CONFIG(TPM2)) {
+	if (tlcl_get_family() == 2) {
    	printk(BIOS_DEBUG, "ACPI:    * TPM2\n");
    	tpm2 = (acpi_tpm2_t *) current;
    	acpi_create_tpm2(tpm2);
diff --git a/src/drivers/crb/tis.c b/src/drivers/crb/tis.c
index 1955466..812947d 100644
--- a/src/drivers/crb/tis.c
+++ b/src/drivers/crb/tis.c
@@ -140,6 +140,9 @@
    uint32_t fw_ver1, fw_ver2;
    uint8_t major_spec_ver, minor_spec_ver;

+	if (tlcl_get_family() == 1)
+		return get_smbios_data(dev, handle, current);
+
    tpm2_get_info(&info);

/* If any of these have invalid values, assume TPM not present or disabled */
diff --git a/src/drivers/pc80/tpm/tis.c b/src/drivers/pc80/tpm/tis.c
index e4cd05b..937de43 100644
--- a/src/drivers/pc80/tpm/tis.c
+++ b/src/drivers/pc80/tpm/tis.c
@@ -20,6 +20,7 @@
 #include <device/device.h>
 #include <console/console.h>
 #include <security/tpm/tis.h>
+#include <security/tpm/tss.h>
 #include <device/pnp.h>
 #include <drivers/tpm/tpm_ppi.h>
 #include <timer.h>
@@ -803,7 +804,7 @@
    acpigen_write_scope(path);
    acpigen_write_device(acpi_device_name(dev));

-	if (CONFIG(TPM2)) {
+	if (tlcl_get_family() == 2) {
    	acpigen_write_name_string("_HID", "MSFT0101");
    	acpigen_write_name_string("_CID", "MSFT0101");
    } else {
diff --git a/src/drivers/tpm/ppi.c b/src/drivers/tpm/ppi.c
index 3e74314..fdbdd8e 100644
--- a/src/drivers/tpm/ppi.c
+++ b/src/drivers/tpm/ppi.c
@@ -5,6 +5,7 @@
 #include <acpi/acpi_device.h>
 #include <cbmem.h>
 #include <console/console.h>
+#include <security/tpm/tss.h>

#include "tpm_ppi.h"

@@ -62,7 +63,7 @@
    acpigen_write_store();
    acpigen_emit_namestring("^FSUP");
    acpigen_emit_byte(LOCAL2_OP);
-	acpigen_emit_byte(CONFIG(TPM1) ? ONE_OP : ZERO_OP);
+	acpigen_emit_byte(tlcl_get_family() == 1 ? ONE_OP : ZERO_OP);
    acpigen_emit_byte(LOCAL1_OP);
    acpigen_write_if_lequal_op_int(LOCAL1_OP, 0);

@@ -84,7 +85,7 @@
    acpigen_write_store();
    acpigen_emit_namestring("^FSUP");
    acpigen_emit_byte(LOCAL2_OP);
-	acpigen_emit_byte(CONFIG(TPM1) ? ZERO_OP : ONE_OP);
+	acpigen_emit_byte(tlcl_get_family() == 1 ? ZERO_OP : ONE_OP);
    acpigen_emit_byte(LOCAL1_OP);

acpigen_write_if_lequal_op_int(LOCAL1_OP, 1);
@@ -114,7 +115,7 @@
  */
 static void tpm_ppi_func1_cb(void *arg)
 {
-	if (CONFIG(TPM2))
+	if (tlcl_get_family() == 2)
    	/* Interface version: 1.3 */
    	acpigen_write_return_string("1.3");
    else
@@ -410,7 +411,7 @@
    acpigen_write_store();
    acpigen_emit_namestring("^FSUP");
    acpigen_emit_byte(LOCAL2_OP);
-	acpigen_emit_byte(CONFIG(TPM1) ? ONE_OP : ZERO_OP);
+	acpigen_emit_byte(tlcl_get_family() == 1 ? ONE_OP : ZERO_OP);
    acpigen_emit_byte(LOCAL1_OP);
    acpigen_write_if_lequal_op_int(LOCAL1_OP, 0);
    acpigen_write_return_byte(0);	/* Not implemented */
@@ -418,7 +419,7 @@

// FIXME: Only advertise supported functions

-	if (CONFIG(TPM1)) {
+	if (tlcl_get_family() == 1) {
    	/*
    	 * Some functions do not require PP depending on configuration.
    	 * Those aren't listed here, so the 'required PP' is always set for those.
@@ -434,7 +435,7 @@
    		acpigen_write_return_integer(PPI8_RET_ALLOWED);
    		acpigen_pop_len();	/* Pop : If */
    	}
-	} else if (CONFIG(TPM2)) {
+	} else if (tlcl_get_family() == 2) {
    	/*
    	 * Some functions do not require PP depending on configuration.
    	 * Those aren't listed here, so the 'required PP' is always set for those.
@@ -574,7 +575,7 @@
    bool found = false;
    /* Fill in defaults, the TPM command executor may overwrite this list */
    memset(ppib->func, 0, sizeof(ppib->func));
-	if (CONFIG(TPM1)) {
+	if (tlcl_get_family() == 1) {
    	for (size_t i = 0; i < ARRAY_SIZE(tpm1_funcs); i++) {
    		ppib->func[tpm1_funcs[i]] = 1;
    		if (ppib->pprq == tpm1_funcs[i])
@@ -716,7 +717,7 @@
    tpm_ppi->tag = LB_TAG_TPM_PPI_HANDOFF;
    tpm_ppi->size = sizeof(*tpm_ppi);
    tpm_ppi->ppi_address = (uintptr_t)ppib;
-	tpm_ppi->tpm_version = CONFIG(TPM1) ? LB_TPM_VERSION_TPM_VERSION_1_2 :
+	tpm_ppi->tpm_version = tlcl_get_family() == 1 ? LB_TPM_VERSION_TPM_VERSION_1_2 :
    	LB_TPM_VERSION_TPM_VERSION_2;

tpm_ppi->ppi_version = BCD(1, 3);
diff --git a/src/security/tpm/tspi/crtm.h b/src/security/tpm/tspi/crtm.h
index bd5bc57..951c250 100644
--- a/src/security/tpm/tspi/crtm.h
+++ b/src/security/tpm/tspi/crtm.h
@@ -16,7 +16,7 @@
  */
 #define TPM_RUNTIME_DATA_PCR 3

-#define TPM_MEASURE_ALGO (CONFIG(TPM1) ? VB2_HASH_SHA1 : VB2_HASH_SHA256)
+#define TPM_MEASURE_ALGO (tlcl_get_family() == 1 ? VB2_HASH_SHA1 : VB2_HASH_SHA256)

/**
  * Measure digests cached in TCPA log entries into PCRs
diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c
index 525522b..d6e0534 100644
--- a/src/security/tpm/tspi/tspi.c
+++ b/src/security/tpm/tspi/tspi.c
@@ -16,6 +16,9 @@
    uint8_t deactivated;
    uint32_t result = TPM_SUCCESS;

+	if (tlcl_get_family() != 1)
+		return result;
+
    /* Check that the TPM is enabled and activated. */
    result = tlcl12_get_flags(&disabled, &deactivated, NULL);
    if (result != TPM_SUCCESS) {
@@ -200,19 +203,19 @@
    	return result;
    }

-#if CONFIG(TPM1)
-	result = tlcl12_set_enable();
-	if (result != TPM_SUCCESS) {
-		printk(BIOS_ERR, "TPM: Can't set enabled state.\n");
-		return result;
-	}
+	if (tlcl_get_family() == 1) {
+		result = tlcl12_set_enable();
+		if (result != TPM_SUCCESS) {
+			printk(BIOS_ERR, "TPM: Can't set enabled state.\n");
+			return result;
+		}

-	result = tlcl12_set_deactivated(0);
-	if (result != TPM_SUCCESS) {
-		printk(BIOS_ERR, "TPM: Can't set deactivated state.\n");
-		return result;
+		result = tlcl12_set_deactivated(0);
+		if (result != TPM_SUCCESS) {
+			printk(BIOS_ERR, "TPM: Can't set deactivated state.\n");
+			return result;
+		}
    }
-#endif

return TPM_SUCCESS;
 }
diff --git a/src/security/tpm/tss.h b/src/security/tpm/tss.h
index 9d6e55a..dc910b9 100644
--- a/src/security/tpm/tss.h
+++ b/src/security/tpm/tss.h
@@ -137,6 +137,11 @@
  */
 uint32_t tlcl_lib_init(void);

+/**
+ * Query active TPM family.  Returns 0 if uninitialized and 1 or 2 otherwise.
+ */
+int tlcl_get_family(void);
+
 /* Commands */

/**
diff --git a/src/security/tpm/tss/tss.c b/src/security/tpm/tss/tss.c
index 36ea575..381c45b1 100644
--- a/src/security/tpm/tss/tss.c
+++ b/src/security/tpm/tss/tss.c
@@ -48,6 +48,11 @@
    return VB2_ERROR_UNKNOWN;
 }

+int tlcl_get_family(void)
+{
+	return tpm_family;
+}
+
 uint32_t tlcl_startup(void)
 {
 #if CONFIG(TPM1)
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c
index ad2d812..9c84ecf 100644
--- a/src/security/vboot/secdata_tpm.c
+++ b/src/security/vboot/secdata_tpm.c
@@ -38,7 +38,7 @@

uint32_t antirollback_read_space_kernel(struct vb2_context *ctx)
 {
-	if (!CONFIG(TPM2)) {
+	if (tlcl_get_family() != 2) {
    	/*
    	 * Before reading the kernel space, verify its permissions. If
    	 * the kernel space has the wrong permission, we give up. This
@@ -233,12 +233,6 @@
    return rv;
 }

-/* Nothing special in the TPM2 path yet. */
-static uint32_t safe_write(uint32_t index, const void *data, uint32_t length)
-{
-	return tlcl_write(index, data, length);
-}
-
 static uint32_t setup_space(const char *name, uint32_t index, const void *data,
    		    uint32_t length, const TPMA_NV nv_attributes,
    		    const uint8_t *nv_policy, size_t nv_policy_size)
@@ -381,7 +375,7 @@
    return TPM_SUCCESS;
 }

-static uint32_t _factory_initialize_tpm(struct vb2_context *ctx)
+static uint32_t _factory_initialize_tpm2(struct vb2_context *ctx)
 {
    RETURN_ON_FAILURE(tlcl_force_clear());

@@ -431,11 +425,6 @@
    return TPM_SUCCESS;
 }

-uint32_t antirollback_lock_space_firmware(void)
-{
-	return tlcl_lock_nv_write(FIRMWARE_NV_INDEX);
-}
-
 uint32_t antirollback_read_space_mrc_hash(uint32_t index, uint8_t *data, uint32_t size)
 {
    if (size != HASH_NV_SIZE) {
@@ -480,25 +469,7 @@
    return tlcl_lock_nv_write(index);
 }

-#else
-
-/**
- * Like tlcl_write(), but checks for write errors due to hitting the 64-write
- * limit and clears the TPM when that happens.  This can only happen when the
- * TPM is unowned, so it is OK to clear it (and we really have no choice).
- * This is not expected to happen frequently, but it could happen.
- */
-
-static uint32_t safe_write(uint32_t index, const void *data, uint32_t length)
-{
-	uint32_t result = tlcl_write(index, data, length);
-	if (result == TPM_E_MAXNVWRITES) {
-		RETURN_ON_FAILURE(tpm_clear_and_reenable());
-		return tlcl_write(index, data, length);
-	} else {
-		return result;
-	}
-}
+#endif /* CONFIG(TPM2) */

/**
  * Similarly to safe_write(), this ensures we don't fail a DefineSpace because
@@ -517,7 +488,7 @@
    }
 }

-static uint32_t _factory_initialize_tpm(struct vb2_context *ctx)
+static uint32_t _factory_initialize_tpm1(struct vb2_context *ctx)
 {
    TPM_PERMANENT_FLAGS pflags;
    uint32_t result;
@@ -575,12 +546,44 @@
    return TPM_SUCCESS;
 }

-uint32_t antirollback_lock_space_firmware(void)
+#endif /* CONFIG(TPM1) */
+
+static uint32_t safe_write(uint32_t index, const void *data, uint32_t length)
 {
-	return tlcl_set_global_lock();
+	uint32_t result;
+
+	if (tlcl_get_family() == 2)
+		/* Nothing special in the TPM2 path yet. */
+		return tlcl_write(index, data, length);
+
+	/**
+	 * Like tlcl_write(), but checks for write errors due to hitting the 64-write
+	 * limit and clears the TPM when that happens.  This can only happen when the
+	 * TPM is unowned, so it is OK to clear it (and we really have no choice).
+	 * This is not expected to happen frequently, but it could happen.
+	 */
+	result = tlcl_write(index, data, length);
+	if (result == TPM_E_MAXNVWRITES) {
+		RETURN_ON_FAILURE(tpm_clear_and_reenable());
+		return tlcl_write(index, data, length);
+	} else {
+		return result;
+	}
 }

-#endif
+static uint32_t _factory_initialize_tpm(struct vb2_context *ctx)
+{
+	if (tlcl_get_family() == 2)
+		return _factory_initialize_tpm2(ctx);
+	return _factory_initialize_tpm1(ctx);
+}
+
+uint32_t antirollback_lock_space_firmware(void)
+{
+	if (tlcl_get_family() == 2)
+		return tlcl_lock_nv_write(FIRMWARE_NV_INDEX);
+	return tlcl_set_global_lock();
+}

/**
  * Perform one-time initializations.
diff --git a/src/security/vboot/tpm_common.c b/src/security/vboot/tpm_common.c
index e67cc01..a757477 100644
--- a/src/security/vboot/tpm_common.c
+++ b/src/security/vboot/tpm_common.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

#include <security/tpm/tspi.h>
+#include <security/tpm/tss.h>
 #include <security/vboot/tpm_common.h>
 #include <vb2_api.h>
 #include <vb2_sha.h>
@@ -45,7 +46,7 @@
     */
    _Static_assert(sizeof(buffer) >= VB2_SHA256_DIGEST_SIZE,
    	       "Buffer needs to be able to fit at least a SHA256");
-	enum vb2_hash_algorithm algo = CONFIG(TPM1) ? VB2_HASH_SHA1 : VB2_HASH_SHA256;
+	enum vb2_hash_algorithm algo = tlcl_get_family() == 1 ? VB2_HASH_SHA1 : VB2_HASH_SHA256;

switch (which_digest) {
    /* SHA1 of (devmode|recmode|keyblock) bits */
diff --git a/src/vendorcode/google/chromeos/cse_board_reset.c b/src/vendorcode/google/chromeos/cse_board_reset.c
index 08db7e2..9b6f878 100644
--- a/src/vendorcode/google/chromeos/cse_board_reset.c
+++ b/src/vendorcode/google/chromeos/cse_board_reset.c
@@ -16,6 +16,11 @@
    int ret;
    struct cr50_firmware_version version;

+	/*
+	 * Assuming that if particular TPM implementation is enabled at compile
+	 * time, it's the one being used.  This isn't generic code, so can
+	 * probably get away with it.
+	 */
    if (CONFIG(TPM2) && CONFIG(TPM_GOOGLE_CR50)) {
    	/* Initialize TPM and get the cr50 firmware version. */
    	ret = tlcl_lib_init();
@@ -27,11 +32,11 @@
    	cr50_get_firmware_version(&version);

/*
-		 * Cr50 firmware versions 0.[3|4].20 or newer support strap
-		 * config 0xe where PLTRST from AP is connected to cr50's
-		 * PLTRST# signal. So return immediately and trigger a global
-		 * reset.
-		 */
+		* Cr50 firmware versions 0.[3|4].20 or newer support strap
+		* config 0xe where PLTRST from AP is connected to cr50's
+		* PLTRST# signal. So return immediately and trigger a global
+		* reset.
+		*/
    	if (version.epoch != 0 || version.major > 4 ||
    	    (version.major >= 3 && version.minor >= 20))
    		return;
diff --git a/src/vendorcode/google/chromeos/tpm2.c b/src/vendorcode/google/chromeos/tpm2.c
index 98fd815..4020155 100644
--- a/src/vendorcode/google/chromeos/tpm2.c
+++ b/src/vendorcode/google/chromeos/tpm2.c
@@ -22,6 +22,10 @@
    	return;
    }

+	/* In case both families are enabled, but TPM1 is in use. */
+	if (tlcl_get_family() != 2)
+		return;
+
    ret = tlcl_disable_platform_hierarchy();
    if (ret != TPM_SUCCESS)
    	printk(BIOS_ERR, "Platform hierarchy disablement failed: %x\n",