[Skiboot] [PATCH v2 2/6] 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>
---
 libstb/secvar/storage/secboot_tpm.c          | 30 +++++++++++++++++++-
 libstb/secvar/test/secvar-test-secboot-tpm.c |  6 ++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
index 5907ff07..932e5fd3 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..f203e57d 100644
--- a/libstb/secvar/test/secvar-test-secboot-tpm.c
+++ b/libstb/secvar/test/secvar-test-secboot-tpm.c
@@ -101,10 +101,15 @@ int run_test(void)
 
 	clear_bank_list(&variable_bank);
 
+	/* Disabling check for now, since bank hash check has been moved to _init().
+	 * This will be re-enabled with the forthcoming rework of the tests/_init() procedure.
+	 */
+#if 0
 	// 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);
+	fprintf(stderr, "rc = %d\n", rc);
 	ASSERT(rc != OPAL_SUCCESS); // TODO: permission?
 
 	// Fix it back...
@@ -113,6 +118,7 @@ int run_test(void)
 	// Should be ok again
 	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
 	ASSERT(rc == OPAL_SUCCESS);
+#endif
 
 	clear_bank_list(&variable_bank);
 	free(secboot_buffer);
-- 
2.29.2