From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"devel@edk2.groups.io" <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
Date: Fri, 11 Dec 2020 05:40:06 +0000 [thread overview]
Message-ID: <BN8PR11MB3666370931769FFE83FE7F1BCACA0@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201211045156.1758-2-michael.d.kinney@intel.com>
> -----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.
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
next prev parent reply other threads:[~2020-12-11 5:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-11 4:51 [Patch v3 0/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior Michael D Kinney
2020-12-11 4:51 ` [Patch v3 1/2] " Michael D Kinney
2020-12-11 5:40 ` Wu, Hao A [this message]
2020-12-11 7:53 ` Michael D Kinney
2020-12-11 4:51 ` [Patch v3 2/2] MdeModulePkg/Variable/RuntimeDxe: Add Variable Lock Protocol Unit Tests Michael D Kinney
2020-12-11 5:43 ` [edk2-devel] " Wu, Hao A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB3666370931769FFE83FE7F1BCACA0@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox