From 881520ea6778953c57d975ca2a9cf3f2114f99c4 Mon Sep 17 00:00:00 2001 From: "Yao, Jiewen" Date: Mon, 30 Nov 2015 19:57:40 +0000 Subject: [PATCH] UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default So that we can use write-protection for code later. This is REPOST. It includes the bug fix from "Paolo Bonzini" : Title: fix generation of 32-bit PAE page tables "Bits 1 and 2 are reserved in 32-bit PAE Page Directory Pointer Table Entries (PDPTEs); see Table 4-8 in the SDM. With VMX extended page tables, the processor notices and fails the VM entry as soon as CR0.PG is set to 1." And thanks "Laszlo Ersek" to validate the fix. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: "Yao, Jiewen" Signed-off-by: "Paolo Bonzini" Reviewed-by: Michael Kinney Tested-by: Laszlo Ersek Cc: "Fan, Jeff" Cc: "Kinney, Michael D" Cc: "Laszlo Ersek" Cc: "Paolo Bonzini" git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@19067 6f19259b-4bc3-4df7-8a09-765794883524 --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 14 ++++++++------ UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 ++++++++++++- UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 12 ++++++------ UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 8 ++++---- UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 14 +++++++------- 7 files changed, 39 insertions(+), 26 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c index edebaabb47..5d299044c4 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -60,7 +60,7 @@ SmmInitPageTable ( if (FeaturePcdGet (PcdCpuSmmStackGuard)) { InitializeIDTSmmStackGuard (); } - return Gen4GPageTable (0); + return Gen4GPageTable (0, TRUE); } /** diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c index 85756d0710..767cb6908b 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c @@ -24,7 +24,7 @@ InitSmmS3Cr3 ( VOID ) { - mSmmS3ResumeState->SmmS3Cr3 = Gen4GPageTable (0); + mSmmS3ResumeState->SmmS3Cr3 = Gen4GPageTable (0, TRUE); return ; } diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index 06ffc6dd86..620b0136c5 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -732,12 +732,14 @@ APHandler ( Create 4G PageTable in SMRAM. @param ExtraPages Additional page numbers besides for 4G memory + @param Is32BitPageTable Whether the page table is 32-bit PAE @return PageTable Address **/ UINT32 Gen4GPageTable ( - IN UINTN ExtraPages + IN UINTN ExtraPages, + IN BOOLEAN Is32BitPageTable ) { VOID *PageTable; @@ -785,7 +787,7 @@ Gen4GPageTable ( // Set Page Directory Pointers // for (Index = 0; Index < 4; Index++) { - Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + IA32_PG_P; + Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + (Is32BitPageTable ? IA32_PAE_PDPTE_ATTRIBUTE_BITS : PAGE_ATTRIBUTE_BITS); } Pte += EFI_PAGE_SIZE / sizeof (*Pte); @@ -793,7 +795,7 @@ Gen4GPageTable ( // Fill in Page Directory Entries // for (Index = 0; Index < EFI_PAGE_SIZE * 4 / sizeof (*Pte); Index++) { - Pte[Index] = (Index << 21) + IA32_PG_PS + IA32_PG_RW + IA32_PG_P; + Pte[Index] = (Index << 21) | IA32_PG_PS | PAGE_ATTRIBUTE_BITS; } if (FeaturePcdGet (PcdCpuSmmStackGuard)) { @@ -802,7 +804,7 @@ Gen4GPageTable ( Pdpte = (UINT64*)PageTable; for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += SIZE_2MB) { Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] & ~(EFI_PAGE_SIZE - 1)); - Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages + IA32_PG_RW + IA32_PG_P; + Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | PAGE_ATTRIBUTE_BITS; // // Fill in Page Table Entries // @@ -819,7 +821,7 @@ Gen4GPageTable ( GuardPage = 0; } } else { - Pte[Index] = PageAddress + IA32_PG_RW + IA32_PG_P; + Pte[Index] = PageAddress | PAGE_ATTRIBUTE_BITS; } PageAddress+= EFI_PAGE_SIZE; } @@ -886,7 +888,7 @@ SetCacheability ( NewPageTable[Index] |= (UINT64)(Index << EFI_PAGE_SHIFT); } - PageTable[PTIndex] = ((UINTN)NewPageTableAddress & gPhyMask) | IA32_PG_P; + PageTable[PTIndex] = ((UINTN)NewPageTableAddress & gPhyMask) | PAGE_ATTRIBUTE_BITS; } ASSERT (PageTable[PTIndex] & IA32_PG_P); diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index f2a91655a3..9920cd1d1e 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -71,15 +71,24 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. /// #define IA32_PG_P BIT0 #define IA32_PG_RW BIT1 +#define IA32_PG_U BIT2 #define IA32_PG_WT BIT3 #define IA32_PG_CD BIT4 #define IA32_PG_A BIT5 +#define IA32_PG_D BIT6 #define IA32_PG_PS BIT7 #define IA32_PG_PAT_2M BIT12 #define IA32_PG_PAT_4K IA32_PG_PS #define IA32_PG_PMNT BIT62 #define IA32_PG_NX BIT63 +#define PAGE_ATTRIBUTE_BITS (IA32_PG_RW | IA32_PG_P) +// +// Bits 1, 2, 5, 6 are reserved in the IA32 PAE PDPTE +// X64 PAE PDPTE does not have such restriction +// +#define IA32_PAE_PDPTE_ATTRIBUTE_BITS (IA32_PG_P) + // // Size of Task-State Segment defined in IA32 Manual // @@ -364,12 +373,14 @@ extern IA32_DESCRIPTOR gcSmiInitGdtr; Create 4G PageTable in SMRAM. @param ExtraPages Additional page numbers besides for 4G memory + @param Is32BitPageTable Whether the page table is 32-bit PAE @return PageTable Address **/ UINT32 Gen4GPageTable ( - IN UINTN ExtraPages + IN UINTN ExtraPages, + IN BOOLEAN Is32BitPageTable ); diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c index ff4e28ec58..ec4ec9b067 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c @@ -557,9 +557,9 @@ InitPaging ( // Split it for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) { - Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); + Pt[Level4] = Address + ((Level4 << 12) | PAGE_ATTRIBUTE_BITS); } // end for PT - *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; + *Pte = (UINTN)Pt | PAGE_ATTRIBUTE_BITS; } // end if IsAddressSplit } // end for PTE } // end for PDE @@ -608,7 +608,7 @@ InitPaging ( // // Patch to remove Present flag and RW flag // - *Pte = *Pte & (INTN)(INT32)(~(IA32_PG_RW | IA32_PG_P)); + *Pte = *Pte & (INTN)(INT32)(~PAGE_ATTRIBUTE_BITS); } if (Nx && mXdSupported) { *Pte = *Pte | IA32_PG_NX; @@ -621,7 +621,7 @@ InitPaging ( } for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, Pt++) { if (!IsAddressValid (Address, &Nx)) { - *Pt = *Pt & (INTN)(INT32)(~(IA32_PG_RW | IA32_PG_P)); + *Pt = *Pt & (INTN)(INT32)(~PAGE_ATTRIBUTE_BITS); } if (Nx && mXdSupported) { *Pt = *Pt | IA32_PG_NX; @@ -1244,7 +1244,7 @@ RestorePageTableBelow4G ( // PageTable[PTIndex] = (PFAddress & ~((1ull << 21) - 1)); PageTable[PTIndex] |= (UINT64)IA32_PG_PS; - PageTable[PTIndex] |= (UINT64)(IA32_PG_RW | IA32_PG_P); + PageTable[PTIndex] |= (UINT64)PAGE_ATTRIBUTE_BITS; if ((ErrorCode & IA32_PF_EC_ID) != 0) { PageTable[PTIndex] &= ~IA32_PG_NX; } @@ -1277,7 +1277,7 @@ RestorePageTableBelow4G ( // Set new entry // PageTable[PTIndex] = (PFAddress & ~((1ull << 12) - 1)); - PageTable[PTIndex] |= (UINT64)(IA32_PG_RW | IA32_PG_P); + PageTable[PTIndex] |= (UINT64)PAGE_ATTRIBUTE_BITS; if ((ErrorCode & IA32_PF_EC_ID) != 0) { PageTable[PTIndex] &= ~IA32_PG_NX; } diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c index a7d790fd8a..5b11e5eb48 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -113,7 +113,7 @@ SmmInitPageTable ( // // Generate PAE page table for the first 4GB memory space // - Pages = Gen4GPageTable (PAGE_TABLE_PAGES + 1); + Pages = Gen4GPageTable (PAGE_TABLE_PAGES + 1, FALSE); // // Set IA32_PG_PMNT bit to mask this entry @@ -127,7 +127,7 @@ SmmInitPageTable ( // Fill Page-Table-Level4 (PML4) entry // PTEntry = (UINT64*)(UINTN)(Pages - EFI_PAGES_TO_SIZE (PAGE_TABLE_PAGES + 1)); - *PTEntry = Pages + IA32_PG_P; + *PTEntry = Pages + PAGE_ATTRIBUTE_BITS; ZeroMem (PTEntry + 1, EFI_PAGE_SIZE - sizeof (*PTEntry)); // // Set sub-entries number @@ -591,7 +591,7 @@ SmiDefaultPFHandler ( // // If the entry is not present, allocate one page from page pool for it // - PageTable[PTIndex] = AllocPage () | IA32_PG_RW | IA32_PG_P; + PageTable[PTIndex] = AllocPage () | PAGE_ATTRIBUTE_BITS; } else { // // Save the upper entry address @@ -621,7 +621,7 @@ SmiDefaultPFHandler ( // Fill the new entry // PageTable[PTIndex] = (PFAddress & gPhyMask & ~((1ull << EndBit) - 1)) | - PageAttribute | IA32_PG_A | IA32_PG_RW | IA32_PG_P; + PageAttribute | IA32_PG_A | PAGE_ATTRIBUTE_BITS; if (UpperEntry != NULL) { SetSubEntriesNum (UpperEntry, GetSubEntriesNum (UpperEntry) + 1); } diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c index c4ec12debb..79e23ef647 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c @@ -45,13 +45,13 @@ InitSmmS3Cr3 ( // // Generate PAE page table for the first 4GB memory space // - Pages = Gen4GPageTable (1); + Pages = Gen4GPageTable (1, FALSE); // // Fill Page-Table-Level4 (PML4) entry // PTEntry = (UINT64*)(UINTN)(Pages - EFI_PAGES_TO_SIZE (1)); - *PTEntry = Pages + IA32_PG_P; + *PTEntry = Pages | PAGE_ATTRIBUTE_BITS; ZeroMem (PTEntry + 1, EFI_PAGE_SIZE - sizeof (*PTEntry)); // @@ -117,7 +117,7 @@ AcquirePage ( // // Link & Record the current uplink // - *Uplink = Address | IA32_PG_P | IA32_PG_RW; + *Uplink = Address | PAGE_ATTRIBUTE_BITS; mPFPageUplink[mPFPageIndex] = Uplink; mPFPageIndex = (mPFPageIndex + 1) % MAX_PF_PAGE_COUNT; @@ -242,9 +242,9 @@ RestorePageTableAbove4G ( // PTE PageTable = (UINT64*)(UINTN)(PageTable[PTIndex] & PHYSICAL_ADDRESS_MASK); for (Index = 0; Index < 512; Index++) { - PageTable[Index] = Address | IA32_PG_RW | IA32_PG_P; + PageTable[Index] = Address | PAGE_ATTRIBUTE_BITS; if (!IsAddressValid (Address, &Nx)) { - PageTable[Index] = PageTable[Index] & (INTN)(INT32)(~(IA32_PG_RW | IA32_PG_P)); + PageTable[Index] = PageTable[Index] & (INTN)(INT32)(~PAGE_ATTRIBUTE_BITS); } if (Nx && mXdSupported) { PageTable[Index] = PageTable[Index] | IA32_PG_NX; @@ -262,7 +262,7 @@ RestorePageTableAbove4G ( // // Patch to remove present flag and rw flag. // - PageTable[PTIndex] = PageTable[PTIndex] & (INTN)(INT32)(~(IA32_PG_RW | IA32_PG_P)); + PageTable[PTIndex] = PageTable[PTIndex] & (INTN)(INT32)(~PAGE_ATTRIBUTE_BITS); } // // Set XD bit to 1 @@ -289,7 +289,7 @@ RestorePageTableAbove4G ( // // Add present flag or clear XD flag to make page fault handler succeed. // - PageTable[PTIndex] |= (UINT64)(IA32_PG_RW | IA32_PG_P); + PageTable[PTIndex] |= (UINT64)(PAGE_ATTRIBUTE_BITS); if ((ErrorCode & IA32_PF_EC_ID) != 0) { // // If page fault is caused by instruction fetch, clear XD bit in the entry.