Re: [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction

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

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 17, 2018 7:02 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Yao, Jiewen; Dong, Eric; Kinney, Michael D
> Subject: Re: [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing
> before RSM instruction
> 
> Beyond comments (1) through (4) which I made under the blurb (v2 0/2):
> 
> On 08/16/18 05:14, Hao Wu wrote:
> > Return Stack Buffer (RSB) is used to predict the target of RET
> > instructions. When the RSB underflows, some processors may fall back to
> > using branch predictors. This might impact software using the retpoline
> > mitigation strategy on those processors.
> >
> > This commit will add RSB stuffing logic before returning from SMM (the RSM
> > instruction) to avoid interfering with non-SMM usage of the retpoline
> > technique.
> >
> > After the stuffing, RSB entries will contain a trap like:
> >
> > @SpecTrap:
> >     pause
> >     lfence
> >     jmp     @SpecTrap
> >
> > A more detailed explanation of the purpose of commit is under the
> > 'Branch target injection mitigation' section of the below link:
> > https://software.intel.com/security-software-guidance/insights/host-firmwa
> > re-speculative-execution-side-channel-mitigation
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm |  3 ++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm  |  3 ++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc  | 55
> ++++++++++++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  |  3 ++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm   |  3 ++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/StuffRsb.inc   | 55
> ++++++++++++++++++++
> >  6 files changed, 122 insertions(+)
> 
> (5) I've had an idea here, but I'm mentioning it only for completeness.
> It will not matter after we fix
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1091>, but until then,
> you could find it useful. Up to you:
> 
> We could move "StuffRsb.inc" to
> "UefiCpuPkg/PiSmmCpuDxeSmm/StuffRsb.inc", thereby eliminating the
> duplication between the Ia32 and X64 subdirectories. And then:
> 
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> > index 509e7a0a66..6bbc339c53 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> > @@ -18,6 +18,8 @@
> >  ;
> >  ;-------------------------------------------------------------------------------
> >
> > +%include "StuffRsb.inc"
> 
> these %include directives could say "../StuffRsb.inc".
> 
> I didn't try this in practice, but I figured it's worth mentioning. Feel
> free to ignore it though; I hope BZ#1091 will be fixed soon.

I did a quick search for pattern ../ within the edk2 repository within
file types .c, .h and .nasm. I found that there are 100+ occurrences, but
most of them appear in 3rd party codes like openssl, BrotliCompress and
etc.

So I am leaning toward to avoid using relative paths for include files,
but anyway, thanks for raising this idea here.

> 
> > +
> >  %define MSR_IA32_MISC_ENABLE 0x1A0
> >  %define MSR_EFER      0xc0000080
> >  %define MSR_EFER_XD   0x800
> > @@ -204,6 +206,7 @@ ASM_PFX(SmiHandler):
> >      wrmsr
> >
> >  .7:
> > +    StuffRsb32
> >      rsm
> >
> >  ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > index 5ff3cd2e73..322b1ab556 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > @@ -18,6 +18,8 @@
> >  ;
> >  ;-------------------------------------------------------------------------------
> >
> > +%include "StuffRsb.inc"
> > +
> >  extern ASM_PFX(SmmInitHandler)
> >  extern ASM_PFX(mRebasedFlag)
> >  extern ASM_PFX(mSmmRelocationOriginalAddress)
> > @@ -75,6 +77,7 @@ BITS 32
> >      mov     esp, strict dword 0         ; source operand will be patched
> >  ASM_PFX(gPatchSmmInitStack):
> >      call    ASM_PFX(SmmInitHandler)
> > +    StuffRsb32
> >      rsm
> >
> >  BITS 16
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc
> > new file mode 100644
> > index 0000000000..3fd481a8d3
> > --- /dev/null
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc
> > @@ -0,0 +1,55 @@
> > +;------------------------------------------------------------------------------
> > +;
> > +; 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.
> > +;
> > +; Abstract:
> > +;
> > +;   This is the equates file for Stuffing the Return Stack Buffer (RSB)
> 
> (6) I think we shouldn't call this an EQU file in the comment above; we
> are providing a macro definition. Maybe say "This file provides macro
> definitions for Stuffing the Return Stack Buffer (RSB)".

Yes. 'macro definitions' is much more better here. I will update the
comments.

> 
> 
> Otherwise, the patch looks good to me. This is to say that what it does
> does not look broken to me (i.e. it appears "sound"); however I can't
> tell whether it actually suffices for the stated purpose (i.e. if it is
> "complete"). For that reason, I prefer to give an A-b rather than an
> R-b:
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> (The stack footprint is 32*4=128 bytes in the IA32 case and 32*8=256
> bytes in the X64 case; I think that should conveniently fit in the
> outermost part of the SMM stack.)
> 
> 
> Furthermore, I've tested this. Using the Q35 machine type, with the
> series built into OVMF with -D SMM_REQUIRE, on top of commit
> b9130c866dc0. I used the tests described here:
> <https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-
> QEMU,-KVM-and-libvirt>.
> 
> - with Linux guests:
>   - Fedora (IA32)
>   - Fedora (IA32X64)
>   - RHEL7 (IA32X64)
> 
> - with Windows guests (all IA32X64):
>   - Windows 7
>   - Windows Server 2008 R2
>   - Windows 8.1
>   - Windows Server 2012 R2
>   - Windows 10
>   - Windows Server 2016 (boot only, didn't test S3)
> 
> Again, see the "sound" vs. "complete" distinction -- I can't test
> whether the patch actively fixes a "retpoline interference" issue; I can
> only say it appears to regress nothing for me.
> 
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> (Please remember that the above tags are conditional on fixing (1)
> through (4), and (6). It's OK to ignore (5).)

Thanks a lot for the review and test effort.
I will send a new series to address your comment 1~4 and 6.

Best Regards,
Hao Wu

> 
> Thanks,
> Laszlo