Comments below. Brian
-----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, Jian J Sent: Wednesday, September 13, 2017 4:25 AM To: edk2-devel@lists.01.org Cc: jus...@ml01.01.org; Eric Dong <eric.d...@intel.com>; kin...@ml01.01.org; Jordan L <jordan.l.jus...@intel.com>; wol...@ml01.01.org; Jiewen Yao <jiewen....@intel.com>; Ayellet <ayellet.wol...@intel.com>; Michael D <michael.d.kin...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Star Zeng <star.z...@intel.com> Subject: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code. 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 <jiewen....@intel.com> Cc: Eric Dong <eric.d...@intel.com> Cc: Star Zeng <star.z...@intel.com> Cc: Laszlo Ersek <ler...@redhat.com> Cc: Justen, Jordan L <jordan.l.jus...@intel.com> Cc: Kinney, Michael D <michael.d.kin...@intel.com> Cc: Wolman, Ayellet <ayellet.wol...@intel.com> Suggested-by: Wolman, Ayellet <ayellet.wol...@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wang, Jian J <jian.j.w...@intel.com> --- 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)) { [Brian] PFAddress is unsigned, so it will always be >= 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.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)); [Brian] Shouldn't the inner cast be (UINTN), not (UINT64)? That would match the PcdCpuSmmStackGuard section above. + 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)); [Brian] Same comment re. the inner cast. + 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) + // // 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)) { [Brian] PFAddress is unsigned, so it will always be >= 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, -- 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
