* [edk2-devel] [PATCH v2 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before
@ 2023-11-06 9:07 Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET Sheng Wei
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Sheng Wei @ 2023-11-06 9:07 UTC (permalink / raw)
To: devel
Patch V2:
No function change with Patch V1.
Split the patch to into 3 separate patches.
Sheng Wei (3):
UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR
IA32_S_CET
UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only
UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value
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(-)
--
2.26.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110742): https://edk2.groups.io/g/devel/message/110742
Mute This Topic: https://groups.io/mt/102416571/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET
2023-11-06 9:07 [edk2-devel] [PATCH v2 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before Sheng Wei
@ 2023-11-06 9:07 ` Sheng Wei
2023-11-08 21:16 ` Laszlo Ersek
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value Sheng Wei
2 siblings, 1 reply; 8+ messages in thread
From: Sheng Wei @ 2023-11-06 9:07 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Wu Jiaxin, Tan Dun
Clear CR4.CET bit before restoring MSR IA32_S_CET.
Backup/restore MSR IA32_U_CET in SMI.
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 | 53 ++++++++++++---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69 ++++++++++++++++----
2 files changed, 98 insertions(+), 24 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/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"
+%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
@@ -276,6 +287,11 @@ CetDone:
cmp al, 0
jz CetDone2
+ mov ecx, MSR_IA32_S_CET
+ xor eax, eax
+ xor edx, edx
+ wrmsr
+
mov eax, 0x668
mov cr4, eax ; disable CET
@@ -284,10 +300,15 @@ CetDone:
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 +326,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..007fbff640 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
@@ -316,13 +327,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 +352,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 +379,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 +427,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
--
2.26.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110743): https://edk2.groups.io/g/devel/message/110743
Mute This Topic: https://groups.io/mt/102416572/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only
2023-11-06 9:07 [edk2-devel] [PATCH v2 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET Sheng Wei
@ 2023-11-06 9:07 ` Sheng Wei
2023-11-08 21:22 ` Laszlo Ersek
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value Sheng Wei
2 siblings, 1 reply; 8+ messages in thread
From: Sheng Wei @ 2023-11-06 9:07 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Wu Jiaxin, Tan Dun
Do not use fixed CR4 value 0x668, change CR4.CET bit only.
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 | 9 ++++++---
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 3 ++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 68332e2c3f..a087576a54 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -260,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
@@ -292,8 +293,10 @@ CetDone:
xor edx, edx
wrmsr
- mov eax, 0x668
- mov cr4, eax ; disable CET
+ ; clear CR4.CET bit
+ mov eax, cr4
+ btr eax, CR4_CET_BIT
+ mov cr4, eax
mov ecx, MSR_IA32_PL0_SSP
pop eax
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 007fbff640..7aed7c8dda 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -287,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
--
2.26.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110744): https://edk2.groups.io/g/devel/message/110744
Mute This Topic: https://groups.io/mt/102416574/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value
2023-11-06 9:07 [edk2-devel] [PATCH v2 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only Sheng Wei
@ 2023-11-06 9:07 ` Sheng Wei
2023-11-08 21:24 ` Laszlo Ersek
2 siblings, 1 reply; 8+ messages in thread
From: Sheng Wei @ 2023-11-06 9:07 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Laszlo Ersek, Wu Jiaxin, Tan Dun
Initial the value of mSmmInterruptSspTables to 0.
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/X64/SmmFuncsArch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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.
--
2.26.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110745): https://edk2.groups.io/g/devel/message/110745
Mute This Topic: https://groups.io/mt/102416578/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET Sheng Wei
@ 2023-11-08 21:16 ` Laszlo Ersek
2023-11-09 7:50 ` Sheng Wei
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2023-11-08 21:16 UTC (permalink / raw)
To: devel, w.sheng; +Cc: Eric Dong, Ray Ni, Wu Jiaxin, Tan Dun
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.
>
> 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 | 53 ++++++++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69 ++++++++++++++++----
> 2 files changed, 98 insertions(+), 24 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/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"
>
> +%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
(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
>
> + 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
>
> + 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
>
> + mov ecx, MSR_IA32_S_CET
> + xor eax, eax
> + xor edx, edx
> + wrmsr
> +
> mov eax, 0x668
> mov cr4, eax ; disable CET
>
(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
>
> - 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)
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:
>
> 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
>
> ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/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
> ;
>
> +%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
> @@ -316,13 +327,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 +352,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 +379,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 +427,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
-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only Sheng Wei
@ 2023-11-08 21:22 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2023-11-08 21:22 UTC (permalink / raw)
To: devel, w.sheng; +Cc: Eric Dong, Ray Ni, Wu Jiaxin, Tan Dun
On 11/6/23 10:07, Sheng Wei wrote:
> Do not use fixed CR4 value 0x668, change CR4.CET bit only.
>
> 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 | 9 ++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 3 ++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> index 68332e2c3f..a087576a54 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> @@ -260,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
> @@ -292,8 +293,10 @@ CetDone:
> xor edx, edx
> wrmsr
>
> - mov eax, 0x668
> - mov cr4, eax ; disable CET
> + ; clear CR4.CET bit
> + mov eax, cr4
> + btr eax, CR4_CET_BIT
> + mov cr4, eax
>
> mov ecx, MSR_IA32_PL0_SSP
> pop eax
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> index 007fbff640..7aed7c8dda 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> @@ -287,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
I didn't understand why the X64 code here didn't contain the "btr"
counterpart of "bts". Well the reason is that the "missing" btr is
actually introduced in the previous patch.
I find that confusing. I think that, once you have "Cet.inc", you should
separately replace CR4_CET with CR4_CET_BIT, both in "Cet.inc" and in
the three existent locations (two in the IA32 entry code and one in the
X64 entry code).
*Then* you could proceed to clearing CR4.CET in the subsequent patch,
using CR4_CET_BIT.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110927): https://edk2.groups.io/g/devel/message/110927
Mute This Topic: https://groups.io/mt/102416574/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value Sheng Wei
@ 2023-11-08 21:24 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2023-11-08 21:24 UTC (permalink / raw)
To: devel, w.sheng; +Cc: Eric Dong, Ray Ni, Wu Jiaxin, Tan Dun
On 11/6/23 10:07, Sheng Wei wrote:
> Initial the value of mSmmInterruptSspTables to 0.
>
> 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/X64/SmmFuncsArch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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.
Please state your reason for this change in the commit message.
(Functionally, the patch is a no-op.)
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110928): https://edk2.groups.io/g/devel/message/110928
Mute This Topic: https://groups.io/mt/102416578/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET
2023-11-08 21:16 ` Laszlo Ersek
@ 2023-11-09 7:50 ` Sheng Wei
0 siblings, 0 replies; 8+ messages in thread
From: Sheng Wei @ 2023-11-09 7:50 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Ni, Ray, Wu, Jiaxin, Tan, Dun
Hi Laszlo,
Please ignore the patch V3. I will refine the patches and raise patch V4.
Thank you.
BR
Sheng Wei
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, November 9, 2023 5:16 AM
> 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 v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm:
> Clear CR4.CET before restoring MSR IA32_S_CET
>
> 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.
>
> >
> > 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 | 53
> ++++++++++++---
> > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69
> ++++++++++++++++----
> > 2 files changed, 98 insertions(+), 24 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/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"
> >
> > +%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
>
> (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
> >
> > + 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
> >
> > + 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
> >
> > + mov ecx, MSR_IA32_S_CET
> > + xor eax, eax
> > + xor edx, edx
> > + wrmsr
> > +
> > mov eax, 0x668
> > mov cr4, eax ; disable CET
> >
>
> (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
> >
> > - 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)
>
> 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:
> >
> > 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
> >
> > ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint diff --git
> > a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/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
> > ;
> >
> > +%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
> > @@ -316,13 +327,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 +352,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 +379,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 +427,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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110951): https://edk2.groups.io/g/devel/message/110951
Mute This Topic: https://groups.io/mt/102416572/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-09 7:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-06 9:07 [edk2-devel] [PATCH v2 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET Sheng Wei
2023-11-08 21:16 ` Laszlo Ersek
2023-11-09 7:50 ` Sheng Wei
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only Sheng Wei
2023-11-08 21:22 ` Laszlo Ersek
2023-11-06 9:07 ` [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Set mSmmInterruptSspTables initial value Sheng Wei
2023-11-08 21:24 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox