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 1FDC921D492DD for ; Wed, 13 Sep 2017 10:28:38 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Sep 2017 10:31:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,389,1500966000"; d="scan'208";a="311307537" Received: from cwbutler-mobl.amr.corp.intel.com (HELO localhost) ([10.254.13.190]) by fmsmga004.fm.intel.com with ESMTP; 13 Sep 2017 10:31:35 -0700 MIME-Version: 1.0 To: "Wang, Jian J" , edk2-devel@lists.01.org Message-ID: <150532389515.21662.10902194007629858133@jljusten-skl> From: Jordan Justen In-Reply-To: <20170913092507.12504-3-jian.j.wang@intel.com> Cc: Jiewen Yao , Eric Dong , Star Zeng , Laszlo Ersek , Justen, Kinney, Michael D , Wolman, Ayellet References: <20170913092507.12504-1-jian.j.wang@intel.com> <20170913092507.12504-3-jian.j.wang@intel.com> User-Agent: alot/0.6 Date: Wed, 13 Sep 2017 10:31:35 -0700 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: Wed, 13 Sep 2017 17:28:38 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-09-13 02:25:05, Wang, Jian J wrote: > The mechanism behind is the same as NULL pointer detection enabled in EDK= -II core. SMM has its own page table and we have to disable page 0 again in= SMM 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/PiSmmC= puDxeSmm/Ia32/PageTbl.c > index f295c2ebf2..d423958783 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -155,6 +155,17 @@ SmiPFHandler ( > } > } > = > + // > + // If NULL pointer was just accessed > + // > + if (NULL_DETECTION_ENABLED && (PFAddress >=3D 0 && PFAddress < EFI_PAG= E_SIZE)) { > + 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/PiSmmCpuD= xeSmm/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_A= TTRIBUTE_BITS; > } > = > + 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; Pag= eIndex +=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 ( > } > } > = > + if (NULL_DETECTION_ENABLED) { > + Pte =3D (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_S= IZE - 1)); > + if ((Pte[0] & IA32_PG_PS) =3D=3D 0) { > + // 4K-page entries are already mapped. Just hide the first one any= way. > + Pte =3D (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_S= IZE - 1)); > + 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 prese= nt left > + for (Index =3D 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) { > + PageAddress +=3D EFI_PAGE_SIZE; > + Pte[Index] =3D PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BI= TS; > + } > + } > + } > + > return (UINT32)(UINTN)PageTable; > } > = > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSm= mCpuDxeSmm/PiSmmCpuDxeSmm.h > index 1cf85c1481..bcb3032db8 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -153,6 +153,8 @@ typedef UINT32 SMM_CPU_A= RRIVAL_EXCEPTIONS; > #define ARRIVAL_EXCEPTION_DELAYED 0x2 > #define ARRIVAL_EXCEPTION_SMI_DISABLED 0x4 > = > +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPrope= rtyMask) & BIT1) !=3D 0) > + Again, I think the NULL_DETECTION_ENABLED macro code style looks odd. Maybe it is just better to include the duplicated code directly in the 3 places? The commit message for this patch has a long line length. -Jordan > // > // Private structure for the SMM CPU module that is stored in DXE Runtim= e memory > // Contains the SMM Configuration Protocols that is produced. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/Pi= SmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > index 099792e6ce..57a14d9f24 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > @@ -138,14 +138,14 @@ > gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable > = > [FeaturePcd] > - 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 > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer ## = CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock ## = CONSUMES > = > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## SO= METIMES_CONSUMES > @@ -159,6 +159,7 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable ## CO= NSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## CO= NSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask = ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask = ## CONSUMES > = > [Depex] > gEfiMpServiceProtocolGuid > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCp= uDxeSmm/X64/PageTbl.c > index 3dde80f9ba..e67bcfe0f6 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -872,6 +872,17 @@ SmiPFHandler ( > } > } > = > + // > + // If NULL pointer was just accessed > + // > + if (NULL_DETECTION_ENABLED && (PFAddress >=3D 0 && PFAddress < EFI_PAG= E_SIZE)) { > + DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n")); > + DEBUG_CODE ( > + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip); > + ); > + CpuDeadLoop (); > + } > + > if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { > SmmProfilePFHandler ( > SystemContext.SystemContextX64->Rip, > -- = > 2.14.1.windows.1 >=20