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


Michael D Kinney
 

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

-----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.
Thank you! You are exactly right. We do not want this logic used because
we do not know what the valid DataSize and Attribute values are for the
existing policy. For the call to ValidateSetVariable(), we should use
DataSize=0 and Attributes=0.

Status = ValidateSetVariable (VariableName, VendorGuid, 0, 0, NULL);


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.