diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c index 3d83affeac..c713df08ee 100644 --- a/FmpDevicePkg/FmpDxe/FmpDxe.c +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c @@ -1389,12 +1389,6 @@ SetTheImage ( Progress (4); - // - // Save LastAttemptStatus as error so that if SetImage never returns the error - // state is recorded. - // - SetLastAttemptStatusInVariable (Private, LastAttemptStatus); - // // Strip off all the headers so the device can process its firmware // @@ -1413,6 +1407,16 @@ SetTheImage ( goto cleanup; } + // + // Record the attempted version and a failure status together. Do not hand + // control to the device writer unless this reset-safe checkpoint is durable. + // + Status = SetUpdateInProgressInVariable (Private, IncomingFwVersion); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - Failed to persist update checkpoint. Status = %r\n", mImageIdName, Status)); + goto cleanup; + } + // // Indicate that control is handed off to FmpDeviceLib // diff --git a/FmpDevicePkg/FmpDxe/VariableSupport.c b/FmpDevicePkg/FmpDxe/VariableSupport.c index 71914f054f..2ce5757fff 100644 --- a/FmpDevicePkg/FmpDxe/VariableSupport.c +++ b/FmpDevicePkg/FmpDxe/VariableSupport.c @@ -725,6 +725,76 @@ SetLastAttemptVersionInVariable ( FreePool (FmpControllerState); } +/** + Records a durable failure checkpoint immediately before device update starts. + + @param[in] Private Private context structure for the managed + controller. + @param[in] LastAttemptVersion Version of the firmware update being attempted. + + @retval EFI_SUCCESS The checkpoint is durable. + @retval Other The checkpoint could not be persisted. +**/ +EFI_STATUS +SetUpdateInProgressInVariable ( + IN FIRMWARE_MANAGEMENT_PRIVATE_DATA *Private, + IN UINT32 LastAttemptVersion + ) +{ + EFI_STATUS Status; + FMP_CONTROLLER_STATE *FmpControllerState; + UINTN Size; + + FmpControllerState = NULL; + Size = 0; + Status = GetVariable2 ( + Private->FmpStateVariableName, + &gEfiCallerIdGuid, + (VOID **)&FmpControllerState, + &Size + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Failed to read update checkpoint. Status = %r\n", mImageIdName, Status)); + return Status; + } + + if ((FmpControllerState == NULL) || (Size != sizeof (*FmpControllerState))) { + if (FmpControllerState != NULL) { + FreePool (FmpControllerState); + } + + DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Update checkpoint has invalid size 0x%x\n", mImageIdName, Size)); + return EFI_COMPROMISED_DATA; + } + + if (FmpControllerState->LastAttemptVersionValid && + FmpControllerState->LastAttemptStatusValid && + (FmpControllerState->LastAttemptVersion == LastAttemptVersion) && + (FmpControllerState->LastAttemptStatus == LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL)) + { + FreePool (FmpControllerState); + return EFI_SUCCESS; + } + + FmpControllerState->LastAttemptVersionValid = TRUE; + FmpControllerState->LastAttemptStatusValid = TRUE; + FmpControllerState->LastAttemptVersion = LastAttemptVersion; + FmpControllerState->LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL; + Status = gRT->SetVariable ( + Private->FmpStateVariableName, + &gEfiCallerIdGuid, + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS, + sizeof (*FmpControllerState), + FmpControllerState + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Failed to write update checkpoint. Status = %r\n", mImageIdName, Status)); + } + + FreePool (FmpControllerState); + return Status; +} + /** Attempts to lock a single UEFI Variable propagating the error state of the first lock attempt that fails. Uses gEfiCallerIdGuid as the variable GUID. diff --git a/FmpDevicePkg/FmpDxe/VariableSupport.h b/FmpDevicePkg/FmpDxe/VariableSupport.h index d11044dd7f..67575b1433 100644 --- a/FmpDevicePkg/FmpDxe/VariableSupport.h +++ b/FmpDevicePkg/FmpDxe/VariableSupport.h @@ -234,6 +234,22 @@ SetLastAttemptVersionInVariable ( IN UINT32 LastAttemptVersion ); +/** + Records a durable failure checkpoint immediately before device update starts. + + @param[in] Private Private context structure for the managed + controller. + @param[in] LastAttemptVersion Version of the firmware update being attempted. + + @retval EFI_SUCCESS The checkpoint is durable. + @retval Other The checkpoint could not be persisted. +**/ +EFI_STATUS +SetUpdateInProgressInVariable ( + IN FIRMWARE_MANAGEMENT_PRIVATE_DATA *Private, + IN UINT32 LastAttemptVersion + ); + /** Locks all the UEFI Variables that use gEfiCallerIdGuid of the currently executing module. diff --git a/FmpDevicePkg/FmpDxe/VariableSupportUnitTest.c b/FmpDevicePkg/FmpDxe/VariableSupportUnitTest.c new file mode 100644 index 0000000000..c51f7655f0 --- /dev/null +++ b/FmpDevicePkg/FmpDxe/VariableSupportUnitTest.c @@ -0,0 +1,322 @@ +/** @file + Unit tests for FMP variable update checkpoints. + + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +#include "FmpDxe.h" +#include "VariableSupport.h" + +#define UNIT_TEST_APP_NAME "FmpDxe Variable Support Unit Tests" +#define UNIT_TEST_APP_VERSION "1.0" + +STATIC FMP_CONTROLLER_STATE mStoredState; +STATIC FIRMWARE_MANAGEMENT_PRIVATE_DATA mPrivate; +STATIC EFI_STATUS mGetStatus; +STATIC EFI_STATUS mSetStatus; +STATIC UINTN mStoredSize; +STATIC UINTN mSetCalls; + +CHAR16 *mImageIdName = L"FmpDxeVariableUnitTest"; + +STATIC +EFI_STATUS +EFIAPI +MockGetVariable ( + IN CHAR16 *VariableName, + IN EFI_GUID *VendorGuid, + OUT UINT32 *Attributes OPTIONAL, + IN OUT UINTN *DataSize, + OUT VOID *Data OPTIONAL + ) +{ + if (EFI_ERROR (mGetStatus)) { + return mGetStatus; + } + + if ((Data == NULL) || (*DataSize < mStoredSize)) { + *DataSize = mStoredSize; + return EFI_BUFFER_TOO_SMALL; + } + + CopyMem (Data, &mStoredState, mStoredSize); + *DataSize = mStoredSize; + return EFI_SUCCESS; +} + +STATIC +EFI_STATUS +EFIAPI +MockSetVariable ( + IN CHAR16 *VariableName, + IN EFI_GUID *VendorGuid, + IN UINT32 Attributes, + IN UINTN DataSize, + IN VOID *Data + ) +{ + mSetCalls++; + if (EFI_ERROR (mSetStatus)) { + return mSetStatus; + } + + if ((Data == NULL) || (DataSize != sizeof (mStoredState))) { + return EFI_INVALID_PARAMETER; + } + + CopyMem (&mStoredState, Data, sizeof (mStoredState)); + return EFI_SUCCESS; +} + +EFI_RUNTIME_SERVICES MockRuntime = { + { + EFI_RUNTIME_SERVICES_SIGNATURE, + EFI_RUNTIME_SERVICES_REVISION, + sizeof (EFI_RUNTIME_SERVICES), + 0, + 0 + }, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL, + MockGetVariable, + NULL, + MockSetVariable, + NULL, + NULL, + NULL, + NULL, + NULL +}; + +STATIC +VOID +ResetStore ( + VOID + ) +{ + ZeroMem (&mStoredState, sizeof (mStoredState)); + ZeroMem (&mPrivate, sizeof (mPrivate)); + mPrivate.FmpStateVariableName = L"FmpState"; + mStoredState.LastAttemptStatusValid = TRUE; + mStoredState.LastAttemptVersionValid = TRUE; + mStoredState.LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS; + mStoredState.LastAttemptVersion = 7; + mGetStatus = EFI_SUCCESS; + mSetStatus = EFI_SUCCESS; + mStoredSize = sizeof (mStoredState); + mSetCalls = 0; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +CheckpointPersistsVersionAndFailure ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + FMP_CONTROLLER_STATE *State; + + ResetStore (); + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_EQUAL (mSetCalls, 1); + UT_ASSERT_EQUAL (mStoredState.LastAttemptVersion, 9); + UT_ASSERT_EQUAL (mStoredState.LastAttemptStatus, LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL); + + ZeroMem (&mPrivate, sizeof (mPrivate)); + mPrivate.FmpStateVariableName = L"FmpState"; + State = GetFmpControllerState (&mPrivate); + UT_ASSERT_NOT_NULL (State); + UT_ASSERT_EQUAL (GetLastAttemptVersionFromFmpControllerState (State), 9); + UT_ASSERT_EQUAL (GetLastAttemptStatusFromFmpControllerState (State), LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL); + FreePool (State); + return UNIT_TEST_PASSED; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +FullStoreBlocksUpdate ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + + ResetStore (); + mSetStatus = EFI_OUT_OF_RESOURCES; + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_STATUS_EQUAL (Status, EFI_OUT_OF_RESOURCES); + UT_ASSERT_EQUAL (mStoredState.LastAttemptVersion, 7); + UT_ASSERT_EQUAL (mStoredState.LastAttemptStatus, LAST_ATTEMPT_STATUS_SUCCESS); + return UNIT_TEST_PASSED; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +ReadFailureBlocksUpdate ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + + ResetStore (); + mGetStatus = EFI_DEVICE_ERROR; + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_STATUS_EQUAL (Status, EFI_DEVICE_ERROR); + UT_ASSERT_EQUAL (mSetCalls, 0); + return UNIT_TEST_PASSED; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +MissingStateBlocksUpdate ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + + ResetStore (); + mGetStatus = EFI_NOT_FOUND; + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_STATUS_EQUAL (Status, EFI_NOT_FOUND); + UT_ASSERT_EQUAL (mSetCalls, 0); + return UNIT_TEST_PASSED; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +WriteFailureBlocksUpdate ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + + ResetStore (); + mSetStatus = EFI_DEVICE_ERROR; + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_STATUS_EQUAL (Status, EFI_DEVICE_ERROR); + UT_ASSERT_EQUAL (mStoredState.LastAttemptVersion, 7); + UT_ASSERT_EQUAL (mStoredState.LastAttemptStatus, LAST_ATTEMPT_STATUS_SUCCESS); + return UNIT_TEST_PASSED; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +MalformedStateBlocksUpdate ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + + ResetStore (); + mStoredSize = sizeof (mStoredState) - 1; + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_STATUS_EQUAL (Status, EFI_COMPROMISED_DATA); + UT_ASSERT_EQUAL (mSetCalls, 0); + return UNIT_TEST_PASSED; +} + +STATIC +UNIT_TEST_STATUS +EFIAPI +DurableCheckpointNeedsNoRewrite ( + IN UNIT_TEST_CONTEXT Context + ) +{ + EFI_STATUS Status; + + ResetStore (); + mStoredState.LastAttemptVersion = 9; + mStoredState.LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL; + mSetStatus = EFI_OUT_OF_RESOURCES; + Status = SetUpdateInProgressInVariable (&mPrivate, 9); + + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_EQUAL (mSetCalls, 0); + return UNIT_TEST_PASSED; +} + +STATIC +EFI_STATUS +EFIAPI +UnitTestingEntry ( + VOID + ) +{ + EFI_STATUS Status; + UNIT_TEST_FRAMEWORK_HANDLE Framework; + UNIT_TEST_SUITE_HANDLE CheckpointTests; + + Framework = NULL; + Status = InitUnitTestFramework ( + &Framework, + UNIT_TEST_APP_NAME, + gEfiCallerBaseName, + UNIT_TEST_APP_VERSION + ); + if (EFI_ERROR (Status)) { + return Status; + } + + Status = CreateUnitTestSuite ( + &CheckpointTests, + Framework, + "FMP variable checkpoints", + "FmpDxe.VariableSupport", + NULL, + NULL + ); + if (EFI_ERROR (Status)) { + FreeUnitTestFramework (Framework); + return Status; + } + + AddTestCase (CheckpointTests, "Checkpoint persists version and failure", "Persist", CheckpointPersistsVersionAndFailure, NULL, NULL, NULL); + AddTestCase (CheckpointTests, "Full store blocks update", "FullStore", FullStoreBlocksUpdate, NULL, NULL, NULL); + AddTestCase (CheckpointTests, "Read failure blocks update", "ReadFailure", ReadFailureBlocksUpdate, NULL, NULL, NULL); + AddTestCase (CheckpointTests, "Missing state blocks update", "MissingState", MissingStateBlocksUpdate, NULL, NULL, NULL); + AddTestCase (CheckpointTests, "Write failure blocks update", "WriteFailure", WriteFailureBlocksUpdate, NULL, NULL, NULL); + AddTestCase (CheckpointTests, "Malformed state blocks update", "MalformedState", MalformedStateBlocksUpdate, NULL, NULL, NULL); + AddTestCase (CheckpointTests, "Durable checkpoint needs no rewrite", "NoRewrite", DurableCheckpointNeedsNoRewrite, NULL, NULL, NULL); + + Status = RunAllTestSuites (Framework); + FreeUnitTestFramework (Framework); + return Status; +} + +#define VariableSupportUnitTestMain main + +INT32 +VariableSupportUnitTestMain ( + IN INT32 Argc, + IN CHAR8 *Argv[] + ) +{ + return EFI_ERROR (UnitTestingEntry ()) ? 1 : 0; +} diff --git a/FmpDevicePkg/FmpDxe/VariableSupportUnitTestHost.inf b/FmpDevicePkg/FmpDxe/VariableSupportUnitTestHost.inf new file mode 100644 index 0000000000..9431509fed --- /dev/null +++ b/FmpDevicePkg/FmpDxe/VariableSupportUnitTestHost.inf @@ -0,0 +1,39 @@ +## @file +# Host unit tests for FMP variable update checkpoints. +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +## + +[Defines] + INF_VERSION = 0x00010017 + BASE_NAME = VariableSupportUnitTest + FILE_GUID = D182A7AB-1BD2-4D1B-8769-5067A6F31A49 + VERSION_STRING = 1.0 + MODULE_TYPE = HOST_APPLICATION + +[Sources] + VariableSupport.c + VariableSupport.h + VariableSupportUnitTest.c + +[Packages] + CryptoPkg/CryptoPkg.dec + FmpDevicePkg/FmpDevicePkg.dec + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec + +[LibraryClasses] + BaseLib + BaseMemoryLib + DebugLib + MemoryAllocationLib + PrintLib + UefiBootServicesTableLib + UefiLib + UefiRuntimeServicesTableLib + UnitTestLib + VariablePolicyHelperLib + +[Protocols] + gEdkiiVariablePolicyProtocolGuid diff --git a/FmpDevicePkg/Test/FmpDeviceHostPkgTest.dsc b/FmpDevicePkg/Test/FmpDeviceHostPkgTest.dsc index faa0608694..9d16544dd3 100644 --- a/FmpDevicePkg/Test/FmpDeviceHostPkgTest.dsc +++ b/FmpDevicePkg/Test/FmpDeviceHostPkgTest.dsc @@ -20,9 +20,15 @@ [LibraryClasses] FmpDependencyLib|FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.inf + VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf [Components] # # Build HOST_APPLICATION that tests the FmpDependencyLib # FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsHost.inf + FmpDevicePkg/FmpDxe/VariableSupportUnitTestHost.inf { + + UefiLib|SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf + UefiRuntimeServicesTableLib|MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeServicesTableLib.inf + }