Re: [PATCH v1 4/5] MdeModulePkg/Variable: Update to consume SpeculationBarrier

["Wang, Jian J" <jian.j.wang@intel.com>]

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>


> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, December 21, 2018 11:11 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: [PATCH v1 4/5] MdeModulePkg/Variable: Update to consume
> SpeculationBarrier
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1417
> 
> Since BaseLib API AsmLfence() is a x86 arch specific API and should be
> avoided using in generic codes, this commit replaces the usage of
> AsmLfence() with arch-generic API SpeculationBarrier().
> 
> Please note that speculation execution barriers are intended to be
> asserted for SMM codes, hence, this commit still preserve an empty
> implementation of the speculation execution barrier for the DXE codes.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> |  2 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> |  2 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> | 10 ++++----
>  MdeModulePkg/Universal/Variable/RuntimeDxe/{LoadFenceDxe.c =>
> SpeculationBarrierDxe.c} | 12 ++++++----
>  MdeModulePkg/Universal/Variable/RuntimeDxe/{LoadFenceSmm.c =>
> SpeculationBarrierSmm.c} | 14 +++++++-----
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                                  |
> 6 ++---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> | 24 ++++++++++----------
>  7 files changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index 868981ccaf..7ef8a97f5d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> @@ -46,7 +46,7 @@
>    TcgMorLockDxe.c
>    VarCheck.c
>    VariableExLib.c
> -  LoadFenceDxe.c
> +  SpeculationBarrierDxe.c
> 
>  [Packages]
>    MdePkg/MdePkg.dec
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 2fe72ff8a4..db7d220e06 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -54,7 +54,7 @@
>    PrivilegePolymorphic.h
>    VariableExLib.c
>    TcgMorLockSmm.c
> -  LoadFenceSmm.c
> +  SpeculationBarrierSmm.c
> 
>  [Packages]
>    MdePkg/MdePkg.dec
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index a324ad2365..7af22a4ad6 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> @@ -85,13 +85,15 @@ SetVariableCheckHandlerMor (
>    );
> 
>  /**
> -  This service is consumed by the variable modules to perform a serializing
> -  operation on all load-from-memory instructions that were issued prior to the
> -  call of this function.
> +  This service is consumed by the variable modules to place a barrier to stop
> +  speculative execution.
> +
> +  Ensures that no later instruction will execute speculatively, until all prior
> +  instructions have completed.
> 
>  **/
>  VOID
> -MemoryLoadFence (
> +VariableSpeculationBarrier (
>    VOID
>    );
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceDxe.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/SpeculationBarrierDxe.c
> similarity index 62%
> rename from MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceDxe.c
> rename to
> MdeModulePkg/Universal/Variable/RuntimeDxe/SpeculationBarrierDxe.c
> index 0f64ee093b..bc3f695335 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/SpeculationBarrierDxe.c
> @@ -1,5 +1,5 @@
>  /** @file
> -  Serialize operation on all load-from-memory instructions (DXE version).
> +  Barrier to stop speculative execution (DXE version).
> 
>  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
> @@ -15,13 +15,15 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include "Variable.h"
> 
>  /**
> -  This service is consumed by the variable modules to perform a serializing
> -  operation on all load-from-memory instructions that were issued prior to the
> -  call of this function.
> +  This service is consumed by the variable modules to place a barrier to stop
> +  speculative execution.
> +
> +  Ensures that no later instruction will execute speculatively, until all prior
> +  instructions have completed.
> 
>  **/
>  VOID
> -MemoryLoadFence (
> +VariableSpeculationBarrier (
>    VOID
>    )
>  {
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/SpeculationBarrierSmm.c
> similarity index 61%
> rename from MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceSmm.c
> rename to
> MdeModulePkg/Universal/Variable/RuntimeDxe/SpeculationBarrierSmm.c
> index 4b0d7e3e95..dbc20f6c4d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceSmm.c
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/SpeculationBarrierSmm.c
> @@ -1,5 +1,5 @@
>  /** @file
> -  Serialize operation on all load-from-memory instructions (SMM version).
> +  Barrier to stop speculative execution (SMM version).
> 
>  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
> @@ -16,15 +16,17 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include "Variable.h"
> 
>  /**
> -  This service is consumed by the variable modules to perform a serializing
> -  operation on all load-from-memory instructions that were issued prior to the
> -  call of this function.
> +  This service is consumed by the variable modules to place a barrier to stop
> +  speculative execution.
> +
> +  Ensures that no later instruction will execute speculatively, until all prior
> +  instructions have completed.
> 
>  **/
>  VOID
> -MemoryLoadFence (
> +VariableSpeculationBarrier (
>    VOID
>    )
>  {
> -  AsmLfence ();
> +  SpeculationBarrier ();
>  }
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index d100b1dcc5..443cf07144 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -3201,11 +3201,11 @@ VariableServiceSetVariable (
>        return EFI_SECURITY_VIOLATION;
>      }
>      //
> -    // The MemoryLoadFence() call here is to ensure the above sanity check
> -    // for the EFI_VARIABLE_AUTHENTICATION_2 descriptor has been completed
> +    // The VariableSpeculationBarrier() call here is to ensure the above sanity
> +    // check for the EFI_VARIABLE_AUTHENTICATION_2 descriptor has been
> completed
>      // before the execution of subsequent codes.
>      //
> -    MemoryLoadFence ();
> +    VariableSpeculationBarrier ();
>      PayloadSize = DataSize - AUTHINFO2_SIZE (Data);
>    } else {
>      PayloadSize = DataSize;
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index 6dc19c24db..8c53f84ff6 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -538,11 +538,11 @@ SmmVariableHandler (
>        }
> 
>        //
> -      // The MemoryLoadFence() call here is to ensure the previous range/content
> -      // checks for the CommBuffer have been completed before the subsequent
> -      // consumption of the CommBuffer content.
> +      // The VariableSpeculationBarrier() call here is to ensure the previous
> +      // range/content checks for the CommBuffer have been completed before
> the
> +      // subsequent consumption of the CommBuffer content.
>        //
> -      MemoryLoadFence ();
> +      VariableSpeculationBarrier ();
>        if (SmmVariableHeader->NameSize < sizeof (CHAR16) ||
> SmmVariableHeader->Name[SmmVariableHeader->NameSize/sizeof (CHAR16) -
> 1] != L'\0') {
>          //
>          // Make sure VariableName is A Null-terminated string.
> @@ -638,11 +638,11 @@ SmmVariableHandler (
>        }
> 
>        //
> -      // The MemoryLoadFence() call here is to ensure the previous range/content
> -      // checks for the CommBuffer have been completed before the subsequent
> -      // consumption of the CommBuffer content.
> +      // The VariableSpeculationBarrier() call here is to ensure the previous
> +      // range/content checks for the CommBuffer have been completed before
> the
> +      // subsequent consumption of the CommBuffer content.
>        //
> -      MemoryLoadFence ();
> +      VariableSpeculationBarrier ();
>        if (SmmVariableHeader->NameSize < sizeof (CHAR16) ||
> SmmVariableHeader->Name[SmmVariableHeader->NameSize/sizeof (CHAR16) -
> 1] != L'\0') {
>          //
>          // Make sure VariableName is A Null-terminated string.
> @@ -779,11 +779,11 @@ SmmVariableHandler (
>        }
> 
>        //
> -      // The MemoryLoadFence() call here is to ensure the previous range/content
> -      // checks for the CommBuffer have been completed before the subsequent
> -      // consumption of the CommBuffer content.
> +      // The VariableSpeculationBarrier() call here is to ensure the previous
> +      // range/content checks for the CommBuffer have been completed before
> the
> +      // subsequent consumption of the CommBuffer content.
>        //
> -      MemoryLoadFence ();
> +      VariableSpeculationBarrier ();
>        if (CommVariableProperty->NameSize < sizeof (CHAR16) ||
> CommVariableProperty->Name[CommVariableProperty->NameSize/sizeof
> (CHAR16) - 1] != L'\0') {
>          //
>          // Make sure VariableName is A Null-terminated string.
> --
> 2.12.0.windows.1