From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from msmail.insydesw.com.tw (ms.insydesw.com [211.75.113.220]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6243921E2BE4F for ; Tue, 29 Aug 2017 10:11:29 -0700 (PDT) Received: from msmail.insydesw.com.tw ([fe80::74f7:f173:f4aa:9a05]) by msmail.insydesw.com.tw ([fe80::74f7:f173:f4aa:9a05%11]) with mapi id 14.01.0438.000; Wed, 30 Aug 2017 01:14:07 +0800 From: Tim Lewis To: "Johnson, Brian (EXL - Eagan)" , "Kinney, Michael D" , "Yao, Jiewen" , "Wang, Jian J" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH 0/2] Implement NULL pointer detection feature Thread-Index: AQHTH6iP0nc8rV1nPkCXuqxYB9SesqKYkNwAgAAD+ICAAAQhgIACVTaAgAAMygCAABN+AIAAhofA Date: Tue, 29 Aug 2017 17:14:06 +0000 Message-ID: <7236196A5DF6C040855A6D96F556A53FCFEBB0@msmail.insydesw.com.tw> References: <20170828025109.5032-1-jian.j.wang@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9A0884@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9A09CB@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-TW X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.16.9.126] 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: Tue, 29 Aug 2017 17:11:30 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Just a +1 for 4 separate PCDs. Tim -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of John= son, Brian (EXL - Eagan) Sent: Tuesday, August 29, 2017 10:12 AM To: Kinney, Michael D ; Yao, Jiewen ; Wang, Jian J ; 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 cumbers= ome to use 4 PCDs. That way you don't have to define the meanings of indiv= idual bits, in the code or in the .dec file. And you could use different P= CD types for the different PCDs, eg. FeatureFlag or FixedAtBuild for PcdNul= lPointerDetectionPei (since the GCDs are built at compile time, and it woul= d take quite a bit of recoding to change that) but Dynamic for PcdNullPoint= erDetectionDxe, 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) ; Yao, Jiewen ; Wang, Jian J ; edk2-devel@lists.0= 1.org; Kinney, Michael D 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=20 > Johnson, Brian (EXL - Eagan) > Sent: Tuesday, August 29, 2017 8:17 AM > To: Yao, Jiewen ; Wang, Jian J=20 > ; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection=20 > feature >=20 > Thank you for implementing this feature! It is very helpful for=20 > catching pointer-related problems. We have used a similar scheme on=20 > our systems for years, and caught several important bugs. Some=20 > comments: >=20 > * It's possible to implement similar protections in PEI (IA32) by=20 > modifying the GDT descriptors. >=20 > * For flexibility, I'd like NULL pointer protection to be controlled=20 > independently in PEI, DXE, and SMM, using separate PCDs. >=20 > * We have seen various option ROMs and OS boot loaders which have NULL=20 > pointer issues, but are outside of our control. It is useful to=20 > enable NULL pointer protection during as much of the boot as possible,=20 > but disable it before running these other executables. So I'd suggest=20 > adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control=20 > NULL pointer protection late in boot. If PcdNullPointerDetection !=3D=20 > PcdNullPointerDetectionPostDxe, install an end-of-DXE event > (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0=20 > using a call to EFI_CPU_ARCH_PROTOCOL. > SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0). >=20 > So ideally I'd like to have 4 PCDs: >=20 > PcdNullPointerDetectionPei > PcdNullPointerDetectionDxe > PcdNullPointerDetectionSmm > PcdNullPointerDetectionPostDxe >=20 > Thanks, > Brian Johnson > HPE >=20 > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > Yao, Jiewen > Sent: Sunday, August 27, 2017 10:39 PM > To: Wang, Jian J ; edk2- devel@lists.01.org > Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection=20 > feature >=20 > Comment in line. >=20 > 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=20 > feature >=20 >=20 > 1) I think this feature should be 'FALSE' by default. I > forgot to reset its default value. This feature makes use of page=20 > mechanism to detect NULL pointer. So any ARCH doesn't support paging=20 > in EDK-II can't use it. Currently validated platform includes VLV2 and=20 > Denlow. Let me know if all platform must be validated or not. >=20 > [Jiewen] Yes, I am OK to set to be FALSE to provide best=20 > compatibility. > I suggest we validate all open source IA platforms, such as Quark, and=20 > OVMF with TRUE. >=20 >=20 > 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=20 > no memory alloc/free action after enabling this feature, there's no=20 > chance to make those change in page table. > Then the usage of feature will be limited in such case. >=20 > [Jiewen] Dynamic means the initial value is dynamic. I am not saying=20 > we need modify the page table. >=20 > An implementation could be: > A platform can set this PCD in PEI phase based upon a setup variable=20 > to choose CSM enable or disable. >=20 > The IPL sets page table based upon this PCD value. The DXE Core cannot=20 > consume PCD directly, because it might be dynamic. But we can pass the=20 > information from IPL via HOB. All the DXE module just checks the value=20 > based upon HOB. >=20 >=20 >=20 >=20 > From: Yao, Jiewen > Sent: Monday, August 28, 2017 11:10 AM > To: Wang, Jian J > >; edk2-=20 > devel@lists.01.org > Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection=20 > feature >=20 > Thank you to enable this feature. >=20 > I have 2 comments, after a very quick review. >=20 >=20 > 1) I notice it is enabled by default > "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE". >=20 > Would you please provide the information on how many open source=20 > platforms are validated? > Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)? > Or do we need validate any ARM platform? >=20 >=20 >=20 > 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=20 > CSM enable/disable? >=20 >=20 > Or if we need update the CSM module to patch the page table=20 > automatically? Once this is feature is ON. >=20 >=20 > Thank you > Yao Jiewen >=20 >=20 > > -----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 > > 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 ++++-- =20 > > 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