From 2613601640be75f79e9dd8d2db21ad45d227d907 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Fri, 17 Jan 2020 11:33:43 +0100 Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RH-Author: Laszlo Ersek Message-id: <20200117113343.30392-2-lersek@redhat.com> Patchwork-id: 93389 O-Subject: [RHEL-8.2.0 edk2 PATCH 1/1] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Bugzilla: 1789335 RH-Acked-by: Philippe Mathieu-Daudé RH-Acked-by: Vitaly Kuznetsov In commit 4eee0cc7cc0d ("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports", 2019-07-12), the Page Directory Entry setting was regressed (corrupted) when splitting a 2MB page to 512 4KB pages, in the InitPaging() function. Consider the following hunk, displayed with $ git show --function-context --ignore-space-change 4eee0cc7cc0db > // > // If it is 2M page, check IsAddressSplit() > // > if (((*Pd & IA32_PG_PS) != 0) && IsAddressSplit (Address)) { > // > // Based on current page table, create 4KB page table for split area. > // > ASSERT (Address == (*Pd & PHYSICAL_ADDRESS_MASK)); > > Pt = AllocatePageTableMemory (1); > ASSERT (Pt != NULL); > > + *Pd = (UINTN) Pt | IA32_PG_RW | IA32_PG_P; > + > // Split it > - for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++) { > - Pt[PtIndex] = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS); > + for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++, Pt++) { > + *Pt = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS); > } // end for PT > *Pd = (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBUTE_BITS; > } // end if IsAddressSplit > } // end for PD First, the new assignment to the Page Directory Entry (*Pd) is superfluous. That's because (a) we set (*Pd) after the Page Table Entry loop anyway, and (b) here we do not attempt to access the memory starting at "Address" (which is mapped by the original value of the Page Directory Entry). Second, appending "Pt++" to the incrementing expression of the PTE loop is a bug. It causes "Pt" to point *right past* the just-allocated Page Table, once we finish the loop. But the PDE assignment that immediately follows the loop assumes that "Pt" still points to the *start* of the new Page Table. The result is that the originally mapped 2MB page disappears from the processor's view. The PDE now points to a "Page Table" that is filled with garbage. The random entries in that "Page Table" will cause some virtual addresses in the original 2MB area to fault. Other virtual addresses in the same range will no longer have a 1:1 physical mapping, but be scattered over random physical page frames. The second phase of the InitPaging() function ("Go through page table and set several page table entries to absent or execute-disable") already manipulates entries in wrong Page Tables, for such PDEs that got split in the first phase. This issue has been caught as follows: - OVMF is started with 2001 MB of guest RAM. - This places the main SMRAM window at 0x7C10_1000. - The SMRAM management in the SMM Core links this SMRAM window into "mSmmMemoryMap", with a FREE_PAGE_LIST record placed at the start of the area. - At "SMM Ready To Lock" time, PiSmmCpuDxeSmm calls InitPaging(). The first phase (quoted above) decides to split the 2MB page at 0x7C00_0000 into 512 4KB pages, and corrupts the PDE. The new Page Table is allocated at 0x7CE0_D000, but the PDE is set to 0x7CE0_E000 (plus attributes 0x67). - Due to the corrupted PDE, the second phase of InitPaging() already looks up the PTE for Address=0x7C10_1000 in the wrong place. The second phase goes on to mark bogus PTEs as "NX". - PiSmmCpuDxeSmm calls SetMemMapAttributes(). Address 0x7C10_1000 is at the base of the SMRAM window, therefore it happens to be listed in the SMRAM map as an EfiConventionalMemory region. SetMemMapAttributes() calls SmmSetMemoryAttributes() to mark the region as XP. However, GetPageTableEntry() in ConvertMemoryPageAttributes() fails -- address 0x7C10_1000 is no longer mapped by anything! -- and so the attribute setting fails with RETURN_UNSUPPORTED. This error goes unnoticed, as SetMemMapAttributes() ignores the return value of SmmSetMemoryAttributes(). - When SetMemMapAttributes() reaches another entry in the SMRAM map, ConvertMemoryPageAttributes() decides it needs to split a 2MB page, and calls SplitPage(). - SplitPage() calls AllocatePageTableMemory() for the new Page Table, which takes us to InternalAllocMaxAddress() in the SMM Core. - The SMM core attempts to read the FREE_PAGE_LIST record at 0x7C10_1000. Because this virtual address is no longer mapped, the firmware crashes in InternalAllocMaxAddress(), when accessing (Pages->NumberOfPages). Remove the useless assignment to (*Pd) from before the loop. Revert the loop incrementing and the PTE assignment to the known good version. Cc: Eric Dong Cc: Ray Ni Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1789335 Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358 Signed-off-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daude Reviewed-by: Ray Ni (cherry picked from commit a5235562444021e9c5aff08f45daa6b5b7952c7a) Signed-off-by: Miroslav Rezanina --- UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c index c513152..c47b557 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c @@ -657,11 +657,9 @@ InitPaging ( Pt = AllocatePageTableMemory (1); ASSERT (Pt != NULL); - *Pd = (UINTN) Pt | IA32_PG_RW | IA32_PG_P; - // Split it - for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++, Pt++) { - *Pt = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS); + for (PtIndex = 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIndex++) { + Pt[PtIndex] = Address + ((PtIndex << 12) | mAddressEncMask | PAGE_ATTRIBUTE_BITS); } // end for PT *Pd = (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBUTE_BITS; } // end if IsAddressSplit -- 1.8.3.1