vboot/secdata: move permissions and uid check to ReadSpaceKernel

Relocate permissions and uid check from RollbackKernelRead
to ReadSpaceKernel.

Restructure test code to set default values in ResetMocks.

BUG=b:124141368, chromium:972956
TEST=make clean && make runtests
BRANCH=none

Change-Id: I72c536042b89684c6db5099412344678e3d9d920
Signed-off-by: Joel Kitching <kitching@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1758146
Reviewed-by: Julius Werner <jwerner@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Commit-Queue: Joel Kitching <kitching@chromium.org>

2 files changed, 78 insertions, 63 deletions

diff --git a/firmware/lib/rollback_index.c b/firmware/lib/rollback_index.c
index b7549bba..b0d4eb58 100644
--- a/firmware/lib/rollback_index.c
+++ b/firmware/lib/rollback_index.c
@@ -141,6 +141,22 @@ vb2_error_t SetVirtualDevMode(int val)
 
 uint32_t ReadSpaceKernel(RollbackSpaceKernel *rsk)
 {
+#ifndef TPM2_MODE
+	/*
+	 * Before reading the kernel space, verify its permissions.  If the
+	 * kernel space has the wrong permission, we give up.  This will need
+	 * to be fixed by the recovery kernel.  We will have to worry about
+	 * this because at any time (even with PP turned off) the TPM owner can
+	 * remove and redefine a PP-protected space (but not write to it).
+	 */
+	uint32_t perms;
+
+	RETURN_ON_FAILURE(TlclGetPermissions(KERNEL_NV_INDEX, &perms));
+
+	if (perms != TPM_NV_PER_PPWRITE)
+		return TPM_E_CORRUPTED_STATE;
+#endif
+
 	uint32_t r;
 
 	r = TlclRead(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
@@ -153,6 +169,9 @@ uint32_t ReadSpaceKernel(RollbackSpaceKernel *rsk)
 	if (rsk->struct_version < ROLLBACK_SPACE_FIRMWARE_VERSION)
 		return TPM_E_STRUCT_VERSION;
 
+	if (rsk->uid != ROLLBACK_SPACE_KERNEL_UID)
+		return TPM_E_CORRUPTED_STATE;
+
 	if (rsk->crc8 != vb2_crc8(rsk, offsetof(RollbackSpaceKernel, crc8))) {
 		VB2_DEBUG("TPM: bad secdatak CRC\n");
 		return TPM_E_CORRUPTED_STATE;
@@ -207,31 +226,7 @@ uint32_t RollbackFwmpRead(struct RollbackSpaceFwmp *fwmp)
 uint32_t RollbackKernelRead(uint32_t* version)
 {
 	RollbackSpaceKernel rsk;
-
-	/*
-	 * Read the kernel space and verify its permissions.  If the kernel
-	 * space has the wrong permission, or it doesn't contain the right
-	 * identifier, we give up.  This will need to be fixed by the
-	 * recovery kernel.  We have to worry about this because at any time
-	 * (even with PP turned off) the TPM owner can remove and redefine a
-	 * PP-protected space (but not write to it).
-	 */
 	RETURN_ON_FAILURE(ReadSpaceKernel(&rsk));
-#ifndef TPM2_MODE
-	/*
-	 * TODO(vbendeb): restore this when it is defined how the kernel space
-	 * gets protected.
-	 */
-	{
-		uint32_t perms, uid;
-
-		RETURN_ON_FAILURE(TlclGetPermissions(KERNEL_NV_INDEX, &perms));
-		memcpy(&uid, &rsk.uid, sizeof(uid));
-		if (TPM_NV_PER_PPWRITE != perms ||
-		    ROLLBACK_SPACE_KERNEL_UID != uid)
-			return TPM_E_CORRUPTED_STATE;
-	}
-#endif
 	memcpy(version, &rsk.kernel_versions, sizeof(*version));
 	VB2_DEBUG("TPM: RollbackKernelRead 0x%x\n", (int)*version);
 	return TPM_SUCCESS;
diff --git a/tests/rollback_index2_tests.c b/tests/rollback_index2_tests.c
index a092406d..9df8b06d 100644
--- a/tests/rollback_index2_tests.c
+++ b/tests/rollback_index2_tests.c
@@ -75,9 +75,20 @@ static void ResetMocks(int fail_on_call, uint32_t fail_with_err)
 	fail_with_error = fail_with_err;
 
 	memset(&mock_pflags, 0, sizeof(mock_pflags));
+
 	memset(&mock_rsf, 0, sizeof(mock_rsf));
+	mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION;
+	mock_rsf.crc8 = vb2_crc8(&mock_rsf,
+				 offsetof(RollbackSpaceFirmware, crc8));
+
 	memset(&mock_rsk, 0, sizeof(mock_rsk));
-	mock_permissions = 0;
+	mock_rsk.uid = ROLLBACK_SPACE_KERNEL_UID;
+	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
+	mock_rsk.kernel_versions = 0x87654321;
+	mock_rsk.crc8 = vb2_crc8(&mock_rsk,
+				 offsetof(RollbackSpaceKernel, crc8));
+
+	mock_permissions = TPM_NV_PER_PPWRITE;
 
 	memset(mock_fwmp.buf, 0, sizeof(mock_fwmp.buf));
 	mock_fwmp.fwmp.struct_size = sizeof(mock_fwmp.fwmp);
@@ -250,31 +261,30 @@ static void FirmwareSpaceTest(void)
 {
 	RollbackSpaceFirmware rsf;
 
-	/* A read with old version should fail */
+	/* Old version, valid CRC */
 	ResetMocks(0, 0);
-	mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION - 1;
+	mock_rsf.struct_version -= 1;
+	mock_rsf.crc8 = vb2_crc8(&mock_rsf,
+				 offsetof(RollbackSpaceFirmware, crc8));
 	TEST_EQ(ReadSpaceFirmware(&rsf), TPM_E_STRUCT_VERSION,
 		"ReadSpaceFirmware(), old version");
 	TEST_STR_EQ(mock_calls,
 		    "TlclRead(0x1007, 10)\n",
 		    "tlcl calls");
 
-	/* A read with current version should fail on bad CRC */
+	/* Current version, bad CRC */
 	ResetMocks(0, 0);
-	mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION;
+	mock_rsf.crc8 = 0;
 	TEST_EQ(ReadSpaceFirmware(&rsf), TPM_E_CORRUPTED_STATE,
-		"ReadSpaceFirmware(), current version, bad CRC");
+		"ReadSpaceFirmware(), bad CRC");
 	TEST_STR_EQ(mock_calls,
 		    "TlclRead(0x1007, 10)\n",
 		    "tlcl calls");
 
-	/* Current version with valid CRC */
+	/* Current version, valid CRC */
 	ResetMocks(0, 0);
-	mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION;
-	mock_rsf.crc8 = vb2_crc8(&mock_rsf,
-				 offsetof(RollbackSpaceFirmware, crc8));
 	TEST_EQ(ReadSpaceFirmware(&rsf), 0,
-		"ReadSpaceFirmware(), current version, good CRC");
+		"ReadSpaceFirmware(), successful read");
 	TEST_STR_EQ(mock_calls,
 		    "TlclRead(0x1007, 10)\n",
 		    "tlcl calls");
@@ -287,32 +297,55 @@ static void KernelSpaceTest(void)
 {
 	RollbackSpaceKernel rsk;
 
-	/* A read with version old version should fail */
+	/* Current version, bad perms, valid CRC, valid UID */
+	ResetMocks(0, 0);
+	mock_permissions = 0;
+	TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_CORRUPTED_STATE,
+		"ReadSpaceKernel(), bad permissions");
+	TEST_STR_EQ(mock_calls,
+		    "TlclGetPermissions(0x1008)\n",
+		    "tlcl calls");
+
+	/* Old version, good perms, valid CRC, valid UID */
 	ResetMocks(0, 0);
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION - 1;
+	mock_rsk.struct_version -= 1;
+	mock_rsk.crc8 = vb2_crc8(&mock_rsk,
+				 offsetof(RollbackSpaceKernel, crc8));
 	TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_STRUCT_VERSION,
 		"ReadSpaceKernel(), old version");
 	TEST_STR_EQ(mock_calls,
+		    "TlclGetPermissions(0x1008)\n"
 		    "TlclRead(0x1008, 13)\n",
 		    "tlcl calls");
 
-	/* A read with current version should fail on bad CRC */
+	/* Current version, good perms, bad CRC, valid UID */
 	ResetMocks(0, 0);
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
+	mock_rsk.crc8 = 0;
 	TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_CORRUPTED_STATE,
-		"ReadSpaceKernel(), current version, bad CRC");
+		"ReadSpaceKernel(), bad CRC");
 	TEST_STR_EQ(mock_calls,
+		    "TlclGetPermissions(0x1008)\n"
 		    "TlclRead(0x1008, 13)\n",
 		    "tlcl calls");
 
