From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web12.5621.1579086504579369689 for ; Wed, 15 Jan 2020 03:08:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QjVfNgVk; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579086503; 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=UHcZRWwHs5qMKBzEzY6108q9QtI+U9iit63naMuKFYo=; b=QjVfNgVkUIk4Fi2caNqWw/VCn9UU+ObWf8vK7xMW+Eob2uIdRuyjPcX5kztxsw5JVJ1KrX j2dcHkrx0pr5B9l3hNRNlzbQ3Jdu49FI5X4/S4sH9Qpys6NNV1I3X/AGwEgihy7UHguriz A5dcjY7lk5SSA/MHRt+4PDCb6JMjZKw= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-391-VPve1OA6PkKv6GF7_Zki8Q-1; Wed, 15 Jan 2020 06:08:22 -0500 Received: by mail-wm1-f69.google.com with SMTP id n17so2396093wmk.1 for ; Wed, 15 Jan 2020 03:08:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=UHcZRWwHs5qMKBzEzY6108q9QtI+U9iit63naMuKFYo=; b=QuRN9IX1PUOFEv8kVc1qWOe6UJ7D+exmBxWw4nGzfZEaIYLl/Mn/lipM381pDycoS/ xikJjoCfGlhbaiyydKnUbOSiNHlouG+XFRE22BrrlgtdhxdsVjv/RKOQUXkHW+CozwpE elcQFn/jRvUyyduWr4+E/U/zBFom2kI+Yl9fGXu42Ztm3GwaRdS5AADr/mdz73z9Mb2+ L4tea2/tB3JIY40I6s4hki9wufWUBjkkHgJbJb2BdnrdKAqQ/Hp9VtXPAL1K65OuptDp kaf27aepRJr8iC3pTpD9PuvLklbRzrEPY+Mt5k9bD4HPJ3b6TCz2m/jxPyYBZ74/SPRm RJ8w== X-Gm-Message-State: APjAAAUhBX82DJiU3lm7VsSdBwzuC8rUNHhBMcyTEXapI2kxLVXXtgJt 994uQSTHcaC6hxpfatlbPhsLAErKJIOICGtAC3FeYNPsy50867QGBt+u/xb/4BahaIlnSSZU0+I BCE4mx0hgOvaa/g== X-Received: by 2002:adf:806e:: with SMTP id 101mr32028047wrk.300.1579086500878; Wed, 15 Jan 2020 03:08:20 -0800 (PST) X-Google-Smtp-Source: APXvYqyGHb7a0i7IjgAO0jddpF3jTogvXK2TN5tqYNM/qm+nIeRVr94VEKuuYpkwT/sErKC5+L2chQ== X-Received: by 2002:adf:806e:: with SMTP id 101mr32028006wrk.300.1579086500438; Wed, 15 Jan 2020 03:08:20 -0800 (PST) Return-Path: Received: from ?IPv6:2a01:cb1d:8a0a:f500:48c1:8eab:256a:caf9? ([2a01:cb1d:8a0a:f500:48c1:8eab:256a:caf9]) by smtp.gmail.com with ESMTPSA id n187sm23197382wme.28.2020.01.15.03.08.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Jan 2020 03:08:19 -0800 (PST) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting regression for PDEs To: devel@edk2.groups.io, lersek@redhat.com Cc: Eric Dong , Ray Ni References: <20200109232512.11022-1-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <7efbb866-7738-dd92-9cee-6bca35207c40@redhat.com> Date: Wed, 15 Jan 2020 12:08:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <20200109232512.11022-1-lersek@redhat.com> X-MC-Unique: VPve1OA6PkKv6GF7_Zki8Q-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 1/10/20 12:25 AM, 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 > Reviewed-by: Philippe Mathieu-Daude