From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (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 E85ED21BADAB2 for ; Wed, 15 Aug 2018 20:08:09 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Aug 2018 20:08:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,245,1531810800"; d="scan'208";a="63505987" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga008.fm.intel.com with ESMTP; 15 Aug 2018 20:08:03 -0700 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 15 Aug 2018 20:08:03 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 15 Aug 2018 20:08:02 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.143]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.205]) with mapi id 14.03.0319.002; Thu, 16 Aug 2018 11:08:00 +0800 From: "Wu, Hao A" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Dong, Eric" Thread-Topic: [edk2] [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction Thread-Index: AQHUMEuhK/+WpTU2PU2b//F3eSWBSKS4kKYAgAkofjA= Date: Thu, 16 Aug 2018 03:07:59 +0000 Message-ID: References: <20180810014348.32036-1-hao.a.wu@intel.com> <140b21ca-6bdf-af12-4f3f-d403ecd2f69e@redhat.com> In-Reply-To: <140b21ca-6bdf-af12-4f3f-d403ecd2f69e@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Thu, 16 Aug 2018 03:08:10 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of La= szlo > 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 >=20 > On 08/10/18 03:43, Hao Wu wrote: > > The series will add RSB stuffing logics to avoid RSB underflow on retur= n > > 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(-) > > >=20 > I haven't tested this patch set yet; first I'd like to make some comments= : >=20 > (1) I think the commit messages are very lacking. Please explain > *precisely* why the Return Stack Buffer has to be stuffed before RSM. >=20 > (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 sta= te. >=20 > (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). >=20 > Is this correct more or less? If so, then why are we stuffing the RSB > just before we *leave* the privileged mode (=3DSMM) for the less > privileged mode (=3Dring 0, IIUC)? Shouldn't we kill the "external > training" of the predictor right after we *enter* SMM? >=20 > (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. >=20 > 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. >=20 >=20 > 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. >=20 >=20 > (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. >=20 > 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: >=20 > https://www.nasm.us/doc/nasmdoc4.html > 4.3.2 Macro-Local Labels >=20 > 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) >=20 > 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=3D1085 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 >=20 > ------------- > %define RSB_STUFF_ENTRIES 0x20 >=20 > ; @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 >=20 > %define StuffRsb32 StuffRsb (eax, esp, 4) > %define StuffRsb64 StuffRsb (rax, rsp, 8) > ------------- >=20 > Thanks, > Laszlo >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel