Revert VarPolicy locking change (#70)

## Description Revert Variable locking change due to side effects (not locking) on some platforms. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? ## How This Was Tested CI and code inspection. Platform testing will be performed ASAP ## Integration Instructions DFCI Feature requires Project Mu Phase Variables. If that is present, then no integration required.
microsoft · Apr 4, 2023 · 0fc28dd · 0fc28dd
1 parent 4bf67f2
commit 0fc28dd
Show file tree

Hide file tree

Showing 8 changed files with 13 additions and 168 deletions.
diff --git a/DfciPkg/DfciManager/DfciManager.h b/DfciPkg/DfciManager/DfciManager.h
@@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/DfciPermissionManagerVariables.h>
 #include <Guid/DfciSettingsGuid.h>
 #include <Guid/DfciSettingsManagerVariables.h>
+#include <Guid/MuVarPolicyFoundationDxe.h>
 #include <Guid/ZeroTouchVariables.h>
 
 #include <Protocol/DfciApplyPacket.h>

diff --git a/DfciPkg/DfciManager/DfciManager.inf b/DfciPkg/DfciManager/DfciManager.inf
@@ -31,6 +31,7 @@
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
+  MsCorePkg/MsCorePkg.dec
   DfciPkg/DfciPkg.dec
   ZeroTouchPkg/ZeroTouchPkg.dec
 
@@ -53,12 +54,11 @@
   gDfciAuthProvisionVarNamespace
   gDfciDeviceIdVarNamespace
   gDfciInternalVariableGuid
-  gDfciLockVariableGuid
   gDfciPermissionManagerVarNamespace
   gDfciSettingsGuid
   gDfciSettingsManagerVarNamespace
   gEfiEndOfDxeEventGroupGuid
-  gEfiEventReadyToBootGuid
+  gMuVarPolicyDxePhaseGuid
   gZeroTouchVariableGuid
 
 [Protocols]

diff --git a/DfciPkg/DfciManager/DfciVarPolicies.c b/DfciPkg/DfciManager/DfciVarPolicies.c
@@ -13,53 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <DfciVariablePolicies.h>
 
-STATIC EDKII_VARIABLE_POLICY_PROTOCOL  *mVariablePolicy = NULL;
-
-/**
- * Event callback for Ready To Boot.
- * This is needed to lock certain variables
- *
- * @param Event
- * @param Context
- *
- * @return VOID EFIAPI
- */
-VOID
-EFIAPI
-ReadyToBootCallback (
-  IN EFI_EVENT  Event,
-  IN VOID       *Context
-  )
-{
-  UINT8       Lock;
-  EFI_STATUS  Status;
-
-  gBS->CloseEvent (Event);
-
-  if (mVariablePolicy == NULL) {
-    ASSERT (mVariablePolicy != NULL);
-    return;
-  }
-
-  //
-  // Lock most variables at ReadyToBoot
-  //
-  Lock   = 1;
-  Status = gRT->SetVariable (
-                  DFCI_LOCK_VAR_NAME,
-                  &gDfciLockVariableGuid,
-                  DFCI_LOCK_VAR_ATTRIBUTES,
-                  sizeof (Lock),
-                  &Lock
-                  );
-
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a %a: Set DfciLock at ReadyToBoot failed. Code=%r!\n", _DBGMSGID_, __FUNCTION__, Status));
-  } else {
-    DEBUG ((DEBUG_INFO, "%a %a: - Dfci Lock variable(%s) set to 1, and locked\n", _DBGMSGID_, __FUNCTION__, DFCI_LOCK_VAR_NAME));
-  }
-}
-
 /**
   InitializeAndSetPolicyForAllDfciVariables
 
@@ -69,51 +22,31 @@ InitializeAndSetPolicyForAllDfciVariables (
   VOID
   )
 {
-  UINTN       i;
-  EFI_STATUS  Status;
-  EFI_EVENT   Event;
-
-  //
-  // Request notification of ReadyToBoot.
-  //
-  // NOTE: If this fails, the variables are not locked
-  //       DELAYED_PROCESSING state.
-  //
-  Status = gBS->CreateEventEx (
-                  EVT_NOTIFY_SIGNAL,
-                  TPL_CALLBACK - 1,  // Lock the Dfci Variables after all other ReadyToBoot handlers
-                  ReadyToBootCallback,
-                  NULL,
-                  &gEfiEventReadyToBootGuid,
-                  &Event
-                  );
+  UINTN                           i;
+  EFI_STATUS                      Status;
+  EDKII_VARIABLE_POLICY_PROTOCOL  *VariablePolicy = NULL;
 
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a %a: ReadyToBoot callback registration failed! %r\n", _DBGMSGID_, __FUNCTION__, Status));
-    // Continue with rest of variable locks if possible.
-  }
-
-  Status = gBS->LocateProtocol (&gEdkiiVariablePolicyProtocolGuid, NULL, (VOID **)&mVariablePolicy);
+  Status = gBS->LocateProtocol (&gEdkiiVariablePolicyProtocolGuid, NULL, (VOID **)&VariablePolicy);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a %a: - Locating Variable Policy failed - Code=%r\n", _DBGMSGID_, __FUNCTION__, Status));
     goto Done;
   }
 
   //
-  // Lock most variables when DfciLock variable is created.
+  // Lock most variables when ReadyToBoot Phase variable is set
   //
   for (i = 0; i < ARRAY_SIZE (gReadyToBootPolicies); i++) {
     Status = RegisterVarStateVariablePolicy (
-               mVariablePolicy,
+               VariablePolicy,
                gReadyToBootPolicies[i].Namespace,
                gReadyToBootPolicies[i].Name,
                gReadyToBootPolicies[i].MinSize,
                gReadyToBootPolicies[i].MaxSize,
                gReadyToBootPolicies[i].AttributesMustHave,
                gReadyToBootPolicies[i].AttributesCantHave,
-               &gDfciLockVariableGuid,
-               DFCI_LOCK_VAR_NAME,
-               1
+               &gMuVarPolicyDxePhaseGuid,
+               READY_TO_BOOT_INDICATOR_VAR_NAME,
+               PHASE_INDICATOR_SET
                );
 
     if (EFI_ERROR (Status)) {
@@ -122,47 +55,12 @@ InitializeAndSetPolicyForAllDfciVariables (
     }
   }
 
-  //
-  // Make sure there is no existing lock variable
-  //
-  Status = gRT->SetVariable (
-                  DFCI_LOCK_VAR_NAME,
-                  &gDfciLockVariableGuid,
-                  0,
-                  0,
-                  NULL
-                  );
-
-  if (Status == EFI_SUCCESS) {
-    DEBUG ((DEBUG_ERROR, "%a %a: Existing variable found, and deleted  This should not happen.\n", _DBGMSGID_, __FUNCTION__));
-  } else if (Status != EFI_NOT_FOUND) {
-    DEBUG ((DEBUG_ERROR, "%a %a: Error deleting Dfci Lock variable(%s). Code=%r\n", _DBGMSGID_, __FUNCTION__, DFCI_LOCK_VAR_NAME, Status));
-  }
-
-  for (i = 0; i < ARRAY_SIZE (gDfciLockPolicy); i++) {
-    Status = RegisterBasicVariablePolicy (
-               mVariablePolicy,
-               gDfciLockPolicy[i].Namespace,
-               gDfciLockPolicy[i].Name,
-               gDfciLockPolicy[i].MinSize,
-               gDfciLockPolicy[i].MaxSize,
-               gDfciLockPolicy[i].AttributesMustHave,
-               gDfciLockPolicy[i].AttributesCantHave,
-               VARIABLE_POLICY_TYPE_LOCK_ON_CREATE
-               );
-
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "%a %a: - RegisterBasicVariablePolicy() DfciLock[%d] returned %r!\n", _DBGMSGID_, __FUNCTION__, i, Status));
-      DEBUG ((DEBUG_ERROR, "%a %a: - Error registering %g:%s\n", _DBGMSGID_, __FUNCTION__, gDfciLockPolicy[i].Namespace, gDfciLockPolicy[i].Name));
-    }
-  }
-
   //
   // The mailboxes are not locked, but set restrictions for variable sizes and attributes
   //
   for (i = 0; i < ARRAY_SIZE (gMailBoxPolicies); i++) {
     Status = RegisterBasicVariablePolicy (
-               mVariablePolicy,
+               VariablePolicy,
                gMailBoxPolicies[i].Namespace,
                gMailBoxPolicies[i].Name,
                gMailBoxPolicies[i].MinSize,

diff --git a/DfciPkg/DfciPkg.dec b/DfciPkg/DfciPkg.dec
@@ -130,11 +130,6 @@
   # Include/Guid/DfciInternalVariableGuid.h
   gDfciInternalVariableGuid = { 0xc6bbd941, 0xbfe0, 0x44b8, { 0xbe, 0xdc, 0x4, 0xd3, 0xa7, 0xe9, 0xa, 0xd9}}
 
-  ## DFCI Variable Lock variable (Dfci is using VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE on this variable to lock DFCI variables)
-  #
-  # Include/Guid/DfciInternalVariable.h
-  gDfciLockVariableGuid = { 0x23c4cdad, 0xddeb, 0x452f, {0x93, 0x8b, 0xdf, 0xaa, 0x3c, 0x3e, 0xc8, 0xee}}
-
   ## DFCI notification that Bds is starting (main DXE dispatch cycle complete)
   #  Still within PlatformAuth
   gDfciStartOfBdsNotifyGuid = { 0xc9341466, 0x1a6c, 0x4ded, { 0x89, 0xc2, 0x78, 0x12, 0xb0, 0x29, 0x9c, 0x45}}

diff --git a/DfciPkg/Include/Guid/DfciInternalVariableGuid.h b/DfciPkg/Include/Guid/DfciInternalVariableGuid.h
@@ -21,13 +21,4 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 extern EFI_GUID  gDfciInternalVariableGuid;
 
-//
-// Dfci Lock Variable.
-//
-#define DFCI_LOCK_VAR_NAME        L"_DLCK"
-#define DFCI_LOCK_VAR_ATTRIBUTES  (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS)
-#define DFCI_LOCK_VAR_SIZE        sizeof (UINT8)
-
-extern EFI_GUID  gDfciLockVariableGuid;
-
 #endif // __DFCI_INTERNAL_VARIABLE_GUID_H__
diff --git a/DfciPkg/Include/Private/DfciVariablePolicies.h b/DfciPkg/Include/Private/DfciVariablePolicies.h
@@ -137,14 +137,4 @@ VARIABLE_POLICY_ELEMENT  gMailBoxPolicies[] =
   },
 };
 
-// Set the policy for the Dfci Lock variable.  All variables listed here will be Lock On Create variables.
-VARIABLE_POLICY_ELEMENT  gDfciLockPolicy[] =
-{
-  {
-    .Namespace          = &gDfciLockVariableGuid, .Name = DFCI_LOCK_VAR_NAME,
-    .MinSize            = DFCI_LOCK_VAR_SIZE, .MaxSize = DFCI_LOCK_VAR_SIZE,
-    .AttributesMustHave = DFCI_LOCK_VAR_ATTRIBUTES, .AttributesCantHave = (UINT32) ~DFCI_LOCK_VAR_ATTRIBUTES,
-  },
-};
-
 #endif // __DFCI_VAR_POLICIES_H__
diff --git a/DfciPkg/UnitTests/DfciVarLockAudit/UEFI/DfciLockTest.c b/DfciPkg/UnitTests/DfciVarLockAudit/UEFI/DfciLockTest.c
@@ -65,18 +65,6 @@ GetVariablePolicy (
     }
   }
 
-  for (i = 0; i < ARRAY_SIZE (gDfciLockPolicy); i++) {
-    if (CompareGuid (varGuid, gDfciLockPolicy[i].Namespace)) {
-      if (gDfciLockPolicy[i].Name == NULL) {
-        return &gDfciLockPolicy[i];
-      }
-
-      if (0 == StrCmp (varName, gDfciLockPolicy[i].Name)) {
-        return &gDfciLockPolicy[i];
-      }
-    }
-  }
-
   if (ShouldBeLocked != NULL) {
     *ShouldBeLocked = FALSE;
   }
@@ -106,11 +94,9 @@ CreateListOfDfciVars (
   XmlNode                  *VarNode;
   BOOLEAN                  IPCVN_Present;
   BOOLEAN                  SPP_Present;
-  BOOLEAN                  DLCK_Present;
 
   IPCVN_Present = FALSE;
   SPP_Present   = FALSE;
-  DLCK_Present  = FALSE;
 
   List = New_VariablesNodeList ();
   if (List == NULL) {
@@ -156,17 +142,6 @@ CreateListOfDfciVars (
               gDfciPolicyFailedCount++;
             }
           }
-
-          // This variable is on present when Dfci variables are using Lock on Variable State.  If present
-          // its policies must be correct, and should be locked.
-          if (CompareGuid (&varGuid, &gDfciLockVariableGuid)) {
-            if (0 == StrCmp (varName, DFCI_LOCK_VAR_NAME)) {
-              DLCK_Present = TRUE;
-            } else {
-              AddDfciErrorToNode (VarNode, "ERROR, Unexpected variable in Lock Variable namespace\n");
-              gDfciPolicyFailedCount++;
-            }
-          }
         }
       }
     }
@@ -195,10 +170,6 @@ CreateListOfDfciVars (
       AddDfciErrorToNode (gDfciStatusNode, "FAIL Required Permissions Library private variable not found");
       gDfciPolicyFailedCount++;
     }
-
-    if (!DLCK_Present) {
-      DEBUG ((DEBUG_ERROR, "This variable will be required in a future release of this test"));
-    }
   }
 
   return List;

diff --git a/DfciPkg/UnitTests/DfciVarLockAudit/UEFI/DfciVarLockAuditTestApp.inf b/DfciPkg/UnitTests/DfciVarLockAudit/UEFI/DfciVarLockAuditTestApp.inf
@@ -57,7 +57,6 @@
 [Guids]
   gDfciDeviceIdVarNamespace
   gDfciInternalVariableGuid
-  gDfciLockVariableGuid
   gDfciAuthProvisionVarNamespace
   gDfciPermissionManagerVarNamespace
   gDfciSettingsGuid