From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-in6.apple.com (mail-out6.apple.com [17.151.62.28]) (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 6F6E42095BB62 for ; Wed, 30 Aug 2017 09:27:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1504110437; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-transfer-encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=GZhUMgEd56n1R3AFPPgb2/MOoNfhDaKFy+A17SlxDVI=; b=FE2O5EIQC/mPk9ajzJoas0+c6FQPxPZisAyNznD3yT0rl1pGMIUb6tfTj+U7h+hm 4z9MPhii1dbKBP0y4r0EGO3QHzvO3Nb2p4IJPuYVkl0Rus//c6hzaBlwchtkq87W +PNj/Cqg1g8ln3QzifXxoQ1s70/dj/HRuP1M7vGsIanQljNZT/XdxOUotBudjYcb WXO+KzwgGfiE6KTTe8RFIezIFQHNnG3ecSXLtWKEIDSnuqkPgcFEFedTLRvEDoKy GgSuSGLKVYvdgMYaPocEAiKlyYxhQDmaU/tM/OM+5T0/ZRXWrNG8j/Q5902DgyDz J+tuFfM0RcH+iNxk5vJWLA==; Received: from relay2.apple.com (relay2.apple.com [17.128.113.67]) (using TLS with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail-in6.apple.com (Apple Secure Mail Relay) with SMTP id 01.17.11091.567E6A95; Wed, 30 Aug 2017 09:27:17 -0700 (PDT) X-AuditID: 11973e15-512379c000002b53-57-59a6e7658b5f Received: from nwk-mmpp-sz11.apple.com (nwk-mmpp-sz11.apple.com [17.128.115.155]) by relay2.apple.com (Apple SCV relay) with SMTP id 72.20.09069.467E6A95; Wed, 30 Aug 2017 09:27:17 -0700 (PDT) MIME-version: 1.0 Received: from da0601a-dhcp66.apple.com ([17.226.15.66]) by nwk-mmpp-sz11.apple.com (Oracle Communications Messaging Server 8.0.1.3.20170825 64bit (built Aug 25 2017)) with ESMTPSA id <0OVI00DQDB1GAE60@nwk-mmpp-sz11.apple.com>; Wed, 30 Aug 2017 09:27:16 -0700 (PDT) Sender: afish@apple.com From: Andrew Fish In-reply-to: <74D8A39837DF1E4DA445A8C0B3885C503A9A43E1@shsmsx102.ccr.corp.intel.com> Date: Wed, 30 Aug 2017 09:27:16 -0700 Cc: "Johnson, Brian (EXL - Eagan)" , "Wang, Jian J" , "edk2-devel@lists.01.org" Message-id: <57E6C9C0-7505-4040-9461-B9836934956F@apple.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> To: "Yao, Jiewen" X-Mailer: Apple Mail (2.3273) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrJLMWRmVeSWpSXmKPExsUi2FDorJv6fFmkwYtrbBaT+rawWew5dJTZ Yt63GawW6z56OLB47NrVyO6xeM9LJo/u2f9YApijuGxSUnMyy1KL9O0SuDI2bd3JUrDep+Lh xsgGxtN2XYycHBICJhJzPm1k7mLk4hASWM0kce32bBaYxNezy6AShxglnrRvZgZJ8AoISvyY fA+oiIODWUBe4uB5WZAws4CWxPdHrSwQ9ROZJGY+O8MKkhAWEJd4d2YTM4TtIfFt6mqwOJuA ssSK+R/YQWxOgTCJ7gn9YDNZBFQlPm4Bm8MsMJtR4tyLhVB7bSS6Vx5hh1iwhVniwcH7jCAJ EQENiRPz5zBDXC0rcWv2JbCrJQT2sEls+7+FaQKj8Cwkh89COHwWksMXMDKvYhTKTczM0c3M M9NLLCjISdVLzs/dxAgK++l2ojsYz6yyOsQowMGoxMOrsGlZpBBrYllxZe4hRmkOFiVxXhch oJBAemJJanZqakFqUXxRaU5q8SFGJg5OqQZGCdWXO5du0+BfdnGlun3Fpkm9OWsvFF3anCD2 iPklS5aA8snd7m//Z1a7/O9cfT6gNrB8+sXd8r+vnUzr5myc6KWn9v/u/j2K+geOlmWv//kw WJ/xhFXw+S87z7Y9VdU/0mYSf9lz/uxb8Zf/XZAwuOscI7ndcyr7Rbdso53/T5xffv9N4XZN GyWW4oxEQy3mouJEAFKp0ddcAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprNIsWRmVeSWpSXmKPExsUi2FA8Wzf1+bJIg58zmC0m9W1hs9hz6Ciz xbxvM1gt1n30cGDx2LWrkd1j8Z6XTB7ds/+xBDBHcdmkpOZklqUW6dslcGVs2rqTpWC9T8XD jZENjKftuhg5OSQETCS+nl3G3MXIxSEkcIhR4kn7ZmaQBK+AoMSPyfdYuhg5OJgF5CUOnpcF CTMLaEl8f9TKAlE/kUli5rMzrCAJYQFxiXdnNjFD2B4S36auBouzCShLrJj/gR3E5hQIk+ie 0A82k0VAVeLjFrA5zAKzGSXOvVgItddGonvlEXaIBVuYJR4cvM8IkhAR0JA4MX8OM8TVshK3 Zl9insAoMAvJrbMQbp2F5NYFjMyrGAWKUnMSK430EgsKclL1kvNzNzGCw7TQeQfjsWVWhxgF OBiVeHgrtiyLFGJNLCuuzAUGBgezkghv9COgEG9KYmVValF+fFFpTmrxIcYqoAcmMkuJJucD YyivJN7QxMTAxNjYzNjY3MScKsJK4rziKUsihQTSE0tSs1NTC1KLYJYzcXBKNTAmyAfHmF85 nqBQ1m0gJLhgZ+KEe7sVv9tdCFTjXM6uxNilvEnEck/KrRV2VefrP4jLtAbHMXhPv3rilfcX MVO/E0VtPL9793C7hDRaMDZNvOFva6bsv2LGL+7MbVMXnTi+0U1nqSuTFpu978SGXXc+WqlN uhE+y9M+oO5J9t2vRVZN86YkrFNiKc5INNRiLipOBABp5T2argIAAA== 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: Wed, 30 Aug 2017 16:27:13 -0000 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII > 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 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. > [Jiewen] Enabling PEI is interesting. > Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task? > If you can help to create a patch for PEI and submit, that will be even better. > 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 processor 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 would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement. Thanks, Andrew Fish > > > * For flexibility, I'd like NULL pointer protection to be controlled independently 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.PcdPerformanceLibraryPropertyMask & 0xFE) == 0 > gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009 > > 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.PcdMemoryProfilePropertyMask & 0x7C) == 0 > gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041 > > 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 page 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 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). > [Jiewen] Good point for the compatibility consideration. > Another thing is that CSM module may need access address 0. We have 2 choices: > 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 beginning, 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 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 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 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 >>; 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> >> 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