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.web11.5435.1579084607210267221 for ; Wed, 15 Jan 2020 02:36:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PycEAXEr; 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=1579084606; 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=CsMGx4I0bsWypcExhLd1nyF/XUkz4dUogstn7ArOf5U=; b=PycEAXErd1UY4mzLMEsaCapcmYGBwQgQ2vlqiTqyAqkQnZIbvnsvp9WfshPcIleKsBl/Wc n2Fb42D1reyAsyjP1ZUC2qTZMNJZmG7djdDpdoi6UVEGAVz5fy0fk2bgTeWNz5NMY0YE8o l2pb4ksK4CPrhk3+Klv7nP14lE1g2JY= 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-18-x_VM23kEND69TKkvoNYiRg-1; Wed, 15 Jan 2020 05:36:43 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 03E751856A60; Wed, 15 Jan 2020 10:36:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-98.ams2.redhat.com [10.36.116.98]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0F56D842A4; Wed, 15 Jan 2020 10:36:40 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs From: "Laszlo Ersek" To: Eric Dong , Ray Ni Cc: edk2-devel-groups-io Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20200109232512.11022-1-lersek@redhat.com> Message-ID: Date: Wed, 15 Jan 2020 11:36:40 +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.15 X-MC-Unique: x_VM23kEND69TKkvoNYiRg-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 Hi Ray, Eric, any comments about the patch below? It hasn't been a week yet (I usually don't ping before a full week), but the patch is not large, so I figure maybe you've missed it. (Traffic on has been quite high recently.) Thanks! Laszlo 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 >