From: "Sheng Wei" <w.sheng@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Dong, Eric" <eric.dong@intel.com>, "Ni, Ray" <ray.ni@intel.com>,
"Wu, Jiaxin" <jiaxin.wu@intel.com>,
"Tan, Dun" <dun.tan@intel.com>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET
Date: Wed, 8 Nov 2023 02:12:19 +0000 [thread overview]
Message-ID: <SA2PR11MB4875FD66524D49AAF2ED4170E1A8A@SA2PR11MB4875.namprd11.prod.outlook.com> (raw)
In-Reply-To: <f04101dc-6444-a72d-a2e7-8a3a859b04e6@redhat.com>
Hi Laszlo,
Thank you for the review comments.
(1) This change is used to fix a hung issue when enable kernel CET-IBT.
Kernel will enable CET-IBT by set IA32_U_CET.bit2.
The issue only happens when it enters SMI with the state machine == WAIT_FOR_ENDBRANCH state.
In SMI handler, when Set CR4.CET bit, CPU will check the next assembly code, if the next code is not ENDBR. It will trigger #CP exception.
So, we need to backup current IA32_U_CET, and clear IA32_U_CET before enable CR4.CET.
And when exit SMI, we need to restore the value in IA32_U_CET.
(2) Yes, I have separated it to 3 patches. But last patch will be removed because of (3)
(3) It is global variable. It is initialized to zero. I will remove this change.
I will raise patch V3.
Thank you.
BR
Sheng Wei
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, November 3, 2023 9:19 PM
> To: devel@edk2.groups.io; Sheng, W <w.sheng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Tan, Dun <dun.tan@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear
> CR4.CET before restoring MSR IA32_S_CET
>
> 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
>
> >
> > Signed-off-by: Sheng Wei <w.sheng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Wu Jiaxin <jiaxin.wu@intel.com>
> > Cc: Tan Dun <dun.tan@intel.com>
> > ---
> > 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(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/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"
> >
> > +%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 CR4_CET 0x800000
> > +%define CR4_CET_BIT 23
> >
> > %define MSR_IA32_MISC_ENABLE 0x1A0
> > %define MSR_EFER 0xc0000080
> > @@ -214,11 +215,21 @@ ASM_PFX(mPatchCetSupported):
> > push edx
> > push eax
> >
> > + mov ecx, MSR_IA32_U_CET
> > + rdmsr
> > + push edx
> > + push eax
> > +
> > mov ecx, MSR_IA32_PL0_SSP
> > rdmsr
> > push edx
> > push eax
> >
> > + 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
> >
> > - mov eax, 0x668 | CR4_CET
> > + mov eax, cr4
> > + bts eax, CR4_CET_BIT
> > mov cr4, eax
> >
> > setssbsy
> > @@ -276,18 +288,30 @@ CetDone:
> > cmp al, 0
> > jz CetDone2
> >
> > - 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
> >
> > mov ecx, MSR_IA32_PL0_SSP
> > pop eax
> > pop edx
> > wrmsr
> >
> > - 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:
> >
> > mov eax, ASM_PFX(mXdSupported)
> > @@ -305,6 +329,18 @@ CetDone2:
> > .7:
> >
> > 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
> >
> > ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint diff --git
> > a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/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
> > ;
> >
> > +%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
> >
> > -%define CR4_CET 0x800000
> > +%define CR4_CET_BIT 23
> >
> > %define MSR_IA32_MISC_ENABLE 0x1A0
> > %define MSR_EFER 0xc0000080
> > @@ -230,6 +231,11 @@ ASM_PFX(mPatchCetSupported):
> > push rdx
> > push rax
> >
> > + 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
> >
> > + 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
> >
> > - mov eax, 0x668 | CR4_CET
> > + mov rax, cr4
> > + bts rax, CR4_CET_BIT
> > mov cr4, rax
> >
> > setssbsy
> > @@ -316,13 +328,20 @@ CpuSmmDebugExitAbsAddr:
> > add rsp, 0x200
> >
> > mov rax, strict qword 0 ; mov rax, ASM_PFX(mCetSupported)
> > -mCetSupportedAbsAddr:
> > +mCetSupportedAbsAddr1:
> > mov al, [rax]
> > cmp al, 0
> > jz CetDone2
> >
> > - 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
> >
> > mov ecx, MSR_IA32_INTERRUPT_SSP_TABLE_ADDR
> > pop rax
> > @@ -334,10 +353,15 @@ mCetSupportedAbsAddr:
> > pop rdx
> > wrmsr
> >
> > - 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:
> >
> > mov rax, strict qword 0 ; lea rax, [ASM_PFX(mXdSupported)]
> > @@ -356,6 +380,19 @@ mXdSupportedAbsAddr:
> > .1:
> >
> > StuffRsb64
> > +
> > + mov rax, strict qword 0 ; mov rax, ASM_PFX(mCetSupported)
> > +mCetSupportedAbsAddr2:
> > + mov al, [rax]
> > + cmp al, 0
> > + jz CetDone3
> > +
> > + mov ecx, MSR_IA32_S_CET
> > + mov eax, ebx
> > + xor edx, edx
> > + wrmsr
> > +CetDone3:
> > +
> > rsm
> >
> > ASM_PFX(gcSmiHandlerSize) DW $ - _SmiEntryPoint
> > @@ -391,6 +428,11 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
> > mov qword [rcx - 8], rax
> >
> > 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/PiSmmCpuDxeSmm/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;
> >
> > -UINTN mSmmInterruptSspTables;
> > +UINTN mSmmInterruptSspTables = 0;
> >
> > /**
> > Initialize IDT IST Field.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110882): https://edk2.groups.io/g/devel/message/110882
Mute This Topic: https://groups.io/mt/102358752/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
prev parent reply other threads:[~2023-11-08 2:12 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-03 5:35 [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET Sheng Wei
2023-11-03 8:38 ` duntan
2023-11-03 13:19 ` Laszlo Ersek
2023-11-08 2:12 ` Sheng Wei [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SA2PR11MB4875FD66524D49AAF2ED4170E1A8A@SA2PR11MB4875.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox