Good comment! Response inline
thank you! Yao, Jiewen > 在 2019年2月23日，上午5:42，Laszlo Ersek <ler...@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 <ler...@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.kin...@intel.com> >> Cc: Liming Gao <liming....@intel.com> >> Cc: Eric Dong <eric.d...@intel.com> >> Cc: Ray Ni <ray...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Yao Jiewen <jiewen....@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 >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
