* [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction
@ 2018-08-10 1:43 Hao Wu
2018-08-10 1:43 ` [PATCH 1/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Hao Wu @ 2018-08-10 1:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Eric Dong, Laszlo Ersek
The series will add RSB stuffing logics to avoid RSB underflow on return
from SMM (rsm instruction).
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Hao Wu (2):
UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before rsm instruction
UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before rsm instruction
UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm | 20 +++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 44 ++++++++++++++++++--
UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm | 20 +++++++++
UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm | 42 ++++++++++++++++++-
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 20 +++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 21 ++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 20 +++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 20 +++++++++
8 files changed, 202 insertions(+), 5 deletions(-)
--
2.12.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before rsm instruction 2018-08-10 1:43 [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction Hao Wu @ 2018-08-10 1:43 ` Hao Wu 2018-08-10 1:43 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Hao Wu 2018-08-10 15:06 ` [PATCH 0/2] UefiCpuPkg: " Laszlo Ersek 2 siblings, 0 replies; 8+ messages in thread From: Hao Wu @ 2018-08-10 1:43 UTC (permalink / raw) To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Eric Dong, Laszlo Ersek System Management Interrupt (SMI) handlers can leave the Return Stack Buffer (RSB) in a state that application program or operating-system does not expect. In order to avoid RSB underflow on return from SMI, this commit will add RSB stuffing logic before instruction 'rsm'. After the stuffing, RSB entries will contain a trap like: @SpecTrap: pause lfence jmp @SpecTrap to keep the speculative execution within control. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Hao Wu <hao.a.wu@intel.com> --- UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm | 20 +++++++++ UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 44 ++++++++++++++++++-- UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm | 20 +++++++++ UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm | 42 ++++++++++++++++++- 4 files changed, 121 insertions(+), 5 deletions(-) diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm index 057ec6d105..c14a5b2789 100644 --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm @@ -37,6 +37,8 @@ %define PROTECT_MODE_DS 0x20 %define TSS_SEGMENT 0x40 +%define RSB_STUFF_ENTRIES 0x20 + extern ASM_PFX(SmiRendezvous) extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard)) extern ASM_PFX(CpuSmmDebugEntry) @@ -206,6 +208,24 @@ CommonHandler: wrmsr .7: + mov eax, RSB_STUFF_ENTRIES / 2 +@Unroll1: + call @Unroll2 +@SpecTrap1: + pause + lfence + jmp @SpecTrap1 +@Unroll2: + call @StuffLoop +@SpecTrap2: + pause + lfence + jmp @SpecTrap2 +@StuffLoop: + dec eax + jnz @Unroll1 + add esp, RSB_STUFF_ENTRIES * 4 ; Restore the stack pointer + rsm diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm index 93dc3005b7..410af08ac3 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 @@ -30,6 +30,8 @@ extern ASM_PFX(gStmSmiHandlerIdtr) %define MSR_EFER 0xc0000080 %define MSR_EFER_XD 0x800 +%define RSB_STUFF_ENTRIES 0x20 + CODE_SEL equ 0x08 DATA_SEL equ 0x20 TSS_SEL equ 0x40 @@ -130,7 +132,25 @@ ASM_PFX(OnStmSetup): wrmsr .71: - rsm + mov eax, RSB_STUFF_ENTRIES / 2 +@Unroll1_1: + call @Unroll2_1 +@SpecTrap1_1: + pause + lfence + jmp @SpecTrap1_1 +@Unroll2_1: + call @StuffLoop_1 +@SpecTrap2_1: + pause + lfence + jmp @SpecTrap2_1 +@StuffLoop_1: + dec eax + jnz @Unroll1_1 + add esp, RSB_STUFF_ENTRIES * 4 ; Restore the stack pointer + + rsm global ASM_PFX(OnStmTeardown) ASM_PFX(OnStmTeardown): @@ -172,4 +192,22 @@ ASM_PFX(OnStmTeardown): wrmsr .72: - rsm + mov eax, RSB_STUFF_ENTRIES / 2 +@Unroll1_2: + call @Unroll2_2 +@SpecTrap1_2: + pause + lfence + jmp @SpecTrap1_2 +@Unroll2_2: + call @StuffLoop_2 +@SpecTrap2_2: + pause + lfence + jmp @SpecTrap2_2 +@StuffLoop_2: + dec eax + jnz @Unroll1_2 + add esp, RSB_STUFF_ENTRIES * 4 ; Restore the stack pointer + + rsm diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm index 90a9fd489b..0ac2318287 100644 --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm @@ -48,6 +48,8 @@ %define TSS_SEGMENT 0x40 %define GDT_SIZE 0x50 +%define RSB_STUFF_ENTRIES 0x20 + extern ASM_PFX(SmiRendezvous) extern ASM_PFX(gStmSmiHandlerIdtr) extern ASM_PFX(CpuSmmDebugEntry) @@ -221,6 +223,24 @@ CommonHandler: wrmsr .1: + mov rax, RSB_STUFF_ENTRIES / 2 +@Unroll1: + call @Unroll2 +@SpecTrap1: + pause + lfence + jmp @SpecTrap1 +@Unroll2: + call @StuffLoop +@SpecTrap2: + pause + lfence + jmp @SpecTrap2 +@StuffLoop: + dec rax + jnz @Unroll1 + add rsp, RSB_STUFF_ENTRIES * 8 ; Restore the stack pointer + rsm _StmSmiHandler: diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm index b0ab87b0d4..53230ecf30 100644 --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm @@ -30,6 +30,8 @@ extern ASM_PFX(gStmSmiHandlerIdtr) %define MSR_EFER 0xc0000080 %define MSR_EFER_XD 0x800 +%define RSB_STUFF_ENTRIES 0x20 + CODE_SEL equ 0x38 DATA_SEL equ 0x20 TR_SEL equ 0x40 @@ -131,7 +133,25 @@ ASM_PFX(OnStmSetup): wrmsr .11: - rsm + mov rax, RSB_STUFF_ENTRIES / 2 +@Unroll1_1: + call @Unroll2_1 +@SpecTrap1_1: + pause + lfence + jmp @SpecTrap1_1 +@Unroll2_1: + call @StuffLoop_1 +@SpecTrap2_1: + pause + lfence + jmp @SpecTrap2_1 +@StuffLoop_1: + dec rax + jnz @Unroll1_1 + add rsp, RSB_STUFF_ENTRIES * 8 ; Restore the stack pointer + + rsm global ASM_PFX(OnStmTeardown) ASM_PFX(OnStmTeardown): @@ -175,4 +195,22 @@ ASM_PFX(OnStmTeardown): wrmsr .12: - rsm + mov rax, RSB_STUFF_ENTRIES / 2 +@Unroll1_2: + call @Unroll2_2 +@SpecTrap1_2: + pause + lfence + jmp @SpecTrap1_2 +@Unroll2_2: + call @StuffLoop_2 +@SpecTrap2_2: + pause + lfence + jmp @SpecTrap2_2 +@StuffLoop_2: + dec rax + jnz @Unroll1_2 + add rsp, RSB_STUFF_ENTRIES * 8 ; Restore the stack pointer + + rsm -- 2.12.0.windows.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before rsm instruction 2018-08-10 1:43 [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction Hao Wu 2018-08-10 1:43 ` [PATCH 1/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu @ 2018-08-10 1:43 ` Hao Wu 2018-08-10 15:06 ` [PATCH 0/2] UefiCpuPkg: " Laszlo Ersek 2 siblings, 0 replies; 8+ messages in thread From: Hao Wu @ 2018-08-10 1:43 UTC (permalink / raw) To: edk2-devel; +Cc: Hao Wu, Jiewen Yao, Eric Dong, Laszlo Ersek System Management Interrupt (SMI) handlers can leave the Return Stack Buffer (RSB) in a state that application program or operating-system does not expect. In order to avoid RSB underflow on return from SMI, this commit will add RSB stuffing logic before instruction 'rsm'. After the stuffing, RSB entries will contain a trap like: @SpecTrap: pause lfence jmp @SpecTrap to keep the speculative execution within control. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Hao Wu <hao.a.wu@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 20 +++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 21 ++++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 20 +++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 20 +++++++++++++++++++ 4 files changed, 81 insertions(+) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm index 509e7a0a66..e5875353a1 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm @@ -37,6 +37,8 @@ %define PROTECT_MODE_DS 0x20 %define TSS_SEGMENT 0x40 +%define RSB_STUFF_ENTRIES 0x20 + extern ASM_PFX(SmiRendezvous) extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard)) extern ASM_PFX(CpuSmmDebugEntry) @@ -204,6 +206,24 @@ ASM_PFX(SmiHandler): wrmsr .7: + mov eax, RSB_STUFF_ENTRIES / 2 +@Unroll1: + call @Unroll2 +@SpecTrap1: + pause + lfence + jmp @SpecTrap1 +@Unroll2: + call @StuffLoop +@SpecTrap2: + pause + lfence + jmp @SpecTrap2 +@StuffLoop: + dec eax + jnz @Unroll1 + add esp, RSB_STUFF_ENTRIES * 4 ; Restore the stack pointer + rsm ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm index 5ff3cd2e73..fd559d25cd 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm @@ -33,6 +33,8 @@ global ASM_PFX(gcSmmInitTemplate) %define PROTECT_MODE_CS 0x8 %define PROTECT_MODE_DS 0x20 +%define RSB_STUFF_ENTRIES 0x20 + SECTION .text ASM_PFX(gcSmiInitGdtr): @@ -75,6 +77,25 @@ BITS 32 mov esp, strict dword 0 ; source operand will be patched ASM_PFX(gPatchSmmInitStack): call ASM_PFX(SmmInitHandler) + + mov eax, RSB_STUFF_ENTRIES / 2 +@Unroll1: + call @Unroll2 +@SpecTrap1: + pause + lfence + jmp @SpecTrap1 +@Unroll2: + call @StuffLoop +@SpecTrap2: + pause + lfence + jmp @SpecTrap2 +@StuffLoop: + dec eax + jnz @Unroll1 + add esp, RSB_STUFF_ENTRIES * 4 ; Restore the stack pointer + rsm BITS 16 diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm index 97c7b01d0d..b955fa1cf1 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm @@ -48,6 +48,8 @@ %define TSS_SEGMENT 0x40 %define GDT_SIZE 0x50 +%define RSB_STUFF_ENTRIES 0x20 + extern ASM_PFX(SmiRendezvous) extern ASM_PFX(gSmiHandlerIdtr) extern ASM_PFX(CpuSmmDebugEntry) @@ -217,6 +219,24 @@ _SmiHandler: wrmsr .1: + mov rax, RSB_STUFF_ENTRIES / 2 +@Unroll1: + call @Unroll2 +@SpecTrap1: + pause + lfence + jmp @SpecTrap1 +@Unroll2: + call @StuffLoop +@SpecTrap2: + pause + lfence + jmp @SpecTrap2 +@StuffLoop: + dec rax + jnz @Unroll1 + add rsp, RSB_STUFF_ENTRIES * 8 ; Restore the stack pointer + rsm ASM_PFX(gcSmiHandlerSize) DW $ - _SmiEntryPoint diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm index 0b0c3f28e5..bff14e809b 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm @@ -34,6 +34,8 @@ global ASM_PFX(gPatchSmmRelocationOriginalAddressPtr32) %define LONG_MODE_CS 0x38 +%define RSB_STUFF_ENTRIES 0x20 + DEFAULT REL SECTION .text @@ -101,6 +103,24 @@ ASM_PFX(gPatchSmmInitStack): movdqa xmm4, [rsp + 0x40] movdqa xmm5, [rsp + 0x50] + mov rax, RSB_STUFF_ENTRIES / 2 +@Unroll1: + call @Unroll2 +@SpecTrap1: + pause + lfence + jmp @SpecTrap1 +@Unroll2: + call @StuffLoop +@SpecTrap2: + pause + lfence + jmp @SpecTrap2 +@StuffLoop: + dec rax + jnz @Unroll1 + add rsp, RSB_STUFF_ENTRIES * 8 ; Restore the stack pointer + rsm BITS 16 -- 2.12.0.windows.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction 2018-08-10 1:43 [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction Hao Wu 2018-08-10 1:43 ` [PATCH 1/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu 2018-08-10 1:43 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Hao Wu @ 2018-08-10 15:06 ` Laszlo Ersek 2018-08-16 0:25 ` Yao, Jiewen 2018-08-16 3:07 ` Wu, Hao A 2 siblings, 2 replies; 8+ messages in thread From: Laszlo Ersek @ 2018-08-10 15:06 UTC (permalink / raw) To: Hao Wu, edk2-devel; +Cc: Jiewen Yao, Eric Dong On 08/10/18 03:43, Hao Wu wrote: > The series will add RSB stuffing logics to avoid RSB underflow on return > from SMM (rsm instruction). > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > > Hao Wu (2): > UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before rsm instruction > UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before rsm instruction > > UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm | 20 +++++++++ > UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 44 ++++++++++++++++++-- > UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm | 20 +++++++++ > UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm | 42 ++++++++++++++++++- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 20 +++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 21 ++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 20 +++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 20 +++++++++ > 8 files changed, 202 insertions(+), 5 deletions(-) > I haven't tested this patch set yet; first I'd like to make some comments: (1) I think the commit messages are very lacking. Please explain *precisely* why the Return Stack Buffer has to be stuffed before RSM. (1a) To my understanding, speculation is micro-architectural (and not architectural) state, therefore it makes no sense to say that "RSB is left in a state that application program or operating-system does not expect". Applications and operating systems can only have expectations for architectural state, and not for micro-architectural state. (1b) Furthermore, to my understanding, speculation can be abused by training the predictor in a non-privileged context, then calling into a higher privilege level, where the previous (unprivileged) training will lead to the speculative execution of privileged code, for example bypassing range checks. In turn, the result of those (invalid and speculative) privileged operations can be sniffed from micro-architectural state, such as timing memory accesses (to see whether something has been cached or not by the speculative privileged execution). Is this correct more or less? If so, then why are we stuffing the RSB just before we *leave* the privileged mode (=SMM) for the less privileged mode (=ring 0, IIUC)? Shouldn't we kill the "external training" of the predictor right after we *enter* SMM? (1c) Or, perhaps, in this kind of attack, the RSB is not used for triggering speculative execution in the more privileged mode, but to *leak* information from the more privileged mode to the less privileged mode. IOW, the RSB is what is used by the attacker as the "read end" of the side-channel; perhaps by timing returns (in non-privileged code) that reflect the training that the predictor picked up while in SMM. Now, if that's the case, then the current commit messages are even more confusing; they should state, "System Management Interrupt (SMI) handlers can leave the Return Stack Buffer (RSB) in a state that leaks information to malicious code that runs with lesser privileges". Because, the point is not whether the OS or the app find the state "unexpected" (a benign OS or app won't care at all); the point is that a malicious OS or app will *definitely* expect some leaked information, and we must prevent that. I imagine that I'm pretty confused about this. Please document the exact threat that the RSB stuffing is supposed to mitigate. I know I can find long articles and blogs about this. The commit messages should nonetheless provide a good concise summary. (2) If I understand correctly, the same pattern is used everywhere -- a loop body is executed 32 times, and in the loop body, we jump (via subroutine calls) twice, and each call is followed by a "trap" for speculative execution. At the end of the loop, we forcefully unwind the stack, and then we proceed to RSM. I think this should be implemented with a lot less code duplication. NASM supports macros with labels that are local to macro *invocation* (not macro *definition*); please see the %%skip example here: https://www.nasm.us/doc/nasmdoc4.html 4.3.2 Macro-Local Labels In addition, it should be possible to pass parameters to macros, such as: - the register to use as counter (eax vs. rax), - the stack pointer to restore (esp vs. rsp), - the size of a stack frame (4 vs. 8) Using all those tools, it should be possible to define the macro only once, in a UefiCpuPkg-level ".inc" file (for example, "UefiCpuPkg/Include/StuffRsb.inc"), and then only invoke the macro near all 10 RSM instructions: ------------- %define RSB_STUFF_ENTRIES 0x20 ; @param 1: register to use as counter (eax vs. rax) ; @param 2: stack pointer to restore (esp vs. rsp) ; @param 3: the size of a stack frame (4 vs. 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 %define StuffRsb32 StuffRsb (eax, esp, 4) %define StuffRsb64 StuffRsb (rax, rsp, 8) ------------- Thanks, Laszlo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction 2018-08-10 15:06 ` [PATCH 0/2] UefiCpuPkg: " Laszlo Ersek @ 2018-08-16 0:25 ` Yao, Jiewen 2018-08-16 3:07 ` Wu, Hao A 1 sibling, 0 replies; 8+ messages in thread From: Yao, Jiewen @ 2018-08-16 0:25 UTC (permalink / raw) To: Laszlo Ersek, Wu, Hao A, edk2-devel@lists.01.org; +Cc: Dong, Eric Thank you Laszlo, for your feedback. The public document is at https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation the "Branch target injection mitigation" section I agree with you that we should add those info in the commit message. Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Friday, August 10, 2018 11:06 PM > To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: Re: [edk2] [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm > instruction > > On 08/10/18 03:43, Hao Wu wrote: > > The series will add RSB stuffing logics to avoid RSB underflow on return > > from SMM (rsm instruction). > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > > > Hao Wu (2): > > UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before rsm instruction > > UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before rsm instruction > > > > UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm | 20 > +++++++++ > > UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 44 > ++++++++++++++++++-- > > UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm | 20 > +++++++++ > > UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm | 42 > ++++++++++++++++++- > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | > 20 +++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | > 21 ++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | > 20 +++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | > 20 +++++++++ > > 8 files changed, 202 insertions(+), 5 deletions(-) > > > > I haven't tested this patch set yet; first I'd like to make some comments: > > (1) I think the commit messages are very lacking. Please explain > *precisely* why the Return Stack Buffer has to be stuffed before RSM. > > (1a) To my understanding, speculation is micro-architectural (and not > architectural) state, therefore it makes no sense to say that "RSB is > left in a state that application program or operating-system does > not expect". Applications and operating systems can only have > expectations for architectural state, and not for micro-architectural state. > > (1b) Furthermore, to my understanding, speculation can be abused by > training the predictor in a non-privileged context, then calling into a > higher privilege level, where the previous (unprivileged) training will > lead to the speculative execution of privileged code, for example > bypassing range checks. In turn, the result of those (invalid and > speculative) privileged operations can be sniffed from > micro-architectural state, such as timing memory accesses (to see > whether something has been cached or not by the speculative privileged > execution). > > Is this correct more or less? If so, then why are we stuffing the RSB > just before we *leave* the privileged mode (=SMM) for the less > privileged mode (=ring 0, IIUC)? Shouldn't we kill the "external > training" of the predictor right after we *enter* SMM? > > (1c) Or, perhaps, in this kind of attack, the RSB is not used for > triggering speculative execution in the more privileged mode, but to > *leak* information from the more privileged mode to the less privileged > mode. IOW, the RSB is what is used by the attacker as the "read end" of > the side-channel; perhaps by timing returns (in non-privileged code) > that reflect the training that the predictor picked up while in SMM. > > Now, if that's the case, then the current commit messages are even more > confusing; they should state, "System Management Interrupt (SMI) > handlers can leave the Return Stack Buffer (RSB) in a state that leaks > information to malicious code that runs with lesser privileges". > Because, the point is not whether the OS or the app find the state > "unexpected" (a benign OS or app won't care at all); the point is that a > malicious OS or app will *definitely* expect some leaked information, > and we must prevent that. > > > I imagine that I'm pretty confused about this. Please document the exact > threat that the RSB stuffing is supposed to mitigate. I know I can find > long articles and blogs about this. The commit messages should > nonetheless provide a good concise summary. > > > (2) If I understand correctly, the same pattern is used everywhere -- a > loop body is executed 32 times, and in the loop body, we jump (via > subroutine calls) twice, and each call is followed by a "trap" for > speculative execution. At the end of the loop, we forcefully unwind the > stack, and then we proceed to RSM. > > I think this should be implemented with a lot less code duplication. > NASM supports macros with labels that are local to macro *invocation* > (not macro *definition*); please see the %%skip example here: > > https://www.nasm.us/doc/nasmdoc4.html > 4.3.2 Macro-Local Labels > > In addition, it should be possible to pass parameters to macros, such as: > - the register to use as counter (eax vs. rax), > - the stack pointer to restore (esp vs. rsp), > - the size of a stack frame (4 vs. 8) > > Using all those tools, it should be possible to define the macro only > once, in a UefiCpuPkg-level ".inc" file (for example, > "UefiCpuPkg/Include/StuffRsb.inc"), and then only invoke the macro near > all 10 RSM instructions: > > ------------- > %define RSB_STUFF_ENTRIES 0x20 > > ; @param 1: register to use as counter (eax vs. rax) > ; @param 2: stack pointer to restore (esp vs. rsp) > ; @param 3: the size of a stack frame (4 vs. 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 > > %define StuffRsb32 StuffRsb (eax, esp, 4) > %define StuffRsb64 StuffRsb (rax, rsp, 8) > ------------- > > Thanks, > Laszlo > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction 2018-08-10 15:06 ` [PATCH 0/2] UefiCpuPkg: " Laszlo Ersek 2018-08-16 0:25 ` Yao, Jiewen @ 2018-08-16 3:07 ` Wu, Hao A 2018-08-16 12:33 ` Laszlo Ersek 2018-08-16 12:46 ` Laszlo Ersek 1 sibling, 2 replies; 8+ messages in thread From: Wu, Hao A @ 2018-08-16 3:07 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Friday, August 10, 2018 11:06 PM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Yao, Jiewen; Dong, Eric > Subject: Re: [edk2] [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm > instruction > > On 08/10/18 03:43, Hao Wu wrote: > > The series will add RSB stuffing logics to avoid RSB underflow on return > > from SMM (rsm instruction). > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > > > Hao Wu (2): > > UefiCpuPkg/SmmCpuFeaturesLib: Add RSB stuffing before rsm instruction > > UefiCpuPkg/PiSmmCpuDxeSmm: Add RSB stuffing before rsm instruction > > > > UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm | 20 > +++++++++ > > UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiException.nasm | 44 > ++++++++++++++++++-- > > UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm | 20 > +++++++++ > > UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiException.nasm | 42 > ++++++++++++++++++- > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 20 > +++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 21 > ++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 20 > +++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 20 > +++++++++ > > 8 files changed, 202 insertions(+), 5 deletions(-) > > > > I haven't tested this patch set yet; first I'd like to make some comments: > > (1) I think the commit messages are very lacking. Please explain > *precisely* why the Return Stack Buffer has to be stuffed before RSM. > > (1a) To my understanding, speculation is micro-architectural (and not > architectural) state, therefore it makes no sense to say that "RSB is > left in a state that application program or operating-system does > not expect". Applications and operating systems can only have > expectations for architectural state, and not for micro-architectural state. > > (1b) Furthermore, to my understanding, speculation can be abused by > training the predictor in a non-privileged context, then calling into a > higher privilege level, where the previous (unprivileged) training will > lead to the speculative execution of privileged code, for example > bypassing range checks. In turn, the result of those (invalid and > speculative) privileged operations can be sniffed from > micro-architectural state, such as timing memory accesses (to see > whether something has been cached or not by the speculative privileged > execution). > > Is this correct more or less? If so, then why are we stuffing the RSB > just before we *leave* the privileged mode (=SMM) for the less > privileged mode (=ring 0, IIUC)? Shouldn't we kill the "external > training" of the predictor right after we *enter* SMM? > > (1c) Or, perhaps, in this kind of attack, the RSB is not used for > triggering speculative execution in the more privileged mode, but to > *leak* information from the more privileged mode to the less privileged > mode. IOW, the RSB is what is used by the attacker as the "read end" of > the side-channel; perhaps by timing returns (in non-privileged code) > that reflect the training that the predictor picked up while in SMM. > > Now, if that's the case, then the current commit messages are even more > confusing; they should state, "System Management Interrupt (SMI) > handlers can leave the Return Stack Buffer (RSB) in a state that leaks > information to malicious code that runs with lesser privileges". > Because, the point is not whether the OS or the app find the state > "unexpected" (a benign OS or app won't care at all); the point is that a > malicious OS or app will *definitely* expect some leaked information, > and we must prevent that. > > > I imagine that I'm pretty confused about this. Please document the exact > threat that the RSB stuffing is supposed to mitigate. I know I can find > long articles and blogs about this. The commit messages should > nonetheless provide a good concise summary. Thanks Laszlo, I will update the commit log message to better clarify the purpose of the series. > > > (2) If I understand correctly, the same pattern is used everywhere -- a > loop body is executed 32 times, and in the loop body, we jump (via > subroutine calls) twice, and each call is followed by a "trap" for > speculative execution. At the end of the loop, we forcefully unwind the > stack, and then we proceed to RSM. > > I think this should be implemented with a lot less code duplication. > NASM supports macros with labels that are local to macro *invocation* > (not macro *definition*); please see the %%skip example here: > > https://www.nasm.us/doc/nasmdoc4.html > 4.3.2 Macro-Local Labels > > In addition, it should be possible to pass parameters to macros, such as: > - the register to use as counter (eax vs. rax), > - the stack pointer to restore (esp vs. rsp), > - the size of a stack frame (4 vs. 8) > > Using all those tools, it should be possible to define the macro only > once, in a UefiCpuPkg-level ".inc" file (for example, > "UefiCpuPkg/Include/StuffRsb.inc"), and then only invoke the macro near > all 10 RSM instructions: Yes. Extracting the common logic to a INC file is a good idea. However, I found that when compiling .NASM files, the current build rule does not support including files other than the .NASM file directory. So including a package-level INC file is not supported at this moment. I have filed a Bugzilla for adding $(INC)-like support when compiling .NASM files: https://bugzilla.tianocore.org/show_bug.cgi?id=1085 After some discussion with the BaseTools owners, some investigation is needed for the above support. Hence, I plan to perform a 2-stage change when extracting the common RSB stuffing logics to INC file: 1. Duplicate the INC file and place them together with the NASM files that uses the RSB stuffing logics. 2. After NASM compiling support the $(INC)-like feature, propose another patch to remove those duplicated INC files and create one under UefiCpuPkg/Include/. Please help to share your thought on this. Thanks in advance. Best Regards, Hao Wu > > ------------- > %define RSB_STUFF_ENTRIES 0x20 > > ; @param 1: register to use as counter (eax vs. rax) > ; @param 2: stack pointer to restore (esp vs. rsp) > ; @param 3: the size of a stack frame (4 vs. 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 > > %define StuffRsb32 StuffRsb (eax, esp, 4) > %define StuffRsb64 StuffRsb (rax, rsp, 8) > ------------- > > Thanks, > Laszlo > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction 2018-08-16 3:07 ` Wu, Hao A @ 2018-08-16 12:33 ` Laszlo Ersek 2018-08-16 12:46 ` Laszlo Ersek 1 sibling, 0 replies; 8+ messages in thread From: Laszlo Ersek @ 2018-08-16 12:33 UTC (permalink / raw) To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric On 08/16/18 05:07, Wu, Hao A wrote: >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo >> Ersek >> Sent: Friday, August 10, 2018 11:06 PM >> (2) If I understand correctly, the same pattern is used everywhere -- a >> loop body is executed 32 times, and in the loop body, we jump (via >> subroutine calls) twice, and each call is followed by a "trap" for >> speculative execution. At the end of the loop, we forcefully unwind the >> stack, and then we proceed to RSM. >> >> I think this should be implemented with a lot less code duplication. >> NASM supports macros with labels that are local to macro *invocation* >> (not macro *definition*); please see the %%skip example here: >> >> https://www.nasm.us/doc/nasmdoc4.html >> 4.3.2 Macro-Local Labels >> >> In addition, it should be possible to pass parameters to macros, such as: >> - the register to use as counter (eax vs. rax), >> - the stack pointer to restore (esp vs. rsp), >> - the size of a stack frame (4 vs. 8) >> >> Using all those tools, it should be possible to define the macro only >> once, in a UefiCpuPkg-level ".inc" file (for example, >> "UefiCpuPkg/Include/StuffRsb.inc"), and then only invoke the macro near >> all 10 RSM instructions: > > Yes. Extracting the common logic to a INC file is a good idea. > > However, I found that when compiling .NASM files, the current build rule > does not support including files other than the .NASM file directory. > So including a package-level INC file is not supported at this moment. > > I have filed a Bugzilla for adding $(INC)-like support when compiling > .NASM files: > > https://bugzilla.tianocore.org/show_bug.cgi?id=1085 > > After some discussion with the BaseTools owners, some investigation is > needed for the above support. Hence, I plan to perform a 2-stage change > when extracting the common RSB stuffing logics to INC file: > > 1. Duplicate the INC file and place them together with the NASM files that > uses the RSB stuffing logics. > > 2. After NASM compiling support the $(INC)-like feature, propose another > patch to remove those duplicated INC files and create one under > UefiCpuPkg/Include/. > > Please help to share your thought on this. Thanks in advance. Sounds good to me. This approach still eliminates as much code duplication as it is possible at the current level of BaseTools support. Also we'll have a reminder (a BZ) for completing the unification. Thanks! Laszlo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction 2018-08-16 3:07 ` Wu, Hao A 2018-08-16 12:33 ` Laszlo Ersek @ 2018-08-16 12:46 ` Laszlo Ersek 1 sibling, 0 replies; 8+ messages in thread From: Laszlo Ersek @ 2018-08-16 12:46 UTC (permalink / raw) To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric On 08/16/18 05:07, Wu, Hao A wrote: > I have filed a Bugzilla for adding $(INC)-like support when compiling > .NASM files: > > https://bugzilla.tianocore.org/show_bug.cgi?id=1085 > > After some discussion with the BaseTools owners, some investigation is > needed for the above support. Hence, I plan to perform a 2-stage change > when extracting the common RSB stuffing logics to INC file: [...] I've also filed <https://bugzilla.tianocore.org/show_bug.cgi?id=1091> now (and assigned it to you, if that's OK with you). BZ#1085 should track the BaseTools update, and then BZ#1091 should take advantage of it in UefiCpuPkg, for unifying the .inc files. I've also set up the dependency between the BZs accordingly. Thanks! Laszlo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-16 12:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-10 1:43 [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction Hao Wu 2018-08-10 1:43 ` [PATCH 1/2] UefiCpuPkg/SmmCpuFeaturesLib: " Hao Wu 2018-08-10 1:43 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: " Hao Wu 2018-08-10 15:06 ` [PATCH 0/2] UefiCpuPkg: " Laszlo Ersek 2018-08-16 0:25 ` Yao, Jiewen 2018-08-16 3:07 ` Wu, Hao A 2018-08-16 12:33 ` Laszlo Ersek 2018-08-16 12:46 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox