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

["Laszlo Ersek" <lersek@redhat.com>]

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.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                | 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
>