secvar/secboot_tpm: unify behavior for bank hash check and secboot header check

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@linux.ibm.com>
Tested-by: Nick Child <nick.child@ibm.com>
Reviewed-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
(cherry picked from commit 8f72fe3071228fe71c0862ba1b5527ff11dbbfd3)
Signed-off-by: Cédric Le Goater <clg@kaod.org>

2 files changed, 29 insertions, 16 deletions

diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
index 45373cf..ff8ea9e 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 798ca28..3088567 100644
--- a/libstb/secvar/test/secvar-test-secboot-tpm.c
+++ b/libstb/secvar/test/secvar-test-secboot-tpm.c
@@ -100,21 +100,6 @@ int run_test(void)
 	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);
 
 	return 0;