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

[Tim Lewis <tim.lewis@insyde.com>]

Just a +1 for 4 separate PCDs.

Tim

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Johnson, Brian (EXL - Eagan)
Sent: Tuesday, August 29, 2017 10:12 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; 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

It makes no difference to me.  But it sounds more flexible and less cumbersome to use 4 PCDs.  That way you don't have to define the meanings of individual bits, in the code or in the .dec file.  And you could use different PCD types for the different PCDs, eg. FeatureFlag or FixedAtBuild for PcdNullPointerDetectionPei (since the GCDs are built at compile time, and it would take quite a bit of recoding to change that) but Dynamic for PcdNullPointerDetectionDxe, as Jiewen requested.

But I'm good either way.

Brian Johnson

-----Original Message-----
From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
Sent: Tuesday, August 29, 2017 11:02 AM
To: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel