Merge 85a9f5cc6b into fb2ac429de

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
   //

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.

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.

FmpDevicePkg/FmpDxe/VariableSupportUnitTest.c

@@ -0,0 +1,322 @@
+/** @file
+  Unit tests for FMP variable update checkpoints.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <stdio.h>
+#include <string.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include <Uefi.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/UnitTestLib.h>
+
+#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;
+}

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

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 {
+    <LibraryClasses>
+      UefiLib|SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
+      UefiRuntimeServicesTableLib|MdeModulePkg/Library/DxeResetSystemLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
+  }