From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 F420E2125F1F8 for ; Wed, 13 Jun 2018 08:10:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 433DE87A82; Wed, 13 Jun 2018 15:10:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-190.rdu2.redhat.com [10.10.122.190]) by smtp.corp.redhat.com (Postfix) with ESMTP id F2FF82166BB2; Wed, 13 Jun 2018 15:10:10 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Ruiyu Ni , Jiewen Yao , Eric Dong References: <20180613053501.4604-1-jian.j.wang@intel.com> <20180613053501.4604-2-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 13 Jun 2018 17:10:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180613053501.4604-2-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 13 Jun 2018 15:10:12 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 13 Jun 2018 15:10:12 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jun 2018 15:10:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Jian, I have three comments: On 06/13/18 07:35, Jian J Wang wrote: >> v2: >> a. add more specific explanations in commit message >> b. add more comments in code >> c. remove redundant logic in IsInSmm() >> d. fix a logic hole in GetCurrentPagingContext() >> e. replace meanless constant macro with meaning ones > > The MdePkg/Library/SmmMemoryAllocationLib, used only by DXE_SMM_DRIVER, > allows to free memory allocated in DXE (before EndOfDxe). This is done > by checking the memory range and calling gBS services to do real > operation if the memory to free is out of SMRAM. If some memory related > features, like Heap Guard, are enabled, gBS interface will turn to > EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), provided by > DXE driver UefiCpuPkg/CpuDxe, to change memory paging attributes. This > means we have part of DXE code running in SMM mode in certain > circumstances. > > Because page table in SMM mode is different from DXE mode and CpuDxe > always uses current registers (CR0, CR3, etc.) to get memory paging > attributes, it cannot get the correct attributes of DXE memory in SMM > mode from SMM page table. This will cause incorrect memory manipulations, > like fail the releasing of Guard pages if Heap Guard is enabled. > > The solution in this patch is to store the DXE page table information > (e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe > driver. If CpuDxe detects it's in SMM mode, it will use this global > variable to access page table instead of current processor registers. > This can avoid retrieving wrong DXE memory paging attributes and changing > SMM page table attributes unexpectedly. > > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Jiewen Yao > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > UefiCpuPkg/CpuDxe/CpuDxe.inf | 1 + > UefiCpuPkg/CpuDxe/CpuPageTable.c | 159 ++++++++++++++++++++++++++++++--------- > 2 files changed, 123 insertions(+), 37 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf > index 3c938cee53..ce2bd3627c 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf > @@ -66,6 +66,7 @@ > [Protocols] > gEfiCpuArchProtocolGuid ## PRODUCES > gEfiMpServiceProtocolGuid ## PRODUCES > + gEfiSmmBase2ProtocolGuid ## CONSUMES (1) In my v1 review, I suggested "SOMETIMES_CONSUMES" for this hint. And that's because CpuDxe can be included in platform builds that don't include the SMM driver stack at all; in that case, gEfiSmmBase2ProtocolGuid will not be available and will not be consumed. In other words, we cannot say that the protocol is always consumed. > > [Guids] > gIdleLoopEventGuid ## CONSUMES ## Event > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index e2595b4d89..b7e75922b6 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -23,10 +23,21 @@ > #include > #include > #include > +#include > +#include > +#include > > #include "CpuDxe.h" > #include "CpuPageTable.h" > > +/// > +/// Paging registers > +/// > +#define CR0_WP BIT16 > +#define CR0_PG BIT31 > +#define CR4_PSE BIT4 > +#define CR4_PAE BIT5 (2a) I think the BITxx -> CRx_xx macro replacements should have been split to a separate patch. > + > /// > /// Page Table Entry > /// > @@ -87,7 +98,46 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > {Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64}, > }; > > -PAGE_TABLE_POOL *mPageTablePool = NULL; > +PAGE_TABLE_POOL *mPageTablePool = NULL; > +PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext; > +EFI_SMM_BASE2_PROTOCOL *mSmmBase2 = NULL; > + > +/** > + Check if current execution environment is in SMM mode or not, via > + EFI_SMM_BASE2_PROTOCOL. > + > + This is necessary because of the fact that MdePkg\Library\SmmMemoryAllocationLib > + supports to free memory outside SMRAM. The library will call gBS->FreePool() or > + gBS->FreePages() and then SetMemorySpaceAttributes interface in turn to change > + memory paging attributes during free operation, if some memory related features > + are enabled (like Heap Guard). > + > + This means that SetMemorySpaceAttributes() has chance to run in SMM mode. This > + will cause incorrect result because SMM mode always loads its own page tables, > + which are usually different from DXE. This function can be used to detect such > + situation and help to avoid further misoperations. > + > + @retval TRUE In SMM mode. > + @retval FALSE Not in SMM mode. > +**/ > +BOOLEAN > +IsInSmm ( > + VOID > + ) > +{ > + BOOLEAN InSmm; > + > + InSmm = FALSE; > + if (mSmmBase2 == NULL) { > + gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL, (VOID **)&mSmmBase2); > + } > + > + if (mSmmBase2 != NULL) { > + mSmmBase2->InSmm (mSmmBase2, &InSmm); > + } > + > + return InSmm; > +} > > /** > Return current paging context. > @@ -99,45 +149,61 @@ GetCurrentPagingContext ( > IN OUT PAGE_TABLE_LIB_PAGING_CONTEXT *PagingContext > ) > { > - UINT32 RegEax; > - UINT32 RegEdx; > + UINT32 RegEax; > + CPUID_EXTENDED_CPU_SIG_EDX RegEdx; > + MSR_IA32_EFER_REGISTER MsrEfer; > > - ZeroMem(PagingContext, sizeof(*PagingContext)); > - if (sizeof(UINTN) == sizeof(UINT64)) { > - PagingContext->MachineType = IMAGE_FILE_MACHINE_X64; > - } else { > - PagingContext->MachineType = IMAGE_FILE_MACHINE_I386; > - } > - if ((AsmReadCr0 () & BIT31) != 0) { > - PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64); > - } else { > - PagingContext->ContextData.X64.PageTableBase = 0; > - } > + // > + // Don't retrieve current paging context from processor if in SMM mode. > + // > + if (!IsInSmm ()) { > + ZeroMem (&mPagingContext, sizeof(mPagingContext)); > > - if ((AsmReadCr4 () & BIT4) != 0) { > - PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE; > - } > - if ((AsmReadCr4 () & BIT5) != 0) { > - PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE; > - } > - if ((AsmReadCr0 () & BIT16) != 0) { > - PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE; > - } > + if (sizeof(UINTN) == sizeof(UINT64)) { > + mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64; > + } else { > + mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386; > + } > + if ((AsmReadCr0 () & CR0_PG) != 0) { > + mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64); > + } else { > + mPagingContext.ContextData.X64.PageTableBase = 0; > + } > > - AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL); > - if (RegEax > 0x80000000) { > - AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx); > - if ((RegEdx & BIT20) != 0) { > - // XD supported > - if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) { > - // XD activated > - PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED; > - } > + if ((AsmReadCr4 () & CR4_PSE) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE; > + } > + if ((AsmReadCr4 () & CR4_PAE) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE; > + } > + if ((AsmReadCr0 () & CR0_WP) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE; > } > - if ((RegEdx & BIT26) != 0) { > - PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT; > + > + AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL); > + if (RegEax >= CPUID_EXTENDED_CPU_SIG) { (2b) Similarly to (2a), this cleanup (which is correct, and welcome) should have been split to a separate patch. > + AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, (UINT32 *)&RegEdx); (3) I think it would be more idiomatic to pass &RegEdx.Uint32 as the last argument. However, this method works as well. I don't think that posting a v3 is necessary just because of these remarks. Please fix (1) though, just before you push the patch. * For this patch, with (1) updated: Reviewed-by: Laszlo Ersek * For the series: Regression-tested-by: Laszlo Ersek For the regression testing, I used OVMF built both with and without SMM. Also, for patch #2, please remember that OVMF does not enable the page guard feature. Thanks! Laszlo > + > + if (RegEdx.Bits.NX != 0) { > + // XD supported > + MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER); > + if (MsrEfer.Bits.NXE != 0) { > + // XD activated > + mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED; > + } > + } > + > + if (RegEdx.Bits.Page1GB != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT; > + } > } > } > + > + // > + // This can avoid getting SMM paging context if in SMM mode. We cannot assume > + // SMM mode shares the same paging context as DXE. > + // > + CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext)); > } > > /** > @@ -507,7 +573,14 @@ IsReadOnlyPageWriteProtected ( > VOID > ) > { > - return ((AsmReadCr0 () & BIT16) != 0); > + // > + // To avoid unforseen consequences, don't touch paging settings in SMM mode > + // in this driver. > + // > + if (!IsInSmm ()) { > + return ((AsmReadCr0 () & CR0_WP) != 0); > + } > + return FALSE; > } > > /** > @@ -518,7 +591,13 @@ DisableReadOnlyPageWriteProtect ( > VOID > ) > { > - AsmWriteCr0 (AsmReadCr0() & ~BIT16); > + // > + // To avoid unforseen consequences, don't touch paging settings in SMM mode > + // in this driver. > + // > + if (!IsInSmm ()) { > + AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP); > + } > } > > /** > @@ -529,7 +608,13 @@ EnableReadOnlyPageWriteProtect ( > VOID > ) > { > - AsmWriteCr0 (AsmReadCr0() | BIT16); > + // > + // To avoid unforseen consequences, don't touch paging settings in SMM mode > + // in this driver. > + // > + if (!IsInSmm ()) { > + AsmWriteCr0 (AsmReadCr0 () | CR0_WP); > + } > } > > /** >