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