From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 7704D20958BC0 for ; Thu, 31 Aug 2017 22:19:26 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Aug 2017 22:22:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,456,1498546800"; d="scan'208";a="144365965" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga005.jf.intel.com with ESMTP; 31 Aug 2017 22:22:10 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 31 Aug 2017 22:22:09 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 31 Aug 2017 22:22:09 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.219]) by shsmsx102.ccr.corp.intel.com ([169.254.2.39]) with mapi id 14.03.0319.002; Fri, 1 Sep 2017 13:22:05 +0800 From: "Wang, Jian J" To: "Wang, Jian J" , "Yao, Jiewen" , "Johnson, Brian (EXL - Eagan)" , "afish@apple.com" CC: "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH 0/2] Implement NULL pointer detection feature Thread-Index: AQHTH6igQmzEVUzn9U+ghaCw9vNBcqKYkNsAgACGtTD//4FkgIACVTaAgAGMZICAABmzAIAAVlEAgAA9fACAAh9NQIAAOFTw Date: Fri, 1 Sep 2017 05:22:05 +0000 Message-ID: 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> <74D8A39837DF1E4DA445A8C0B3885C503A9A4642@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: 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 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: Fri, 01 Sep 2017 05:19:26 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Updated version according to some internal discussions: a) Use one PCD with bitmask to enable/disable this feature for different bo= ot phases. This PCD is a "fixed" PCD with type of UINT8, and all bits are c= leared by default. Please note that setting BIT7 with BIT0 set is used to "= DISABLE" NULL pointer detection for code after EndOfDxe as a workaround for= non-fixable NULL access in OpROM or boot loaders. gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask # BIT0 - Enable NULL pointer detection for UEFI.
# BIT1 - Enable NULL pointer detection for SMM.
# BIT2-6 - Reserved for PEI and other possible uses.
# BIT7 - Disable NULL pointer detection after EndOfDxe.
b) Add PEI support. Since PEI requirement has been covered by following new= bug tracker, it won't be included by current patch update but a separate o= ne. https://bugzilla.tianocore.org/show_bug.cgi?id=3D687 c) CSM code lines which access page 0 will be enclosed by a= nd then code. This is subject to change if critical perfor= mance issue is found. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang= , Jian J Sent: Friday, September 01, 2017 10:27 AM To: Yao, Jiewen ; Johnson, Brian (EXL - Eagan) ; afish@apple.com Cc: edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature Let me summarize the changes to this feature. If no new comments, I'll star= t to make changes to current implementation. a) Use one PCD with bit set/clear to enable/disable this feature for differ= ent boot phases. To make it easier to use, I'd suggest to use the bit seque= nce to follow the boot phase sequence (bit0->bit1->bit2......bit7 =3D=3D=3D= > PEI->DXE->PostDxe......SMM), just like below. SMM is a little bit special= so it consumes the MSB. gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask # BIT0 - Enable NULL pointer detection for PEI.
# BIT1 - Enable NULL pointer detection for DXE.
# BIT2 - Enable NULL pointer detection after EndOfDxe.
# BIT7 - Disable NULL pointer detection for SMM.
This PCD is a "fixed" PCD and all bits are cleared by default. Since th= is is a feature for debug purpose, I see no reason to make it dynamic.=20 b) Add PEI and post DXE phase support. Since PEI phase requirement has been= covered by following new bug tracker, it won't be included by current patc= h update but a separate one. Changes for Post DXE phase will be included. https://bugzilla.tianocore.org/show_bug.cgi?id=3D687 Thanks, Wang, Jian J -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao,= Jiewen Sent: Thursday, August 31, 2017 9:16 AM To: Johnson, Brian (EXL - Eagan) ; afish@apple.com Cc: edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature Thanks. I have filed https://bugzilla.tianocore.org/show_bug.cgi?id=3D687 for PEI p= hase, with suggestion from Brian Johnson and Andrew Fish. If you share some of your PEI code to elimination the duplicated effort, th= at would be great. Yes, we can calculate the perf data in CSM to see what is the best way to e= nable this feature. Good suggestion. Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of John= son, Brian (EXL - Eagan) Sent: Thursday, August 31, 2017 5:36 AM To: afish@apple.com; Yao, Jiewen Cc: edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature Jiewen, Certainly PEI could be done later. There's no need to get all the code in = at once, and you have the DXE/SMM part pretty much ready to go. I'm willing to go with a single PCD with a bitmap, or multiple PCDs. Whate= ver seems best. IIRC protecting page 0 in PEI involves modifying the main GCD descriptor to= be a grows-downward variety, with a limit above page 0. That way you get = an exception if page 0 is accessed. I'd have to check on the steps needed = to release the actual code, which may take quite a while. You may be bette= r off just doing it yourself. I'll take another look at the code to make s= ure I'm explaining this properly, though. CSM is one of the few pieces of BIOS which has a legitimate need to access = page 0. We get the best protection by having it enable, access, and then d= isable page 0. But if that affects performance too badly, we may need to h= ave it leave page 0 enabled. I don't enable CSM on the platforms I work on= , so it's not something I have much to say about. Those who use CSM would = want to weigh in. Brian Johnson -----Original Message----- From: afish@apple.com [mailto:afish@apple.com] Sent: Wednesday, August 30, 2017 11:27 AM To: Yao, Jiewen > Cc: Johnson, Brian (EXL - Eagan) >; Wang, Jian J >; 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 _______________________________________________ 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