render / rpms / edk2

Forked from rpms/edk2 2 months ago
Clone

Blame SOURCES/edk2-UefiCpuPkg-PiSmmCpuDxeSmm-fix-2M-4K-page-splitting-r.patch

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