From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D7F8121D1E2DA for ; Fri, 25 Aug 2017 02:47:22 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 53610C058EA3; Fri, 25 Aug 2017 09:49:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 53610C058EA3 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-78.phx2.redhat.com [10.3.116.78]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2E6F560BE7; Fri, 25 Aug 2017 09:49:57 +0000 (UTC) To: Star Zeng , edk2-devel@lists.01.org Cc: Jiewen Yao , Eric Dong References: <1503544809-166388-1-git-send-email-star.zeng@intel.com> From: Laszlo Ersek Message-ID: <0432078c-6490-139e-20d8-c6eac58ffd3f@redhat.com> Date: Fri, 25 Aug 2017 11:49:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1503544809-166388-1-git-send-email-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 25 Aug 2017 09:49:58 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Aug 2017 09:47:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Star, On 08/24/17 05:20, Star Zeng wrote: > https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports > memory protection crash in PiSmmCpuDxeSmm, Ia32 build with > RAM above 4GB (of which 2GB are placed in 64-bit address). > It is because UEFI builds identity mapping page tables, >> 4G address is not supported at Ia32 build. > > This patch is to get the PhysicalAddressBits that is used > to build in PageTbl.c(Ia32/X64), and use it to check whether > the address is supported or not in ConvertMemoryPageAttributes(). > > With this patch, the debug messages will be like below. > UefiMemory protection: 0x0 - 0x9F000 Success > UefiMemory protection: 0x100000 - 0x807000 Success > UefiMemory protection: 0x808000 - 0x810000 Success > UefiMemory protection: 0x818000 - 0x820000 Success > UefiMemory protection: 0x1510000 - 0x7B798000 Success > UefiMemory protection: 0x7B79B000 - 0x7E538000 Success > UefiMemory protection: 0x7E539000 - 0x7E545000 Success > UefiMemory protection: 0x7E55A000 - 0x7E61F000 Success > UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success > UefiMemory protection: 0x7F703000 - 0x7F70B000 Success > UefiMemory protection: 0x7F70F000 - 0x7F778000 Success > UefiMemory protection: 0x100000000 - 0x180000000 Unsupported > > Cc: Jiewen Yao > Cc: Laszlo Ersek > Cc: Eric Dong > Originally-suggested-by: Jiewen Yao > Reported-by: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 4 +++ > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 + > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 +++++++++++++++++----- > 3 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 32ce5958c59c..e88b42d73343 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #include "PiSmmCpuDxeSmm.h" > > +UINT8 mPhysicalAddressBits; > + > /** > Create PageTable for SMM use. > > @@ -36,6 +38,8 @@ SmmInitPageTable ( > // > InitializeSpinLock (mPFLock); > > + mPhysicalAddressBits = 32; > + > if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { > // > // Set own Page Fault entry instead of the default one, because SMM Profile > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index dbce9ec520fe..1cf85c1481a7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -419,6 +419,7 @@ extern SPIN_LOCK *mConfigSmmCodeAccessCheckLock; > extern SPIN_LOCK *mMemoryMappedLock; > extern EFI_SMRAM_DESCRIPTOR *mSmmCpuSmramRanges; > extern UINTN mSmmCpuSmramRangeCount; > +extern UINT8 mPhysicalAddressBits; > > // > // Copy of the PcdPteMemoryEncryptionAddressOrMask > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index a535389c26ce..3ad5256f1e03 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -1,6 +1,6 @@ > /** @file > > -Copyright (c) 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes ( > PAGE_ATTRIBUTE SplitAttribute; > RETURN_STATUS Status; > BOOLEAN IsEntryModified; > + EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress; > > ASSERT (Attributes != 0); > ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0); > @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes ( > return RETURN_INVALID_PARAMETER; > } > > + MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, mPhysicalAddressBits) - 1); > + if (BaseAddress > MaximumSupportMemAddress) { > + return RETURN_UNSUPPORTED; > + } > + if (Length > MaximumSupportMemAddress) { > + return RETURN_UNSUPPORTED; > + } > + if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) { > + return RETURN_UNSUPPORTED; > + } > + > // DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes)); > > if (IsSplitted != NULL) { > @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes ( > VOID > ) > { > + EFI_STATUS Status; > EFI_MEMORY_DESCRIPTOR *MemoryMap; > UINTN MemoryMapEntryCount; > UINTN Index; > @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes ( > MemoryMap = mUefiMemoryMap; > for (Index = 0; Index < MemoryMapEntryCount; Index++) { > if (IsUefiPageNotPresent(MemoryMap)) { > - DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages))); > - SmmSetMemoryAttributes ( > + Status = SmmSetMemoryAttributes ( > + MemoryMap->PhysicalStart, > + EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), > + EFI_MEMORY_RP > + ); > + DEBUG (( > + DEBUG_INFO, > + "UefiMemory protection: 0x%lx - 0x%lx %r\n", > MemoryMap->PhysicalStart, > - EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), > - EFI_MEMORY_RP > - ); > + MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), > + Status > + )); > } > MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize); > } > before applying this patch for local testing, I figured I'd look it over quickly. I think that you missed adding the X64 changes to the commit, with "git add". Because, the "mPhysicalAddressBits" variable is declared in common code, it is also consumed in common code, but it is only defined (i.e., allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking. I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch. If I'm right and you decide to post v2, then I suggest another (very small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific. Thanks Laszlo