From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.820.1572646779826253276 for ; Fri, 01 Nov 2019 15:19:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SlccEMgs; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1572646779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HtcaWslZ+WTotKoo5OBlpHCvRRMw9TzBoKaoXQ9YDF4=; b=SlccEMgsV8HpxNWLfnjKewaiF7g+cEc9B9S5lg6dPz0Zaq/s0kj7oyEa52ExidL6qNocMA w2ej09iP1QfKjevMt3QNqD8k+kiyuI77LyYvYb+e78ZqG6B3juluYvyKd1FhQWu1RxKSjC ujvM4tCgQ8duJ8Vp85MvNpT62OxaRMs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-68-nTefFszWOeCH2aYuEGhSqQ-1; Fri, 01 Nov 2019 18:19:34 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 208551800D67; Fri, 1 Nov 2019 22:19:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-69.ams2.redhat.com [10.36.116.69]) by smtp.corp.redhat.com (Postfix) with ESMTP id B1DBA5C28C; Fri, 1 Nov 2019 22:19:28 +0000 (UTC) Subject: Re: [PATCH V7 00/10] UEFI Variable SMI Reduction To: Michael Kubacki , devel@edk2.groups.io Cc: Dandan Bi , Ard Biesheuvel , Eric Dong , Liming Gao , Michael D Kinney , Ray Ni , Jian J Wang , Hao A Wu , Jiewen Yao References: <20191101173457.11956-1-michael.a.kubacki@intel.com> From: "Laszlo Ersek" Message-ID: <3fbfa373-2c35-a583-eb5d-ad084aea774b@redhat.com> Date: Fri, 1 Nov 2019 23:19:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191101173457.11956-1-michael.a.kubacki@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: nTefFszWOeCH2aYuEGhSqQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hello Michael, On 11/01/19 18:34, Michael Kubacki wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2220 >=20 > V7 Changes: > [PATCH V6 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache suppo= rt > * Remove VariableRuntimeCache.c and VariableRuntimeCache.h from > VariableSmmRuntimeDxe.inf since they are not needed to build the modul= e. >=20 > V6 Changes: > [PATCH V5 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache suppo= rt > The most significant change is: > * Free mVariableRuntimeHobCacheBuffer in CheckForRuntimeCacheSync () in > VariableSmmRuntimeDxe.c with FreePages () instead of FreePool (). > This issue was not found in earlier testing because on the initial set o= f > platforms tested, the variable HOB flush was finished prior to the varia= ble > HOB runtime cache buffer being allocated so the FreePages () call was no= t > executed. >=20 > The remaining changes did not affect testing but are included for robust= ness: > * Pass EFI_OPTIONAL_PTR for the DebugDisposition type in the EfiConvertP= ointer () > calls for mVariableRuntimeHobCacheBuffer, mVariableRuntimeNvCacheBuffe= r, and > mVariableRuntimeVolatileCacheBuffer in VariableAddressChangeEvent () i= n > VariableSmmRuntimeDxe.c as these buffers will not be allocated if the = runtime > cache is disabled. > * In the SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT case = in > SmmVariableHandler () in VariableSmm.c, explicitly verify that > VariableRuntimeHobCache.Store is not NULL in addition to checking that > VariableGlobal.HobVariableBase is not set to zero (variable HOB is flu= shed) > before writing to VariableRuntimeHobCache.Store. >=20 > V5 Changes: > [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache suppo= rt > * Increased validation of the runtime buffers passed in the SMM comm buf= fer > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT structure to t= he > 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 memor= y > range does not overlap with SMRAM ranges via VariableSmmIsBufferOutsid= eSmmValid (). >=20 > V4 Changes: > [PATCH V3 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support > * Set gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache to FA= LSE > by default in MdeModulePkg.dec. >=20 > * Added a new patch to set gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVaria= bleRuntimeCache > 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. I gave my R-b for the OvmfPkg patch in the v4 posting: https://edk2.groups.io/g/devel/message/48979 (alternative link: http://mid.mail-archive.com/b89583ed-06ef-ccd2-2e29-d054f581ea6a@redhat.com ) In the v5 posting -- assuming you had not changed that specific OvmfPkg patch, relative to v4 -- you should have picked up my R-b, and carried it forward ever since (to the present v7). Basically, do a git-rebase, select the "reword" action for the patch, then cut&paste my R-b from my v4 review email to the bottom of the commit message. Then every further posting will contain it. If there *have* been changes to the patch, relative to v4, then it's indeed right to drop (or simply not pick up) my R-b. FWIW, the changelog above does not suggest the particular patch has seen any changes since v4. Thanks! Laszlo >=20 > V3 Changes: > [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing functio= ns > * 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() s= tore list > * RenamedGetNextVariableEx () to VariableServiceGetNextVariableInternal = () > * Updated comments in VariableServiceGetNextVariableInternal () to refer= to > "FindVariablEx ()" instead of "FindVariable ()" since FindVariableEx (= ) > is not used in the function. >=20 > [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY b= uffer > * Updated the commit message to clarify the message "structure can be up= dated > outside the fixed global variable". >=20 > [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status= in VariableParsing > * Remove the function InitVariableParsing () > * Remove the mAuthFormat global variable and instead add a BOOLEAN param= eter > 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 p= lace in > each variable driver in the SMM variable solution (VariableSmmRuntim= eDxe > and VariableSmm). >=20 > [edk2-devel] [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV var= iable functions > * Added the following non-volatile related functions to VariableNonVolat= ile.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 FlushPendingRuntimeVariableCacheUpdates= () > to reduce duplication of=20 > 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 CommBuff= er > 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 rest= rictions > on Runtime Services callers. >=20 > * Added a new patch to disable gEfiMdeModulePkgTokenSpaceGuid.PcdEnableV= ariableRuntimeCache > for all OvmfPkg package builds as requested by maintainer Laszlo Ersek= . >=20 > V2 Changes: >=20 > Patch #1 in V1 both moved functions to VariableParsing.c and modified som= e > functionality in those functions. In V2, the functions are first moved an= d > then functionality is modified in subsequent patches. This resulted in th= e > 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 change= s > were made. >=20 > Overview > --------- > This patch series reduces SMM usage when using VariableSmmRuntimeDxe with > VariableSmm. It does so by eliminating SMM usage for runtime service > GetVariable () and GetNextVariableName () invocations. Most UEFI variable > 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 regul= arly > exceed 100 per boot while SetVariable () calls typically remain less than= 10 > per boot. By focusing on the common case, the majority of overhead associ= ated > with SMM can be avoided while still using existing and proven code for > operations such as variable authentication that require an isolated execu= tion > 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.d= ec: > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache >=20 > The reaminder of this blurb describes the changes and behavior when the P= CD > 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 se= rve > runtime service GetVariable () and GetNextVariableName () caller= s. > b. "SMM cache" - Maintained in VariableSmm to service SMM GetVariabl= e () > and GetNextVariableName () callers. > i. A cache in SMRAM is retained so SMM modules do not operate on= data > outside SMRAM. > 2. A new UEFI variable read and write flow will be used as described bel= ow. >=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 of= a > non-volatile UEFI variable, the variable must also be successfully writte= n 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 se= curity > threat presented by serving runtime services callers from a ring 0 mem= ory > buffer of EfiRuntimeServicesData type. The conclusion was that this ch= ange > does not fundamentally alter the attack surface. The UEFI variable Run= time > 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. Ev= en > today if ring 0 is assumed to be malicious, the malicious code may kee= p one > AP in a loop to monitor the communication data, when the BSP gets an > (authenticated) variable. When the communication buffer is updated and= the > status is set to EFI_SUCCESS, the AP may modify the communication buff= er > contents such the tampered data is returned to the BSP caller. Or an > interrupt handler on the BSP may alter the communication buffer conten= ts > before the data is returned to the caller. In summary, this was not fo= und to > introduce any attack not possible today. >=20 > 2. VarCheckLib impact >=20 > VarCheckLib plays a role in SetVariable () calls. This patch series on= ly > changes GetVariable () behavior. Therefore, VarCheckLib is expected to > have no impact due to these changes. >=20 > Testing Performed > ------------------ > This code was tested with the master branch of edk2 on an Intel Kaby Lake= 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 writte= n. > 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 correct= ness. > 11. Set OsIndications to stop at FW UI to verify cache load of non-volati= le > 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 can > 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 variabl= e > store. > * Non-volatile and volatile variables are maintained in separate buffers > (variable stores). >=20 > Runtime & SMM Cache Coherency > ------------------------------ > The non-volatile cache should always accurately reflect non-volatile stor= age > contents (done today) and the "SMM cache" and "Runtime cache" should alwa= ys be > coherent on access. The runtime cache is updated by VariableSmm. >=20 > Updating both caches from within a SMM SetVariable () operation is fairly > straightforward but a race condition can occur if an SMI occurs during th= e > execution of runtime code reading from the runtime cache. To handle this = case, > a runtime cache read lock is introduced that explicitly moves pending upd= ates > from SMM to the runtime cache if an SMM update occurs while the runtime c= ache > is locked. Note that it is not expected a Runtime services call will inte= rrupt > 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 code= of a > pending cache update in SMM. >=20 > SMM (VariableSmm) > 1. FlushRuntimeCachePendingUpdate SMI - A SW SMI handler that synchroniz= es > the runtime cache buffer with th= e 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 RuntimeCachePendingLeng= th to > RuntimeCache > 3. Clear the RuntimeCachePendingUpdate flag >=20 > Proposed SMM Write Flow > ------------------------ > 1. Perform variable authentication and non-volatile write. If either fai= l, > 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 a= re > 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 | = 18 +- > 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 | 1= 373 ++++---------------- > 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 | = 195 ++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | = 655 +++++++++- > 21 files changed, 2924 insertions(+), 1329 deletions(-) > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNo= nVolatile.h > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePa= rsing.h > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRu= ntimeCache.h > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNo= nVolatile.c > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePa= rsing.c > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRu= ntimeCache.c >=20