From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web10.8780.1579177437227073221 for ; Thu, 16 Jan 2020 04:23:57 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jan 2020 04:23:56 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,326,1574150400"; d="scan'208";a="305837454" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 16 Jan 2020 04:23:56 -0800 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 16 Jan 2020 04:22:26 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 16 Jan 2020 04:22:25 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.197]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.245]) with mapi id 14.03.0439.000; Thu, 16 Jan 2020 20:22:24 +0800 From: "Ni, Ray" To: Laszlo Ersek , edk2-devel-groups-io CC: "Dong, Eric" Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Thread-Topic: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs Thread-Index: AQHVx0QRJhglwVuMz0K0XJVTupe0daftP+zw Date: Thu, 16 Jan 2020 12:22:23 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3F6BD9@SHSMSX104.ccr.corp.intel.com> References: <20200109232512.11022-1-lersek@redhat.com> In-Reply-To: <20200109232512.11022-1-lersek@redhat.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Thanks for finding and fixing the bug. The code change for 5level paging was done many years ago before mAddressEncMask was added. The two lines of *Pd assignment might be caused when resolving the local merge conflict. Reviewed-by: Ray Ni > -----Original Message----- > From: Laszlo Ersek > Sent: Friday, January 10, 2020 7:25 AM > To: edk2-devel-groups-io > Cc: Dong, Eric ; Ni, Ray > Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting reg= ression for PDEs >=20 > In commit 4eee0cc7cc0d ("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when > CPU supports", 2019-07-12), the Page Directory Entry setting was regresse= d > (corrupted) when splitting a 2MB page to 512 4KB pages, in the > InitPaging() function. >=20 > Consider the following hunk, displayed with >=20 > $ git show --function-context --ignore-space-change 4eee0cc7cc0db >=20 > > // > > // If it is 2M page, check IsAddressSplit() > > // > > if (((*Pd & IA32_PG_PS) !=3D 0) && IsAddressSplit (Address))= { > > // > > // Based on current page table, create 4KB page table for = split area. > > // > > ASSERT (Address =3D=3D (*Pd & PHYSICAL_ADDRESS_MASK)); > > > > Pt =3D AllocatePageTableMemory (1); > > ASSERT (Pt !=3D NULL); > > > > + *Pd =3D (UINTN) Pt | IA32_PG_RW | IA32_PG_P; > > + > > // Split it > > - for (PtIndex =3D 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtInde= x++) { > > - Pt[PtIndex] =3D Address + ((PtIndex << 12) | mAddressEncMa= sk | PAGE_ATTRIBUTE_BITS); > > + for (PtIndex =3D 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtIn= dex++, Pt++) { > > + *Pt =3D Address + ((PtIndex << 12) | mAddressEncMask | P= AGE_ATTRIBUTE_BITS); > > } // end for PT > > *Pd =3D (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBU= TE_BITS; > > } // end if IsAddressSplit > > } // end for PD >=20 > 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). >=20 > Second, appending "Pt++" to the incrementing expression of the PTE loop i= s > 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. >=20 > 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 wit= h > 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. >=20 > 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. >=20 > This issue has been caught as follows: >=20 > - OVMF is started with 2001 MB of guest RAM. >=20 > - This places the main SMRAM window at 0x7C10_1000. >=20 > - The SMRAM management in the SMM Core links this SMRAM window into > "mSmmMemoryMap", with a FREE_PAGE_LIST record placed at the start of th= e > area. >=20 > - 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). >=20 > - Due to the corrupted PDE, the second phase of InitPaging() already look= s > up the PTE for Address=3D0x7C10_1000 in the wrong place. The second pha= se > goes on to mark bogus PTEs as "NX". >=20 > - 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(). >=20 > - When SetMemMapAttributes() reaches another entry in the SMRAM map, > ConvertMemoryPageAttributes() decides it needs to split a 2MB page, and > calls SplitPage(). >=20 > - SplitPage() calls AllocatePageTableMemory() for the new Page Table, > which takes us to InternalAllocMaxAddress() in the SMM Core. >=20 > - 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). >=20 > Remove the useless assignment to (*Pd) from before the loop. Revert the > loop incrementing and the PTE assignment to the known good version. >=20 > Cc: Eric Dong > Cc: Ray Ni > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=3D1789335 > Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358 > Signed-off-by: Laszlo Ersek > --- >=20 > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: smm_cpu_page_split_corrupt_pde >=20 > UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/SmmProfile.c > index c5131526f0c6..c47b5573e366 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > @@ -657,11 +657,9 @@ InitPaging ( > Pt =3D AllocatePageTableMemory (1); > ASSERT (Pt !=3D NULL); >=20 > - *Pd =3D (UINTN) Pt | IA32_PG_RW | IA32_PG_P; > - > // Split it > - for (PtIndex =3D 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtInde= x++, Pt++) { > - *Pt =3D Address + ((PtIndex << 12) | mAddressEncMask | PAG= E_ATTRIBUTE_BITS); > + for (PtIndex =3D 0; PtIndex < SIZE_4KB / sizeof(*Pt); PtInde= x++) { > + Pt[PtIndex] =3D Address + ((PtIndex << 12) | mAddressEncMa= sk | PAGE_ATTRIBUTE_BITS); > } // end for PT > *Pd =3D (UINT64)(UINTN)Pt | mAddressEncMask | PAGE_ATTRIBUTE= _BITS; > } // end if IsAddressSplit > -- > 2.19.1.3.g30247aa5d201