Reviewed-by: Ray Ni <ray...@intel.com> I will create PR by end of my today since this patch fixes a critical issue when enabling CET in SMM.
> -----Original Message----- > From: Sheng, W <w.sh...@intel.com> > Sent: Friday, November 12, 2021 9:40 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Kumar, > Rahul1 <rahul1.ku...@intel.com> > Subject: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Use SMM Interrupt Shadow Stack > > When CET shadow stack feature is enabled, it needs to use IST for the > exceptions, and uses interrupt shadow stack for the stack switch. > Shadow stack should be 32 bytes aligned. > Check IST field, when clear shadow stack token busy bit when using retf. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3728 > > Signed-off-by: Sheng Wei <w.sh...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Rahul Kumar <rahul1.ku...@intel.com> > Reviewed-by: Ray Ni <ray...@intel.com> > --- > .../X64/Xcode5ExceptionHandlerAsm.nasm | 66 ++++++++++++------ > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 61 +++++++++++----- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 14 ++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 12 +++- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 81 > ++++++++++++---------- > 5 files changed, 157 insertions(+), 77 deletions(-) > > diff --git > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm > index 4881a02848..84a12ddb88 100644 > --- > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm > +++ > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm > @@ -15,17 +15,36 @@ > > ;------------------------------------------------------------------------------ > %include "Nasm.inc" > > +; > +; Equivalent NASM structure of IA32_DESCRIPTOR > +; > +struc IA32_DESCRIPTOR > + .Limit CTYPE_UINT16 1 > + .Base CTYPE_UINTN 1 > +endstruc > + > +; > +; Equivalent NASM structure of IA32_IDT_GATE_DESCRIPTOR > +; > +struc IA32_IDT_GATE_DESCRIPTOR > + .OffsetLow CTYPE_UINT16 1 > + .Selector CTYPE_UINT16 1 > + .Reserved_0 CTYPE_UINT8 1 > + .GateType CTYPE_UINT8 1 > + .OffsetHigh CTYPE_UINT16 1 > + .OffsetUpper CTYPE_UINT32 1 > + .Reserved_1 CTYPE_UINT32 1 > +endstruc > + > ; > ; CommonExceptionHandler() > ; > > %define VC_EXCEPTION 29 > -%define PF_EXCEPTION 14 > > extern ASM_PFX(mErrorCodeFlag) ; Error code flags for exceptions > extern ASM_PFX(mDoFarReturnFlag) ; Do far return flag > extern ASM_PFX(CommonExceptionHandler) > -extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard)) > > SECTION .data > > @@ -282,42 +301,49 @@ DrFinish: > > ; The follow algorithm is used for clear shadow stack token busy bit. > ; The comment is based on the sample shadow stack. > + ; Shadow stack is 32 bytes aligned. > ; The sample shadow stack layout : > ; Address | Context > ; +-------------------------+ > - ; 0xFD0 | FREE | it is 0xFD8|0x02|(LMA & CS.L), > after SAVEPREVSSP. > + ; 0xFB8 | FREE | It is 0xFC0|0x02|(LMA & CS.L), > after SAVEPREVSSP. > ; +-------------------------+ > - ; 0xFD8 | Prev SSP | > + ; 0xFC0 | Prev SSP | > ; +-------------------------+ > - ; 0xFE0 | RIP | > + ; 0xFC8 | RIP | > ; +-------------------------+ > - ; 0xFE8 | CS | > + ; 0xFD0 | CS | > ; +-------------------------+ > - ; 0xFF0 | 0xFF0 | BUSY | BUSY flag cleared after CLRSSBSY > + ; 0xFD8 | 0xFD8 | BUSY | BUSY flag cleared after CLRSSBSY > ; +-------------------------+ > - ; 0xFF8 | 0xFD8|0x02|(LMA & CS.L) | > + ; 0xFE0 | 0xFC0|0x02|(LMA & CS.L) | > ; +-------------------------+ > ; Instructions for Intel Control Flow Enforcement Technology (CET) are > supported since NASM version 2.15.01. > cmp qword [ASM_PFX(mDoFarReturnFlag)], 0 > jz CetDone > - cmp qword [rbp + 8], PF_EXCEPTION ; check if it is a Page Fault > - jnz CetDone > - cmp byte [dword ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard))], 0 > - jz CetDone > mov rax, cr4 > - and rax, 0x800000 ; check if CET is enabled > + and rax, 0x800000 ; Check if CET is enabled > + jz CetDone > + sub rsp, 0x10 > + sidt [rsp] > + mov rcx, qword [rsp + IA32_DESCRIPTOR.Base]; Get IDT base address > + add rsp, 0x10 > + mov rax, qword [rbp + 8]; Get exception number > + sal rax, 0x04 ; Get IDT offset > + add rax, rcx ; Get IDT gate descriptor address > + mov al, byte [rax + IA32_IDT_GATE_DESCRIPTOR.Reserved_0] > + and rax, 0x01 ; Check IST field > jz CetDone > - ; SSP should be 0xFD8 at this point > + ; SSP should be 0xFC0 at this point > mov rax, 0x04 ; advance past cs:lip:prevssp;supervisor > shadow stack token > - INCSSP_RAX ; After this SSP should be 0xFF8 > - SAVEPREVSSP ; now the shadow stack restore token will be > created at 0xFD0 > - READSSP_RAX ; Read new SSP, SSP should be 0x1000 > + INCSSP_RAX ; After this SSP should be 0xFE0 > + SAVEPREVSSP ; now the shadow stack restore token will be > created at 0xFB8 > + READSSP_RAX ; Read new SSP, SSP should be 0xFE8 > sub rax, 0x10 > - CLRSSBSY_RAX ; Clear token at 0xFF0, SSP should be 0 > after this > + CLRSSBSY_RAX ; Clear token at 0xFD8, SSP should be 0 > after this > sub rax, 0x20 > - RSTORSSP_RAX ; Restore to token at 0xFD0, new SSP will be > 0xFD0 > + RSTORSSP_RAX ; Restore to token at 0xFB8, new SSP will be > 0xFB8 > mov rax, 0x01 ; Pop off the new save token created > - INCSSP_RAX ; SSP should be 0xFD8 now > + INCSSP_RAX ; SSP should be 0xFC0 now > CetDone: > > cli > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > index 67ad9a4c07..2b2e1a5390 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -861,35 +861,58 @@ PiCpuSmmEntry ( > mSmmStackSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (PcdGet32 > (PcdCpuSmmStackSize))); > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > // > - // 2 more pages is allocated for each processor. > - // one is guard page and the other is known good stack. > + // SMM Stack Guard Enabled > + // 2 more pages is allocated for each processor, one is guard page and > the other is known good stack. > // > - // > +-------------------------------------------+-----+-------------------------------------------+ > - // | Known Good Stack | Guard Page | SMM Stack | ... | Known Good Stack > | Guard Page | SMM Stack | > - // > +-------------------------------------------+-----+-------------------------------------------+ > - // | | | > | > - // |<-------------- Processor 0 -------------->| |<-------------- > Processor n -------------->| > + // > +--------------------------------------------------+-----+--------------------------------------------------+ > + // | Known Good Stack | Guard Page | SMM Stack | ... | Known Good > Stack | Guard Page | SMM Stack | > + // > +--------------------------------------------------+-----+--------------------------------------------------+ > + // | 4K | 4K PcdCpuSmmStackSize| | 4K > | 4K PcdCpuSmmStackSize| > + // |<---------------- mSmmStackSize ----------------->| > |<---------------- mSmmStackSize ----------------->| > + // | | | > | > + // |<------------------ Processor 0 ----------------->| > |<------------------ Processor n ----------------->| > // > mSmmStackSize += EFI_PAGES_TO_SIZE (2); > } > > mSmmShadowStackSize = 0; > if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) != 0) && > mCetSupported) { > - // > - // Append Shadow Stack after normal stack > - // > - // |= Stacks > - // > +--------------------------------------------------+---------------------------------------------------------------+ > - // | Known Good Stack | Guard Page | SMM Stack | Known Good > Shadow Stack | Guard Page | SMM Shadow Stack | > - // > +--------------------------------------------------+---------------------------------------------------------------+ > - // | |PcdCpuSmmStackSize| > |PcdCpuSmmShadowStackSize| > - // |<---------------- mSmmStackSize > ----------------->|<--------------------- mSmmShadowStackSize > ------------------->| > - // | > | > - // |<-------------------------------------------- Processor N > ------------------------------------------------------->| > - // > mSmmShadowStackSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (PcdGet32 > (PcdCpuSmmShadowStackSize))); > + > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > + // > + // SMM Stack Guard Enabled > + // Append Shadow Stack after normal stack > + // 2 more pages is allocated for each processor, one is guard page > and the other is known good shadow stack. > + // > + // |= Stacks > + // > +--------------------------------------------------+---------------------------------------------------------------+ > + // | Known Good Stack | Guard Page | SMM Stack | Known Good > Shadow Stack | Guard Page | SMM Shadow Stack | > + // > +--------------------------------------------------+---------------------------------------------------------------+ > + // | 4K | 4K |PcdCpuSmmStackSize| 4K > | 4K |PcdCpuSmmShadowStackSize| > + // |<---------------- mSmmStackSize > ----------------->|<--------------------- mSmmShadowStackSize > ------------------->| > + // | > | > + // |<-------------------------------------------- Processor N > ------------------------------------------------------->| > + // > mSmmShadowStackSize += EFI_PAGES_TO_SIZE (2); > + } else { > + // > + // SMM Stack Guard Disabled (Known Good Stack is still required for > potential stack switch.) > + // Append Shadow Stack after normal stack with 1 more page as known > good shadow stack. > + // 1 more pages is allocated for each processor, it is known good > stack. > + // > + // > + // |= Stacks > + // > +-------------------------------------+--------------------------------------------------+ > + // | Known Good Stack | SMM Stack | Known Good Shadow Stack | > SMM Shadow Stack | > + // > +-------------------------------------+--------------------------------------------------+ > + // | 4K |PcdCpuSmmStackSize| 4K > |PcdCpuSmmShadowStackSize| > + // |<---------- mSmmStackSize ---------->|<--------------- > mSmmShadowStackSize ------------>| > + // | > | > + // |<-------------------------------- Processor N > ----------------------------------------->| > + // > + mSmmShadowStackSize += EFI_PAGES_TO_SIZE (1); > + mSmmStackSize += EFI_PAGES_TO_SIZE (1); > } > } > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 2248a8c5ee..fc9b748948 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -557,6 +557,20 @@ InitializeIDTSmmStackGuard ( > VOID > ); > > +/** > + Initialize IDT IST Field. > + > + @param[in] ExceptionType Exception type. > + @param[in] Ist IST value. > + > +**/ > +VOID > +EFIAPI > +InitializeIdtIst ( > + IN EFI_EXCEPTION_TYPE ExceptionType, > + IN UINT8 Ist > + ); > + > /** > Initialize Gdt for all processors. > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index d6f8dd94d3..211a78b1c4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -481,7 +481,17 @@ SmmInitPageTable ( > // Additional SMM IDT initialization for SMM stack guard > // > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > - InitializeIDTSmmStackGuard (); > + DEBUG ((DEBUG_INFO, "Initialize IDT IST field for SMM Stack Guard\n")); > + InitializeIdtIst (EXCEPT_IA32_PAGE_FAULT, 1); > + } > + > + // > + // Additional SMM IDT initialization for SMM CET shadow stack > + // > + if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) != 0) && > mCetSupported) { > + DEBUG ((DEBUG_INFO, "Initialize IDT IST field for SMM Shadow Stack\n")); > + InitializeIdtIst (EXCEPT_IA32_PAGE_FAULT, 1); > + InitializeIdtIst (EXCEPT_IA32_MACHINE_CHECK, 1); > } > > // > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index ca3f5ff91a..ce7afce6d4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -24,24 +24,24 @@ UINT32 mCetInterruptSspTable; > UINTN mSmmInterruptSspTables; > > /** > - Initialize IDT for SMM Stack Guard. > + Initialize IDT IST Field. > + > + @param[in] ExceptionType Exception type. > + @param[in] Ist IST value. > > **/ > VOID > EFIAPI > -InitializeIDTSmmStackGuard ( > - VOID > +InitializeIdtIst ( > + IN EFI_EXCEPTION_TYPE ExceptionType, > + IN UINT8 Ist > ) > { > IA32_IDT_GATE_DESCRIPTOR *IdtGate; > > - // > - // If SMM Stack Guard feature is enabled, set the IST field of > - // the interrupt gate for Page Fault Exception to be 1 > - // > IdtGate = (IA32_IDT_GATE_DESCRIPTOR *)gcSmiIdtr.Base; > - IdtGate += EXCEPT_IA32_PAGE_FAULT; > - IdtGate->Bits.Reserved_0 = 1; > + IdtGate += ExceptionType; > + IdtGate->Bits.Reserved_0 = Ist; > } > > /** > @@ -89,7 +89,7 @@ InitGdt ( > GdtDescriptor->Bits.BaseMid = (UINT8)((UINTN)TssBase >> 16); > GdtDescriptor->Bits.BaseHigh = (UINT8)((UINTN)TssBase >> 24); > > - if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) || ((PcdGet32 > (PcdControlFlowEnforcementPropertyMask) != 0) && > mCetSupported)) { > // > // Setup top of known good stack as IST1 for each processor. > // > @@ -177,8 +177,16 @@ InitShadowStack ( > > if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) != 0) && > mCetSupported) { > SmmShadowStackSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (PcdGet32 > (PcdCpuSmmShadowStackSize))); > + // > + // Add 1 page as known good shadow stack > + // > + SmmShadowStackSize += EFI_PAGES_TO_SIZE (1); > + > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > - SmmShadowStackSize += EFI_PAGES_TO_SIZE (2); > + // > + // Add one guard page between Known Good Shadow Stack and SMM Shadow > Stack. > + // > + SmmShadowStackSize += EFI_PAGES_TO_SIZE (1); > } > mCetPl0Ssp = (UINT32)((UINTN)ShadowStack + SmmShadowStackSize - > sizeof(UINT64)); > PatchInstructionX86 (mPatchCetPl0Ssp, mCetPl0Ssp, 4); > @@ -186,33 +194,32 @@ InitShadowStack ( > DEBUG ((DEBUG_INFO, "ShadowStack - 0x%x\n", ShadowStack)); > DEBUG ((DEBUG_INFO, " SmmShadowStackSize - 0x%x\n", > SmmShadowStackSize)); > > - if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > - if (mSmmInterruptSspTables == 0) { > - mSmmInterruptSspTables = (UINTN)AllocateZeroPool(sizeof(UINT64) * 8 > * gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); > - ASSERT (mSmmInterruptSspTables != 0); > - DEBUG ((DEBUG_INFO, "mSmmInterruptSspTables - 0x%x\n", > mSmmInterruptSspTables)); > - } > - > - // > - // The highest address on the stack (0xFF8) is a save-previous-ssp > token pointing to a location that is 40 bytes away - 0xFD0. > - // The supervisor shadow stack token is just above it at address > 0xFF0. This is where the interrupt SSP table points. > - // So when an interrupt of exception occurs, we can use > SAVESSP/RESTORESSP/CLEARSSBUSY for the supervisor shadow > stack, > - // due to the reason the RETF in SMM exception handler cannot clear > the BUSY flag with same CPL. > - // (only IRET or RETF with different CPL can clear BUSY flag) > - // Please refer to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64 for > the full stack frame at runtime. > - // > - InterruptSsp = (UINT32)((UINTN)ShadowStack + EFI_PAGES_TO_SIZE(1) - > sizeof(UINT64)); > - *(UINT64 *)(UINTN)InterruptSsp = (InterruptSsp - sizeof(UINT64) * 4) | > 0x2; > - mCetInterruptSsp = InterruptSsp - sizeof(UINT64); > - > - mCetInterruptSspTable = (UINT32)(UINTN)(mSmmInterruptSspTables + > sizeof(UINT64) * 8 * CpuIndex); > - InterruptSspTable = (UINT64 *)(UINTN)mCetInterruptSspTable; > - InterruptSspTable[1] = mCetInterruptSsp; > - PatchInstructionX86 (mPatchCetInterruptSsp, mCetInterruptSsp, 4); > - PatchInstructionX86 (mPatchCetInterruptSspTable, > mCetInterruptSspTable, 4); > - DEBUG ((DEBUG_INFO, "mCetInterruptSsp - 0x%x\n", mCetInterruptSsp)); > - DEBUG ((DEBUG_INFO, "mCetInterruptSspTable - 0x%x\n", > mCetInterruptSspTable)); > + if (mSmmInterruptSspTables == 0) { > + mSmmInterruptSspTables = (UINTN)AllocateZeroPool(sizeof(UINT64) * 8 * > gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); > + ASSERT (mSmmInterruptSspTables != 0); > + DEBUG ((DEBUG_INFO, "mSmmInterruptSspTables - 0x%x\n", > mSmmInterruptSspTables)); > } > + > + // > + // The highest address on the stack (0xFE0) is a save-previous-ssp token > pointing to a location that is 40 bytes away - 0xFB8. > + // The supervisor shadow stack token is just above it at address 0xFD8. > This is where the interrupt SSP table points. > + // So when an interrupt of exception occurs, we can use > SAVESSP/RESTORESSP/CLEARSSBUSY for the supervisor shadow > stack, > + // due to the reason the RETF in SMM exception handler cannot clear the > BUSY flag with same CPL. > + // (only IRET or RETF with different CPL can clear BUSY flag) > + // Please refer to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64 for the > full stack frame at runtime. > + // According to SDM (ver. 075 June 2021), shadow stack should be 32 > bytes aligned. > + // > + InterruptSsp = (UINT32)(((UINTN)ShadowStack + EFI_PAGES_TO_SIZE(1) - > (sizeof(UINT64) * 4)) & ~0x1f); > + *(UINT64 *)(UINTN)InterruptSsp = (InterruptSsp - sizeof(UINT64) * 4) | > 0x2; > + mCetInterruptSsp = InterruptSsp - sizeof(UINT64); > + > + mCetInterruptSspTable = (UINT32)(UINTN)(mSmmInterruptSspTables + > sizeof(UINT64) * 8 * CpuIndex); > + InterruptSspTable = (UINT64 *)(UINTN)mCetInterruptSspTable; > + InterruptSspTable[1] = mCetInterruptSsp; > + PatchInstructionX86 (mPatchCetInterruptSsp, mCetInterruptSsp, 4); > + PatchInstructionX86 (mPatchCetInterruptSspTable, mCetInterruptSspTable, > 4); > + DEBUG ((DEBUG_INFO, "mCetInterruptSsp - 0x%x\n", mCetInterruptSsp)); > + DEBUG ((DEBUG_INFO, "mCetInterruptSspTable - 0x%x\n", > mCetInterruptSspTable)); > } > } > > -- > 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#83678): https://edk2.groups.io/g/devel/message/83678 Mute This Topic: https://groups.io/mt/86997767/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
