From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 956CC82008 for ; Sun, 18 Dec 2016 17:29:51 -0800 (PST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP; 18 Dec 2016 17:29:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,371,1477983600"; d="scan'208";a="913771271" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga003.jf.intel.com with ESMTP; 18 Dec 2016 17:29:51 -0800 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 18 Dec 2016 17:29:50 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 18 Dec 2016 17:29:50 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.54]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.235]) with mapi id 14.03.0248.002; Mon, 19 Dec 2016 09:29:48 +0800 From: "Fan, Jeff" To: "Yao, Jiewen" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , Laszlo Ersek Thread-Topic: [PATCH V2] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection. Thread-Index: AQHSV6LluEbwEvuPDUi7c3vcG16W8KEOf3ZQ Date: Mon, 19 Dec 2016 01:29:47 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4C4EFBE5@shsmsx102.ccr.corp.intel.com> References: <1481895993-24476-1-git-send-email-jiewen.yao@intel.com> In-Reply-To: <1481895993-24476-1-git-send-email-jiewen.yao@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOWRjZWM4ZjctZDA5Mi00NmIzLTk5ZDctNjhmMGMzYTkyZGEwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlM1UURpWEpaeWl6eTlKN0hRSzAxTGxDVGp6Tzl2QjBaRFRkcmRxSjZiSHM9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V2] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Dec 2016 01:29:51 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jeff Fan -----Original Message----- From: Yao, Jiewen=20 Sent: Friday, December 16, 2016 9:47 PM To: edk2-devel@lists.01.org Cc: Fan, Jeff; Kinney, Michael D; Laszlo Ersek Subject: [PATCH V2] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protect= ion. This patch sets the normal OS buffer EfiLoaderCode/Data, EfiBootServicesCod= e/Data, EfiConventionalMemory, EfiACPIReclaimMemory to be not present after= SmmReadyToLock. To access these region in OS runtime phase is not a good solution. Previously, we did similar check in SmmMemLib to help SMI handler do the ch= eck. But if SMI handler forgets the check, it can still access these OS reg= ion and bring risk. So here we enforce the policy to prevent it happening. Cc: Jeff Fan Cc: Michael D Kinney Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jiewen Yao Reviewed-by: Jeff Fan --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 7 + UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 23 +- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 29 +++ UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 270 +++++++++++++++++= +++ UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 7 + 5 files changed, 324 insertions(+), 12 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/Ia32/PageTbl.c index ba79477..c1f4b7e 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -149,6 +149,13 @@ SmiPFHandler ( ); CpuDeadLoop (); } + if (IsSmmCommBufferForbiddenAddress (PFAddress)) { + DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x= %x)!\n", PFAddress)); + DEBUG_CODE ( + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip); + ); + CpuDeadLoop (); + } } =20 if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { diff --git a/UefiCpuPkg/Pi= SmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c index 4bef60a..fc7714a 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c @@ -504,6 +504,11 @@ SmmReadyToLockEventNotify ( GetAcpiCpuData (); =20 // + // Cache a copy of UEFI memory map before we start profiling feature. + // + GetUefiMemoryMap (); + + // // Set SMM ready to lock flag and return // mSmmReadyToLock =3D TRUE; @@ -1154,17 +1159,6 @@ ConfigSmmCodeAccessCheck ( } =20 /** - Set code region to be read only and data region to be execute disable. -**/ -VOID -SetRegionAttributes ( - VOID - ) -{ - SetMemMapAttributes (); -} - -/** This API provides a way to allocate memory for page table. =20 This API can be called more once to allocate memory for page tables. @@ -1320,7 +1314,12 @@ PerformRemainingTasks ( // // Mark critical region to be read-only in page table // - SetRegionAttributes (); + SetMemMapAttributes (); + + // + // For outside SMRAM, we only map SMM communication buffer or MMIO. + // + SetUefiMemMapAttributes (); =20 // // Set page table itself to be read-only diff --git a/UefiCpuPkg/PiSmm= CpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index 9160fa8..69c54fb 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -839,6 +839,35 @@ SetMemMapAttributes ( ); =20 /** + This function sets UEFI memory attribute according to UEFI memory map. +**/ +VOID +SetUefiMemMapAttributes ( + VOID + ); + +/** + Return if the Address is forbidden as SMM communication buffer. + + @param[in] Address the address to be checked + + @return TRUE The address is forbidden as SMM communication buffer. + @return FALSE The address is allowed as SMM communication buffer. +**/ +BOOLEAN +IsSmmCommBufferForbiddenAddress ( + IN UINT64 Address + ); + +/** + This function caches the UEFI memory map information. +**/ +VOID +GetUefiMemoryMap ( + VOID + ); + +/** This function sets memory attribute for page table. **/ VOID diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPk= g/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c index 588aa27..f4716f3 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c @@ -16,6 +16,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHE= R EXPRESS OR IMPLIED. #define NEXT_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \ ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) + (Size))) =20 +#define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \ + ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size))) + +EFI_MEMORY_DESCRIPTOR *mUefiMemoryMap; +UINTN mUefiMemoryMapSize; +UINTN mUefiDescriptorSize; + PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] =3D { {Page4K, SIZE_4KB, PAGING_4K_ADDRESS_MASK_64}, {Page2M, SIZE_2MB, PAGING_2M_ADDRESS_MASK_64}, @@ -823,3 +830,266 @@ Se= tMemMapAttributes ( =20 return ; } + +/** + Sort memory map entries based upon PhysicalStart, from low to high. + + @param MemoryMap A pointer to the buffer in which firmware= places + the current memory map. + @param MemoryMapSize Size, in bytes, of the MemoryMap buffer. + @param DescriptorSize Size, in bytes, of an individual EFI_MEMO= RY_DESCRIPTOR. +**/ +STATIC +VOID +SortMemoryMap ( + IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap, + IN UINTN MemoryMapSize, + IN UINTN DescriptorSize + ) +{ + EFI_MEMORY_DESCRIPTOR *MemoryMapEntry; + EFI_MEMORY_DESCRIPTOR *NextMemoryMapEntry; + EFI_MEMORY_DESCRIPTOR *MemoryMapEnd; + EFI_MEMORY_DESCRIPTOR TempMemoryMap; + + MemoryMapEntry =3D MemoryMap; + NextMemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,=20 + DescriptorSize); MemoryMapEnd =3D (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *)=20 + MemoryMap + MemoryMapSize); while (MemoryMapEntry < MemoryMapEnd) { + while (NextMemoryMapEntry < MemoryMapEnd) { + if (MemoryMapEntry->PhysicalStart > NextMemoryMapEntry->PhysicalStar= t) { + CopyMem (&TempMemoryMap, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIP= TOR)); + CopyMem (MemoryMapEntry, NextMemoryMapEntry, sizeof(EFI_MEMORY_DES= CRIPTOR)); + CopyMem (NextMemoryMapEntry, &TempMemoryMap, sizeof(EFI_MEMORY_DES= CRIPTOR)); + } + + NextMemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, D= escriptorSize); + } + + MemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, Descri= ptorSize); + NextMemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,=20 +DescriptorSize); + } +} + +/** + Return if a UEFI memory page should be marked as not present in SMM page= table. + If the memory map entries type is + EfiLoaderCode/Data, EfiBootServicesCode/Data, EfiConventionalMemory, + EfiUnusableMemory, EfiACPIReclaimMemory, return TRUE. + Or return FALSE. + + @param[in] MemoryMap A pointer to the memory descriptor. + + @return TRUE The memory described will be marked as not present in SMM = page table. + @return FALSE The memory described will not be marked as not present in = SMM page table. +**/ +BOOLEAN +IsUefiPageNotPresent ( + IN EFI_MEMORY_DESCRIPTOR *MemoryMap + ) +{ + switch (MemoryMap->Type) { + case EfiLoaderCode: + case EfiLoaderData: + case EfiBootServicesCode: + case EfiBootServicesData: + case EfiConventionalMemory: + case EfiUnusableMemory: + case EfiACPIReclaimMemory: + return TRUE; + default: + return FALSE; + } +} + +/** + Merge continous memory map entries whose type is + EfiLoaderCode/Data, EfiBootServicesCode/Data, EfiConventionalMemory, + EfiUnusableMemory, EfiACPIReclaimMemory, because the memory described=20 +by + these entries will be set as NOT present in SMM page table. + + @param[in, out] MemoryMap A pointer to the buffer in which= firmware places + the current memory map. + @param[in, out] MemoryMapSize A pointer to the size, in bytes,= of the + MemoryMap buffer. On input, this= is the size of + the current memory map. On outp= ut, + it is the size of new memory map= after merge. + @param[in] DescriptorSize Size, in bytes, of an individual= EFI_MEMORY_DESCRIPTOR. +**/ +STATIC +VOID +MergeMemoryMapForNotPresentEntry ( + IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap, + IN OUT UINTN *MemoryMapSize, + IN UINTN DescriptorSize + ) +{ + EFI_MEMORY_DESCRIPTOR *MemoryMapEntry; + EFI_MEMORY_DESCRIPTOR *MemoryMapEnd; + UINT64 MemoryBlockLength; + EFI_MEMORY_DESCRIPTOR *NewMemoryMapEntry; + EFI_MEMORY_DESCRIPTOR *NextMemoryMapEntry; + + MemoryMapEntry =3D MemoryMap; + NewMemoryMapEntry =3D MemoryMap; + MemoryMapEnd =3D (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap +=20 + *MemoryMapSize); while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) { + CopyMem (NewMemoryMapEntry, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPT= OR)); + NextMemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,=20 + DescriptorSize); + + do { + MemoryBlockLength =3D (UINT64) (EFI_PAGES_TO_SIZE((UINTN)MemoryMapEn= try->NumberOfPages)); + if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) && + IsUefiPageNotPresent(MemoryMapEntry) && IsUefiPageNotPresent(Nex= tMemoryMapEntry) && + ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) =3D=3D Next= MemoryMapEntry->PhysicalStart)) { + MemoryMapEntry->NumberOfPages +=3D NextMemoryMapEntry->NumberOfPag= es; + if (NewMemoryMapEntry !=3D MemoryMapEntry) { + NewMemoryMapEntry->NumberOfPages +=3D NextMemoryMapEntry->Number= OfPages; + } + + NextMemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry,= DescriptorSize); + continue; + } else { + MemoryMapEntry =3D PREVIOUS_MEMORY_DESCRIPTOR (NextMemoryMapEntry,= DescriptorSize); + break; + } + } while (TRUE); + + MemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorS= ize); + NewMemoryMapEntry =3D NEXT_MEMORY_DESCRIPTOR (NewMemoryMapEntry,=20 + DescriptorSize); } + + *MemoryMapSize =3D (UINTN)NewMemoryMapEntry - (UINTN)MemoryMap; + + return ; +} + +/** + This function caches the UEFI memory map information. +**/ +VOID +GetUefiMemoryMap ( + VOID + ) +{ + EFI_STATUS Status; + UINTN MapKey; + UINT32 DescriptorVersion; + EFI_MEMORY_DESCRIPTOR *MemoryMap; + UINTN UefiMemoryMapSize; + + DEBUG ((DEBUG_INFO, "GetUefiMemoryMap\n")); + + UefiMemoryMapSize =3D 0; + MemoryMap =3D NULL; + Status =3D gBS->GetMemoryMap ( + &UefiMemoryMapSize, + MemoryMap, + &MapKey, + &mUefiDescriptorSize, + &DescriptorVersion + ); + ASSERT (Status =3D=3D EFI_BUFFER_TOO_SMALL); + + do { + Status =3D gBS->AllocatePool (EfiBootServicesData, UefiMemoryMapSize, = (VOID **)&MemoryMap); + ASSERT (MemoryMap !=3D NULL); + if (MemoryMap =3D=3D NULL) { + return ; + } + + Status =3D gBS->GetMemoryMap ( + &UefiMemoryMapSize, + MemoryMap, + &MapKey, + &mUefiDescriptorSize, + &DescriptorVersion + ); + if (EFI_ERROR (Status)) { + gBS->FreePool (MemoryMap); + MemoryMap =3D NULL; + } + } while (Status =3D=3D EFI_BUFFER_TOO_SMALL); + + SortMemoryMap (MemoryMap, UefiMemoryMapSize, mUefiDescriptorSize); =20 + MergeMemoryMapForNotPresentEntry (MemoryMap, &UefiMemoryMapSize,=20 + mUefiDescriptorSize); + + mUefiMemoryMapSize =3D UefiMemoryMapSize; mUefiMemoryMap =3D=20 + AllocateCopyPool (UefiMemoryMapSize, MemoryMap); ASSERT=20 + (mUefiMemoryMap !=3D NULL); + + gBS->FreePool (MemoryMap); +} + +/** + This function sets UEFI memory attribute according to UEFI memory map. + + The normal memory region is marked as not present, such as + EfiLoaderCode/Data, EfiBootServicesCode/Data, EfiConventionalMemory, + EfiUnusableMemory, EfiACPIReclaimMemory. +**/ +VOID +SetUefiMemMapAttributes ( + VOID + ) +{ + EFI_MEMORY_DESCRIPTOR *MemoryMap; + UINTN MemoryMapEntryCount; + UINTN Index; + + DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n")); + + if (mUefiMemoryMap =3D=3D NULL) { + DEBUG ((DEBUG_INFO, "UefiMemoryMap - NULL\n")); + return ; + } + + MemoryMapEntryCount =3D mUefiMemoryMapSize/mUefiDescriptorSize; + MemoryMap =3D mUefiMemoryMap; + for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { + if (IsUefiPageNotPresent(MemoryMap)) { + DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", Memory= Map->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((U= INTN)MemoryMap->NumberOfPages))); + SmmSetMemoryAttributes ( + MemoryMap->PhysicalStart, + EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), + EFI_MEMORY_RP + ); + } + MemoryMap =3D NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize); = =20 + } + + // + // Do free mUefiMemoryMap, it will be checked in IsSmmCommBufferForbidde= nAddress(). + // +} + +/** + Return if the Address is forbidden as SMM communication buffer. + + @param[in] Address the address to be checked + + @return TRUE The address is forbidden as SMM communication buffer. + @return FALSE The address is allowed as SMM communication buffer. +**/ +BOOLEAN +IsSmmCommBufferForbiddenAddress ( + IN UINT64 Address + ) +{ + EFI_MEMORY_DESCRIPTOR *MemoryMap; + UINTN MemoryMapEntryCount; + UINTN Index; + + MemoryMap =3D mUefiMemoryMap; + MemoryMapEntryCount =3D mUefiMemoryMapSize/mUefiDescriptorSize; + for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { + if (IsUefiPageNotPresent (MemoryMap)) { + if ((Address >=3D MemoryMap->PhysicalStart) && + (Address < MemoryMap->PhysicalStart + EFI_PAGES_TO_SIZE((UINTN)M= emoryMap->NumberOfPages)) ) { + return TRUE; + } + } + MemoryMap =3D NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize); + } + return FALSE; +} diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuD= xeSmm/X64/PageTbl.c index 7237e57..17b2f4c 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -867,6 +867,13 @@ SmiPFHandler ( ); CpuDeadLoop (); } + if (IsSmmCommBufferForbiddenAddress (PFAddress)) { + DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x= %lx)!\n", PFAddress)); + DEBUG_CODE ( + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip); + ); + CpuDeadLoop (); + } } =20 if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { -- 2.7.4.windows.1