Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET

["Laszlo Ersek" <lersek@redhat.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-