Re: [PATCH v3 00/11] Implement stack guard feature

["Wang, Jian J" <jian.j.wang@intel.com>]

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.wang@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-bounces@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
> >   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
> >
> UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
> >
> > --
> > 2.14.1.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel