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

["Michael D Kinney" <michael.d.kinney@intel.com>]

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, December 10, 2020 9:40 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Bret Barkelew <bret.barkelew@microsoft.com>; Liming Gao <gaoliming@byosoft.com.cn>; Bret Barkelew
> <Bret.Barkelew@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@intel.com>
> > Sent: Friday, December 11, 2020 12:52 PM
> > To: devel@edk2.groups.io
> > Cc: Bret Barkelew <bret.barkelew@microsoft.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Bret
> > Barkelew <Bret.Barkelew@microsoft.com>
> > Subject: [Patch v3 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/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