From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org 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 11A1721EC8D02 for ; Wed, 27 Sep 2017 20:28:12 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Sep 2017 20:31:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,448,1500966000"; d="scan'208";a="1176534552" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 27 Sep 2017 20:31:26 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Sep 2017 20:31:26 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Sep 2017 20:31:25 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Thu, 28 Sep 2017 11:31:24 +0800 From: "Zeng, Star" To: "Wang, Jian J" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , Laszlo Ersek , "Yao, Jiewen" , "Kinney, Michael D" , "Justen, Jordan L" , "Wolman, Ayellet" , "Zeng, Star" Thread-Topic: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection Thread-Index: AQHTN/WyKFom9+WfGECReA/0xBikRKLJoEQwgAAEC3A= Date: Thu, 28 Sep 2017 03:31:23 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B97BEDD@shsmsx102.ccr.corp.intel.com> References: <20170928010353.11968-1-jian.j.wang@intel.com> <20170928010353.11968-3-jian.j.wang@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B97BEA2@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B97BEA2@shsmsx102.ccr.corp.intel.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 Subject: Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection 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, 28 Sep 2017 03:28:13 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Another comment. Should IsNullDetectionEnabled() be checked before calling ClearLegacyMemory= ()? Thanks, Star -----Original Message----- From: Zeng, Star=20 Sent: Thursday, September 28, 2017 11:24 AM To: Wang, Jian J ; edk2-devel@lists.01.org Cc: Dong, Eric ; Laszlo Ersek ; Yao= , Jiewen ; Kinney, Michael D ; Justen, Jordan L ; Wolman, Ayellet ; Zeng, Star Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer det= ection Minor comments to this patch. 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is shared by = all ARCHs, and the function is consuming PcdNullPointerDetectionPropertyMas= k, but PcdNullPointerDetectionPropertyMask is only declared in "[Pcd.IA32,P= cd.X64]" in inf. I am not sure whether it will cause build failure or not for non-IA32/X64 A= RCHs. 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" t= o be more clear? Thanks, Star -----Original Message----- From: Wang, Jian J Sent: Thursday, September 28, 2017 9:04 AM To: edk2-devel@lists.01.org Cc: Zeng, Star ; Dong, Eric ; Las= zlo Ersek ; Yao, Jiewen ; Kinney, = Michael D ; Justen, Jordan L ; Wolman, Ayellet Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detecti= on > According to Jiewen's feedback, change the page split condition for=20 > NULL pointer detection to exclude IsExecuteDisableBitAvailable() > (Ia32/DxeLoadFunc.c) NULL pointer detection is done by making use of paging mechanism of CPU. During page table setup, if enabled, the first 4-K page (0-4095) will be ma= rked as NOT PRESENT. Any code which unintentionally access memory between 0-4095 will trigger a Page Fault exception which warns users that there's p= otential illegal code in BIOS. This also means that legacy code which has to access memory between 0-4095 = should be cautious to temporarily disable this feature before the access an= d re-enable it afterwards; or disalbe this feature at all. Cc: Star Zeng Cc: Eric Dong Cc: Laszlo Ersek Cc: Jiewen Yao Cc: Michael Kinney Cc: Jordan Justen Cc: Ayellet Wolman Suggested-by: Ayellet Wolman Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/Core/DxeIplPeim/DxeIpl.h | 25 +++++++++ MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 1 + MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 65 ++++++++++++++++++++= ++++ MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 11 +++- MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 2 + MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++--- 6 files changed, 126 insertions(+), 9 deletions(-) diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeI= plPeim/DxeIpl.h index 72d2532f50..1654bcd2dc 100644 --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h @@ -240,4 +240,29 @@ Decompress ( OUT UINTN *OutputSize ); =20 +/** + Clear legacy memory located at the first 4K-page. + + This function traverses the whole HOB list to check if memory from 0 to= 4095 + exists and has not been allocated, and then clear it if so. + + @param HoStart The start of HobList passed to DxeCore. + +**/ +VOID +ClearLegacyMemory ( + IN VOID *HobStart + ); + +/** + Return configure status of NULL pointer detection feature + + @return TRUE NULL pointer detection feature is enabled + @return FALSE NULL pointer detection feature is disabled **/=20 +BOOLEAN IsNullDetectionEnabled ( + VOID + ); + #endif diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/Dx= eIplPeim/DxeIpl.inf index c54afe4aa6..9d0e76a293 100644 --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf @@ -115,6 +115,7 @@ [Pcd.IA32,Pcd.X64] gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##= SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ##= CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ##= CONSUMES =20 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64] gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## SOMETIM= ES_CONSUMES diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/Dxe= IplPeim/DxeLoad.c index 50b5440d15..0a71b1f3de 100644 --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c @@ -825,3 +825,68 @@ UpdateStackHob ( Hob.Raw =3D GET_NEXT_HOB (Hob); } } + +/** + Clear legacy memory located at the first 4K-page, if available. + + This function traverses the whole HOB list to check if memory from 0 to= 4095 + exists and has not been allocated, and then clear it if so. + + @param HoStart The start of HobList passed to DxeCore= . + +**/ +VOID +ClearLegacyMemory ( + IN VOID *HobStart + ) +{ + EFI_PEI_HOB_POINTERS RscHob; + EFI_PEI_HOB_POINTERS MemHob; + BOOLEAN DoClear; + + RscHob.Raw =3D HobStart; + MemHob.Raw =3D HobStart; + DoClear =3D FALSE; + + // + // Check if page 0 exists and free + // + while ((RscHob.Raw =3D GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, + RscHob.Raw)) !=3D NULL) { + if (RscHob.ResourceDescriptor->ResourceType =3D=3D EFI_RESOURCE_SYSTEM= _MEMORY && + RscHob.ResourceDescriptor->PhysicalStart =3D=3D 0) { + DoClear =3D TRUE; + // + // Make sure memory at 0-4095 has not been allocated. + // + while ((MemHob.Raw =3D GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, + MemHob.Raw)) !=3D NULL) { + if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress + < EFI_PAGE_SIZE) { + DoClear =3D FALSE; + break; + } + MemHob.Raw =3D GET_NEXT_HOB (MemHob); + } + break; + } + RscHob.Raw =3D GET_NEXT_HOB (RscHob); } + + if (DoClear) { + DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n")); + SetMem (NULL, EFI_PAGE_SIZE, 0); + } + + return; +} + +BOOLEAN +IsNullDetectionEnabled ( + VOID + ) +{ + return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) !=3D 0) = ? + TRUE : FALSE); +} + diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg= /Core/DxeIplPeim/Ia32/DxeLoadFunc.c index 1957326caf..7a8421dd16 100644 --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae ( PageDirectoryPointerEntry->Bits.Present =3D 1; =20 for (IndexOfPageDirectoryEntries =3D 0; IndexOfPageDirectoryEntries < = 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += =3D SIZE_2MB) { - if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress += SIZE_2MB) > StackBase)) { + if ((IsNullDetectionEnabled () && PhysicalAddress =3D=3D 0) + || ((PhysicalAddress < StackBase + StackSize) + && ((PhysicalAddress + SIZE_2MB) > StackBase))) { // // Need to split this 2M page that covers stack range. // @@ -240,6 +242,8 @@ HandOffToDxeCore ( EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi; BOOLEAN BuildPageTablesIa32Pae; =20 + ClearLegacyMemory (HobList.Raw); + Status =3D PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PA= GES (STACK_SIZE), &BaseOfStack); ASSERT_EFI_ERROR (Status); =20 @@ -379,7 +383,10 @@ HandOffToDxeCore ( TopOfStack =3D (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStac= k, CPU_STACK_ALIGNMENT); =20 PageTables =3D 0; - BuildPageTablesIa32Pae =3D (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&= IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()); + BuildPageTablesIa32Pae =3D (BOOLEAN) (IsIa32PaeSupport () && + (IsNullDetectionEnabled () || + (PcdGetBool (PcdSetNxForStack) && + IsExecuteDisableBitAvailable=20 + ()))); if (BuildPageTablesIa32Pae) { PageTables =3D Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE); EnableExecuteDisableBit (); diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/= Core/DxeIplPeim/X64/DxeLoadFunc.c index 6488880eab..d93a9c5a2d 100644 --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c @@ -42,6 +42,8 @@ HandOffToDxeCore ( EFI_VECTOR_HANDOFF_INFO *VectorInfo; EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi; =20 + ClearLegacyMemory (HobList.Raw); + // // Get Vector Hand-off Info PPI and build Guided HOB // diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePk= g/Core/DxeIplPeim/X64/VirtualMemory.c index 48150be4e1..80c1821eca 100644 --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c @@ -90,8 +90,16 @@ Split2MPageTo4K ( // PageTableEntry->Uint64 =3D (UINT64) PhysicalAddress4K | AddressEncMask= ; PageTableEntry->Bits.ReadWrite =3D 1; - PageTableEntry->Bits.Present =3D 1; - if ((PhysicalAddress4K >=3D StackBase) && (PhysicalAddress4K < StackBa= se + StackSize)) { + + if (IsNullDetectionEnabled () && PhysicalAddress4K =3D=3D 0) { + PageTableEntry->Bits.Present =3D 0; + } else { + PageTableEntry->Bits.Present =3D 1; + } + + if (PcdGetBool (PcdSetNxForStack) + && (PhysicalAddress4K >=3D StackBase) + && (PhysicalAddress4K < StackBase + StackSize)) { // // Set Nx bit for stack. // @@ -137,9 +145,12 @@ Split1GPageTo2M ( =20 PhysicalAddress2M =3D PhysicalAddress; for (IndexOfPageDirectoryEntries =3D 0; IndexOfPageDirectoryEntries < 51= 2; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += =3D SIZE_2MB) { - if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M= + SIZE_2MB) > StackBase)) { + if ((IsNullDetectionEnabled () && PhysicalAddress2M =3D=3D 0) + || (PcdGetBool (PcdSetNxForStack) + && (PhysicalAddress2M < StackBase + StackSize) + && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) { // - // Need to split this 2M page that covers stack range. + // Need to split this 2M page that covers NULL or stack range. // Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry, S= tackBase, StackSize); } else { @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables ( PageDirectory1GEntry =3D (VOID *) PageDirectoryPointerEntry; =20 for (IndexOfPageDirectoryEntries =3D 0; IndexOfPageDirectoryEntries = < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += =3D SIZE_1GB) { - if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + St= ackSize) && ((PageAddress + SIZE_1GB) > StackBase)) { + if ((IsNullDetectionEnabled () && PageAddress =3D=3D 0) + || (PcdGetBool (PcdSetNxForStack) + && (PageAddress < StackBase + StackSize) + && ((PageAddress + SIZE_1GB) > StackBase))) { Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, S= tackBase, StackSize); } else { // @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables ( PageDirectoryPointerEntry->Bits.Present =3D 1; =20 for (IndexOfPageDirectoryEntries =3D 0; IndexOfPageDirectoryEntrie= s < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += =3D SIZE_2MB) { - if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + = StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) { + if ((IsNullDetectionEnabled () && PageAddress =3D=3D 0) + || (PcdGetBool (PcdSetNxForStack) + && (PageAddress < StackBase + StackSize) + && ((PageAddress + SIZE_2MB) > StackBase))) { // - // Need to split this 2M page that covers stack range. + // Need to split this 2M page that covers NULL or stack range. // Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, S= tackBase, StackSize); } else { -- 2.14.1.windows.1