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
