Re: [PATCH V7 00/10] UEFI Variable SMI Reduction

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

Hi Laszlo,

I did not make any changes to the OvmfPkg patch and I forgot to carry forward the R-b.
I'll keep that in mind in the future.

For V7, I request the MdeModulePkg maintainers please add the R-b for the patches
not changed. If this is not acceptable, I will be happy to send out a patch series
with all of the R-b present.

Thanks,
Michael

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, November 1, 2019 3:19 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH V7 00/10] UEFI Variable SMI Reduction
> 
> Hello Michael,
> 
> On 11/01/19 18:34, Michael Kubacki wrote:
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> >
> > V7 Changes:
> >  [PATCH V6 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache
> > support
> >  * Remove VariableRuntimeCache.c and VariableRuntimeCache.h from
> >    VariableSmmRuntimeDxe.inf since they are not needed to build the
> module.
> >
> > V6 Changes:
> >  [PATCH V5 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache
> > support  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 of  platforms tested, the variable HOB flush was finished prior to
> > the variable  HOB runtime cache buffer being allocated so the
> > FreePages () call was not  executed.
> >
> >  The remaining changes did not affect testing but are included for
> robustness:
> >  * Pass EFI_OPTIONAL_PTR for the DebugDisposition type in the
> EfiConvertPointer ()
> >    calls for mVariableRuntimeHobCacheBuffer,
> mVariableRuntimeNvCacheBuffer, and
> >    mVariableRuntimeVolatileCacheBuffer in VariableAddressChangeEvent ()
> in
> >    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
> flushed)
> >    before writing to VariableRuntimeHobCache.Store.
> >
> > V5 Changes:
> >  [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache
> > support
> >  * Increased validation of the runtime buffers passed in the SMM comm
> buffer
> >
> 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
> memory
> >    range does not overlap with SMRAM ranges via
> VariableSmmIsBufferOutsideSmmValid ().
> >
> > V4 Changes:
> >  [PATCH V3 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache
> > support
> >  * Set
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache to
> FALSE
> >    by default in MdeModulePkg.dec.
> >
> >  * 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.
> 
> 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
> 
> >
> > 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.
> >
> >  [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> > GetNextVariableEx() store list
> >  * RenamedGetNextVariableEx () to
> > VariableServiceGetNextVariableInternal ()
> >  * Updated comments in VariableServiceGetNextVariableInternal () to
> refer to
> >    "FindVariablEx ()" instead of "FindVariable ()" since FindVariableEx ()
> >    is not used in the function.
> >
> >  [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".
> >
> >  [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
> 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
> (VariableSmmRuntimeDxe
> >      and VariableSmm).
> >
> >  [edk2-devel] [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV
> > variable functions
> >  * Added the following non-volatile related functions to
> VariableNonVolatile.c
> >    from Variable.c:
> >    * InitRealNonVolatileVariableStore ()
> >    * InitEmuNonVolatileVariableStore ()
> >    * InitNonVolatileVariableStore ()
> >
> >  [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
> >    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.
> >
> >  [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.
> >
> >  * Added a new patch to disable
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> >    for all OvmfPkg package builds as requested by maintainer Laszlo Ersek.
> >
> > V2 Changes:
> >
> > Patch #1 in V1 both moved functions to VariableParsing.c and modified
> > some functionality in those functions. In V2, the functions are first
> > moved and then functionality is modified in subsequent patches. This
> > resulted in the following new patches in the V2 patch series:
> >
> >  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
> >
> > Apart from this refactor in the patches, no functionally impacting
> > changes were made.
> >
> > 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
> > regularly exceed 100 per boot while SetVariable () calls typically
> > remain less than 10 per boot. By focusing on the common case, the
> > majority of overhead associated with SMM can be avoided while still
> > using existing and proven code for operations such as variable
> > authentication that require an isolated execution environment.
> >
> >  * Advantage: Reduces overall system SMM usage
> >  * Disadvantage: Requires more Runtime data memory usage
> >
> > 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
> >
> > The reaminder of this blurb describes the changes and behavior when
> > the PCD is set to TRUE.
> >
> > Initial Performance Observations
> > ---------------------------------
> >  * With these proposed changes, an Intel Atom based SoC saw GetVariable
> ( )
> >    time for an existing variable reduce from ~220us to ~5us.
> >
> > Major Changes
> > --------------
> >  1. Two UEFI variable caches will be maintained.
> >      a. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to
> serve
> >          runtime service GetVariable () and GetNextVariableName () callers.
> >      b. "SMM cache" - Maintained in VariableSmm to service SMM
> GetVariable ()
> >          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 below.
> >
> > 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 written to non-volatile storage.
> >
> > Prior RFC Feedback Addressed
> > -----------------------------
> > RFC sent Sept. 5, 2019: https://edk2.groups.io/g/devel/message/46939
> >
> > 1. UEFI variable data retrieval from a ring 0 buffer
> >
> >    A common concern with this proposed set of changes is the potential
> security
> >    threat presented by serving runtime services callers from a ring 0 memory
> >    buffer of EfiRuntimeServicesData type. The conclusion was that this
> change
> >    does not fundamentally alter the attack surface. The UEFI variable
> Runtime
> >    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. Even
> >    today if ring 0 is assumed to be malicious, the malicious code may keep
> 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
> buffer
> >    contents such the tampered data is returned to the BSP caller. Or an
> >    interrupt handler on the BSP may alter the communication buffer
> contents
> >    before the data is returned to the caller. In summary, this was not found
> to
> >    introduce any attack not possible today.
> >
> > 2. VarCheckLib impact
> >
> >    VarCheckLib plays a role in SetVariable () calls. This patch series only
> >    changes GetVariable () behavior. Therefore, VarCheckLib is expected to
> >    have no impact due to these changes.
> >
> > 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:
> >
> > 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 written.
> > 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
> correctness.
> > 11. Set OsIndications to stop at FW UI to verify cache load of non-volatile
> >     contents.
> >
> > 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.
> >
> > 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 variable
> >    store.
> >  * Non-volatile and volatile variables are maintained in separate buffers
> >   (variable stores).
> >
> > Runtime & SMM Cache Coherency
> > ------------------------------
> > The non-volatile cache should always accurately reflect non-volatile
> > storage contents (done today) and the "SMM cache" and "Runtime cache"
> > should always be coherent on access. The runtime cache is updated by
> VariableSmm.
> >
> > Updating both caches from within a SMM SetVariable () operation is
> > fairly straightforward but a race condition can occur if an SMI occurs
> > during the execution of runtime code reading from the runtime cache.
> > To handle this case, a runtime cache read lock is introduced that
> > explicitly moves pending updates 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 interrupt SMM processing since all
> CPU cores rendezvous in SMM.
> >
> > 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.
> >
> > SMM (VariableSmm)
> >  1. FlushRuntimeCachePendingUpdate SMI - A SW SMI handler that
> synchronizes
> >                                          the runtime cache buffer with the SMM
> >                                          cache buffer.
> >
> > 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
> >
> > 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
> >
> > Proposed SMM Write Flow
> > ------------------------
> >  1. Perform variable authentication and non-volatile write. If either fail,
> >     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.
> >
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> >
> > 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
> >
> >  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.i
> nf |   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                | 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             |
> 195 ++-
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
> c   |  655 +++++++++-
> >  21 files changed, 2924 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
> >