From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web10.2713.1607665212712029093 for ; Thu, 10 Dec 2020 21:40:13 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=NIHV+YRp; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: hao.a.wu@intel.com) IronPort-SDR: Dxbjk3qR8zIqdfnzJNCk3nYMmG8OcsQFeHARvD0PZBmnYVhZ9Ume/m/Zm6rAcVDpd7IOLYA9AN lxeHJWAre+BA== X-IronPort-AV: E=McAfee;i="6000,8403,9831"; a="153617215" X-IronPort-AV: E=Sophos;i="5.78,410,1599548400"; d="scan'208";a="153617215" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 21:40:11 -0800 IronPort-SDR: 5Hh0L1xXrs43NdYdFPvuVX7dbEMqRiARKveirEMOoP3/Kn6cVgAqamnuZdpF0pndPTIyBP5MYe WXtqYxpY39nQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.78,410,1599548400"; d="scan'208";a="333829074" Received: from orsmsx601.amr.corp.intel.com ([10.22.229.14]) by orsmga003.jf.intel.com with ESMTP; 10 Dec 2020 21:40:11 -0800 Received: from orsmsx612.amr.corp.intel.com (10.22.229.25) by ORSMSX601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 10 Dec 2020 21:40:10 -0800 Received: from orsmsx611.amr.corp.intel.com (10.22.229.24) by ORSMSX612.amr.corp.intel.com (10.22.229.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 10 Dec 2020 21:40:10 -0800 Received: from orsedg603.ED.cps.intel.com (10.7.248.4) by orsmsx611.amr.corp.intel.com (10.22.229.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Thu, 10 Dec 2020 21:40:10 -0800 Received: from NAM12-BN8-obe.outbound.protection.outlook.com (104.47.55.171) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1713.5; Thu, 10 Dec 2020 21:40:10 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FrsJ3ISqZlV6wMyWAKTChRgbt7QGvKbw8FJlMItMLGTnhyjISAMs4o77JtpHYM4baMAo/9/2ibzFZgmSzoPGvgeORqDlgXLbKEBHt0qtraEpWa3aIiRKMqgONVISLUVUtWBqRxciOiSFdWZKk8VsxYEFqIaKAxuw7ekLv3/ZS2a+p8oppVSJTmeDUQvIuPvibFahG0/hZwGRY8mvV14DVAEgZl5kTNxvAfegYW2hs6iuNzk+qWYgz6Gl+XfVRh/aLfwNrpPxraQksVFp2caPQ65xBACIzbBiSchEyLi8u3Qxvy4jOVZoeneEXLHMRoPGFokLwG7lFpxceZUIoqRfOw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=9c0kz6y2sEFsoi+6KVGxyJ5DTBTomONZX3bQ/IL7id8=; b=JA/f7qezck658A+jslSi5Z6vbGgD2PbhLdUGosf9fUfmxvmhW3TcvoE/fy7j8KsVyfZnnECEHgOMsxH78YuL2v5JoZAKsUKcsxa19OxC/J8MQ3G92ZBcwUr89MPn0NbKZT9jET9uZuEwFzJ8uxrS0LUYleuAcFeKdK4IDt48GwK3lszFJ3PymoefbEzRK/12EI3IpVhe+a8CzXfINFBuR0WZd/QatKfHRv8xQiWoxH37WkgSybqqWimNULTEqhSxd8VOTXECmVYiSCiOjtbZZzAqYlzfwdC1ZPRCST5Dyjg5Id/0iK+E6Y/jahML4JXHVrFEBUiW0+HfSHqsBJSCRw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=9c0kz6y2sEFsoi+6KVGxyJ5DTBTomONZX3bQ/IL7id8=; b=NIHV+YRpefHAEL9pzOZsdsEQxbPoDnpYPsmTxLWPHHX9hhru8RRYGVTyWsz1AtN5VMk7a0fb4L0zPmGzDhbIew07DGZfi7Fd/hX80f0UjTQEJEiD1R+EKsrhli6Uh5ZHDRYYjy2lxz8AqoSTY8VTMQ14XTWsVQUE2B15kMn1sw0= Received: from BN8PR11MB3666.namprd11.prod.outlook.com (2603:10b6:408:8c::19) by BN6PR11MB1473.namprd11.prod.outlook.com (2603:10b6:405:a::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3654.15; Fri, 11 Dec 2020 05:40:06 +0000 Received: from BN8PR11MB3666.namprd11.prod.outlook.com ([fe80::4cda:993f:ffc1:f169]) by BN8PR11MB3666.namprd11.prod.outlook.com ([fe80::4cda:993f:ffc1:f169%6]) with mapi id 15.20.3654.014; Fri, 11 Dec 2020 05:40:06 +0000 From: "Wu, Hao A" To: "Kinney, Michael D" , "devel@edk2.groups.io" CC: Bret Barkelew , Liming Gao , Bret Barkelew Subject: Re: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior Thread-Topic: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore Variable Lock Protocol behavior Thread-Index: AQHWz3llZ5UsGOxVikat56p6YODHbKnxXr5Q Date: Fri, 11 Dec 2020 05:40:06 +0000 Message-ID: References: <20201211045156.1758-1-michael.d.kinney@intel.com> <20201211045156.1758-2-michael.d.kinney@intel.com> In-Reply-To: <20201211045156.1758-2-michael.d.kinney@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.147.218] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: bd26c7d0-f80f-4034-f35e-08d89d97374e x-ms-traffictypediagnostic: BN6PR11MB1473: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:4125; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 0w9NtuKqP5BQdHldShOs47UufCp+x6xY/lo7svKYyG0oh112zxpsemNlb6wM4+a66evL8G/7N8zThHMvHAr0izTbuG/goRTWrs6VTO7gl5OP3aHZkAKmW4k4euHj6V7wpCfF09tdC0NDatxdYG0l8+c0gSBKfibhrkocrcOnWHlKuVcz/+OUT6417c+sIcqM9KZ70QvyK2Tiy70QILnYOhg1a9apJNHTqjzXNoVs/+SGYdOUY2PaX8tGxuKWrUQDJoa7Ic4Qk7ThmML5cYzGG9d9TN+SLyiJspjXy6T35XYJHr99qCwBqW6IVQ5i6zKQvTxQfLRAMPmUKDCV3SuHAyswWQNygAzNkuP+EciB01IIHdlrHL9ausFGTC0B5xUby5oxw9VX0mPlquYSTRdR1g== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN8PR11MB3666.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(136003)(376002)(366004)(346002)(33656002)(9686003)(186003)(26005)(55016002)(45080400002)(53546011)(508600001)(6506007)(8676002)(4326008)(52536014)(8936002)(66946007)(66446008)(64756008)(66556008)(66476007)(86362001)(5660300002)(7696005)(2906002)(966005)(110136005)(83380400001)(54906003)(76116006)(71200400001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: =?us-ascii?Q?dFQi6zE2Pd5rCMH2N4YxUuShZ70cWGse64KTYMXe8jaAXbHzb4kR7oTTwkSl?= =?us-ascii?Q?XkQygku5yh/XT1RksJtWgiPY6EMhNjplm3sDI/8CwdWx//HU/JwMTQFE7bFH?= =?us-ascii?Q?3Z1nfWQNDz9AhVAlujOhtsux9xI+tcDRP8dK3soi3vSGiWWwLKOdX5VjQzzF?= =?us-ascii?Q?sfF8zYbEKf0ZWQkwJBRi6JaPMlHiOVglOvfTLhkUD47m38WKOC2Rs/MiNB6D?= =?us-ascii?Q?PeO/BK+gs7vuUAs50GqySDSH31xdzUeNogBD/frIxr4EOojFUJznfsxlRbfs?= =?us-ascii?Q?7pwobsOMdaO69wBV0N3DSSCcueYHexEHVfkZh7mOMVvAmoNDvkHqGcGasJc0?= =?us-ascii?Q?AQapsP0SIFLhrOSxc+H6voK3ub46DNq4uPjaw2gujhvvOoORvtXWI2/VKcKJ?= =?us-ascii?Q?bQs0zr08X58f4TJbQX5eUSPIyG9GVYWIMTSN18vhJZ1oIM3E8kArffrmh8mT?= =?us-ascii?Q?5tA1sZUYpwx2W+u9pPKuGthhTYvWM+QmVmfs96Tae8WSGl0VJTdBkzuz+Ite?= =?us-ascii?Q?cD7n6D8ZqygX58dslnhZxFnn37wzISmhxmzWp5CBP29NcSOJJZsc2HTIvnwB?= =?us-ascii?Q?0fp1kLjrrCRqwfHjUi/p7F/72BhhnNHytfHjV5sIS1q76DdqRsqgqis67N4e?= =?us-ascii?Q?3RmTFoLQdX5OoSg1v0sHqSGne+ZWnDerJXERrt+CYuu09LLm9DoZ1snO+ywV?= =?us-ascii?Q?4uBApM0aHhdx0zkSOsq6uvg8lL3pRruVy+oJVtApZrGOraqOuXvLJAvVNKfQ?= =?us-ascii?Q?fiQldGLDuzl/2fG2/Dx4ndMPTqC9871pnYt8UsZiFkpljKJminmNtxz6teRB?= =?us-ascii?Q?bv30rnA1/dgGkmOXyg6YOqhjPEtefHBOqGiTWlvRnLh52JR5991hSkRKZGuo?= =?us-ascii?Q?SPDUFApwHQyohhi8GOcGTiVUhrEx9Ji48poauOtMItNsMCtHsJ9TdHssPmeh?= =?us-ascii?Q?tIrxWyatSMAGmINwGFnLM0AiNPiw7WXMG/tfuNpSgwUbTTiKkXVmU0rWj67f?= =?us-ascii?Q?XPJT?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN8PR11MB3666.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: bd26c7d0-f80f-4034-f35e-08d89d97374e X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Dec 2020 05:40:06.4289 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: u4wqnma9ZZncwilYFzbi8sU0xFcnAtgHp2bnhNraAcke9j2J3HfQGtxW97IYdzhe/CMMiZSdu6CvuthNVRXNuw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1473 Return-Path: hao.a.wu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Michael D Kinney > Sent: Friday, December 11, 2020 12:52 PM > To: devel@edk2.groups.io > Cc: Bret Barkelew ; Wu, Hao A > ; Liming Gao ; Bret > Barkelew > Subject: [Patch v3 1/2] MdeModulePkg/Variable/RuntimeDxe: Restore > Variable Lock Protocol behavior >=20 > From: Bret Barkelew >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D3111 >=20 > The VariableLock shim currently fails if called twice because the underly= ing > Variable Policy engine returns an error if a policy is set on an existing= variable. >=20 > This breaks existing code which expect it to silently pass if a variable = is locked > multiple times (because it should "be locked"). >=20 > 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. >=20 > 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(-) >=20 > 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. >=20 > -Copyright (c) Microsoft Corporation. > -SPDX-License-Identifier: BSD-2-Clause-Patent > + Copyright (c) Microsoft Corporation. > + SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ >=20 > #include > - > #include > #include > - > -#include > - > -#include > #include > #include > - > +#include >=20 > /** > 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. >=20 > @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 mad= e > 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 mad= e > + read-only subsequently. >=20 > - @retval EFI_SUCCESS The variable specified by the VariableNa= me and > the VendorGuid was marked > - as pending to be read-only. > + @retval EFI_SUCCESS The variable specified by the VariableNa= me 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 alread= y 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)); >=20 > NewPolicy =3D NULL; > - Status =3D 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 =3D 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 =3D RegisterVariablePolicy( NewPolicy ); > + Status =3D RegisterVariablePolicy (NewPolicy); > + > + // > + // If the error returned is EFI_ALREADY_STARTED, we need to check th= e > + // 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 =3D=3D EFI_ALREADY_STARTED) { > + Status =3D ValidateSetVariable (VariableName, VendorGuid, 0, sizeo= f(""), > ""); Hello Mike, Sorry for one thing to confirm. Is it possible when passing "0" as the "Attributes" parameter to function V= alidateSetVariable() causes the below case in ValidateSetVariable(): // Check for attribute constraints. if ((ActivePolicy->AttributesMustHave & Attributes) !=3D ActivePolicy= ->AttributesMustHave || (ActivePolicy->AttributesCantHave & Attributes) !=3D 0) { ReturnStatus =3D 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 =3D=3D EFI_WRITE_PROTECTED) { > + DEBUG ((DEBUG_ERROR, " Variable: %g %s is already locked!\n", > VendorGuid, VariableName)); > + Status =3D EFI_SUCCESS; > + } else { > + DEBUG ((DEBUG_ERROR, " Variable: %g %s can not be locked!\n", > VendorGuid, VariableName)); > + Status =3D 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 !=3D NULL) { > FreePool( NewPolicy ); > -- > 2.29.2.windows.2