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


Wu, Hao A
 

-----Original Message-----
From: Michael D Kinney <michael.d.kinney@...>
Sent: Friday, December 11, 2020 12:52 PM
To: devel@edk2.groups.io
Cc: Bret Barkelew <bret.barkelew@...>; Wu, Hao A
<hao.a.wu@...>; Liming Gao <gaoliming@...>; Bret
Barkelew <Bret.Barkelew@...>
Subject: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore
Variable Lock Protocol behavior

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/VariableLockRequestToL
ock.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
index 4aa854aaf260..0715b527a0f7 100644
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
ock.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToL
o
+++ ck.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, sizeof(""),
"");

Hello Mike,

Sorry for one thing to confirm.

Is it possible when passing "0" as the "Attributes" parameter to function ValidateSetVariable()
causes the below case in ValidateSetVariable():

// Check for attribute constraints.
if ((ActivePolicy->AttributesMustHave & Attributes) != ActivePolicy->AttributesMustHave ||
(ActivePolicy->AttributesCantHave & Attributes) != 0) {
ReturnStatus = EFI_INVALID_PARAMETER;
DEBUG(( DEBUG_VERBOSE, "%a - Bad Attributes. 0x%X <> 0x%X:0x%X\n", __FUNCTION__,
Attributes, ActivePolicy->AttributesMustHave, ActivePolicy->AttributesCantHave ));
goto Exit;
}

if "ActivePolicy->AttributesMustHave" have any bit set?
This will lead to VariableLockRequestToLock() to return an error status.

Best Regards,
Hao Wu


+ 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.