Re: [PATCH V3 0/4] Add SMM CET support

["Yao, Jiewen" <jiewen.yao@intel.com>]

Good comment!
Response inline


thank you!
Yao, Jiewen


> 在 2019年2月23日，上午5:42，Laszlo Ersek <lersek@redhat.com> 写道：
> 
> Hi Jiewen,
> 
>> On 02/22/19 14:30, Jiewen Yao wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1521
>> 
>> V3:
>> Add Nasm.inc to include CET related instruction as MACRO.
>> This is the only place to use DB.
>> Any other NASM just use the MACRO - 
>> SETSSBSY, READSSP_[E|R]AX, INCSSP_[E|R]AX
>> =====================
>> 
>> V2:
>> Fix emulation platform issue.
>> The NT32 platform cannot access CR4 register.
>> So we add a global PCD to choose disable CR4 access in SetJump/LongJump.
>> gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask
>> =====================
> 
> (1) I think there is another difference (I don't know if it was
> introduced in v2 or in v3; I only compared v1<->v3). It seems that the
> LongJump / SetJump changes for IA32 MSFT were implemented in v2/v3 as well.
[jiewen] you are right. I realize that I forgot to Chang the C file. I only changed the nasm file in V1. This is not caught because we don’t have IA32 CET enabled platform, as I mentioned in V1 comment.
I think we should only have 1 solution. Both C and Nasm is a bad choice, that increase the maintenance effort and validation effort. 
I have talked with Liming. Hope we will do sth after Q1 release. 

> 
> (2) When we introduce another bit for
> PcdControlFlowEnforcementPropertyMask, we'll have to update the checks,
> because currently we check the whole PCD against zero. When the next bit
> is introduced, we'll have to use a bitmask (with value 1) for checking.
> Anyway that can indeed be a later enhancement, just stating what I've
> noticed.
[jiewen] Yes I did think a lot what check we should do.
The potential future bit is: 1) SMM IBT support. 2) DXE SSP support. 3) DXE IBT support. We have not done IBT yet today because it depends upon compiler update. For DXE I did POC as test environment. 
If we add SMM IBT, some check should be global CET. Some should be SSP specific. Case by case. I think we can cross the bridge when we come to it. 

Anyway both 1 and 2 are excellent feedback. Appreciate your review.  


> 
> (3) For the series:
> 
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
[jiewen] thank you!
> 
> Thanks,
> Laszlo
> 
>> 
>> This patch series implement add CET ShadowStack support for SMM.
>> 
>> The CET document can be found at:
>> https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf
>> 
>> Patch 1 adds SSP (ShadowStackPointer) to JUMP_BUFFER.
>> Patch 2 adds Control Protection exception (CP#) dump info.
>> Patch 3 adds CET ShadowStack support in SMM.
>> 
>> For more detail please refer to each patch. 
>> 
>> I also post all update to https://github.com/jyao1/edk2/tree/CET_V2
>> 
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Yao Jiewen <jiewen.yao@intel.com>
>> 
>> Jiewen Yao (4):
>>  MdePkg/Include: Add Nasm.inc
>>  MdePkg/BaseLib: Add Shadow Stack Support for X86.
>>  UefiCpuPkg/ExceptionLib: Add CET support.
>>  UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86 SMM.
>> 
>> MdePkg/Include/Ia32/Nasm.inc                  |  28 ++++
>> MdePkg/Include/Library/BaseLib.h              |   2 +
>> MdePkg/Include/X64/Nasm.inc                   |  28 ++++
>> MdePkg/Library/BaseLib/BaseLib.inf            |   3 +-
>> MdePkg/Library/BaseLib/Ia32/LongJump.c        |  28 +++-
>> MdePkg/Library/BaseLib/Ia32/LongJump.nasm     |  25 +++-
>> MdePkg/Library/BaseLib/Ia32/SetJump.c         |  28 +++-
>> MdePkg/Library/BaseLib/Ia32/SetJump.nasm      |  23 +++-
>> MdePkg/Library/BaseLib/X64/LongJump.nasm      |  27 +++-
>> MdePkg/Library/BaseLib/X64/SetJump.nasm       |  23 +++-
>> MdePkg/MdePkg.dec                             |   7 +
>> .../Include/Library/SmmCpuFeaturesLib.h       |  23 +++-
>> .../CpuExceptionCommon.c                      |   7 +-
>> .../CpuExceptionCommon.h                      |   3 +-
>> .../Ia32/ArchExceptionHandler.c               |   5 +-
>> .../X64/ArchExceptionHandler.c                |   5 +-
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm       |  39 ++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c      |  38 +++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm  |  99 ++++++++++++++-
>> .../PiSmmCpuDxeSmm/Ia32/SmiException.nasm     |   6 +-
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c |  57 ++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c         |  12 +-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c    |  97 ++++++++++++--
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    | 103 ++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |   6 +-
>> .../PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c   |  85 ++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c        |  18 ++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h        |   4 +-
>> UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c    |   4 +-
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm        |  40 ++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c       |  39 +++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm   | 120 +++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  |  58 ++++++++-
>> UefiCpuPkg/UefiCpuPkg.dec                     |   6 +-
>> 34 files changed, 1034 insertions(+), 62 deletions(-)
>> create mode 100644 MdePkg/Include/Ia32/Nasm.inc
>> create mode 100644 MdePkg/Include/X64/Nasm.inc
>> create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
>> create mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
>> 
>