From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com []) by mx.groups.io with SMTP id smtpd.web09.3735.1607673687007706411 for ; Fri, 11 Dec 2020 00:01:27 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=fail (domain: intel.com, ip: , mailfrom: michael.d.kinney@intel.com) IronPort-SDR: u0lV6KzVqa1ErHnrzbdBhcfTw77aOMkn9vdr8yscjE4vJi24/a9Hqq5YiqUmPj/+Geb8x5DLXN Z3vQMu33Q7YQ== X-IronPort-AV: E=McAfee;i="6000,8403,9831"; a="192732139" X-IronPort-AV: E=Sophos;i="5.78,410,1599548400"; d="scan'208";a="192732139" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Dec 2020 00:01:26 -0800 IronPort-SDR: 2QtEBpkB0aiCmQNwR2Tlpxuyx1+WokHodpXzi69uh/5R3vUpA/w8/ZEfyPlBb4uVDt4Z0do9Cg zFp3EVyhdPWg== X-IronPort-AV: E=Sophos;i="5.78,410,1599548400"; d="scan'208";a="333917981" Received: from mdkinney-mobl2.amr.corp.intel.com ([10.212.202.206]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Dec 2020 00:01:25 -0800 From: "Michael D Kinney" To: devel@edk2.groups.io Cc: Bret Barkelew , Hao A Wu , Liming Gao , Bret Barkelew Subject: [Patch v4 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior Date: Fri, 11 Dec 2020 00:01:17 -0800 Message-Id: <20201211080118.1885-2-michael.d.kinney@intel.com> X-Mailer: git-send-email 2.29.2.windows.2 In-Reply-To: <20201211080118.1885-1-michael.d.kinney@intel.com> References: <20201211080118.1885-1-michael.d.kinney@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit From: Bret Barkelew 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 Cc: Hao A Wu Cc: Liming Gao Signed-off-by: Bret Barkelew --- .../RuntimeDxe/VariableLockRequestToLock.c | 95 ++++++++++++------- 1 file changed, 59 insertions(+), 36 deletions(-) diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c index 4aa854aaf260..7d87e50efdcd 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.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 - #include #include - -#include - -#include #include #include - +#include /** 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