From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=17.171.2.34; helo=mail-in24.apple.com; envelope-from=afish@apple.com; receiver=edk2-devel@lists.01.org Received: from mail-in24.apple.com (mail-out24.apple.com [17.171.2.34]) (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 021D4211E00EE for ; Wed, 13 Jun 2018 08:14:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1528902846; x=2392816446; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-transfer-encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=eI7GeCgfkjcNrI5odG18kIICstACeMgSpJZwma44EYc=; b=lwobaYaEHUbNywfojUThQxKq2dsmanwmmqrvfKz7p8bg1jTcQaGsxie6w/qIT35P U4GSax1Jyj2nTVo7B08I+EixGn0GMVgmCxkpUvAuccQQwY08fs6R+ck02azuf821 D77IZ6Oji8sg9+JpsGx/ahAVO/ALEQsQv0CJWQ6MTHxQXpBdjjmsoQ6mx9wUtEzs 6cvReXFDh+QsuQaN00yZAxVN4tsqwIQdm4OwzGDV2pwz+L1iqqPqS2E7+0nvpOWe dF25eCWoOeqcuno1xQ/yfAIftv08nyixoYF6ppLN9/ugf5p2FIJ1tV8w4cOT9HBo cut4ztwyZZxWIdrhzpNgBQ==; X-AuditID: 11ab0218-0d1ff70000001a2c-4c-5b2134bee653 Received: from ma1-mtap-s03.corp.apple.com (ma1-mtap-s03.corp.apple.com [17.40.76.7]) (using TLS with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail-in24.apple.com (Apple Secure Mail Relay) with SMTP id 31.D4.06700.EB4312B5; Wed, 13 Jun 2018 08:14:06 -0700 (PDT) MIME-version: 1.0 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by ma1-mtap-s03.corp.apple.com (Oracle Communications Messaging Server 8.0.2.2.20180329 64bit (built Mar 29 2018)) with ESMTPS id <0PA9004G4OZIV090@ma1-mtap-s03.corp.apple.com>; Wed, 13 Jun 2018 08:14:06 -0700 (PDT) Received: from [17.235.42.33] (unknown [17.235.42.33]) by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.2.20180403 64bit (built Apr 3 2018)) with ESMTPSA id <0PA900DEKOZGNW90@nwk-mmpp-sz13.apple.com>; Wed, 13 Jun 2018 08:14:06 -0700 (PDT) X-Va-A: X-Va-T-CD: 539f254ca45f5b5491546d0afe0a330e X-Va-E-CD: 18ed0a517014bf57b97d942e0ff436bc X-Va-R-CD: e7729fba3acaf2ec2c6aab0abbdae470 X-Va-CD: 0 X-Va-ID: 9eae2a3b-2ba8-453d-a9ad-40ecf2d4ecf8 X-V-A: X-V-T-CD: 2c8d5f55c82ae0607ee559b8293ed86e X-V-E-CD: 18ed0a517014bf57b97d942e0ff436bc X-V-R-CD: e7729fba3acaf2ec2c6aab0abbdae470 X-V-CD: 0 X-V-ID: bbfd801e-ab38-4b07-a14f-7362c767579a X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-13_06:,, signatures=0 Sender: afish@apple.com From: Andrew Fish In-reply-to: <20180613053501.4604-2-jian.j.wang@intel.com> Date: Wed, 13 Jun 2018 08:14:03 -0700 Cc: edk2-devel , Ruiyu Ni , Jiewen Yao , Laszlo Ersek , Eric Dong Message-id: References: <20180613053501.4604-1-jian.j.wang@intel.com> <20180613053501.4604-2-jian.j.wang@intel.com> To: Jian J Wang X-Mailer: Apple Mail (2.3445.6.18) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrCIsWRmVeSWpSXmKPExsUiqOHDrrvPRDHaYOFGU4s9h44yW2x+EWwx 79sMVot1Hz0slh3bwWLxsmc1uwObx+I9L5k8umf/Y/F4v+8qWwBzFJdNSmpOZllqkb5dAldG Y/8upoKuoIrdbw8wNTB+seti5OSQEDCRuHz+GksXIxeHkMB+Joml746ygCR4BQQlfky+B2Rz cDALyEscPC8LEmYW0JL4/qgVqn4jk0T/0utsEE4/UPOzL+wQU9kl/vzawQJha0tMOPCEGcbe OGcfK4x94MVfNgibS2LB1tNQcV2J/inLoOJsEutPLGECOUICaPPtGYwQYS2J021rWWHsVWen Qa3ilDj/ZSLUCToSC95cgqrJltje2Q02UlhAXOLdmU3MEHaKxI2OZjCbTUBZYsX8D2C9nAJW EvcndzCB2CwCqhLvjrxjBfmRWWAjo8SPXTsZIQFkI/Gy4xUryG1CApkS77dagJgiAuoSxxa7 Q6xVkvi/6wjzBEa5WUghOgsRorOQQnQBI/MqRuHcxMwc3cw8IxO9xIKCnFS95PzcTYygZLCa SWIH45fXhocYBTgYlXh4I2QUo4VYE8uKK3MPMUpzsCiJ837cJRYtJJCeWJKanZpakFoUX1Sa k1p8iJGJg1OqgTFbMGXLIYaXgk0Ozn8shG8zl/2/xtT3yf6sQuRCtaYbzYeTY3RW/y8rmynH 8Nv+62HZMifbcy2/K7rkkr7PiZdT/8hrKtwhNH1SorP2gxd39FoL56RL7VxkV+l1vmmWRNR/ L/9VrwIy2Iv5/m00lQ9fUTOj5Lx6JJ+I7sc/oftur7VZUjhDXImlOCPRUIu5qDgRAFpAdRPn AgAA 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:14:08 -0000 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII > On Jun 12, 2018, at 10:35 PM, 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. > Are there any security implications having SMM depend on attacker controlled data (DXE page tables)? Thanks, Andrew Fish > 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 > > [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 > + > /// > /// 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) { > + AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, (UINT32 *)&RegEdx); > + > + 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); > + } > } > > /** > -- > 2.16.2.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel