From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DCEC6210DBE8A for ; Fri, 10 Aug 2018 08:06:17 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C611440241CB; Fri, 10 Aug 2018 15:06:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-207.rdu2.redhat.com [10.10.120.207]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1BE0B2026D65; Fri, 10 Aug 2018 15:06:13 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Jiewen Yao , Eric Dong References: <20180810014348.32036-1-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <140b21ca-6bdf-af12-4f3f-d403ecd2f69e@redhat.com> Date: Fri, 10 Aug 2018 17:06:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180810014348.32036-1-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 10 Aug 2018 15:06:16 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 10 Aug 2018 15:06:16 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Aug 2018 15:06:19 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > Cc: Laszlo Ersek > > 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