From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web10.1335.1571365905732315918 for ; Thu, 17 Oct 2019 19:31:45 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: jian.j.wang@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Oct 2019 19:31:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,310,1566889200"; d="scan'208";a="190231803" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga008.jf.intel.com with ESMTP; 17 Oct 2019 19:31:44 -0700 Received: from fmsmsx162.amr.corp.intel.com (10.18.125.71) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 17 Oct 2019 19:31:44 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx162.amr.corp.intel.com (10.18.125.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 17 Oct 2019 19:31:44 -0700 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.33]) by shsmsx102.ccr.corp.intel.com ([169.254.2.176]) with mapi id 14.03.0439.000; Fri, 18 Oct 2019 10:31:41 +0800 From: "Wang, Jian J" To: "devel@edk2.groups.io" , "Kubacki, Michael A" CC: "Bi, Dandan" , Ard Biesheuvel , "Dong, Eric" , Laszlo Ersek , "Gao, Liming" , "Kinney, Michael D" , "Ni, Ray" , "Wu, Hao A" , "Yao, Jiewen" Subject: Re: [edk2-devel] [PATCH V5 00/10] UEFI Variable SMI Reduction Thread-Topic: [edk2-devel] [PATCH V5 00/10] UEFI Variable SMI Reduction Thread-Index: AQHVhUj/qs7D1HkyzkGD1f63cu/D46dfrVRA Date: Fri, 18 Oct 2019 02:31:41 +0000 Message-ID: References: <20191018001410.27328-1-michael.a.kubacki@intel.com> In-Reply-To: <20191018001410.27328-1-michael.a.kubacki@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiODI0N2U3ZWQtZTMwYS00NzhhLWE2NTgtZDQ3MDhkNTc1MmFjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMVdXSGVDaXVlbEFVTGh5WW56RTJyREh5NHBmOTB3ZnpzakxsRE9jNmViNE5UZDFobTBTMzVjUnFpTGdlZ1M1aSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable For the whole patch series (1-10), Reviewed-by: Jian J Wang > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Kubacki, > Michael A > Sent: Friday, October 18, 2019 8:14 AM > To: devel@edk2.groups.io > Cc: Bi, Dandan ; Ard Biesheuvel > ; Dong, Eric ; Laszlo Er= sek > ; Gao, Liming ; Kinney, Michael= D > ; Ni, Ray ; Wang, Jian J > ; Wu, Hao A ; Yao, Jiewen > > Subject: [edk2-devel] [PATCH V5 00/10] UEFI Variable SMI Reduction >=20 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2220 >=20 > V5 Changes: > [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache supp= ort > * Increased validation of the runtime buffers passed in the SMM comm bu= ffer > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT > structure to the > SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT > function in > SmmVariableHandler () in VariableSmm.c. > * Most notably, each runtime buffer given is checked to ensure its memo= ry > range does not overlap with SMRAM ranges via > VariableSmmIsBufferOutsideSmmValid (). >=20 > V4 Changes: > [PATCH V3 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache suppor= t > * Set gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache to > FALSE > by default in MdeModulePkg.dec. >=20 > * Added a new patch to set > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > to TRUE at the end of the patch series. This allows someone to bisect= an issue > at > patch #7 or patch #8 in the series with no change in variable caching= behavior. > The > runtime cache variable logic would be applied explicitly in V4 patch = #10. >=20 > V3 Changes: > [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing > functions > * Removed GUIDs added to VariableStandaloneMm.inf that are not required= . > * Added more details to the commit message describing the criteria of > moving the chosen functions to VariableParsing.c. >=20 > [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() > store list > * RenamedGetNextVariableEx () to VariableServiceGetNextVariableInternal= () > * Updated comments in VariableServiceGetNextVariableInternal () to refe= r to > "FindVariablEx ()" instead of "FindVariable ()" since FindVariableEx = () > is not used in the function. >=20 > [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize > VARIABLE_INFO_ENTRY buffer > * Updated the commit message to clarify the message "structure can be > updated > outside the fixed global variable". >=20 > [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth statu= s in > VariableParsing > * Remove the function InitVariableParsing () > * Remove the mAuthFormat global variable and instead add a BOOLEAN > parameter > to all functions that require variable authentication information to > indicate if authenticated variables are used. > * This allows the authenticated variable status to be tracked in one = place in > each variable driver in the SMM variable solution (VariableSmmRunti= meDxe > and VariableSmm). >=20 > [edk2-devel] [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV > variable functions > * Added the following non-volatile related functions to VariableNonVola= tile.c > from Variable.c: > * InitRealNonVolatileVariableStore () > * InitEmuNonVolatileVariableStore () > * InitNonVolatileVariableStore () >=20 > [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() > cache support > * Added a FeaturePCD to control enabling the runtime variable cache - > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache. > * Removed usage of the TimerLib and the wait to acquire > mVariableRuntimeCacheReadLock. Can rely on the UEFI specification > restrictions on Runtime Services callers. > * Removed the EFIAPI keyword from internal functions. > * Removed PCDs in VariableSmmRuntimeDxe.inf not required. > * Removed the HobVariableBackupBase variable no longer required. > * Renamed SynchronizeRuntimeVariableCacheEx () to better reflect usage. > * Renamed functions in VariableRuntimeCache.c to better reflect usage. > * Introduced a local variable in FlushPendingRuntimeVariableCacheUpdate= s () > to reduce duplication of > mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext. > * Corrected the macro used in SmmVariableHandler () to > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT. > * Remove usage of the EDKII_PI_SMM_COMMUNICATION_REGION_TABLE to > acquire a > CommBuffer from the EFI System Table and use the same runtime > CommBuffer > allocated for variable SMM communicate calls. >=20 > [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() > cache support > * Removed usage of the TimerLib and the wait to acquire > mVariableRuntimeCacheReadLock. Can rely on the UEFI specification > restrictions > on Runtime Services callers. >=20 > * Added a new patch to disable > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > for all OvmfPkg package builds as requested by maintainer Laszlo Erse= k. >=20 > V2 Changes: >=20 > Patch #1 in V1 both moved functions to VariableParsing.c and modified so= me > functionality in those functions. In V2, the functions are first moved a= nd > then functionality is modified in subsequent patches. This resulted in t= he > following new patches in the V2 patch series: >=20 > 1. MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list > 2. MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer > 3. MdeModulePkg/Variable: Add local auth status in VariableParsing > 4. MdeModulePkg/Variable: Add a file for NV variable functions >=20 > Apart from this refactor in the patches, no functionally impacting chang= es > were made. >=20 > Overview > --------- > This patch series reduces SMM usage when using VariableSmmRuntimeDxe wit= h > VariableSmm. It does so by eliminating SMM usage for runtime service > GetVariable () and GetNextVariableName () invocations. Most UEFI variabl= e > usage in typical systems after the variable store is initialized > (e.g. manufacturing boots) is due to GetVariable ( ) and > GetNextVariableName () not SetVariable (). GetVariable () calls can regu= larly > exceed 100 per boot while SetVariable () calls typically remain less tha= n 10 > per boot. By focusing on the common case, the majority of overhead assoc= iated > with SMM can be avoided while still using existing and proven code for > operations such as variable authentication that require an isolated exec= ution > environment. >=20 > * Advantage: Reduces overall system SMM usage > * Disadvantage: Requires more Runtime data memory usage >=20 > The runtime cache behavior described for this patch series is enabled by > default but can be disabled with a new FeaturePCD added to > MdeModulePkg.dec: > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache >=20 > The reaminder of this blurb describes the changes and behavior when the = PCD > is set to TRUE. >=20 > Initial Performance Observations > --------------------------------- > * With these proposed changes, an Intel Atom based SoC saw GetVariable = ( ) > time for an existing variable reduce from ~220us to ~5us. >=20 > Major Changes > -------------- > 1. Two UEFI variable caches will be maintained. > a. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to s= erve > runtime service GetVariable () and GetNextVariableName () calle= rs. > b. "SMM cache" - Maintained in VariableSmm to service SMM GetVariab= le () > and GetNextVariableName () callers. > i. A cache in SMRAM is retained so SMM modules do not operate o= n data > outside SMRAM. > 2. A new UEFI variable read and write flow will be used as described be= low. >=20 > At any given time, the two caches would be coherent. On a variable write= , the > runtime cache is only updated after validation in SMM and, in the case o= f a > non-volatile UEFI variable, the variable must also be successfully writt= en to > non-volatile storage. >=20 > Prior RFC Feedback Addressed > ----------------------------- > RFC sent Sept. 5, 2019: https://edk2.groups.io/g/devel/message/46939 >=20 > 1. UEFI variable data retrieval from a ring 0 buffer >=20 > A common concern with this proposed set of changes is the potential s= ecurity > threat presented by serving runtime services callers from a ring 0 me= mory > buffer of EfiRuntimeServicesData type. The conclusion was that this c= hange > does not fundamentally alter the attack surface. The UEFI variable Ru= ntime > Services are invoked from ring 0 and the data already travels through= ring > 0 buffers (such as the SMM communicate buffer) to reach the caller. E= ven > today if ring 0 is assumed to be malicious, the malicious code may ke= ep one > AP in a loop to monitor the communication data, when the BSP gets an > (authenticated) variable. When the communication buffer is updated an= d the > status is set to EFI_SUCCESS, the AP may modify the communication buf= fer > contents such the tampered data is returned to the BSP caller. Or an > interrupt handler on the BSP may alter the communication buffer conte= nts > before the data is returned to the caller. In summary, this was not f= ound to > introduce any attack not possible today. >=20 > 2. VarCheckLib impact >=20 > VarCheckLib plays a role in SetVariable () calls. This patch series o= nly > changes GetVariable () behavior. Therefore, VarCheckLib is expected t= o > have no impact due to these changes. >=20 > Testing Performed > ------------------ > This code was tested with the master branch of edk2 on an Intel Kaby Lak= e U > and Intel Whiskey Lake U reference validation platform. The set of tests > performed included: >=20 > 1. Boot from S5 to Windows 10 OS with SMM variables enabled and > the variable runtime cache enabled. > 2. Boot from S5 to Ubuntu 18.04.1 LTS with SMM variable enabled > and the variable runtime cache enabled. > 3. Boot from S5 to Windows 10 OS with SMM variables enabled and > the variable runtime cache disabled. > 4. Boot from S5 to Ubuntu 18.04.1 LTS with SMM variable enabled > and the variable runtime cache disabled. > 5. Boot from S5 to EFI shell with DXE variables enabled. > (commit 2de1f611be broke OvmfPkgIa32X64 boot regardless of > this patch series; testing without this commit was successful) > 6. Dump UEFI variable store at shell with dmpstore to verify contents. > 7. Dump NvStorage FV from SPI flash after boot to verify contents writt= en. > 8. Dump UEFI variable statistics with VariableInfo at shell. > 9. Boot with emulated variables enabled. > 10. Cycles of adding and deleting a UEFI variable to verify cache correc= tness. > 11. Set OsIndications to stop at FW UI to verify cache load of non-volat= ile > contents. >=20 > Why Keep SMM on Variable Writes > -------------------------------- > * SMM provides a ubiquitous isolated execution environment in x86 for > authenticated UEFI variables. > * BIOS region SPI flash write restrictions to SMM in platforms today ca= n > be retained. >=20 > Today's UEFI Variable Cache (for reference) > -------------------------------------------- > * Maintained in SMRAM via VariableSmm. > * A "write-through" cache of variable data in the form of a UEFI variab= le > store. > * Non-volatile and volatile variables are maintained in separate buffer= s > (variable stores). >=20 > Runtime & SMM Cache Coherency > ------------------------------ > The non-volatile cache should always accurately reflect non-volatile sto= rage > contents (done today) and the "SMM cache" and "Runtime cache" should alw= ays > be > coherent on access. The runtime cache is updated by VariableSmm. >=20 > Updating both caches from within a SMM SetVariable () operation is fairl= y > straightforward but a race condition can occur if an SMI occurs during t= he > execution of runtime code reading from the runtime cache. To handle this= case, > a runtime cache read lock is introduced that explicitly moves pending up= dates > from SMM to the runtime cache if an SMM update occurs while the runtime > cache > is locked. Note that it is not expected a Runtime services call will int= errupt > SMM processing since all CPU cores rendezvous in SMM. >=20 > New Key Elements for Coherence > ------------------------------- > Runtime DXE (VariableSmmRuntimeDxe) > 1. RuntimeCacheReadLock - A global lock used to lock read access to the > runtime cache. > 2. RuntimeCachePendingUpdate - A global flag used to notify runtime cod= e of a > pending cache update in SMM. >=20 > SMM (VariableSmm) > 1. FlushRuntimeCachePendingUpdate SMI - A SW SMI handler that synchroni= zes > the runtime cache buffer with t= he SMM > cache buffer. >=20 > Proposed Runtime DXE Read Flow > ------------------------------- > 1. Acquire RuntimeCacheReadLock > 2. If RuntimeCachePendingUpdate flag (rare) is set then: > 2.a. Trigger FlushRuntimeCachePendingUpdate SMI > 2.b. Verify RuntimeCachePendingUpdate flag is cleared > 3. Perform read from RuntimeCache > 4. Release RuntimeCacheReadLock >=20 > Proposed FlushRuntimeCachePendingUpdate SMI > -------------------------------------------- > 1. If RuntimeCachePendingUpdate flag is not set: > 1.a. Return > 2. Copy the data at RuntimeCachePendingOffset of > RuntimeCachePendingLength to > RuntimeCache > 3. Clear the RuntimeCachePendingUpdate flag >=20 > Proposed SMM Write Flow > ------------------------ > 1. Perform variable authentication and non-volatile write. If either fa= il, > return an error to the caller. > 2. If RuntimeCacheReadLock is set then: > 2.a. Set RuntimeCachePendingUpdate flag > 2.b. Update RuntimeCachePendingOffset and RuntimeCachePendingLength > to > cover the a superset of the pending chunk (for simplicity, the > entire variable store is currently synchronized). > 3. Else: > 3.a. Update RuntimeCache > 4. Update SmmCache > - Note: RT read cannot occur during SMI processing since all cores = are > locked in SMM. >=20 > Cc: Dandan Bi > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Ray Ni > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Jiewen Yao > Signed-off-by: Michael Kubacki >=20 > Michael Kubacki (10): > MdeModulePkg/Variable: Consolidate common parsing functions > MdeModulePkg/Variable: Parameterize GetNextVariableInternal () stores > MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer > MdeModulePkg/Variable: Parameterize auth status in VariableParsing > MdeModulePkg/Variable: Add a file for NV variable functions > MdeModulePkg VariableInfo: Always consider RT DXE and SMM stats > MdeModulePkg/Variable: Add RT GetVariable() cache support > MdeModulePkg/Variable: Add RT GetNextVariableName() cache support > OvmfPkg: Disable variable runtime cache > MdeModulePkg: Enable variable runtime cache by default >=20 > MdeModulePkg/MdeModulePkg.dec | = 12 + > OvmfPkg/OvmfPkgIa32.dsc | = 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | = 1 + > OvmfPkg/OvmfPkgX64.dsc | = 1 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | = 6 > + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | = 6 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | > 20 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | > 6 + > MdeModulePkg/Include/Guid/SmmVariableCommon.h | = 29 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | = 151 +-- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h | = 67 > + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h | = 347 > +++++ > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h | > 51 + > MdeModulePkg/Application/VariableInfo/VariableInfo.c | = 37 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | = 1373 > ++++---------------- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c | = 24 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c | = 334 > +++++ > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | = 786 > +++++++++++ > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c | > 153 +++ > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | = 196 > ++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | > 655 +++++++++- > 21 files changed, 2927 insertions(+), 1329 deletions(-) > create mode 100644 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h > create mode 100644 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > create mode 100644 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h > create mode 100644 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c > create mode 100644 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > create mode 100644 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c >=20 > -- > 2.16.2.windows.1 >=20 >=20 >=20