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 F14CE941E74 for ; Wed, 8 Nov 2023 21:16:17 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=cGUg78QIgxSC4Bk0LuYqChD5wTD3Pivhotum+zm493c=; 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=1699478176; v=1; b=J4ajITEQOieW8roM+g1xfxXe4ZkhS3uLz5gERnBg2AEFVFBdSKIUZ0HIaU7fJwXd4/zplNNE 5gCcOuJOXzh1NHWAN27Iw+QNqEDKJgW7sik4sTJ/hULclRg+K8OZ3qgRu3Pz+K8f68Y5Urkt/Y3 nEy3Vlv7Xq9Z+SIeiehkmcg8= X-Received: by 127.0.0.2 with SMTP id LXJpYY7687511xsqFY5N2HOI; Wed, 08 Nov 2023 13:16:16 -0800 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.web11.104403.1699478175806018264 for ; Wed, 08 Nov 2023 13:16:16 -0800 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-544-mLRKLCHHOEe5xioXsiAnEA-1; Wed, 08 Nov 2023 16:16:08 -0500 X-MC-Unique: mLRKLCHHOEe5xioXsiAnEA-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 75323185A780; Wed, 8 Nov 2023 21:16:08 +0000 (UTC) X-Received: from [10.39.192.41] (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D237CC1290F; Wed, 8 Nov 2023 21:16:06 +0000 (UTC) Message-ID: <21060bb6-faf0-ce45-d68d-06750f09e5d8@redhat.com> Date: Wed, 8 Nov 2023 22:16:05 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 1/3] 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: <20231106090754.820-1-w.sheng@intel.com> <20231106090754.820-2-w.sheng@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231106090754.820-2-w.sheng@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 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: RNWsrbgQNi4QviZAlGHiR4Igx7686176AA= 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=J4ajITEQ; 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/6/23 10:07, Sheng Wei wrote: > Clear CR4.CET bit before restoring MSR IA32_S_CET. > Backup/restore MSR IA32_U_CET in SMI. (1) As far as I understand, these are still two separate fixes. And I think this patch has issues due to trying to fix both issues at the same time. (I could be wrong of course, I'm not familiar with CET, but this is my impression.) More details on this below. (2) Each issue / fix (i.e., the one issue / fix per patch) should be explained in detail, even if you think the issue that each patch fixes is obvious. >=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 | 53 ++++++++++++--- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69 ++++++++++++++++---- > 2 files changed, 98 insertions(+), 24 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/Pi= SmmCpuDxeSmm/Ia32/SmiEntry.nasm > index 19de5f614e..68332e2c3f 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 (3) These assembly language macros should have been introduced in an include file (*.inc). These "SmiEntry.nasm" files already %include "StuffRsbNasm.inc" and "Nasm.inc", so placing the CET-related macros side-by-side with those files, for example in a new file called "Cet.inc", would be the right thing. It would eliminate the duplication between the IA32 and X64 nasm files. Please prepend a patch to the series that moves the existent macros to "Cet.nasm", and then in this patch, add the new macros to "Cet.nasm" / modify the old ones inside "Cet.nasm". > @@ -214,11 +215,21 @@ ASM_PFX(mPatchCetSupported): > push edx > push eax > =20 > + mov ecx, MSR_IA32_U_CET > + rdmsr > + push edx > + push eax > + So this is related to saving CET_U state; we're pushing the MSR contents to the stack right after having saving CET_S state similarly. > 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 This seems to clear CET_U state. Why is that necessary? The commit message only says "backup/restore"; it does not say "clear". I assume your main goal is *clearing* CET_U state for the duration of the SMI, and that's why you want to backup/restore (so that the clearing does not destroy information). I guess. (4) But this should be explained in the commit message. > @@ -276,6 +287,11 @@ CetDone: > cmp al, 0 > jz CetDone2 > =20 > + mov ecx, MSR_IA32_S_CET > + xor eax, eax > + xor edx, edx > + wrmsr > + > mov eax, 0x668 > mov cr4, eax ; disable CET > =20 (5) This logic then appears to belong to the *other* fix, namely nulling (?) CET_S state before clearing CR4.CET. > @@ -284,10 +300,15 @@ CetDone: > 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) I admit again that I know effectively nothing about CET, but this looks like a bug. The patch tries to pop CET_U (for the purpose of restoring it) before it (already) pops CET_S (for the purpose of restoring it). The ordering seems fine; it mirrors the pushing. (6) However, the last instruction before the CetDone2 label should be "wrmsr"; should it not? I have no idea why we move EAX to EBX instead; it makes no sense to me. > @@ -305,6 +326,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 This makes my head spin! First, it explains why we store EAX to EBX under (6). The reason is that we want to delay the CET_S restoration right until the RSM. And because the MSR_IA32_MISC_ENABLE restoration clobbers EAX, we stash it in EBX. (7) But *why* do we have to delay CET_S restoration right until the RSM? It is not explained anywhere at all in the patch. (8) The stashing of EAX in EBX needs a conspicuous comment in the code, so that future code additions don't clobber EBX. (9) Why don't we stash EDX similary? Why is it safe to set EDX to 0 before the WRMSR? If we don't care about the EDX that we just popped from the stack, then why do we push it originally? (10) Given that we skip the CET_S restoration if "mCetSupported" is zero, why do we *not* skip the CET_U restoration similarly? Sorry, I'm totally confused by this patch. It feels like the patch is trying to do at least three separate things. I don't think I can sensibly review the X64 counterpart until this patch is further split into *minimal* actions. (In fact the "clear CR4.CET" in the subject line and in the commit message seems to apply only to the X64 code!) Thanks Laszlo > +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..007fbff640 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 > @@ -316,13 +327,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 +352,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 +379,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 +427,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 -=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 (#110926): https://edk2.groups.io/g/devel/message/110926 Mute This Topic: https://groups.io/mt/102416572/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-