[PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction

[PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction

[Hao Wu]

V2 changes:
A. Refine commit log message to clarify the purpose of the series

B. Extract the RSB stuffing logic to INC files to avoid code duplication:
When compiling .NASM source files, the current build rule does not support
including files other than the .NASM file directory, this series will
duplicate the StuffRsb.inc file together with the .NASM files at this
moment.

Please consider this approach as the first stage, I have filed a Bugzilla
for adding $(INC)-like support when compiling .NASM files:
https://bugzilla.tianocore.org/show_bug.cgi?id=1085

After the above support is added, the next step will be taken to remove
those duplicated StuffRsb.inc files and put it under a common include
directory like:
UefiCpuPkg/Include/

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>

Hao Wu (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
  UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction

 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm     |  3 ++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 10 ++--
 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc      | 55 ++++++++++++++++++++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm      |  3 ++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm  |  8 ++-
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc       | 55 ++++++++++++++++++++
 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 ++++++++++++++++++++
 12 files changed, 251 insertions(+), 5 deletions(-)
 create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc
 create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/StuffRsb.inc
 create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/X64/StuffRsb.inc

-- 
2.12.0.windows.1

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

[Hao Wu]

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(+)

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"
+
 %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)
+;
+;------------------------------------------------------------------------------
+
+%define RSB_STUFF_ENTRIES 0x20
+
+;
+; parameters:
+; @param 1: register to use as counter (e.g. IA32:eax, X64:rax)
+; @param 2: stack pointer to restore   (IA32:esp, X64:rsp)
+; @param 3: the size of a stack frame  (IA32:4, X64:8)
+;
+%macro StuffRsb 3
+      mov     %1, RSB_STUFF_ENTRIES / 2
+  %%Unroll1:
+      call    %%Unroll2
+  %%SpecTrap1:
+      pause
+      lfence
+      jmp     %%SpecTrap1
+  %%Unroll2:
+      call    %%StuffLoop
+  %%SpecTrap2:
+      pause
+      lfence
+      jmp     %%SpecTrap2
+  %%StuffLoop:
+      dec     %1
+      jnz     %%Unroll1
+      add     %2, RSB_STUFF_ENTRIES * %3 ; Restore the stack pointer
+%endmacro
+
+;
+; RSB stuffing macros for IA32 and X64
+;
+%macro StuffRsb32 0
+      StuffRsb     eax, esp, 4
+%endmacro
+
+%macro StuffRsb64 0
+      StuffRsb     rax, rsp, 8
+%endmacro
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 97c7b01d0d..315d0f8670 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -18,6 +18,8 @@
 ;
 ;-------------------------------------------------------------------------------
 
+%include "StuffRsb.inc"
+
 ;
 ; Variables referrenced by C code
 ;
@@ -217,6 +219,7 @@ _SmiHandler:
     wrmsr
 
 .1:
+    StuffRsb64
     rsm
 
 ASM_PFX(gcSmiHandlerSize)    DW      $ - _SmiEntryPoint
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
index 0b0c3f28e5..24357d5870 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
@@ -18,6 +18,8 @@
 ;
 ;-------------------------------------------------------------------------------
 
+%include "StuffRsb.inc"
+
 extern ASM_PFX(SmmInitHandler)
 extern ASM_PFX(mRebasedFlag)
 extern ASM_PFX(mSmmRelocationOriginalAddress)
@@ -101,6 +103,7 @@ ASM_PFX(gPatchSmmInitStack):
     movdqa  xmm4, [rsp + 0x40]
     movdqa  xmm5, [rsp + 0x50]
 
+    StuffRsb64
     rsm
 
 BITS 16
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/StuffRsb.inc b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/StuffRsb.inc
new file mode 100644
index 0000000000..3fd481a8d3
--- /dev/null
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/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)
+;
+;------------------------------------------------------------------------------
+
+%define RSB_STUFF_ENTRIES 0x20
+
+;
+; parameters:
+; @param 1: register to use as counter (e.g. IA32:eax, X64:rax)
+; @param 2: stack pointer to restore   (IA32:esp, X64:rsp)
+; @param 3: the size of a stack frame  (IA32:4, X64:8)
+;
+%macro StuffRsb 3
+      mov     %1, RSB_STUFF_ENTRIES / 2
+  %%Unroll1:
+      call    %%Unroll2
+  %%SpecTrap1:
+      pause
+      lfence
+      jmp     %%SpecTrap1
+  %%Unroll2:
+      call    %%StuffLoop
+  %%SpecTrap2:
+      pause
+      lfence
+      jmp     %%SpecTrap2
+  %%StuffLoop:
+      dec     %1
+      jnz     %%Unroll1
+      add     %2, RSB_STUFF_ENTRIES * %3 ; Restore the stack pointer
+%endmacro
+
+;
+; RSB stuffing macros for IA32 and X64
+;
+%macro StuffRsb32 0
+      StuffRsb     eax, esp, 4
+%endmacro
+
+%macro StuffRsb64 0
+      StuffRsb     rax, rsp, 8
+%endmacro
-- 
2.12.0.windows.1

[PATCH v2 2/2] UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction

[Hao Wu]

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/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm     |  3 ++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 10 ++--
 UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc      | 55 ++++++++++++++++++++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm      |  3 ++
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm  |  8 ++-
 UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc       | 55 ++++++++++++++++++++
 6 files changed, 129 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm
index 057ec6d105..31754734bc 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm
@@ -18,6 +18,8 @@
 ;
 ;-------------------------------------------------------------------------------
 
+%include "StuffRsb.inc"
+
 %define MSR_IA32_MISC_ENABLE 0x1A0
 %define MSR_EFER      0xc0000080
 %define MSR_EFER_XD   0x800
@@ -206,6 +208,7 @@ CommonHandler:
     wrmsr
 
 .7:
+    StuffRsb32
     rsm
 
 
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm
index 93dc3005b7..bc8dbfe20b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm
@@ -1,5 +1,5 @@
 ;------------------------------------------------------------------------------ ;
-; Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2009 - 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
@@ -18,6 +18,8 @@
 ;
 ;-------------------------------------------------------------------------------
 
+%include "StuffRsb.inc"
+
 global  ASM_PFX(gcStmPsd)
 
 extern  ASM_PFX(SmmStmExceptionHandler)
@@ -130,7 +132,8 @@ ASM_PFX(OnStmSetup):
     wrmsr
 
 .71:
-  rsm
+    StuffRsb32
+    rsm
 
 global  ASM_PFX(OnStmTeardown)
 ASM_PFX(OnStmTeardown):
@@ -172,4 +175,5 @@ ASM_PFX(OnStmTeardown):
     wrmsr
 
 .72:
-  rsm
+    StuffRsb32
+    rsm
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc
new file mode 100644
index 0000000000..3fd481a8d3
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/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)
+;
+;------------------------------------------------------------------------------
+
+%define RSB_STUFF_ENTRIES 0x20
+
+;
+; parameters:
+; @param 1: register to use as counter (e.g. IA32:eax, X64:rax)
+; @param 2: stack pointer to restore   (IA32:esp, X64:rsp)
+; @param 3: the size of a stack frame  (IA32:4, X64:8)
+;
+%macro StuffRsb 3
+      mov     %1, RSB_STUFF_ENTRIES / 2
+  %%Unroll1:
+      call    %%Unroll2
+  %%SpecTrap1:
+      pause
+      lfence
+      jmp     %%SpecTrap1
+  %%Unroll2:
+      call    %%StuffLoop
+  %%SpecTrap2:
+      pause
+      lfence
+      jmp     %%SpecTrap2
+  %%StuffLoop:
+      dec     %1
+      jnz     %%Unroll1
+      add     %2, RSB_STUFF_ENTRIES * %3 ; Restore the stack pointer
+%endmacro
+
+;
+; RSB stuffing macros for IA32 and X64
+;
+%macro StuffRsb32 0
+      StuffRsb     eax, esp, 4
+%endmacro
+
+%macro StuffRsb64 0
+      StuffRsb     rax, rsp, 8
+%endmacro
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm
index 90a9fd489b..c0a0f98f11 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm
@@ -18,6 +18,8 @@
 ;
 ;-------------------------------------------------------------------------------
 
+%include "StuffRsb.inc"
+
 ;
 ; Variables referrenced by C code
 ;
@@ -221,6 +223,7 @@ CommonHandler:
     wrmsr
 
 .1:
+    StuffRsb64
     rsm
 
 _StmSmiHandler:
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm
index b0ab87b0d4..3e5295986b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm
@@ -18,6 +18,8 @@
 ;
 ;-------------------------------------------------------------------------------
 
+%include "StuffRsb.inc"
+
 global  ASM_PFX(gcStmPsd)
 
 extern  ASM_PFX(SmmStmExceptionHandler)
@@ -131,7 +133,8 @@ ASM_PFX(OnStmSetup):
     wrmsr
 
 .11:
-  rsm
+    StuffRsb64
+    rsm
 
 global ASM_PFX(OnStmTeardown)
 ASM_PFX(OnStmTeardown):
@@ -175,4 +178,5 @@ ASM_PFX(OnStmTeardown):
     wrmsr
 
 .12:
-  rsm
+    StuffRsb64
+    rsm
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc
new file mode 100644
index 0000000000..3fd481a8d3
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/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)
+;
+;------------------------------------------------------------------------------
+
+%define RSB_STUFF_ENTRIES 0x20
+
+;
+; parameters:
+; @param 1: register to use as counter (e.g. IA32:eax, X64:rax)
+; @param 2: stack pointer to restore   (IA32:esp, X64:rsp)
+; @param 3: the size of a stack frame  (IA32:4, X64:8)
+;
+%macro StuffRsb 3
+      mov     %1, RSB_STUFF_ENTRIES / 2
+  %%Unroll1:
+      call    %%Unroll2
+  %%SpecTrap1:
+      pause
+      lfence
+      jmp     %%SpecTrap1
+  %%Unroll2:
+      call    %%StuffLoop
+  %%SpecTrap2:
+      pause
+      lfence
+      jmp     %%SpecTrap2
+  %%StuffLoop:
+      dec     %1
+      jnz     %%Unroll1
+      add     %2, RSB_STUFF_ENTRIES * %3 ; Restore the stack pointer
+%endmacro
+
+;
+; RSB stuffing macros for IA32 and X64
+;
+%macro StuffRsb32 0
+      StuffRsb     eax, esp, 4
+%endmacro
+
+%macro StuffRsb64 0
+      StuffRsb     rax, rsp, 8
+%endmacro
-- 
2.12.0.windows.1

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

[Laszlo Ersek]

Hello Hao,

On 08/16/18 05:14, Hao Wu wrote:
> V2 changes:
> A. Refine commit log message to clarify the purpose of the series
> 
> B. Extract the RSB stuffing logic to INC files to avoid code duplication:
> When compiling .NASM source files, the current build rule does not support
> including files other than the .NASM file directory, this series will
> duplicate the StuffRsb.inc file together with the .NASM files at this
> moment.
> 
> Please consider this approach as the first stage, I have filed a Bugzilla
> for adding $(INC)-like support when compiling .NASM files:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1085
> 
> After the above support is added, the next step will be taken to remove
> those duplicated StuffRsb.inc files and put it under a common include
> directory like:
> UefiCpuPkg/Include/
> 
> 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>
> 
> Hao Wu (2):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
>   UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction

this looks better, much appreciated.

I've checked the reference from Jiewen, namely
<https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation>.
Related to that, I have a number of questions / requests.

The Intel publication linked above names two CVEs, CVE-2017-5753 and
CVE-2017-5715.

The patches are clearly relevant for CVE-2017-5715 (RSB stuffing before
RSM).

However, I'm unsure if the patches are also relevant for CVE-2017-5753
("LFENCE after validation of untrusted data but before use"). The
patches contain LFENCE instructions, but they don't seem to separate
data validation from data use -- they are in the middle of the SpecTrap
loops. What is their purpose? Are they meant to prevent speculation past
the JMP instructions?

(1) So, my first request is, please add the *exact* CVE number(s) to the
subject lines of the patches. (Even if this makes the subjects a bit too
long.) It is important to see the CVE numbers in a shortlog, such as
"git log --oneline".

(2) The URL of the Intel publication linked above is wrapped in both
commit messages. Please make sure they aren't wrapped. It's OK if they
end up being so long that we would normally not accept them in commit
messages. They are URLs and should be easy to click, or copy&paste.

(3) If we have (hidden) TianoCore BZs for these CVEs, they should be
opened up to the public, and they should be referenced in the commit
messages (in parallel to (1) -- that is, let's state which CVEs are
addressed by the patches, and then name the matching TianoCore BZs as well).

Other than that, the commit messages do a good job at explaining that
these firmware patches protect the retpolines in the *OS*. The article
says the same, but including those sentences in the commit messages is best.

I'll proceed to reviewing and testing the patches.

Thanks
Laszlo

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

[Laszlo Ersek]

On 08/16/18 22:04, Laszlo Ersek wrote:

> (1) [...]
> (2) [...]
> (3) [...]

(4) Please reference
<https://bugzilla.tianocore.org/show_bug.cgi?id=1091> in the commit
messages as well; which is about the unification of the INC files.

Thanks!
Laszlo

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

[Laszlo Ersek]

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/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm     |  3 ++
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 10 ++--
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/StuffRsb.inc      | 55 ++++++++++++++++++++
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm      |  3 ++
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm  |  8 ++-
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/StuffRsb.inc       | 55 ++++++++++++++++++++
>  6 files changed, 129 insertions(+), 5 deletions(-)

I'm going to skip patch #2 (and I defer to the other reviewers on CC)
because this patch seems to affect the "SmmCpuFeaturesLibStm" instance.

And, OVMF uses none of the UefiCpuPkg/Library/SmmCpuFeaturesLib
instances. It uses "OvmfPkg/Library/SmmCpuFeaturesLib", which originates
from the UefiCpuPkg instance that does not support STM.

Thanks!
Laszlo

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

[Laszlo Ersek]

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.

> +
>  %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)".


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,
Laszlo

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

[Wu, Hao A]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 17, 2018 4:05 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Yao, Jiewen; Dong, Eric
> Subject: Re: [edk2] [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM
> instruction
> 
> Hello Hao,
> 
> On 08/16/18 05:14, Hao Wu wrote:
> > V2 changes:
> > A. Refine commit log message to clarify the purpose of the series
> >
> > B. Extract the RSB stuffing logic to INC files to avoid code duplication:
> > When compiling .NASM source files, the current build rule does not support
> > including files other than the .NASM file directory, this series will
> > duplicate the StuffRsb.inc file together with the .NASM files at this
> > moment.
> >
> > Please consider this approach as the first stage, I have filed a Bugzilla
> > for adding $(INC)-like support when compiling .NASM files:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1085
> >
> > After the above support is added, the next step will be taken to remove
> > those duplicated StuffRsb.inc files and put it under a common include
> > directory like:
> > UefiCpuPkg/Include/
> >
> > 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>
> >
> > Hao Wu (2):
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
> >   UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction
> 
> this looks better, much appreciated.
> 
> I've checked the reference from Jiewen, namely
> <https://software.intel.com/security-software-guidance/insights/host-
> firmware-speculative-execution-side-channel-mitigation>.
> Related to that, I have a number of questions / requests.
> 
> The Intel publication linked above names two CVEs, CVE-2017-5753 and
> CVE-2017-5715.
> 
> The patches are clearly relevant for CVE-2017-5715 (RSB stuffing before
> RSM).
> 
> However, I'm unsure if the patches are also relevant for CVE-2017-5753
> ("LFENCE after validation of untrusted data but before use"). The
> patches contain LFENCE instructions, but they don't seem to separate
> data validation from data use -- they are in the middle of the SpecTrap
> loops. What is their purpose? Are they meant to prevent speculation past
> the JMP instructions?

There is a public document for retpoline at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/Retpoline-A-Branch-Target-Injection-Mitigation.pdf

Within section '4.4 Speculation Barriers', I find that:

"
The architectural specification for LFENCE defines that it does not
execute until all prior instructions have completed, and no later
instructions begin execution until LFENCE completes. This specification
limits the speculative execution that a processor implementation can
perform around the LFENCE, ...
"

I think, just as you mentioned, the lfence within the 'trap' is to limit
the speculative execution beyond JMP.

> 
> (1) So, my first request is, please add the *exact* CVE number(s) to the
> subject lines of the patches. (Even if this makes the subjects a bit too
> long.) It is important to see the CVE numbers in a shortlog, such as
> "git log --oneline".

Yes. I will refine the subject of each commits to contain the CVE number.

> 
> (2) The URL of the Intel publication linked above is wrapped in both
> commit messages. Please make sure they aren't wrapped. It's OK if they
> end up being so long that we would normally not accept them in commit
> messages. They are URLs and should be easy to click, or copy&paste.

Yes. I will modify the log message and keep the URL in one line.

> 
> (3) If we have (hidden) TianoCore BZs for these CVEs, they should be
> opened up to the public, and they should be referenced in the commit
> messages (in parallel to (1) -- that is, let's state which CVEs are
> addressed by the patches, and then name the matching TianoCore BZs as well).

It seems I cannot find a BZ for this, I will submit one and update the log
message to reference the BZ.

Best Regards,
Hao Wu

> 
> Other than that, the commit messages do a good job at explaining that
> these firmware patches protect the retpolines in the *OS*. The article
> says the same, but including those sentences in the commit messages is best.
> 
> I'll proceed to reviewing and testing the patches.
> 
> Thanks
> Laszlo

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

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Friday, August 17, 2018 5:09 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Yao, Jiewen; Dong, Eric
> Subject: Re: [edk2] [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM
> instruction
> 
> On 08/16/18 22:04, Laszlo Ersek wrote:
> 
> > (1) [...]
> > (2) [...]
> > (3) [...]
> 
> (4) Please reference
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1091> in the commit
> messages as well; which is about the unification of the INC files.

Yes, I will mention this BZ dependency in the log message.

Best Regards,
Hao Wu

> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Wu, Hao A]

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

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

[Yao, Jiewen]

Laszlo
Good question on CVE.

This patch only handles the Spectre variant 2 - CVE 2017-5715.

There will be separate patch for Spectre variant 1 - CVE 2017-5753. It is under way. :)

Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 17, 2018 4:05 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM
> instruction
> 
> Hello Hao,
> 
> On 08/16/18 05:14, Hao Wu wrote:
> > V2 changes:
> > A. Refine commit log message to clarify the purpose of the series
> >
> > B. Extract the RSB stuffing logic to INC files to avoid code duplication:
> > When compiling .NASM source files, the current build rule does not support
> > including files other than the .NASM file directory, this series will
> > duplicate the StuffRsb.inc file together with the .NASM files at this
> > moment.
> >
> > Please consider this approach as the first stage, I have filed a Bugzilla
> > for adding $(INC)-like support when compiling .NASM files:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1085
> >
> > After the above support is added, the next step will be taken to remove
> > those duplicated StuffRsb.inc files and put it under a common include
> > directory like:
> > UefiCpuPkg/Include/
> >
> > 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>
> >
> > Hao Wu (2):
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
> >   UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM
> instruction
> 
> this looks better, much appreciated.
> 
> I've checked the reference from Jiewen, namely
> <https://software.intel.com/security-software-guidance/insights/host-firmwar
> e-speculative-execution-side-channel-mitigation>.
> Related to that, I have a number of questions / requests.
> 
> The Intel publication linked above names two CVEs, CVE-2017-5753 and
> CVE-2017-5715.
> 
> The patches are clearly relevant for CVE-2017-5715 (RSB stuffing before
> RSM).
> 
> However, I'm unsure if the patches are also relevant for CVE-2017-5753
> ("LFENCE after validation of untrusted data but before use"). The
> patches contain LFENCE instructions, but they don't seem to separate
> data validation from data use -- they are in the middle of the SpecTrap
> loops. What is their purpose? Are they meant to prevent speculation past
> the JMP instructions?
> 
> (1) So, my first request is, please add the *exact* CVE number(s) to the
> subject lines of the patches. (Even if this makes the subjects a bit too
> long.) It is important to see the CVE numbers in a shortlog, such as
> "git log --oneline".
> 
> (2) The URL of the Intel publication linked above is wrapped in both
> commit messages. Please make sure they aren't wrapped. It's OK if they
> end up being so long that we would normally not accept them in commit
> messages. They are URLs and should be easy to click, or copy&paste.
> 
> (3) If we have (hidden) TianoCore BZs for these CVEs, they should be
> opened up to the public, and they should be referenced in the commit
> messages (in parallel to (1) -- that is, let's state which CVEs are
> addressed by the patches, and then name the matching TianoCore BZs as well).
> 
> Other than that, the commit messages do a good job at explaining that
> these firmware patches protect the retpolines in the *OS*. The article
> says the same, but including those sentences in the commit messages is best.
> 
> I'll proceed to reviewing and testing the patches.
> 
> Thanks
> Laszlo