From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.2822.1579254598121232638 for ; Fri, 17 Jan 2020 01:49:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Xl2Z4j3w; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579254597; h=from:from:reply-to: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=eXlO85Vq+1OGkDG1dob2ne2ozNe/C7PmoMRsthvx0yo=; b=Xl2Z4j3wwFQSVVZRHNwqSBJ+h/CX3KooAH/yTHoxPQcQJxgzSojvi0MyaJ68dBut7bTJXi GoWzuVq0+od5UNdJslmZ0ArbY3KyHJ47N0yW/jYAQ10nzIqcdciidBf2Ywhko9Ne6uX/e0 qmMqILGFF1UlG9OEtJ0e0pLQsmnRGEg= 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-121-GHjnSYJ5MQuqIqNj_u05CQ-1; Fri, 17 Jan 2020 04:49:55 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 187FB800D4C; Fri, 17 Jan 2020 09:49:54 +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 C812A5C545; Fri, 17 Jan 2020 09:49:50 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Eric Dong , Ray Ni , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20200109232512.11022-1-lersek@redhat.com> Message-ID: Date: Fri, 17 Jan 2020 10:49:49 +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: <20200109232512.11022-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: GHjnSYJ5MQuqIqNj_u05CQ-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/10/20 00:25, Laszlo Ersek wrote: > 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 > Pushed as commit a52355624440; thank you, Phil & Ray! Laszlo