I'll use the tool to check the format. For the macro, it's for readability purpose. How's the library replacement suggestion from Laszlo?
-----Original Message----- From: Justen, Jordan L Sent: Thursday, September 14, 2017 1:32 AM To: Wang, Jian J <[email protected]>; [email protected] Cc: Yao, Jiewen <[email protected]>; Dong, Eric <[email protected]>; Zeng, Star <[email protected]>; Laszlo Ersek <[email protected]>; Justen; Kinney; Kinney, Michael D <[email protected]>; Wolman; Wolman, Ayellet <[email protected]> Subject: Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code. 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 <[email protected]> > Cc: Eric Dong <[email protected]> > Cc: Star Zeng <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Justen, Jordan L <[email protected]> > Cc: Kinney, Michael D <[email protected]> > Cc: Wolman, Ayellet <[email protected]> > Suggested-by: Wolman, Ayellet <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wang, Jian J <[email protected]> > --- > 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/PiSmmCpuDxeSmm/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 >= 0 && PFAddress < > EFI_PAGE_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/PiSmmCpuDxeSmm/MpService.c > index f086b97c30..81c5ac9d11 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -855,10 +855,10 @@ Gen4GPageTable ( > Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | > PAGE_ATTRIBUTE_BITS; > } > > + Pdpte = (UINT64*)PageTable; > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5); > GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE; > - Pdpte = (UINT64*)PageTable; > for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex > += SIZE_2MB) { > Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, > 31)] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1)); > Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | > mAddressEncMask | PAGE_ATTRIBUTE_BITS; > @@ -886,6 +886,29 @@ Gen4GPageTable ( > } > } > > + if (NULL_DETECTION_ENABLED) { > + Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - > 1)); > + if ((Pte[0] & IA32_PG_PS) == 0) { > + // 4K-page entries are already mapped. Just hide the first one anyway. > + Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - > 1)); > + Pte[0] &= ~1; // Hide page 0 > + } else { > + // Create 4K-page entries > + Pages = (UINTN)AllocatePageTableMemory (1); > + ASSERT (Pages != 0); > + > + Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS); > + > + Pte = (UINT64*)Pages; > + PageAddress = 0; > + Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left > + for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) { > + PageAddress += EFI_PAGE_SIZE; > + Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS; > + } > + } > + } > + > return (UINT32)(UINTN)PageTable; > } > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 1cf85c1481..bcb3032db8 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -153,6 +153,8 @@ typedef UINT32 > SMM_CPU_ARRIVAL_EXCEPTIONS; > #define ARRIVAL_EXCEPTION_DELAYED 0x2 > #define ARRIVAL_EXCEPTION_SMI_DISABLED 0x4 > > +#define NULL_DETECTION_ENABLED > ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 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 Runtime > memory > // Contains the SMM Configuration Protocols that is produced. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > b/UefiCpuPkg/PiSmmCpuDxeSmm/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 ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer ## > CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock ## > CONSUMES > + 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 ## > SOMETIMES_CONSUMES > @@ -159,6 +159,7 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable ## > CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## > CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## > CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## > CONSUMES > > [Depex] > gEfiMpServiceProtocolGuid > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/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 >= 0 && PFAddress < > EFI_PAGE_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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
