回复: [edk2-devel] [Patch v4 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior

["gaoliming" <gaoliming@byosoft.com.cn>]

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: bounce+27952+68702+4905953+8761045@groups.io
> <bounce+27952+68702+4905953+8761045@groups.io> 代表 Michael D
> Kinney
> 发送时间: 2020年12月11日 16:01
> 收件人: devel@edk2.groups.io
> 抄送: Bret Barkelew <bret.barkelew@microsoft.com>; Hao A Wu
> <hao.a.wu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Bret
> Barkelew <Bret.Barkelew@microsoft.com>
> 主题: [edk2-devel] [Patch v4 1/2] MdeModulePkg/Variable/RuntimeDxe:
> Restore Variable Lock Protocol behavior
> 
> From: Bret Barkelew <bret.barkelew@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@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Bret Barkelew <Bret.Barkelew@microsoft.com>
> ---
>  .../RuntimeDxe/VariableLockRequestToLock.c    | 95 ++++++++++++-------
>  1 file changed, 59 insertions(+), 36 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLo
> ck.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLo
> ck.c
> index 4aa854aaf260..7d87e50efdcd 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLo
> ck.c
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLo
> 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, 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
> 
> 
> 
> 
>