From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.1201.1579246453485574634 for ; Thu, 16 Jan 2020 23:34:13 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Y57DrB9z; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579246452; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/CmfQFwtVBIlx/nFmaU7OcSubtgyqdrej5mt8TqHqsg=; b=Y57DrB9z5bc8ZBz+s0sfc/LYI4g2qZ5a06rPJG6A13VxWeHN9BHWcDMsxEk9EH658/7l4g bBPhyiT/9IACdWKGUu/8EYXCvJn1p1YNEHRyrNka1Q3Ys7WnMMGUMMfzyjM//lt6aJ9Ou0 RoYPwfhQVxN+ivhBs0L+ScmgKjP0MaQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-398-FouQYzoEPDCfSB5hBsi-Hw-1; Fri, 17 Jan 2020 02:34:10 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AA43018B9FCB; Fri, 17 Jan 2020 07:34:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-211.ams2.redhat.com [10.36.116.211]) by smtp.corp.redhat.com (Postfix) with ESMTP id A11817DB4F; Fri, 17 Jan 2020 07:34:08 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs To: "Ni, Ray" , edk2-devel-groups-io Cc: "Dong, Eric" References: <20200109232512.11022-1-lersek@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C3F6BD9@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 17 Jan 2020 08:34:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3F6BD9@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-MC-Unique: FouQYzoEPDCfSB5hBsi-Hw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/16/20 13:22, Ni, Ray wrote: > 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. Ah, that makes total sense. I didn't expect this reason! > Reviewed-by: Ray Ni Thanks! Laszlo > >> -----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 regression for PDEs >> >> 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 >> --- >> >> Notes: >> Repo: https://github.com/lersek/edk2.git >> Branch: smm_cpu_page_split_corrupt_pde >> >> 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 c5131526f0c6..c47b5573e366 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 >> -- >> 2.19.1.3.g30247aa5d201 >