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-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....@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-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-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

Reply via email to