From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id F0B2FAC120C for ; Fri, 3 Nov 2023 13:19:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=d6oXlv93s8v1NEEPSIk20h8cid+f4+tENH914bzUvM8=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699017572; v=1; b=LKx6mMBOVyIXwzrTBquOBpeBB3NV35L63d7amtYMv/1pe/ESiabl4fyJIIKA+cDYHF2fkj8L KtMDlXSc7TcpNDSRmBMZK9tY0m5TcxL1vEv5TFiap+jj56UnJ5/CucJ6WG2jcmBWieFsQ4dDUc9 X8jk6joItnPBvGrtdYoe9a1w= X-Received: by 127.0.0.2 with SMTP id JFpLYY7687511xq0VNRAGvDJ; Fri, 03 Nov 2023 06:19:32 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.51634.1699017572013340872 for ; Fri, 03 Nov 2023 06:19:32 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-52-mXHHqP5tOxiMxgvLlBOE2A-1; Fri, 03 Nov 2023 09:19:27 -0400 X-MC-Unique: mXHHqP5tOxiMxgvLlBOE2A-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 65929856F65; Fri, 3 Nov 2023 13:19:27 +0000 (UTC) X-Received: from [10.39.192.20] (unknown [10.39.192.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 43FFF1121309; Fri, 3 Nov 2023 13:19:26 +0000 (UTC) Message-ID: Date: Fri, 3 Nov 2023 14:19:25 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET To: devel@edk2.groups.io, w.sheng@intel.com Cc: Eric Dong , Ray Ni , Wu Jiaxin , Tan Dun References: <20231103053510.1943-1-w.sheng@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231103053510.1943-1-w.sheng@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: G8dRxtlWrEGfildDhkF9jPTrx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=LKx6mMBO; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/3/23 06:35, Sheng Wei wrote: > Clear CR4.CET bit before restoring MSR IA32_S_CET. > Backup/restore MSR IA32_U_CET in SMI. > Use current CR4 value when changing CR4.CET. (1) Why? (It's fine if you can provide a reference from the Intel SDM, but then please do provide it.) No problem has been stated. What is broken, and how does the proposed patch solve the issue? (2) I could be mistaken, but the above changes are three separate fixes. If you agree, then please split the patch in three patches. > Initial mSmmInterruptSspTables to 0. (3) The "mSmmInterruptSspTables" object has static storage duration (it is a "global variable"), and its current definition UINTN mSmmInterruptSspTables; already ensures that it is initialized to zero. Therefore this change is unnecessary. It does not hurt either, of course, so if you argument is that we should improve readability, I don't mind, but then it too belongs in a separate patch. Laszlo >=20 > Signed-off-by: Sheng Wei > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Wu Jiaxin > Cc: Tan Dun > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 62 +++++++++++++---- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 72 ++++++++++++++++---- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 2 +- > 3 files changed, 107 insertions(+), 29 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/Pi= SmmCpuDxeSmm/Ia32/SmiEntry.nasm > index 19de5f614e..a087576a54 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > @@ -16,18 +16,19 @@ > %include "StuffRsbNasm.inc" > %include "Nasm.inc" > =20 > +%define MSR_IA32_U_CET 0x6A0 > %define MSR_IA32_S_CET 0x6A2 > -%define MSR_IA32_CET_SH_STK_EN 0x1 > -%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > -%define MSR_IA32_CET_ENDBR_EN 0x4 > -%define MSR_IA32_CET_LEG_IW_EN 0x8 > -%define MSR_IA32_CET_NO_TRACK_EN 0x10 > -%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > -%define MSR_IA32_CET_SUPPRESS 0x400 > -%define MSR_IA32_CET_TRACKER 0x800 > +%define MSR_IA32_CET_SH_STK_EN 0x1 > +%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > +%define MSR_IA32_CET_ENDBR_EN 0x4 > +%define MSR_IA32_CET_LEG_IW_EN 0x8 > +%define MSR_IA32_CET_NO_TRACK_EN 0x10 > +%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > +%define MSR_IA32_CET_SUPPRESS 0x400 > +%define MSR_IA32_CET_TRACKER 0x800 > %define MSR_IA32_PL0_SSP 0x6A4 > =20 > -%define CR4_CET 0x800000 > +%define CR4_CET_BIT 23 > =20 > %define MSR_IA32_MISC_ENABLE 0x1A0 > %define MSR_EFER 0xc0000080 > @@ -214,11 +215,21 @@ ASM_PFX(mPatchCetSupported): > push edx > push eax > =20 > + mov ecx, MSR_IA32_U_CET > + rdmsr > + push edx > + push eax > + > mov ecx, MSR_IA32_PL0_SSP > rdmsr > push edx > push eax > =20 > + mov ecx, MSR_IA32_U_CET > + xor eax, eax > + xor edx, edx > + wrmsr > + > mov ecx, MSR_IA32_S_CET > mov eax, MSR_IA32_CET_SH_STK_EN > xor edx, edx > @@ -249,7 +260,8 @@ CetInterruptDone: > bts ecx, 16 ; set WP > mov cr0, ecx > =20 > - mov eax, 0x668 | CR4_CET > + mov eax, cr4 > + bts eax, CR4_CET_BIT > mov cr4, eax > =20 > setssbsy > @@ -276,18 +288,30 @@ CetDone: > cmp al, 0 > jz CetDone2 > =20 > - mov eax, 0x668 > - mov cr4, eax ; disable CET > + mov ecx, MSR_IA32_S_CET > + xor eax, eax > + xor edx, edx > + wrmsr > + > + ; clear CR4.CET bit > + mov eax, cr4 > + btr eax, CR4_CET_BIT > + mov cr4, eax > =20 > mov ecx, MSR_IA32_PL0_SSP > pop eax > pop edx > wrmsr > =20 > - mov ecx, MSR_IA32_S_CET > + mov ecx, MSR_IA32_U_CET > pop eax > pop edx > wrmsr > + > + mov ecx, MSR_IA32_S_CET > + pop eax > + pop edx > + mov ebx, eax > CetDone2: > =20 > mov eax, ASM_PFX(mXdSupported) > @@ -305,6 +329,18 @@ CetDone2: > .7: > =20 > StuffRsb32 > + > + mov eax, ASM_PFX(mCetSupported) > + mov al, [eax] > + cmp al, 0 > + jz CetDone3 > + > + mov ecx, MSR_IA32_S_CET > + mov eax, ebx > + xor edx, edx > + wrmsr > +CetDone3: > + > rsm > =20 > ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiS= mmCpuDxeSmm/X64/SmiEntry.nasm > index d302ca8d01..7aed7c8dda 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > @@ -20,19 +20,20 @@ > ; Variables referenced by C code > ; > =20 > +%define MSR_IA32_U_CET 0x6A0 > %define MSR_IA32_S_CET 0x6A2 > -%define MSR_IA32_CET_SH_STK_EN 0x1 > -%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > -%define MSR_IA32_CET_ENDBR_EN 0x4 > -%define MSR_IA32_CET_LEG_IW_EN 0x8 > -%define MSR_IA32_CET_NO_TRACK_EN 0x10 > -%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > -%define MSR_IA32_CET_SUPPRESS 0x400 > -%define MSR_IA32_CET_TRACKER 0x800 > +%define MSR_IA32_CET_SH_STK_EN 0x1 > +%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > +%define MSR_IA32_CET_ENDBR_EN 0x4 > +%define MSR_IA32_CET_LEG_IW_EN 0x8 > +%define MSR_IA32_CET_NO_TRACK_EN 0x10 > +%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > +%define MSR_IA32_CET_SUPPRESS 0x400 > +%define MSR_IA32_CET_TRACKER 0x800 > %define MSR_IA32_PL0_SSP 0x6A4 > %define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR 0x6A8 > =20 > -%define CR4_CET 0x800000 > +%define CR4_CET_BIT 23 > =20 > %define MSR_IA32_MISC_ENABLE 0x1A0 > %define MSR_EFER 0xc0000080 > @@ -230,6 +231,11 @@ ASM_PFX(mPatchCetSupported): > push rdx > push rax > =20 > + mov ecx, MSR_IA32_U_CET > + rdmsr > + push rdx > + push rax > + > mov ecx, MSR_IA32_PL0_SSP > rdmsr > push rdx > @@ -240,6 +246,11 @@ ASM_PFX(mPatchCetSupported): > push rdx > push rax > =20 > + mov ecx, MSR_IA32_U_CET > + xor eax, eax > + xor edx, edx > + wrmsr > + > mov ecx, MSR_IA32_S_CET > mov eax, MSR_IA32_CET_SH_STK_EN > xor edx, edx > @@ -276,7 +287,8 @@ CetInterruptDone: > bts ecx, 16 ; set WP > mov cr0, rcx > =20 > - mov eax, 0x668 | CR4_CET > + mov rax, cr4 > + bts rax, CR4_CET_BIT > mov cr4, rax > =20 > setssbsy > @@ -316,13 +328,20 @@ CpuSmmDebugExitAbsAddr: > add rsp, 0x200 > =20 > mov rax, strict qword 0 ; mov rax, ASM_PFX(mCetSup= ported) > -mCetSupportedAbsAddr: > +mCetSupportedAbsAddr1: > mov al, [rax] > cmp al, 0 > jz CetDone2 > =20 > - mov eax, 0x668 > - mov cr4, rax ; disable CET > + mov ecx, MSR_IA32_S_CET > + xor eax, eax > + xor edx, edx > + wrmsr > + > + ; clear CR4.CET bit > + mov rax, cr4 > + btr rax, CR4_CET_BIT > + mov cr4, rax > =20 > mov ecx, MSR_IA32_INTERRUPT_SSP_TABLE_ADDR > pop rax > @@ -334,10 +353,15 @@ mCetSupportedAbsAddr: > pop rdx > wrmsr > =20 > - mov ecx, MSR_IA32_S_CET > + mov ecx, MSR_IA32_U_CET > pop rax > pop rdx > wrmsr > + > + mov ecx, MSR_IA32_S_CET > + pop rax > + pop rdx > + mov ebx, eax > CetDone2: > =20 > mov rax, strict qword 0 ; lea rax, [ASM_PFX(mX= dSupported)] > @@ -356,6 +380,19 @@ mXdSupportedAbsAddr: > .1: > =20 > StuffRsb64 > + > + mov rax, strict qword 0 ; mov rax, ASM_PFX(mCetSup= ported) > +mCetSupportedAbsAddr2: > + mov al, [rax] > + cmp al, 0 > + jz CetDone3 > + > + mov ecx, MSR_IA32_S_CET > + mov eax, ebx > + xor edx, edx > + wrmsr > +CetDone3: > + > rsm > =20 > ASM_PFX(gcSmiHandlerSize) DW $ - _SmiEntryPoint > @@ -391,6 +428,11 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress): > mov qword [rcx - 8], rax > =20 > lea rax, [ASM_PFX(mCetSupported)] > - lea rcx, [mCetSupportedAbsAddr] > + lea rcx, [mCetSupportedAbsAddr1] > mov qword [rcx - 8], rax > + > + lea rax, [ASM_PFX(mCetSupported)] > + lea rcx, [mCetSupportedAbsAddr2] > + mov qword [rcx - 8], rax > + > ret > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/Pi= SmmCpuDxeSmm/X64/SmmFuncsArch.c > index c4f21e2155..6c53213b0b 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -20,7 +20,7 @@ UINT32 mCetPl0Ssp; > UINT32 mCetInterruptSsp; > UINT32 mCetInterruptSspTable; > =20 > -UINTN mSmmInterruptSspTables; > +UINTN mSmmInterruptSspTables =3D 0; > =20 > /** > Initialize IDT IST Field. -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110622): https://edk2.groups.io/g/devel/message/110622 Mute This Topic: https://groups.io/mt/102358752/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-