From aedfd3f439e5c78d110e178a41ae0def3c409d5c Mon Sep 17 00:00:00 2001 From: Joel Kitching Date: Tue, 6 Aug 2019 16:51:04 +0800 Subject: vboot/secdata: remove legacy version checks on rollback spaces The code to deal with version < 2 rollback spaces has been around since 2013. Legacy devices will not be updating to this code, thus we can remove the legacy silent upgrade. BUG=b:124141368, chromium:972956 TEST=make clean && make runtests BRANCH=none Change-Id: I8ce22c37418ddc56cb74cc792540b54b3ee7bbd7 Signed-off-by: Joel Kitching Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1727949 Reviewed-by: Joel Kitching Tested-by: Joel Kitching Commit-Queue: Joel Kitching --- firmware/lib/rollback_index.c | 34 ++-------------- tests/rollback_index2_tests.c | 95 ++++++++++++++++++------------------------- 2 files changed, 44 insertions(+), 85 deletions(-) diff --git a/firmware/lib/rollback_index.c b/firmware/lib/rollback_index.c index 07c7deec..7a1c0405 100644 --- a/firmware/lib/rollback_index.c +++ b/firmware/lib/rollback_index.c @@ -77,18 +77,8 @@ uint32_t ReadSpaceFirmware(RollbackSpaceFirmware *rsf) if (TPM_SUCCESS != r) return r; - /* - * No CRC in this version, so we'll create one when we write - * it. Note that we're marking this as version 2, not - * ROLLBACK_SPACE_FIRMWARE_VERSION, because version 2 just - * added the CRC. Later versions will need to set default - * values for any extra fields explicitly (probably here). - */ - if (rsf->struct_version < 2) { - /* Danger Will Robinson! Danger! */ - rsf->struct_version = 2; - return TPM_SUCCESS; - } + if (rsf->struct_version < ROLLBACK_SPACE_FIRMWARE_VERSION) + return TPM_E_STRUCT_VERSION; if (rsf->crc8 != vb2_crc8(rsf, offsetof(RollbackSpaceFirmware, crc8))) { VB2_DEBUG("TPM: bad CRC\n"); @@ -100,9 +90,6 @@ uint32_t ReadSpaceFirmware(RollbackSpaceFirmware *rsf) uint32_t WriteSpaceFirmware(RollbackSpaceFirmware *rsf) { - /* All writes should use struct_version 2 or greater. */ - if (rsf->struct_version < 2) - rsf->struct_version = 2; rsf->crc8 = vb2_crc8(rsf, offsetof(RollbackSpaceFirmware, crc8)); return SafeWrite(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware)); @@ -142,18 +129,8 @@ uint32_t ReadSpaceKernel(RollbackSpaceKernel *rsk) if (TPM_SUCCESS != r) return r; - /* - * No CRC in this version, so we'll create one when we write - * it. Note that we're marking this as version 2, not - * ROLLBACK_SPACE_KERNEL_VERSION, because version 2 just added - * the CRC. Later versions will need to set default values for - * any extra fields explicitly (probably here). - */ - if (rsk->struct_version < 2) { - /* Danger Will Robinson! Danger! */ - rsk->struct_version = 2; - return TPM_SUCCESS; - } + if (rsk->struct_version < ROLLBACK_SPACE_FIRMWARE_VERSION) + return TPM_E_STRUCT_VERSION; if (rsk->crc8 != vb2_crc8(rsk, offsetof(RollbackSpaceKernel, crc8))) { VB2_DEBUG("TPM: bad CRC\n"); @@ -165,9 +142,6 @@ uint32_t ReadSpaceKernel(RollbackSpaceKernel *rsk) uint32_t WriteSpaceKernel(RollbackSpaceKernel *rsk) { - /* All writes should use struct_version 2 or greater. */ - if (rsk->struct_version < 2) - rsk->struct_version = 2; rsk->crc8 = vb2_crc8(rsk, offsetof(RollbackSpaceKernel, crc8)); return SafeWrite(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel)); diff --git a/tests/rollback_index2_tests.c b/tests/rollback_index2_tests.c index daf55e6c..a092406d 100644 --- a/tests/rollback_index2_tests.c +++ b/tests/rollback_index2_tests.c @@ -15,6 +15,12 @@ #include "test_common.h" #include "tlcl.h" +_Static_assert(ROLLBACK_SPACE_FIRMWARE_VERSION > 0, + "ROLLBACK_SPACE_FIRMWARE_VERSION must be greater than 0"); + +_Static_assert(ROLLBACK_SPACE_KERNEL_VERSION > 0, + "ROLLBACK_SPACE_KERNEL_VERSION must be greater than 0"); + /* * Buffer to hold accumulated list of calls to mocked Tlcl functions. * Each function appends itself to the buffer and updates mock_cnext. @@ -240,104 +246,75 @@ uint32_t TlclGetPermissions(uint32_t index, uint32_t* permissions) extern uint32_t ReadSpaceFirmware(RollbackSpaceFirmware *rsf); extern uint32_t WriteSpaceFirmware(RollbackSpaceFirmware *rsf); -static void CrcTestFirmware(void) +static void FirmwareSpaceTest(void) { RollbackSpaceFirmware rsf; - /* Empty structure, so CRC is valid */ - ResetMocks(0, 0); - TEST_EQ(ReadSpaceFirmware(&rsf), 0, "ReadSpaceFirmware(), v0"); - TEST_STR_EQ(mock_calls, - "TlclRead(0x1007, 10)\n", - "tlcl calls"); - - /* version == 1 without CRC value gets silently updated */ + /* A read with old version should fail */ ResetMocks(0, 0); - mock_rsf.struct_version = 1; - TEST_EQ(ReadSpaceFirmware(&rsf), 0, "ReadSpaceFirmware(), v1"); + mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION - 1; + TEST_EQ(ReadSpaceFirmware(&rsf), TPM_E_STRUCT_VERSION, + "ReadSpaceFirmware(), old version"); TEST_STR_EQ(mock_calls, "TlclRead(0x1007, 10)\n", "tlcl calls"); - /* If version == 2, CRC is no longer valid */ + /* A read with current version should fail on bad CRC */ ResetMocks(0, 0); - mock_rsf.struct_version = 2; + mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION; TEST_EQ(ReadSpaceFirmware(&rsf), TPM_E_CORRUPTED_STATE, - "ReadSpaceFirmware(), v2, bad CRC"); + "ReadSpaceFirmware(), current version, bad CRC"); TEST_STR_EQ(mock_calls, "TlclRead(0x1007, 10)\n", "tlcl calls"); - /* Version set with valid CRC */ + /* Current version with valid CRC */ ResetMocks(0, 0); - mock_rsf.struct_version = 2; + mock_rsf.struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION; mock_rsf.crc8 = vb2_crc8(&mock_rsf, offsetof(RollbackSpaceFirmware, crc8)); TEST_EQ(ReadSpaceFirmware(&rsf), 0, - "ReadSpaceFirmware(), v2, good CRC"); + "ReadSpaceFirmware(), current version, good CRC"); TEST_STR_EQ(mock_calls, "TlclRead(0x1007, 10)\n", "tlcl calls"); - - /* A write with version < 2 should convert to v2 and create the CRC */ - ResetMocks(0, 0); - memset(&rsf, 0, sizeof(rsf)); - TEST_EQ(WriteSpaceFirmware(&rsf), 0, "WriteSpaceFirmware(), v0"); - TEST_EQ(mock_rsf.struct_version, 2, "WriteSpaceFirmware(), check v2"); - TEST_STR_EQ(mock_calls, - "TlclWrite(0x1007, 10)\n", - "tlcl calls"); } extern uint32_t ReadSpaceKernel(RollbackSpaceKernel *rsk); extern uint32_t WriteSpaceKernel(RollbackSpaceKernel *rsk); -static void CrcTestKernel(void) +static void KernelSpaceTest(void) { RollbackSpaceKernel rsk; - /* Empty structure, so CRC is valid */ + /* A read with version old version should fail */ ResetMocks(0, 0); - TEST_EQ(ReadSpaceKernel(&rsk), 0, "ReadSpaceKernel(), v0"); + mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION - 1; + TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_STRUCT_VERSION, + "ReadSpaceKernel(), old version"); TEST_STR_EQ(mock_calls, "TlclRead(0x1008, 13)\n", "tlcl calls"); - /* version == 1 without CRC value gets silently updated */ + /* A read with current version should fail on bad CRC */ ResetMocks(0, 0); - mock_rsk.struct_version = 1; - TEST_EQ(ReadSpaceKernel(&rsk), 0, "ReadSpaceKernel(), v1"); - TEST_STR_EQ(mock_calls, - "TlclRead(0x1008, 13)\n", - "tlcl calls"); - - /* If version == 2, CRC is no longer valid */ - ResetMocks(0, 0); - mock_rsk.struct_version = 2; + mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION; TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_CORRUPTED_STATE, - "ReadSpaceKernel(), v2, bad CRC"); + "ReadSpaceKernel(), current version, bad CRC"); TEST_STR_EQ(mock_calls, "TlclRead(0x1008, 13)\n", "tlcl calls"); - /* Version set with valid CRC */ + /* Current version with valid CRC */ ResetMocks(0, 0); - mock_rsk.struct_version = 2; + mock_rsk.struct_version = ROLLBACK_SPACE_KERNEL_VERSION; mock_rsk.crc8 = vb2_crc8(&mock_rsk, offsetof(RollbackSpaceKernel, crc8)); - TEST_EQ(ReadSpaceKernel(&rsk), 0, "ReadSpaceKernel(), v2, good CRC"); + TEST_EQ(ReadSpaceKernel(&rsk), 0, + "ReadSpaceKernel(), current version, good CRC"); TEST_STR_EQ(mock_calls, "TlclRead(0x1008, 13)\n", "tlcl calls"); - - /* A write with version < 2 should convert to v2 and create the CRC */ - ResetMocks(0, 0); - memset(&rsk, 0, sizeof(rsk)); - TEST_EQ(WriteSpaceKernel(&rsk), 0, "WriteSpaceKernel(), v0"); - TEST_EQ(mock_rsk.struct_version, 2, "WriteSpaceKernel(), check v2"); - TEST_STR_EQ(mock_calls, - "TlclWrite(0x1008, 13)\n", - "tlcl calls"); } /****************************************************************************/ @@ -388,8 +365,11 @@ static void RollbackKernelTest(void) /* Normal read */ ResetMocks(0, 0); mock_rsk.uid = ROLLBACK_SPACE_KERNEL_UID; - mock_permissions = TPM_NV_PER_PPWRITE; + 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" @@ -408,18 +388,23 @@ static void RollbackKernelTest(void) /* 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; 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; 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"); @@ -538,8 +523,8 @@ static void RollbackFwmpTest(void) int main(int argc, char* argv[]) { - CrcTestFirmware(); - CrcTestKernel(); + FirmwareSpaceTest(); + KernelSpaceTest(); MiscTest(); RollbackKernelTest(); RollbackFwmpTest(); -- cgit v1.2.1