-	/* Current version with valid CRC */
+	/* Current version, good perms, valid CRC, bad UID */
 	ResetMocks(0, 0);
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
+	mock_rsk.uid = 0;
 	mock_rsk.crc8 = vb2_crc8(&mock_rsk,
 				 offsetof(RollbackSpaceKernel, crc8));
+	TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_CORRUPTED_STATE,
+		"ReadSpaceKernel(), bad UID");
+	TEST_STR_EQ(mock_calls,
+		    "TlclGetPermissions(0x1008)\n"
+		    "TlclRead(0x1008, 13)\n",
+		    "tlcl calls");
+
+	/* Current version, good perms, valid CRC, valid UID */
+	ResetMocks(0, 0);
 	TEST_EQ(ReadSpaceKernel(&rsk), 0,
-		"ReadSpaceKernel(), current version, good CRC");
+		"ReadSpaceKernel(), successful read");
 	TEST_STR_EQ(mock_calls,
+		    "TlclGetPermissions(0x1008)\n"
 		    "TlclRead(0x1008, 13)\n",
 		    "tlcl calls");
 }
@@ -364,16 +397,10 @@ static void RollbackKernelTest(void)
 
 	/* Normal read */
 	ResetMocks(0, 0);
-	mock_rsk.uid = ROLLBACK_SPACE_KERNEL_UID;
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
-	mock_rsk.kernel_versions = 0x87654321;
-	mock_rsk.crc8 = vb2_crc8(&mock_rsk,
-				 offsetof(RollbackSpaceKernel, crc8));
-	mock_permissions = TPM_NV_PER_PPWRITE;
 	TEST_EQ(RollbackKernelRead(&version), 0, "RollbackKernelRead()");
 	TEST_STR_EQ(mock_calls,
-		    "TlclRead(0x1008, 13)\n"
-		    "TlclGetPermissions(0x1008)\n",
+		    "TlclGetPermissions(0x1008)\n"
+		    "TlclRead(0x1008, 13)\n",
 		    "tlcl calls");
 	TEST_EQ(version, 0x87654321, "RollbackKernelRead() version");
 
@@ -381,34 +408,27 @@ static void RollbackKernelTest(void)
 	ResetMocks(1, TPM_E_IOERROR);
 	TEST_EQ(RollbackKernelRead(&version), TPM_E_IOERROR,
 		"RollbackKernelRead() error");
-	TEST_STR_EQ(mock_calls,
-		    "TlclRead(0x1008, 13)\n",
-		    "tlcl calls");
 
 	/* Wrong permission or UID will return error */
 	ResetMocks(0, 0);
-	mock_rsk.uid = ROLLBACK_SPACE_KERNEL_UID + 1;
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
-	mock_permissions = TPM_NV_PER_PPWRITE;
+	mock_rsk.uid = 0;
+	mock_rsk.crc8 = vb2_crc8(&mock_rsk,
+				 offsetof(RollbackSpaceKernel, crc8));
 	TEST_EQ(RollbackKernelRead(&version), TPM_E_CORRUPTED_STATE,
 		"RollbackKernelRead() bad uid");
 
 	ResetMocks(0, 0);
-	mock_rsk.uid = ROLLBACK_SPACE_KERNEL_UID;
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
-	mock_permissions = TPM_NV_PER_PPWRITE + 1;
+	mock_permissions = 0;
 	TEST_EQ(RollbackKernelRead(&version), TPM_E_CORRUPTED_STATE,
 		"RollbackKernelRead() bad permissions");
 
 	/* Test write */
 	ResetMocks(0, 0);
-	mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION;
-	mock_rsk.crc8 = vb2_crc8(&mock_rsk,
-				 offsetof(RollbackSpaceKernel, crc8));
 	TEST_EQ(RollbackKernelWrite(0xBEAD4321), 0, "RollbackKernelWrite()");
 	TEST_EQ(mock_rsk.kernel_versions, 0xBEAD4321,
 		"RollbackKernelWrite() version");
 	TEST_STR_EQ(mock_calls,
+		    "TlclGetPermissions(0x1008)\n"
 		    "TlclRead(0x1008, 13)\n"
 		    "TlclWrite(0x1008, 13)\n",
 		    "tlcl calls");