153 lines
6.7 KiB
Diff
153 lines
6.7 KiB
Diff
From 2613601640be75f79e9dd8d2db21ad45d227d907 Mon Sep 17 00:00:00 2001
|
|
From: Laszlo Ersek <lersek@redhat.com>
|
|
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 <lersek@redhat.com>
|
|
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é <philmd@redhat.com>
|
|
RH-Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>
|
|
|
|
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 <eric.dong@intel.com>
|
|
Cc: Ray Ni <ray.ni@intel.com>
|
|
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1789335
|
|
Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358
|
|
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
|
|
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
|
|
Reviewed-by: Ray Ni <ray.ni@intel.com>
|
|
(cherry picked from commit a5235562444021e9c5aff08f45daa6b5b7952c7a)
|
|
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
|
|
---
|
|
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
|
|
|