From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 3EC9320945C16 for ; Wed, 13 Sep 2017 18:28:59 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Sep 2017 18:31:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,390,1500966000"; d="scan'208";a="1014231402" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga003.jf.intel.com with ESMTP; 13 Sep 2017 18:31:56 -0700 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 13 Sep 2017 18:31:56 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 13 Sep 2017 18:31:55 -0700 Received: from shsmsx152.ccr.corp.intel.com ([169.254.6.93]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.98]) with mapi id 14.03.0319.002; Thu, 14 Sep 2017 09:31:53 +0800 From: "Wang, Jian J" To: "Johnson, Brian (EXL - Eagan)" , "edk2-devel@lists.01.org" CC: "Justen@ml01.01.org" , "Dong, Eric" , "Kinney@ml01.01.org" , "Justen, Jordan L" , "Wolman@ml01.01.org" , "Yao, Jiewen" , "Wolman, Ayellet" , "Kinney, Michael D" , Laszlo Ersek , "Zeng, Star" Thread-Topic: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code. Thread-Index: AQHTLHJFBm3N8RT2wkKIcfTiDyYjXqKy/7eggACYHKA= Date: Thu, 14 Sep 2017 01:31:52 +0000 Message-ID: References: <20170913092507.12504-1-jian.j.wang@intel.com> <20170913092507.12504-3-jian.j.wang@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 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code. 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, 14 Sep 2017 01:28:59 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks for the comments. See my comment start with [Jian] below. -----Original Message----- From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]=20 Sent: Thursday, September 14, 2017 12:34 AM To: Wang, Jian J ; edk2-devel@lists.01.org Cc: Justen@ml01.01.org; Dong, Eric ; Kinney@ml01.01.or= g; Justen, Jordan L ; Wolman@ml01.01.org; Yao, J= iewen ; Wolman, Ayellet ; K= inney, Michael D ; Laszlo Ersek ; Zeng, Star Subject: RE: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL p= ointer detection for SMM mode code. Comments below. Brian -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang= , Jian J Sent: Wednesday, September 13, 2017 4:25 AM To: edk2-devel@lists.01.org Cc: Justen@ml01.01.org; Eric Dong ; Kinney@ml01.01.org= ; Jordan L ; Wolman@ml01.01.org; Jiewen Yao ; Ayellet ; Michael D ; Laszlo Ersek ; Star Zeng Subject: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL point= er detection for SMM mode code. The mechanism behind is the same as NULL pointer detection enabled in EDK-I= I core. SMM has its own page table and we have to disable page 0 again in S= MM mode. Cc: Jiewen Yao Cc: Eric Dong Cc: Star Zeng Cc: Laszlo Ersek Cc: Justen, Jordan L Cc: Kinney, Michael D Cc: Wolman, Ayellet Suggested-by: Wolman, Ayellet Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wang, Jian J --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 11 +++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 25 ++++++++++++++++++++++++= - UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 2 ++ UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++-------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 11 +++++++++++ 5 files changed, 57 insertions(+), 9 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/Ia32/PageTbl.c index f295c2ebf2..d423958783 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -155,6 +155,17 @@ SmiPFHandler ( } } =20 + // + // If NULL pointer was just accessed + // + if (NULL_DETECTION_ENABLED && (PFAddress >=3D 0 && PFAddress < EFI_PAGE_= SIZE)) { [Brian] PFAddress is unsigned, so it will always be >=3D 0. Some compilers= complain about this.... Should probably remove that part of the test. [Jian] You're right. The first test is not necessary. + DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n")); + DEBUG_CODE ( + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip); + ); + CpuDeadLoop (); + } + if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { SmmProfilePFHandler ( SystemContext.SystemContextIa32->Eip, diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxe= Smm/MpService.c index f086b97c30..81c5ac9d11 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -855,10 +855,10 @@ Gen4GPageTable ( Pte[Index] =3D (Index << 21) | mAddressEncMask | IA32_PG_PS | PAGE_ATT= RIBUTE_BITS; } =20 + Pdpte =3D (UINT64*)PageTable; if (FeaturePcdGet (PcdCpuSmmStackGuard)) { Pages =3D (UINTN)PageTable + EFI_PAGES_TO_SIZE (5); GuardPage =3D mSmmStackArrayBase + EFI_PAGE_SIZE; - Pdpte =3D (UINT64*)PageTable; for (PageIndex =3D Low2MBoundary; PageIndex <=3D High2MBoundary; PageI= ndex +=3D SIZE_2MB) { Pte =3D (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30= , 31)] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1)); Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] =3D (UINT64)Pages | = mAddressEncMask | PAGE_ATTRIBUTE_BITS; @@ -886,6 +886,29 @@ Gen4GPageTable ( } } =20 + if (NULL_DETECTION_ENABLED) { + Pte =3D (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZ= E - 1)); [Brian] Shouldn't the inner cast be (UINTN), not (UINT64)? That would matc= h the PcdCpuSmmStackGuard section above. [Jian] You're right. Inner cast should be UINTN. + if ((Pte[0] & IA32_PG_PS) =3D=3D 0) { + // 4K-page entries are already mapped. Just hide the first one anywa= y. + Pte =3D (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZ= E - 1)); [Brian] Same comment re. the inner cast. + Pte[0] &=3D ~1; // Hide page 0 + } else { + // Create 4K-page entries + Pages =3D (UINTN)AllocatePageTableMemory (1); + ASSERT (Pages !=3D 0); + + Pte[0] =3D (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS); + + Pte =3D (UINT64*)Pages; + PageAddress =3D 0; + Pte[0] =3D PageAddress | mAddressEncMask; // Hide page 0 but present= left + for (Index =3D 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) { + PageAddress +=3D EFI_PAGE_SIZE; + Pte[Index] =3D PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS= ; + } + } + } + return (UINT32)(UINTN)PageTable; } =20 diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmC= puDxeSmm/PiSmmCpuDxeSmm.h index 1cf85c1481..bcb3032db8 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -153,6 +153,8 @@ typedef UINT32 SMM_CPU_ARR= IVAL_EXCEPTIONS; #define ARRIVAL_EXCEPTION_DELAYED 0x2 #define ARRIVAL_EXCEPTION_SMI_DISABLED 0x4 =20 +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropert= yMask) & BIT1) !=3D 0) + // // Private structure for the SMM CPU module that is stored in DXE Runtime = memory // Contains the SMM Configuration Protocols that is produced. diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSm= mCpuDxeSmm/PiSmmCpuDxeSmm.inf index 099792e6ce..57a14d9f24 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf @@ -138,14 +138,14 @@ gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable =20 [FeaturePcd] - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer ## CONS= UMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock ## CONS= UMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock ## CO= NSUMES =20 [Pcd] gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## SOME= TIMES_CONSUMES @@ -159,6 +159,7 @@ gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable ## CONS= UMES gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## CONS= UMES gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ##= CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ##= CONSUMES =20 [Depex] gEfiMpServiceProtocolGuid diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuD= xeSmm/X64/PageTbl.c index 3dde80f9ba..e67bcfe0f6 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -872,6 +872,17 @@ SmiPFHandler ( } } =20 + // + // If NULL pointer was just accessed + // + if (NULL_DETECTION_ENABLED && (PFAddress >=3D 0 && PFAddress < EFI_PAGE_= SIZE)) { [Brian] PFAddress is unsigned, so it will always be >=3D 0. Some compilers= complain about this.... Should probably remove that part of the test. + DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n")); + DEBUG_CODE ( + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip); + ); + CpuDeadLoop (); + } + if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { SmmProfilePFHandler ( SystemContext.SystemContextX64->Rip, --=20 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel