Re: [PATCH v1 1/5] MdePkg/BaseLib: Add new LoadFence API

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Thursday, September 20, 2018 9:13 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Yao, Jiewen; Gao, Liming
> Subject: Re: [edk2] [PATCH v1 1/5] MdePkg/BaseLib: Add new LoadFence API
> 
> On 09/20/18 08:40, Hao Wu wrote:
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1193
> >
> > This commit will add a new BaseLib API LoadFence(). This API will perform
> > a serializing operation on all load-from-memory instructions that were
> > issued prior to the call of this function.
> >
> > The purpose of adding this API is to mitigate of the [CVE-2017-5753]
> > Bounds Check Bypass issue when untrusted data are being processed within
> > SMM. More details can be referred at the 'Bounds check bypass mitigation'
> > section at the below link:
> >
> > https://software.intel.com/security-software-guidance/insights/host-
> firmware-speculative-execution-side-channel-mitigation
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> >  MdePkg/Include/Library/BaseLib.h           | 12 +++++++
> >  MdePkg/Library/BaseLib/Arm/LoadFence.c     | 26 ++++++++++++++
> >  MdePkg/Library/BaseLib/BaseLib.inf         |  4 +++
> >  MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c | 15 +++++++-
> >  MdePkg/Library/BaseLib/Ia32/LoadFence.nasm | 37 +++++++++++++++++++
> >  MdePkg/Library/BaseLib/X64/LoadFence.nasm  | 38
> ++++++++++++++++++++
> >  6 files changed, 131 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Include/Library/BaseLib.h
> b/MdePkg/Include/Library/BaseLib.h
> > index 123ae19dc2..194726ca35 100644
> > --- a/MdePkg/Include/Library/BaseLib.h
> > +++ b/MdePkg/Include/Library/BaseLib.h
> > @@ -4939,6 +4939,18 @@ MemoryFence (
> >
> >
> >  /**
> > +  Performs a serializing operation on all load-from-memory instructions that
> > +  were issued prior to the call of this function.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +LoadFence (
> > +  VOID
> > +  );
> > +
> > +
> > +/**
> >    Saves the current CPU context that can be restored with a call to LongJump()
> >    and returns 0.
> >
> > diff --git a/MdePkg/Library/BaseLib/Arm/LoadFence.c
> b/MdePkg/Library/BaseLib/Arm/LoadFence.c
> > new file mode 100644
> > index 0000000000..69f0c3a07e
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseLib/Arm/LoadFence.c
> > @@ -0,0 +1,26 @@
> > +/** @file
> > +  LoadFence() function for ARM.
> > +
> > +  Copyright (C) 2018, Intel Corporation. All rights reserved.<BR>
> > +
> > +  This program and the accompanying materials are licensed and made
> available
> > +  under the terms and conditions of the BSD License which accompanies this
> > +  distribution.  The full text of the license may be found at
> > +  http://opensource.org/licenses/bsd-license.php.
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS, WITHOUT
> > +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> > +**/
> > +
> > +/**
> > +  Performs a serializing operation on all load-from-memory instructions that
> > +  were issued prior to the call of this function.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +LoadFence (
> > +  VOID
> > +  )
> > +{
> > +}
> > diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
> b/MdePkg/Library/BaseLib/BaseLib.inf
> > index a1b5ec4b75..f028fbc75a 100644
> > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > @@ -68,6 +68,7 @@
> >
> >  [Sources.Ia32]
> >    Ia32/WriteTr.nasm
> > +  Ia32/LoadFence.nasm
> >
> >    Ia32/Wbinvd.c | MSFT
> >    Ia32/WriteMm7.c | MSFT
> > @@ -346,6 +347,7 @@
> >    X64/EnableCache.nasm
> >    X64/DisableCache.nasm
> >    X64/WriteTr.nasm
> > +  X64/LoadFence.nasm
> >
> >    X64/CpuBreakpoint.c | MSFT
> >    X64/WriteMsr64.c | MSFT
> > @@ -580,6 +582,7 @@
> >  [Sources.ARM]
> >    Arm/InternalSwitchStack.c
> >    Arm/Unaligned.c
> > +  Arm/LoadFence.c
> >    Math64.c                   | RVCT
> >    Math64.c                   | MSFT
> >
> > @@ -613,6 +616,7 @@
> >  [Sources.AARCH64]
> >    Arm/InternalSwitchStack.c
> >    Arm/Unaligned.c
> > +  Arm/LoadFence.c
> >    Math64.c
> >
> >    AArch64/MemoryFence.S             | GCC
> > diff --git a/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> b/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > index 9b7d875664..a79461cfbf 100644
> > --- a/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > +++ b/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Base Library CPU Functions for EBC
> >
> > -  Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of the BSD
> License
> >    which accompanies this distribution.  The full text of the license may be
> found at
> > @@ -52,6 +52,19 @@ MemoryFence (
> >  }
> >
> >  /**
> > +  Performs a serializing operation on all load-from-memory instructions that
> > +  were issued prior to the call of this function.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +LoadFence (
> > +  VOID
> > +  )
> > +{
> > +}
> > +
> > +/**
> >    Disables CPU interrupts.
> >
> >  **/
> > diff --git a/MdePkg/Library/BaseLib/Ia32/LoadFence.nasm
> b/MdePkg/Library/BaseLib/Ia32/LoadFence.nasm
> > new file mode 100644
> > index 0000000000..11600bea76
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseLib/Ia32/LoadFence.nasm
> > @@ -0,0 +1,37 @@
> > +;------------------------------------------------------------------------------ ;
> > +; Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> > +; This program and the accompanying materials
> > +; are licensed and made available under the terms and conditions of the BSD
> License
> > +; which accompanies this distribution.  The full text of the license may be
> found at
> > +; http://opensource.org/licenses/bsd-license.php.
> > +;
> > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> > +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > +;
> > +; Module Name:
> > +;
> > +;   LoadFence.nasm
> > +;
> > +; Abstract:
> > +;
> > +;   Performs a serializing operation on all load-from-memory instructions
> that
> > +;   were issued prior to the call of this function.
> > +;
> > +; Notes:
> > +;
> > +;------------------------------------------------------------------------------
> > +
> > +    SECTION .text
> > +
> > +;------------------------------------------------------------------------------
> > +; VOID
> > +; EFIAPI
> > +; LoadFence (
> > +;   VOID
> > +;   );
> > +;------------------------------------------------------------------------------
> > +global ASM_PFX(LoadFence)
> > +ASM_PFX(LoadFence):
> > +    lfence
> > +    ret
> > +
> > diff --git a/MdePkg/Library/BaseLib/X64/LoadFence.nasm
> b/MdePkg/Library/BaseLib/X64/LoadFence.nasm
> > new file mode 100644
> > index 0000000000..c076d9789d
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseLib/X64/LoadFence.nasm
> > @@ -0,0 +1,38 @@
> > +;------------------------------------------------------------------------------ ;
> > +; Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> > +; This program and the accompanying materials
> > +; are licensed and made available under the terms and conditions of the BSD
> License
> > +; which accompanies this distribution.  The full text of the license may be
> found at
> > +; http://opensource.org/licenses/bsd-license.php.
> > +;
> > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> > +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > +;
> > +; Module Name:
> > +;
> > +;   LoadFence.nasm
> > +;
> > +; Abstract:
> > +;
> > +;   Performs a serializing operation on all load-from-memory instructions
> that
> > +;   were issued prior to the call of this function.
> > +;
> > +; Notes:
> > +;
> > +;------------------------------------------------------------------------------
> > +
> > +    DEFAULT REL
> > +    SECTION .text
> > +
> > +;------------------------------------------------------------------------------
> > +; VOID
> > +; EFIAPI
> > +; LoadFence (
> > +;   VOID
> > +;   );
> > +;------------------------------------------------------------------------------
> > +global ASM_PFX(LoadFence)
> > +ASM_PFX(LoadFence):
> > +    lfence
> > +    ret
> > +
> >
> 
> Comments in no particular order:
> 
> (1) I think the EBC stub implementation should go into a separate file
> under "MdePkg/Library/BaseLib/Ebc".

Yes, I will do this in later version of the series.

> 
> (2) Given that the ARM memory model is laxer than x86, I'm doubtful that
> an empty implementation is appropriate. I expect a DMB variant should be
> used, but I totally defer to Ard and Leif on that.
> 
> (3) We have Arm/ and AArch64/ subdirectories, but only one common
> variant is provided, under Arm/. (I expect this fact (i.e., "common
> variant") might remain true even after considering (2).) What I find
> inconsistent though is that Ia32 and X64 get separate NASM files,
> despite them sharing the implementation between each other as well.

Thanks for the comments.
I will start a new discussion to loop in Ard and Leif to have a discussion
on this.

Best Regards,
Hao Wu

> 
> IOW, this remark isn't about the actual implementation of the new API;
> it's about consistency. If we decide for one ISA that the 32-bit and
> 64-bit platforms use a common set of files, then the other ISA (also
> with 32-bit and 64-bit platforms) should act similarly, if a common
> implementation is possible.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel