* [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
2018-08-16 3:14 [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction Hao Wu
@ 2018-08-16 3:14 ` Hao Wu
2018-08-16 23:01 ` Laszlo Ersek
2018-08-16 3:14 ` [PATCH v2 2/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu
2018-08-16 20:04 ` [PATCH v2 0/2] UefiCpuPkg: " Laszlo Ersek
2 siblings, 1 reply; 11+ messages in thread
From: Hao Wu @ 2018-08-16 3:14 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Eric Dong, Laszlo Ersek, Michael D Kinney
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
2018-08-16 3:14 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Hao Wu
@ 2018-08-16 23:01 ` Laszlo Ersek
2018-08-17 1:39 ` Wu, Hao A
0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2018-08-16 23:01 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Jiewen Yao, Eric Dong, Michael D Kinney
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before RSM instruction
2018-08-16 23:01 ` Laszlo Ersek
@ 2018-08-17 1:39 ` Wu, Hao A
0 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2018-08-17 1:39 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric, Kinney, Michael D
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction
2018-08-16 3:14 [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction Hao Wu
2018-08-16 3:14 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Hao Wu
@ 2018-08-16 3:14 ` Hao Wu
2018-08-16 21:33 ` Laszlo Ersek
2018-08-16 20:04 ` [PATCH v2 0/2] UefiCpuPkg: " Laszlo Ersek
2 siblings, 1 reply; 11+ messages in thread
From: Hao Wu @ 2018-08-16 3:14 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Eric Dong, Laszlo Ersek, Michael D Kinney
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before RSM instruction
2018-08-16 3:14 ` [PATCH v2 2/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu
@ 2018-08-16 21:33 ` Laszlo Ersek
0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2018-08-16 21:33 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Jiewen Yao, Eric Dong, Michael D Kinney
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction
2018-08-16 3:14 [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction Hao Wu
2018-08-16 3:14 ` [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Hao Wu
2018-08-16 3:14 ` [PATCH v2 2/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu
@ 2018-08-16 20:04 ` Laszlo Ersek
2018-08-16 21:08 ` Laszlo Ersek
` (2 more replies)
2 siblings, 3 replies; 11+ messages in thread
From: Laszlo Ersek @ 2018-08-16 20:04 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Eric Dong
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction
2018-08-16 20:04 ` [PATCH v2 0/2] UefiCpuPkg: " Laszlo Ersek
@ 2018-08-16 21:08 ` Laszlo Ersek
2018-08-17 1:20 ` Wu, Hao A
2018-08-17 1:18 ` Wu, Hao A
2018-08-17 2:41 ` Yao, Jiewen
2 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2018-08-16 21:08 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Eric Dong
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction
2018-08-16 21:08 ` Laszlo Ersek
@ 2018-08-17 1:20 ` Wu, Hao A
0 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2018-08-17 1:20 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Yao, Jiewen, Dong, Eric
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction
2018-08-16 20:04 ` [PATCH v2 0/2] UefiCpuPkg: " Laszlo Ersek
2018-08-16 21:08 ` Laszlo Ersek
@ 2018-08-17 1:18 ` Wu, Hao A
2018-08-17 2:41 ` Yao, Jiewen
2 siblings, 0 replies; 11+ messages in thread
From: Wu, Hao A @ 2018-08-17 1:18 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Yao, Jiewen, Dong, Eric
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 0/2] UefiCpuPkg: Add RSB stuffing before RSM instruction
2018-08-16 20:04 ` [PATCH v2 0/2] UefiCpuPkg: " Laszlo Ersek
2018-08-16 21:08 ` Laszlo Ersek
2018-08-17 1:18 ` Wu, Hao A
@ 2018-08-17 2:41 ` Yao, Jiewen
2 siblings, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2018-08-17 2:41 UTC (permalink / raw)
To: Laszlo Ersek, Wu, Hao A, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Dong, Eric
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
^ permalink raw reply [flat|nested] 11+ messages in thread