[edk2-devel] [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior
Michael D Kinney
michael.d.kinney at intel.com
Fri Dec 11 07:53:35 UTC 2020
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu at intel.com>
> Sent: Thursday, December 10, 2020 9:40 PM
> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io
> Cc: Bret Barkelew <bret.barkelew at microsoft.com>; Liming Gao <gaoliming at byosoft.com.cn>; Bret Barkelew
> <Bret.Barkelew at microsoft.com>
> Subject: RE: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior
>
> > -----Original Message-----
> > From: Michael D Kinney <michael.d.kinney at intel.com>
> > Sent: Friday, December 11, 2020 12:52 PM
> > To: devel at edk2.groups.io
> > Cc: Bret Barkelew <bret.barkelew at microsoft.com>; Wu, Hao A
> > <hao.a.wu at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>; Bret
> > Barkelew <Bret.Barkelew at microsoft.com>
> > Subject: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore
> > Variable Lock Protocol behavior
> >
> > From: Bret Barkelew <bret.barkelew at microsoft.com>
> >
> > 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 at intel.com>
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Liming Gao <gaoliming at byosoft.com.cn>
> > Signed-off-by: Bret Barkelew <Bret.Barkelew at microsoft.com>
> > ---
> > .../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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68700): https://edk2.groups.io/g/devel/message/68700
Mute This Topic: https://groups.io/mt/78873159/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list