Re: [PATCH 0/2] Implement NULL pointer detection feature

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Brian,

Do you prefer 4 new PCDs or one new PCD with a bitmask to enable
detection in different phases?

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Johnson, Brian (EXL - Eagan)
> Sent: Tuesday, August 29, 2017 8:17 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Thank you for implementing this feature!  It is very  helpful
> for catching pointer-related problems.  We have used a similar
> scheme on our systems for years, and caught several important
> bugs.  Some comments:
> 
> * It's possible to implement similar protections in PEI (IA32)
> by modifying the GDT descriptors.
> 
> * For flexibility, I'd like NULL pointer protection to be
> controlled independently in PEI, DXE, and SMM, using separate
> PCDs.
> 
> * We have seen various option ROMs and OS boot loaders which
> have NULL pointer issues, but are outside of our control.  It
> is useful to enable NULL pointer protection during as much of
> the boot as possible, but disable it before running these other
> executables.  So I'd suggest adding another PCD, perhaps
> PcdNullPointerDetectionPostDxe, to control NULL pointer
> protection late in boot.  If PcdNullPointerDetection !=
> PcdNullPointerDetectionPostDxe, install an end-of-DXE event
> (gEfiEndOfDxeEventGroupGuid) which changes the protection of
> page 0 using a call to EFI_CPU_ARCH_PROTOCOL.
> SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> 
> So ideally I'd like to have 4 PCDs:
> 
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
> 
> Thanks,
> Brian Johnson
> HPE
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Comment in line.
> 
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> 
> 1)      I think this feature should be 'FALSE' by default. I
> forgot to reset its default value. This feature makes use of
> page mechanism to detect NULL pointer. So any ARCH doesn't
> support paging in EDK-II can't use it. Currently validated
> platform includes VLV2 and Denlow. Let me know if all platform
> must be validated or not.
> 
> [Jiewen] Yes, I am OK to set to be FALSE to provide best
> compatibility.
> I suggest we validate all open source IA platforms, such as
> Quark, and OVMF with TRUE.
> 
> 
> 2)      It's hard to make it a dynamic feature because we need
> to setup page table for physical address 0-4095 in advance. If
> there's no memory alloc/free action after enabling this
> feature, there's no chance to make those change in page table.
> Then the usage of feature will be limited in such case.
> 
> [Jiewen] Dynamic means the initial value is dynamic. I am not
> saying we need modify the page table.
> 
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup
> variable to choose CSM enable or disable.
> 
> The IPL sets page table based upon this PCD value. The DXE Core
> cannot consume PCD directly, because it might be dynamic. But
> we can pass the information from IPL via HOB. All the DXE
> module just checks the value based upon HOB.
> 
> 
> 
> 
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J
> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-
> devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Thank you  to enable this feature.
> 
> I have 2 comments, after a very quick review.
> 
> 
> 1)      I notice it is enabled by default
> "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
> 
> Would you please provide the information on how many open
> source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
> 
> 
> 
> 2)      I am thinking about CSM platform. Do you think we can
> make it dynamic, as such, a platform may set the validate based
> upon CSM enable/disable?
> 
> 
> Or if we need update the CSM module to patch the page table
> automatically? Once this is feature is ON.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Wang,
> > Jian J
> > Sent: Monday, August 28, 2017 10:51 AM
> > To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection
> feature
> >
> > This patch is the implementation of NULL pointer detection
> feature,
> > which is one of the small features of Special Pool.
> >
> > Wang, Jian J (2):
> >   Implement NULL pointer detection for EDK-II Core
> >   Implement NULL pointer detection for EDK-II SMM Core and
> driver
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
> >  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> > ++++++++++++++++--------
> >  MdeModulePkg/MdeModulePkg.dec                    |  7
> +++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12
> +++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> > ++++++++++++++++++++++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12
> +++++++++++
> >  10 files changed, 84 insertions(+), 14 deletions(-)
> >
> > --
> > 2.11.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel