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 8CA4521AE30DB for ; Thu, 16 Aug 2018 16:01:53 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 685AE4074458; Thu, 16 Aug 2018 23:01:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-198.rdu2.redhat.com [10.10.120.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id 785BE1007252; Thu, 16 Aug 2018 23:01:51 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Jiewen Yao , Eric Dong , Michael D Kinney References: <20180816031422.15340-1-hao.a.wu@intel.com> <20180816031422.15340-2-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <6e69fae1-c2a3-5911-a49f-d48cbcef3acc@redhat.com> Date: Fri, 17 Aug 2018 01:01:50 +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: <20180816031422.15340-2-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 16 Aug 2018 23:01:52 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 16 Aug 2018 23:01:52 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: 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 23:01:53 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Hao Wu > --- > 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 , 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.
> +; 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 (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: . - 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 (Please remember that the above tags are conditional on fixing (1) through (4), and (6). It's OK to ignore (5).) Thanks, Laszlo