[Skiboot] [PATCH v3 2/3] secvar/secboot_tpm: unify behavior for bank hash check and secboot header check

[Eric Richter]

As the PNOR variable space cannot be locked, the data must be integrity
checked when loaded to ensure it has not beeen modified by an unauthorized
party. In the event that a modification has been detected (i.e. hash mismatch),
we must not load in data that could potentially be compromised.

However, the previous code was a bit overzealous with its reaction to detecting
a compromised SECBOOT partition, and also had some inconsistencies in behavior.

Case 1: SECBOOT partition cleared.
.init() checks the header for the magic number and version. As neither matches,
will reformat the entire partition. Now, .load_bank() will pass, as the data
was just freshly reformatted (note: this also could trigger the bug addressed
in the previous patch). Only variables in the TPM will be loaded by
.load_bank() as the data in SECBOOT is now empty.

Case 2: Bank hash mismatch.
.load_bank() panics and returns an error code, causing secvar_main() to jump
to the error scenario, which prevents the secvar API from being exposed.
os-secure-enforcing is set unconditionally, and the user will have no API to
manage or attempt to fix their system without issuing a key clear request.

This patch unifies the behavior of both of these cases. Now, .init() handles
checking the header AND comparing the bank hash. If either check fails, the
SECBOOT partition will be reformatted. Variables in the TPM will still be
loaded in the .load_bank() step, and provided the backend stores its
secure boot state in the TPM, secure boot state can be preserved.

Signed-off-by: Eric Richter <erichte at linux.ibm.com>
Tested-by: Nick Child <nick.child at ibm.com>
Reviewed-by: Daniel Axtens <dja at axtens.net>
---
 libstb/secvar/storage/secboot_tpm.c          | 30 +++++++++++++++++++-
 libstb/secvar/test/secvar-test-secboot-tpm.c | 15 ----------
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
index 45373cf4..ff8ea9e0 100644
--- a/libstb/secvar/storage/secboot_tpm.c
+++ b/libstb/secvar/storage/secboot_tpm.c
@@ -374,7 +374,9 @@ fail:
 	return rc;
 }
 
-static int secboot_tpm_load_variable_bank(struct list_head *bank)
+
+/* Helper to validate the current active SECBOOT bank's data against the hash stored in the TPM */
+static int compare_bank_hash(void)
 {
 	char bank_hash[SHA256_DIGEST_LENGTH];
 	uint64_t bit = tpmnv_control_image->active_bit;
@@ -394,6 +396,15 @@ static int secboot_tpm_load_variable_bank(struct list_head *bank)
 		/* Tampered pnor space detected, abandon ship */
 		return OPAL_PERMISSION;
 
+	return OPAL_SUCCESS;
+}
+
+
+static int secboot_tpm_load_variable_bank(struct list_head *bank)
+{
+	uint64_t bit = tpmnv_control_image->active_bit;
+	int rc;
+
 	rc = secboot_tpm_deserialize_from_buffer(bank, tpmnv_vars_image->vars, tpmnv_vars_size, SECVAR_FLAG_PROTECTED);
 	if (rc)
 		return rc;
@@ -692,8 +703,25 @@ static int secboot_tpm_store_init(void)
 		rc = secboot_format();
 		if (rc)
 			goto error;
+		goto done;
 	}
 
+	/* Verify the active bank's integrity by comparing against the hash in TPM.
+	 * Reformat if it does not match -- we do not want to load potentially
+	 * compromised data.
+	 * Ideally, the backend driver should retain secure boot state in
+	 * protected (TPM) storage, so secure boot state should be the same, albeit
+	 * without the data in unprotected (PNOR) storage.
+	 */
+	rc = compare_bank_hash();
+	if (rc == OPAL_PERMISSION) {
+		rc = secboot_format();
+		if (rc)
+			goto error;
+	}
+	else if (rc)
+		goto error;
+
 done:
 	return OPAL_SUCCESS;
 
diff --git a/libstb/secvar/test/secvar-test-secboot-tpm.c b/libstb/secvar/test/secvar-test-secboot-tpm.c
index 798ca281..30885674 100644
--- a/libstb/secvar/test/secvar-test-secboot-tpm.c
+++ b/libstb/secvar/test/secvar-test-secboot-tpm.c
@@ -99,21 +99,6 @@ int run_test(void)
 	ASSERT(*((uint64_t*) secboot_image->bank[0]) != 0llu);
 	ASSERT(*((uint64_t*) secboot_image->bank[1]) != 0llu);
 
-	clear_bank_list(&variable_bank);
-
-	// Tamper with pnor, hash check should catch this
-	secboot_image->bank[0][0] = ~secboot_image->bank[0][0];
-
-	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
-	ASSERT(rc != OPAL_SUCCESS); // TODO: permission?
-
-	// Fix it back...
-	secboot_image->bank[0][0] = ~secboot_image->bank[0][0];
-
-	// Should be ok again
-	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
-	ASSERT(rc == OPAL_SUCCESS);
-
 	clear_bank_list(&variable_bank);
 	free(secboot_buffer);
 
-- 
2.29.2