From 0fc28dd31eaa21ff68a373dd1da40e42954ba46a Mon Sep 17 00:00:00 2001 From: Sean Brogan Date: Mon, 3 Apr 2023 17:42:05 -0700 Subject: [PATCH] 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. --- DfciPkg/DfciManager/DfciManager.h | 1 + DfciPkg/DfciManager/DfciManager.inf | 4 +- DfciPkg/DfciManager/DfciVarPolicies.c | 122 ++---------------- DfciPkg/DfciPkg.dec | 5 - .../Include/Guid/DfciInternalVariableGuid.h | 9 -- .../Include/Private/DfciVariablePolicies.h | 10 -- .../DfciVarLockAudit/UEFI/DfciLockTest.c | 29 ----- .../UEFI/DfciVarLockAuditTestApp.inf | 1 - 8 files changed, 13 insertions(+), 168 deletions(-) diff --git a/DfciPkg/DfciManager/DfciManager.h b/DfciPkg/DfciManager/DfciManager.h index 752c686c..3e089586 100644 --- a/DfciPkg/DfciManager/DfciManager.h +++ b/DfciPkg/DfciManager/DfciManager.h @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include #include +#include #include #include diff --git a/DfciPkg/DfciManager/DfciManager.inf b/DfciPkg/DfciManager/DfciManager.inf index ae7090ad..8a4aa50e 100644 --- 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 index 5db8bb81..14c695b7 100644 --- a/DfciPkg/DfciManager/DfciVarPolicies.c +++ b/DfciPkg/DfciManager/DfciVarPolicies.c @@ -13,53 +13,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include -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 index f13a0913..99d8b1d8 100644 --- 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 index 5b2338a0..eed17acc 100644 --- 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 index d40f21e3..928cd315 100644 --- 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 index bf06e71d..143e0a49 100644 --- 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 index f06a8a4f..5dfaf44f 100644 --- 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