From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 A747921EC8D0A for ; Wed, 27 Sep 2017 22:07:57 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 27 Sep 2017 22:11:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,448,1500966000"; d="scan'208";a="317105306" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 27 Sep 2017 22:11:11 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Sep 2017 22:11:11 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Sep 2017 22:11:10 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.213]) with mapi id 14.03.0319.002; Thu, 28 Sep 2017 13:11:09 +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/0xBikRKLJoEQwgAAIqACAABdoAA== Date: Thu, 28 Sep 2017 05:11:08 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B97C08B@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: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 05:07:57 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I am curious about what is potential future change. Thanks, Star -----Original Message----- From: Wang, Jian J=20 Sent: Thursday, September 28, 2017 11:50 AM To: Zeng, Star ; edk2-devel@lists.01.org Cc: Dong, Eric ; Laszlo Ersek ; Yao= , Jiewen ; Kinney, Michael D ; Justen, Jordan L ; Wolman, Ayellet Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer det= ection Please see my comments inline below. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 11:24 AM > To: Wang, Jian J ; edk2-devel@lists.01.org > Cc: Dong, Eric ; Laszlo Ersek=20 > ; Yao, Jiewen ; Kinney,=20 > Michael D ; Justen, Jordan L=20 > ; Wolman, Ayellet=20 > ; Zeng, Star > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL=20 > pointer detection >=20 > Minor comments to this patch. >=20 > 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is=20 > shared by all ARCHs, and the function is consuming=20 > PcdNullPointerDetectionPropertyMask, > but PcdNullPointerDetectionPropertyMask is only declared in=20 > "[Pcd.IA32,Pcd.X64]" in inf. > I am not sure whether it will cause build failure or not for non-IA32/X64= ARCHs. >=20 You're right. The PCD should not be under this section. > 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" > to be more clear? >=20 I prefer a bit more general name in case of potential future change. >=20 > 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=20 > ; Laszlo Ersek ; Yao, Jiewen=20 > ; Kinney, Michael D=20 > ; Justen, Jordan L=20 > ; Wolman, Ayellet=20 > > Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer=20 > detection >=20 > > According to Jiewen's feedback, change the page split condition for=20 > > NULL pointer detection to exclude IsExecuteDisableBitAvailable() > > (Ia32/DxeLoadFunc.c) >=20 > 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=20 > be marked as NOT PRESENT. Any code which unintentionally access memory=20 > between > 0-4095 will trigger a Page Fault exception which warns users that=20 > there's potential illegal code in BIOS. >=20 > This also means that legacy code which has to access memory between=20 > 0-4095 should be cautious to temporarily disable this feature before=20 > the access and re- enable it afterwards; or disalbe this feature at all. >=20 > 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(-) >=20 > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h > b/MdeModulePkg/Core/DxeIplPeim/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/DxeIplPeim/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 ## > SOMETIMES_CONSUMES > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c > b/MdeModulePkg/Core/DxeIplPeim/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 DxeCo= re. > + > +**/ > +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= =20 > < 512; > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress=20 > IndexOfPageDirectoryEntries+++=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,=20 > EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack); > ASSERT_EFI_ERROR (Status); >=20 > @@ -379,7 +383,10 @@ HandOffToDxeCore ( > TopOfStack =3D (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER=20 > (TopOfStack, CPU_STACK_ALIGNMENT); >=20 > PageTables =3D 0; > - BuildPageTablesIa32Pae =3D (BOOLEAN) (PcdGetBool (PcdSetNxForStack) = && > IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()); > + BuildPageTablesIa32Pae =3D (BOOLEAN) (IsIa32PaeSupport () && > + (IsNullDetectionEnabled () || > + (PcdGetBool (PcdSetNxForStack) = && > + =20 > + IsExecuteDisableBitAvailable ()))); > 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/MdeModulePkg/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 | AddressEncMa= sk; > PageTableEntry->Bits.ReadWrite =3D 1; > - PageTableEntry->Bits.Present =3D 1; > - if ((PhysicalAddress4K >=3D StackBase) && (PhysicalAddress4K < Stack= Base + > 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 <= =20 > 512; > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M=20 > IndexOfPageDirectoryEntries+++=3D > SIZE_2MB) { > - if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress= 2M + > 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 *)=20 > PageDirectoryEntry, StackBase, StackSize); > } else { > @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables ( > PageDirectory1GEntry =3D (VOID *) PageDirectoryPointerEntry; >=20 > for (IndexOfPageDirectoryEntries =3D 0;=20 > IndexOfPageDirectoryEntries < 512; > IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=3D > SIZE_1GB) { > - if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + > StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) { > + if ((IsNullDetectionEnabled () && PageAddress =3D=3D 0) > + || (PcdGetBool (PcdSetNxForStack) > + && (PageAddress < StackBase + StackSize) > + && ((PageAddress + SIZE_1GB) > StackBase))) { > Split1GPageTo2M (PageAddress, (UINT64 *)=20 > PageDirectory1GEntry, StackBase, StackSize); > } else { > // > @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables ( > PageDirectoryPointerEntry->Bits.Present =3D 1; >=20 > for (IndexOfPageDirectoryEntries =3D 0;=20 > IndexOfPageDirectoryEntries < 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 rang= e. > // > Split2MPageTo4K (PageAddress, (UINT64 *)=20 > PageDirectoryEntry, StackBase, StackSize); > } else { > -- > 2.14.1.windows.1