[edk2-devel] [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior
Wu, Hao A
hao.a.wu at intel.com
Fri Dec 11 05:40:06 UTC 2020
> -----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.
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 (#68695): https://edk2.groups.io/g/devel/message/68695
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