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-boun...@lists.01.org] On > Behalf Of Johnson, Brian (EXL - Eagan) > Sent: Tuesday, August 29, 2017 8:17 AM > To: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J > <jian.j.w...@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-boun...@lists.01.org] On > Behalf Of Yao, Jiewen > Sent: Sunday, August 27, 2017 10:39 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2- > de...@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....@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.w...@intel.com<mailto:jian.j.w...@intel.com>>; edk2- > de...@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-boun...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
