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.sh...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Wu Jiaxin <jiaxin...@intel.com> > Cc: Tan Dun <dun....@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 (#110622): https://edk2.groups.io/g/devel/message/110622 Mute This Topic: https://groups.io/mt/102358752/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
