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

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

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: 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