[Patch v4 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior


Michael D Kinney
 

From: Bret Barkelew <bret.barkelew@...>

https://bugzilla.tianocore.org/show_bug.cgi?id=3111

The VariableLock shim currently fails if called twice because the
underlying Variable Policy engine returns an error if a policy is set
on an existing variable.

This breaks existing code which expect it to silently pass if a variable
is locked multiple times (because it should "be locked").

Refactor the shim to confirm that the variable is indeed locked and then
change the error to EFI_SUCCESS and generate a DEBUG_ERROR message so
the duplicate lock can be reported in a debug log and removed.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Bret Barkelew <Bret.Barkelew@...>
---
.../RuntimeDxe/VariableLockRequestToLock.c | 95 ++++++++++++-------
1 file changed, 59 insertions(+), 36 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
index 4aa854aaf260..7d87e50efdcd 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
@@ -1,67 +1,90 @@
-/** @file -- VariableLockRequestToLock.c
-Temporary location of the RequestToLock shim code while
-projects are moved to VariablePolicy. Should be removed when deprecated.
+/** @file
+ Temporary location of the RequestToLock shim code while projects
+ are moved to VariablePolicy. Should be removed when deprecated.

-Copyright (c) Microsoft Corporation.
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) Microsoft Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#include <Uefi.h>
-
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
-
-#include <Protocol/VariableLock.h>
-
-#include <Protocol/VariablePolicy.h>
#include <Library/VariablePolicyLib.h>
#include <Library/VariablePolicyHelperLib.h>
-
+#include <Protocol/VariableLock.h>

/**
DEPRECATED. THIS IS ONLY HERE AS A CONVENIENCE WHILE PORTING.
- Mark a variable that will become read-only after leaving the DXE phase of execution.
- Write request coming from SMM environment through EFI_SMM_VARIABLE_PROTOCOL is allowed.
+ Mark a variable that will become read-only after leaving the DXE phase of
+ execution. Write request coming from SMM environment through
+ EFI_SMM_VARIABLE_PROTOCOL is allowed.

@param[in] This The VARIABLE_LOCK_PROTOCOL instance.
- @param[in] VariableName A pointer to the variable name that will be made read-only subsequently.
- @param[in] VendorGuid A pointer to the vendor GUID that will be made read-only subsequently.
+ @param[in] VariableName A pointer to the variable name that will be made
+ read-only subsequently.
+ @param[in] VendorGuid A pointer to the vendor GUID that will be made
+ read-only subsequently.

- @retval EFI_SUCCESS The variable specified by the VariableName and the VendorGuid was marked
- as pending to be read-only.
+ @retval EFI_SUCCESS The variable specified by the VariableName and
+ the VendorGuid was marked as pending to be
+ read-only.
@retval EFI_INVALID_PARAMETER VariableName or VendorGuid is NULL.
Or VariableName is an empty string.
- @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID or EFI_EVENT_GROUP_READY_TO_BOOT has
- already been signaled.
- @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold the lock request.
+ @retval EFI_ACCESS_DENIED EFI_END_OF_DXE_EVENT_GROUP_GUID or
+ EFI_EVENT_GROUP_READY_TO_BOOT has already been
+ signaled.
+ @retval EFI_OUT_OF_RESOURCES There is not enough resource to hold the lock
+ request.
**/
EFI_STATUS
EFIAPI
VariableLockRequestToLock (
- IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
- IN CHAR16 *VariableName,
- IN EFI_GUID *VendorGuid
+ IN CONST EDKII_VARIABLE_LOCK_PROTOCOL *This,
+ IN CHAR16 *VariableName,
+ IN EFI_GUID *VendorGuid
)
{
- EFI_STATUS Status;
- VARIABLE_POLICY_ENTRY *NewPolicy;
+ EFI_STATUS Status;
+ VARIABLE_POLICY_ENTRY *NewPolicy;
+
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! %a() will go away soon!\n", __FUNCTION__));
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! Please move to use Variable Policy!\n"));
+ DEBUG ((DEBUG_ERROR, "!!! DEPRECATED INTERFACE !!! Variable: %g %s\n", VendorGuid, VariableName));

NewPolicy = NULL;
- Status = CreateBasicVariablePolicy( VendorGuid,
- VariableName,
- VARIABLE_POLICY_NO_MIN_SIZE,
- VARIABLE_POLICY_NO_MAX_SIZE,
- VARIABLE_POLICY_NO_MUST_ATTR,
- VARIABLE_POLICY_NO_CANT_ATTR,
- VARIABLE_POLICY_TYPE_LOCK_NOW,
- &NewPolicy );
+ Status = CreateBasicVariablePolicy(
+ VendorGuid,
+ VariableName,
+ VARIABLE_POLICY_NO_MIN_SIZE,
+ VARIABLE_POLICY_NO_MAX_SIZE,
+ VARIABLE_POLICY_NO_MUST_ATTR,
+ VARIABLE_POLICY_NO_CANT_ATTR,
+ VARIABLE_POLICY_TYPE_LOCK_NOW,
+ &NewPolicy
+ );
if (!EFI_ERROR( Status )) {
- Status = RegisterVariablePolicy( NewPolicy );
+ Status = RegisterVariablePolicy (NewPolicy);
+
+ //
+ // If the error returned is EFI_ALREADY_STARTED, we need to check the
+ // current database for the variable and see whether it's locked. If it's
+ // locked, we're still fine, but also generate a DEBUG_ERROR message so the
+ // duplicate lock can be removed.
+ //
+ if (Status == EFI_ALREADY_STARTED) {
+ Status = ValidateSetVariable (VariableName, VendorGuid, 0, 0, NULL);
+ if (Status == EFI_WRITE_PROTECTED) {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s is already locked!\n", VendorGuid, VariableName));
+ Status = EFI_SUCCESS;
+ } else {
+ DEBUG ((DEBUG_ERROR, " Variable: %g %s can not be locked!\n", VendorGuid, VariableName));
+ Status = EFI_ACCESS_DENIED;
+ }
+ }
}
- if (EFI_ERROR( Status )) {
+ if (EFI_ERROR (Status)) {
DEBUG(( DEBUG_ERROR, "%a - Failed to lock variable %s! %r\n", __FUNCTION__, VariableName, Status ));
- ASSERT_EFI_ERROR( Status );
}
if (NewPolicy != NULL) {
FreePool( NewPolicy );
--
2.29.2.windows.2

Join devel@edk2.groups.io to automatically receive all group messages.