From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 936052095BB63 for ; Wed, 30 Aug 2017 17:54:43 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Aug 2017 17:55:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,451,1498546800"; d="scan'208,217";a="1009468462" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga003.jf.intel.com with ESMTP; 30 Aug 2017 17:55:41 -0700 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 30 Aug 2017 17:55:41 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 30 Aug 2017 17:55:41 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.39]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.168]) with mapi id 14.03.0319.002; Thu, 31 Aug 2017 08:55:33 +0800 From: "Yao, Jiewen" To: Andrew Fish CC: "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH 0/2] Implement NULL pointer detection feature Thread-Index: AQHTH6igjkiL4DJEAEOnxf834F2bE6KZFIjg//+ATICAAIhi0IAB0PSAgAIHrqD//55pAIABEPMQ Date: Thu, 31 Aug 2017 00:55:33 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9A4601@shsmsx102.ccr.corp.intel.com> References: <20170828025109.5032-1-jian.j.wang@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9A0884@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9A09CB@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9A43E1@shsmsx102.ccr.corp.intel.com> <57E6C9C0-7505-4040-9461-B9836934956F@apple.com> In-Reply-To: <57E6C9C0-7505-4040-9461-B9836934956F@apple.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH 0/2] Implement NULL pointer detection feature X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Aug 2017 00:54:43 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Andrew You raised a good point. I have seen a system reset when the below 1MiB address is access before mem= ory initialization. I think NULL pointer access before MRC should never hap= pen in a production BIOS. The debug ability is a big problem. Fully agree. I think we may extend that topic to separate PEI to pre-mem and post-mem by= using different strategy. I will file a bugzillar to record this. Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andr= ew Fish Sent: Thursday, August 31, 2017 12:27 AM To: Yao, Jiewen Cc: edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature > On Aug 30, 2017, at 7:55 AM, Yao, Jiewen > wrote: > > Hi Brian > Good feedback. > Comment in line. > > From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com] > Sent: Tuesday, August 29, 2017 11:17 PM > To: Yao, Jiewen >; Wang= , Jian J >; 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 catchin= g pointer-related problems. We have used a similar scheme on our systems f= or years, and caught several important bugs. Some comments: > > * It's possible to implement similar protections in PEI (IA32) by modifyi= ng the GDT descriptors. > [Jiewen] Enabling PEI is interesting. > Since we are working on DXE and SMM as our first priority, can we treat t= he PEI enabling as a separate task? > If you can help to create a patch for PEI and submit, that will be even b= etter. > Jiewen, This is slightly off topic but I've noticed in early PEI the issue on some = x86 chipsets is not that a NULL pointer reference is bad, but any reference= to an address space that is not decoded will triple bus fault the processo= r and that is really hard to debug. If the GDT trick could be used to cause= an exception that drops into the debugger vs. a triple bus fault that woul= d be a real win for debugging. While the memory ranges would be platform sp= ecific it would be awesome to have some plumbing in the edk2 to make this e= asier to implement. Thanks, Andrew Fish > > > * For flexibility, I'd like NULL pointer protection to be controlled inde= pendently in PEI, DXE, and SMM, using separate PCDs. > [Jiewen] I think we can existing example on using one PCD to control all = phase or using one PCD to control different phase. > For example, we use below PCD to control all phases, PEI/DXE/SMM > ## The mask is used to control PerformanceLib behavior.

> # BIT0 - Enable Performance Measurement.
> # @Prompt Performance Measurement Property. > # @Expression 0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibr= aryPropertyMask & 0xFE) =3D=3D 0 > gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x000= 00009 > > And we use below PCD to control different phase, DXE/SMM > ## The mask is used to control memory profile behavior.

> # BIT0 - Enable UEFI memory profile.
> # BIT1 - Enable SMRAM profile.
> # BIT7 - Disable recording at the start.
> # @Prompt Memory Profile Property. > # @Expression 0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryPro= filePropertyMask & 0x7C) =3D=3D 0 > gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x= 30001041 > > In order to make the code consistent with the existing PCD, I am thinking= to use one PCD named PcdNullPointerDetectionPropertyMask > # BIT0 - Enable NULL pointer detection for UEFI.
> # BIT1 - Enable NULL pointer detection for SMM.
> # BIT2 - Enable NULL pointer detection for PEI.
> # BIT7 - Disable NULL pointer detection after EndOfDxe.
> > I am not so worried about pre-memory initialization PEI phase, because pa= ge 0 is always invalid. > For post-memory initialization PEI phase, we can modify the page table in= memory, and dynamic PCD can be used at that time. > > > * We have seen various option ROMs and OS boot loaders which have NULL po= inter 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 b= efore running these other executables. So I'd suggest adding another PCD, = perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection = late in boot. If PcdNullPointerDetection !=3D PcdNullPointerDetectionPostD= xe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes = the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAt= tributes(CpuArch, 0, EFI_PAGE_SIZE, 0). > [Jiewen] Good point for the compatibility consideration. > Another thing is that CSM module may need access address 0. We have 2 cho= ices: > 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access = page 0, then disable page 0. > 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the b= eginning, and always keep it enabled. > Any though on that? > > > 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 Ya= o, Jiewen > Sent: Sunday, August 27, 2017 10:39 PM > To: Wang, Jian J >>; edk2-devel@list= s.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 >>; 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 re= set its default value. This feature makes use of page mechanism to detect N= ULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Cur= rently validated platform includes VLV2 and Denlow. Let me know if all plat= form 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 OV= MF with TRUE. > > > 2) It's hard to make it a dynamic feature because we need to setup p= age table for physical address 0-4095 in advance. If there's no memory allo= c/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 cas= e. > > [Jiewen] Dynamic means the initial value is dynamic. I am not saying we n= eed modify the page table. > > An implementation could be: > A platform can set this PCD in PEI phase based upon a setup variable to c= hoose CSM enable or disable. > > The IPL sets page table based upon this PCD value. The DXE Core cannot co= nsume PCD directly, because it might be dynamic. But we can pass the inform= ation 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 >>>; 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 platform= s 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 dyn= amic, as such, a platform may set the validate based upon CSM enable/disabl= e? > > > 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 W= ang, >> Jian J >> Sent: Monday, August 28, 2017 10:51 AM >> To: 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>> >> 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