Re: [edk2] [PATCH v3 00/11] Implement stack guard feature
Jiewen,
Thanks for the comments.
1) It's only used for Ex version of the API so I added Ex. But I don't have
strong opinion on the name.
2) Do you mean we need to use separate the definition for IA32 and X64 even
they share the same data?
3) Sure.
4) I'm not sure if it's necessary. But a version field won't do any harm.
5) You're right it's just for exceptions needing stack switch. I'll change the
wording.
6) Yes. ExceptionTss is better.
7) You're right. I'll add sanity checks.
8) Yes. I think current data meets SMM requirement. Let me know if you find
anything missing.
Jian
> -Original Message-
> From: Yao, Jiewen
> Sent: Tuesday, December 05, 2017 10:03 AM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH v3 00/11] Implement stack guard feature
>
> Good enhancement. I think it resolved my compatibility concern for the old
> API.
>
> Some comment:
>
> 1) Can we just use CPU_EXCEPTION_INIT_DATA instead of
> CPU_EXCEPTION_INIT_DATA_EX? I am not sure why we add _EX here.
>
> 2) Is CPU_EXCEPTION_INIT_DATA_EX for IA32 only or both IA32/X64? I found
> "Ia32" may bring confusing here. See EFI_DEBUG_SUPPORT_PROTOCOL, we
> have Ia32 for IA32 arch, X64 for X64 arch.
>
> //
> // Flag to indicate if default handlers should be initialized or not.
> //
> BOOLEAN InitDefaultHandlers;
> } Ia32;
> } CPU_EXCEPTION_INIT_DATA_EX;
>
> 3) Can we add IdtTableSize in CPU_EXCEPTION_INIT_DATA_EX?
>
> 4) Can we add Version field in CPU_EXCEPTION_INIT_DATA_EX? I am not sure if
> we need add more entry later.
>
> 5) You mentioned "KnownGoodStackTop is for *ALL* exceptions".
> Does ALL here mean StackSwitchExceptionNumber, or arch specific number such
> as 0x20 for X86 system?
> I think StackSwitchExceptionNumber is enough.
>
> 6) There might be more than one TSS entry in GDT.
> Does TssDesc/Tss in CPU_EXCEPTION_INIT_DATA_EX mean the exception Tss?
> (normal TSS does not need be reported here)
> If so, I suggest we use ExceptionTss as the keyword.
>
> 7) Below code may cause buffer overrun on IST.
>
> for (Index = 0; Index < StackSwitchData->Ia32.StackSwitchExceptionNumber;
> ++Index) {
> //
> // Fixup IST
> //
> Tss->IST[Index] = StackTop;
>
> I suggest we add some basic check for StackSwitchExceptionNumber.
>
> 8) Do you think we need mention the TssDesc/Tss size requirement for that?
>
> TssDesc = StackSwitchData->Ia32.TssDesc;
> Tss = StackSwitchData->Ia32.Tss;
> for (Index = 0; Index < StackSwitchData->Ia32.StackSwitchExceptionNumber;
> ++Index) {
> TssDesc += 1;
> Tss += 1;
>
> I suggest we add TssDescSize and TssSize in CPU_EXCEPTION_INIT_DATA_EX
> and check the size in the code.
>
> 9) Last but not least important, have you evaluated if current
> CPU_EXCEPTION_INIT_DATA_EX is enough for SMM version stack guard
> exception?
>
>
> Thank you
> Yao Jiewen
>
>
>
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian
> J
> > Wang
> > Sent: Friday, December 1, 2017 10:37 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v3 00/11] Implement stack guard feature
> >
> > > v3:
> > > a. Change new API InitializeCpuExceptionStackSwitchHandlers() to
> > > InitializeCpuExceptionHandlersEx(). Related code are updated
> > accordingly.
> > > b. Move EXCEPTION_STACK_SWITCH_DATA to CpuExceptionHandlerLib.h
> > > and change the name to CPU_EXCEPTION_INIT_DATA_EX for the sake
> > > of the API name change.
> > > c. Add more general macros in BaseLib.h.
> > > d. Add dummy implementation of InitializeCpuExceptionHandlersEx for
> > > SEC, PEI and SMM but implement a full version for DXE.
> > > e. Add dummy InitializeCpuExceptionHandlersEx for ARM's
> > CpuExceptionHandlerLib
> > > and NULL version of CpuExceptionHandlerLib
> > > f. Call InitializeCpuExceptionHandlersEx() in DxeMain instead of
> > > InitializeCpuExceptionHandlers().
> >
> >
> > > v2:
> > > a. Introduce and implement new API
> > InitializeCpuExceptionStackSwitchHandlers().
> > > b. Add stack switch related general definitions of IA32 in BaseLib.h.
> > > c. Add two new PCDs to configure exception vector list and stack size.
> > > d. Add code to save/restore GDTR, IDTR and TR for AP.
> > > e. Refactor exception handler code for stack switch.
> > > f. Add code to setup stack switch for AP besides BSP.
&
Re: [edk2] [PATCH v3 00/11] Implement stack guard feature
Good enhancement. I think it resolved my compatibility concern for the old API.
Some comment:
1) Can we just use CPU_EXCEPTION_INIT_DATA instead of
CPU_EXCEPTION_INIT_DATA_EX? I am not sure why we add _EX here.
2) Is CPU_EXCEPTION_INIT_DATA_EX for IA32 only or both IA32/X64? I found "Ia32"
may bring confusing here. See EFI_DEBUG_SUPPORT_PROTOCOL, we have Ia32 for IA32
arch, X64 for X64 arch.
//
// Flag to indicate if default handlers should be initialized or not.
//
BOOLEAN InitDefaultHandlers;
} Ia32;
} CPU_EXCEPTION_INIT_DATA_EX;
3) Can we add IdtTableSize in CPU_EXCEPTION_INIT_DATA_EX?
4) Can we add Version field in CPU_EXCEPTION_INIT_DATA_EX? I am not sure if we
need add more entry later.
5) You mentioned "KnownGoodStackTop is for *ALL* exceptions".
Does ALL here mean StackSwitchExceptionNumber, or arch specific number such as
0x20 for X86 system?
I think StackSwitchExceptionNumber is enough.
6) There might be more than one TSS entry in GDT.
Does TssDesc/Tss in CPU_EXCEPTION_INIT_DATA_EX mean the exception Tss? (normal
TSS does not need be reported here)
If so, I suggest we use ExceptionTss as the keyword.
7) Below code may cause buffer overrun on IST.
for (Index = 0; Index < StackSwitchData->Ia32.StackSwitchExceptionNumber;
++Index) {
//
// Fixup IST
//
Tss->IST[Index] = StackTop;
I suggest we add some basic check for StackSwitchExceptionNumber.
8) Do you think we need mention the TssDesc/Tss size requirement for that?
TssDesc = StackSwitchData->Ia32.TssDesc;
Tss = StackSwitchData->Ia32.Tss;
for (Index = 0; Index < StackSwitchData->Ia32.StackSwitchExceptionNumber;
++Index) {
TssDesc += 1;
Tss += 1;
I suggest we add TssDescSize and TssSize in CPU_EXCEPTION_INIT_DATA_EX and
check the size in the code.
9) Last but not least important, have you evaluated if current
CPU_EXCEPTION_INIT_DATA_EX is enough for SMM version stack guard exception?
Thank you
Yao Jiewen
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, December 1, 2017 10:37 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v3 00/11] Implement stack guard feature
>
> > v3:
> > a. Change new API InitializeCpuExceptionStackSwitchHandlers() to
> > InitializeCpuExceptionHandlersEx(). Related code are updated
> accordingly.
> > b. Move EXCEPTION_STACK_SWITCH_DATA to CpuExceptionHandlerLib.h
> > and change the name to CPU_EXCEPTION_INIT_DATA_EX for the sake
> > of the API name change.
> > c. Add more general macros in BaseLib.h.
> > d. Add dummy implementation of InitializeCpuExceptionHandlersEx for
> > SEC, PEI and SMM but implement a full version for DXE.
> > e. Add dummy InitializeCpuExceptionHandlersEx for ARM's
> CpuExceptionHandlerLib
> > and NULL version of CpuExceptionHandlerLib
> > f. Call InitializeCpuExceptionHandlersEx() in DxeMain instead of
> > InitializeCpuExceptionHandlers().
>
>
> > v2:
> > a. Introduce and implement new API
> InitializeCpuExceptionStackSwitchHandlers().
> > b. Add stack switch related general definitions of IA32 in BaseLib.h.
> > c. Add two new PCDs to configure exception vector list and stack size.
> > d. Add code to save/restore GDTR, IDTR and TR for AP.
> > e. Refactor exception handler code for stack switch.
> > f. Add code to setup stack switch for AP besides BSP.
>
> Stack guard feature makes use of paging mechanism to monitor if there's a
> stack overflow occurred during boot. A new PCD PcdCpuStackGuard is added to
> enable/disable this feature. PCD PcdCpuStackSwitchExceptionList and
> PcdCpuKnownGoodStackSize are introduced to configure the required
> exceptions
> and stack size.
>
> If this feature is enabled, DxeIpl will setup page tables and set page where
> the stack bottom is at to be NON-PRESENT. If stack overflow occurs, Page
> Fault exception will be triggered.
>
> In order to make sure exception handler works normally even when the stack
> is corrupted, stack switching is implemented in exception library.
>
> Due to the mechanism behind Stack Guard, this feature is only avaiable for
> UEFI drivers (memory avaiable). That also means it doesn't support NT32
> emulated platform (paging not supported).
>
> Jian J Wang (11):
> MdeModulePkg/metafile: Add PCD PcdCpuStackGuard
> UefiCpuPkg/UefiCpuPkg.dec: Add two new PCDs for stack switch
> MdeModulePkg/CpuExceptionHandlerLib.h: Add a new API
> InitializeCpuExceptionHandlersEx
> MdePkg/BaseLib: Add stack switch related definitions for IA32
> UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support
> Md
[edk2] [PATCH v3 00/11] Implement stack guard feature
> v3: > a. Change new API InitializeCpuExceptionStackSwitchHandlers() to > InitializeCpuExceptionHandlersEx(). Related code are updated accordingly. > b. Move EXCEPTION_STACK_SWITCH_DATA to CpuExceptionHandlerLib.h > and change the name to CPU_EXCEPTION_INIT_DATA_EX for the sake > of the API name change. > c. Add more general macros in BaseLib.h. > d. Add dummy implementation of InitializeCpuExceptionHandlersEx for > SEC, PEI and SMM but implement a full version for DXE. > e. Add dummy InitializeCpuExceptionHandlersEx for ARM's > CpuExceptionHandlerLib > and NULL version of CpuExceptionHandlerLib > f. Call InitializeCpuExceptionHandlersEx() in DxeMain instead of > InitializeCpuExceptionHandlers(). > v2: > a. Introduce and implement new API > InitializeCpuExceptionStackSwitchHandlers(). > b. Add stack switch related general definitions of IA32 in BaseLib.h. > c. Add two new PCDs to configure exception vector list and stack size. > d. Add code to save/restore GDTR, IDTR and TR for AP. > e. Refactor exception handler code for stack switch. > f. Add code to setup stack switch for AP besides BSP. Stack guard feature makes use of paging mechanism to monitor if there's a stack overflow occurred during boot. A new PCD PcdCpuStackGuard is added to enable/disable this feature. PCD PcdCpuStackSwitchExceptionList and PcdCpuKnownGoodStackSize are introduced to configure the required exceptions and stack size. If this feature is enabled, DxeIpl will setup page tables and set page where the stack bottom is at to be NON-PRESENT. If stack overflow occurs, Page Fault exception will be triggered. In order to make sure exception handler works normally even when the stack is corrupted, stack switching is implemented in exception library. Due to the mechanism behind Stack Guard, this feature is only avaiable for UEFI drivers (memory avaiable). That also means it doesn't support NT32 emulated platform (paging not supported). Jian J Wang (11): MdeModulePkg/metafile: Add PCD PcdCpuStackGuard UefiCpuPkg/UefiCpuPkg.dec: Add two new PCDs for stack switch MdeModulePkg/CpuExceptionHandlerLib.h: Add a new API InitializeCpuExceptionHandlersEx MdePkg/BaseLib: Add stack switch related definitions for IA32 UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support MdeModulePkg/CpuExceptionHandlerLibNull: Add new API implementation ArmPkg/ArmExceptionLib: Add implementation of new API UefiCpuPkg/MpLib: Add GDTR, IDTR and TR in saved AP data UefiCpuPkg/CpuDxe: Initialize stack switch for MP MdeModulePkg/Core/Dxe: Call new API InitializeCpuExceptionHandlersEx instead MdeModulePkg/DxeIpl: Enable paging for Stack Guard ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c | 33 ++ MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c| 2 +- MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf| 5 +- MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c| 4 + MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 1 + MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 51 ++- .../Include/Library/CpuExceptionHandlerLib.h | 78 .../CpuExceptionHandlerLibNull.c | 34 ++ MdeModulePkg/MdeModulePkg.dec | 7 + MdeModulePkg/MdeModulePkg.uni | 7 + MdePkg/Include/Library/BaseLib.h | 117 ++ MdePkg/Library/BaseLib/BaseLib.inf | 3 + MdePkg/Library/BaseLib/Ia32/WriteTr.nasm | 36 ++ MdePkg/Library/BaseLib/X64/WriteTr.nasm| 37 ++ UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 + UefiCpuPkg/CpuDxe/CpuMp.c | 177 + .../CpuExceptionHandlerLib/CpuExceptionCommon.h| 39 ++ .../DxeCpuExceptionHandlerLib.inf | 6 + .../Library/CpuExceptionHandlerLib/DxeException.c | 79 .../Ia32/ArchExceptionHandler.c| 167 + .../Ia32/ArchInterruptDefs.h | 8 + .../Ia32/ExceptionTssEntryAsm.nasm | 398 + .../CpuExceptionHandlerLib/PeiCpuException.c | 34 +- .../PeiCpuExceptionHandlerLib.inf | 1 + .../CpuExceptionHandlerLib/SecPeiCpuException.c| 34 +- .../SecPeiCpuExceptionHandlerLib.inf | 1 + .../SmmCpuExceptionHandlerLib.inf | 1 + .../Library/CpuExceptionHandlerLib/SmmException.c | 34 +- .../X64/ArchExceptionHandler.c | 134 +++ .../CpuExceptionHandlerLib/X64/ArchInterruptDefs.h | 3 + UefiCpuPkg/Library/MpInitLib/MpLib.c | 17 + UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 + UefiCpuPkg/UefiCpuPkg.dec | 12 + 33 files changed, 1547 insertions(+), 19 deletions(-) create mode 100644 MdePkg/Library/BaseLib/Ia32/WriteTr.nasm create mode 100644 MdePkg/Library/BaseLib/X64/WriteTr.nasm create mode 100644
