From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web08.27105.1658150328221476289 for ; Mon, 18 Jul 2022 06:18:50 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=UDt4zQaC; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: ray.ni@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658150329; x=1689686329; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=FYN/GLF6M9a/18XZXOgavuuJ3PjYg/TTJMMxjQyA39k=; b=UDt4zQaCjhr38U4AJeTuTpGK3Tzk1ASQo3Ymq1GrRmSesSRr2YqMVd9k hthS1OGi1XZ+yvGlls0GBoR78IhPlrTBwr/xN4/Po0Bc3e2/tMcWnNrGW IW+OZ5iQbXSYpbW0H+m6kLK1dqyjB05VVnSu/hPM9AIlgvI1JHMZ//iVi qVFIML1nOSFeC1Q2+rTyndPBOi/KK5k/eMKpcdiDUPJytImF8wWtJKjoN S0Azggc0nsWLB0des6m1GoX+d5sOzIPecFPBXQVZ0Sw401UzhsdIVYA7/ NBpUPW79J6KO9fdPXpz47WdOQXEX12ZUEoXuHR95pS1fm/WAlRoIl0wIL Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10411"; a="287363943" X-IronPort-AV: E=Sophos;i="5.92,281,1650956400"; d="scan'208";a="287363943" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 06:18:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.92,281,1650956400"; d="scan'208";a="624724995" Received: from shwdeopenlab706.ccr.corp.intel.com ([10.239.183.102]) by orsmga008.jf.intel.com with ESMTP; 18 Jul 2022 06:18:47 -0700 From: "Ni, Ray" To: devel@edk2.groups.io Cc: Zhiguang Liu , Eric Dong Subject: [PATCH 07/10] CpuPageTableLib: Fix parent attributes are not inherited properly Date: Mon, 18 Jul 2022 21:18:28 +0800 Message-Id: <20220718131831.660-8-ray.ni@intel.com> X-Mailer: git-send-email 2.35.1.windows.2 In-Reply-To: <20220718131831.660-1-ray.ni@intel.com> References: <20220718131831.660-1-ray.ni@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable With the following paging structure that maps [0, 2G] with ReadWrite bit set. PML4[0] --> PDPTE[0] --> PDE[0-255] \-> PDPTE[1] --> PDE[0-255] If ReadWrite bit is cleared in PML4[0] and PageTableMap() is called to change [0, 2M] as writable, today's logic doesn't inherit the parent entry's attributes when determining the child entry's attributes. It just sets the PDPTE[0].PDE[0].ReadWrite bit. But since the PML4[0].ReadWrite is 0, [0, 2M] is still read-only. The change fixes the bug. If the inheritable attributes in ParentPagingEntry conflicts with the requested attributes, let the child entries take the parent attributes and loosen the attribute in the parent entry. E.g.: when PDPTE[0].ReadWrite =3D 0 but caller wants to map [0-2MB as ReadWrite =3D 1 (PDE[0].ReadWrite =3D 1), we need to change PDPTE[0].ReadWrite =3D 1 and let all PDE[0-255].ReadWrite =3D 0 first. Then change PDE[0].ReadWrite =3D 1. Signed-off-by: Zhiguang Liu Signed-off-by: Ray Ni Cc: Eric Dong --- .../Library/CpuPageTableLib/CpuPageTable.h | 20 ++- .../Library/CpuPageTableLib/CpuPageTableMap.c | 144 ++++++++++++++++-- 2 files changed, 147 insertions(+), 17 deletions(-) diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h b/UefiCpuPkg= /Library/CpuPageTableLib/CpuPageTable.h index c041ea3f56..627f84e4e2 100644 --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h @@ -21,7 +21,11 @@ #define REGION_LENGTH(l) LShiftU64 (1, (l) * 9 + 3)=0D =0D typedef struct {=0D - UINT64 Present : 1; // 0 =3D Not present in memory, 1 = =3D Present in memory=0D + UINT64 Present : 1; // 0 =3D Not present in memory, 1 = =3D Present in memory=0D + UINT64 ReadWrite : 1; // 0 =3D Read-Only, 1=3D Read/Write= =0D + UINT64 UserSupervisor : 1; // 0 =3D Supervisor, 1=3DUser=0D + UINT64 Reserved : 58;=0D + UINT64 Nx : 1; // No Execute bit=0D } IA32_PAGE_COMMON_ENTRY;=0D =0D ///=0D @@ -201,4 +205,18 @@ PageTableLibGetPleBMapAttribute ( IN IA32_MAP_ATTRIBUTE *ParentMapAttribute=0D );=0D =0D +/**=0D + Return the attribute of a non-leaf page table entry.=0D +=0D + @param[in] Pnle Pointer to a non-leaf page table entry.=0D + @param[in] ParentMapAttribute Pointer to the parent attribute.=0D +=0D + @return Attribute of the non-leaf page table entry.=0D +**/=0D +UINT64=0D +PageTableLibGetPnleMapAttribute (=0D + IN IA32_PAGE_NON_LEAF_ENTRY *Pnle,=0D + IN IA32_MAP_ATTRIBUTE *ParentMapAttribute=0D + );=0D +=0D #endif=0D diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpu= Pkg/Library/CpuPageTableLib/CpuPageTableMap.c index a6aa1a352b..ac5d1c79f4 100644 --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c @@ -183,18 +183,32 @@ PageTableLibSetPle ( =0D @param[in] Pnle Pointer to IA32_PML5, IA32_PML4, IA32_PDPTE or IA32= _PDE. All share the same structure definition.=0D @param[in] Attribute The attribute of the page directory referenced by t= he non-leaf.=0D + @param[in] Mask The mask of the page directory referenced by the no= n-leaf.=0D **/=0D VOID=0D PageTableLibSetPnle (=0D IN IA32_PAGE_NON_LEAF_ENTRY *Pnle,=0D - IN IA32_MAP_ATTRIBUTE *Attribute=0D + IN IA32_MAP_ATTRIBUTE *Attribute,=0D + IN IA32_MAP_ATTRIBUTE *Mask=0D )=0D {=0D - Pnle->Bits.Present =3D Attribute->Bits.Present;=0D - Pnle->Bits.ReadWrite =3D Attribute->Bits.ReadWrite;=0D - Pnle->Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor;=0D - Pnle->Bits.Nx =3D Attribute->Bits.Nx;=0D - Pnle->Bits.Accessed =3D 0;=0D + if (Mask->Bits.Present) {=0D + Pnle->Bits.Present =3D Attribute->Bits.Present;=0D + }=0D +=0D + if (Mask->Bits.ReadWrite) {=0D + Pnle->Bits.ReadWrite =3D Attribute->Bits.ReadWrite;=0D + }=0D +=0D + if (Mask->Bits.UserSupervisor) {=0D + Pnle->Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor;=0D + }=0D +=0D + if (Mask->Bits.Nx) {=0D + Pnle->Bits.Nx =3D Attribute->Bits.Nx;=0D + }=0D +=0D + Pnle->Bits.Accessed =3D 0;=0D =0D //=0D // Set the attributes (WT, CD, A) to 0.=0D @@ -210,6 +224,7 @@ PageTableLibSetPnle ( Update page table to map [LinearAddress, LinearAddress + Length) with sp= ecified attribute in the specified level.=0D =0D @param[in] ParentPagingEntry The pointer to the page table entry to= update.=0D + @param[in] ParentAttribute The accumulated attribute of all paren= ts' attribute.=0D @param[in] Modify FALSE to indicate Buffer is not used a= nd BufferSize is increased by the required buffer size.=0D @param[in] Buffer The free buffer to be used for page ta= ble creation/updating.=0D When Modify is TRUE, it's used from th= e end.=0D @@ -232,6 +247,7 @@ PageTableLibSetPnle ( RETURN_STATUS=0D PageTableLibMapInLevel (=0D IN IA32_PAGING_ENTRY *ParentPagingEntry,=0D + IN IA32_MAP_ATTRIBUTE *ParentAttribute,=0D IN BOOLEAN Modify,=0D IN VOID *Buffer,=0D IN OUT INTN *BufferSize,=0D @@ -259,6 +275,8 @@ PageTableLibMapInLevel ( IA32_MAP_ATTRIBUTE NopAttribute;=0D BOOLEAN CreateNew;=0D IA32_PAGING_ENTRY OneOfPagingEntry;=0D + IA32_MAP_ATTRIBUTE ChildAttribute;=0D + IA32_MAP_ATTRIBUTE ChildMask;=0D =0D ASSERT (Level !=3D 0);=0D ASSERT ((Attribute !=3D NULL) && (Mask !=3D NULL));=0D @@ -291,7 +309,7 @@ PageTableLibMapInLevel ( //=0D // Set default attribute bits for PML5E/PML4E/PDPTE/PDE.=0D //=0D - PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute);=0D + PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOn= eMask);=0D } else {=0D //=0D // Just make sure Present and MustBeZero (PageSize) bits are accurat= e.=0D @@ -307,7 +325,7 @@ PageTableLibMapInLevel ( // Use NOP attributes as the attribute of grand-parents because CPU wi= ll consider=0D // the actual attributes of grand-parents when determing the memory ty= pe.=0D //=0D - PleBAttribute.Uint64 =3D PageTableLibGetPleBMapAttribute (&ParentPagin= gEntry->PleB, &NopAttribute);=0D + PleBAttribute.Uint64 =3D PageTableLibGetPleBMapAttribute (&ParentPagin= gEntry->PleB, ParentAttribute);=0D if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBU= TE_ATTRIBUTES (Mask))=0D =3D=3D (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRI= BUTE_ATTRIBUTES (Mask)))=0D {=0D @@ -334,7 +352,7 @@ PageTableLibMapInLevel ( // Note: Should NOT inherit the attributes from the original entry b= ecause a zero RW bit=0D // will make the entire region read-only even the child entrie= s set the RW bit.=0D //=0D - PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute);=0D + PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOn= eMask);=0D =0D RegionLength =3D REGION_LENGTH (Level);=0D PagingEntry =3D (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BA= SE_ADDRESS (&ParentPagingEntry->Pnle);=0D @@ -343,6 +361,77 @@ PageTableLibMapInLevel ( SubOffset +=3D RegionLength;=0D }=0D }=0D + } else {=0D + //=0D + // It's a non-leaf entry=0D + //=0D + ChildAttribute.Uint64 =3D 0;=0D + ChildMask.Uint64 =3D 0;=0D +=0D + //=0D + // If the inheritable attributes in the parent entry conflicts with th= e requested attributes,=0D + // let the child entries take the parent attributes and=0D + // loosen the attribute in the parent entry=0D + // E.g.: when PDPTE[0].ReadWrite =3D 0 but caller wants to map [0-2MB]= as ReadWrite =3D 1 (PDE[0].ReadWrite =3D 1)=0D + // we need to change PDPTE[0].ReadWrite =3D 1 and let all P= DE[0-255].ReadWrite =3D 0 in this step.=0D + // when PDPTE[0].Nx =3D 1 but caller wants to map [0-2MB] as Nx = =3D 0 (PDT[0].Nx =3D 0)=0D + // we need to change PDPTE[0].Nx =3D 0 and let all PDE[0-25= 5].Nx =3D 1 in this step.=0D + if ((ParentPagingEntry->Pnle.Bits.Present =3D=3D 0) && (Mask->Bits.Pre= sent =3D=3D 1) && (Attribute->Bits.Present =3D=3D 1)) {=0D + if (Modify) {=0D + ParentPagingEntry->Pnle.Bits.Present =3D 1;=0D + }=0D +=0D + ChildAttribute.Bits.Present =3D 0;=0D + ChildMask.Bits.Present =3D 1;=0D + }=0D +=0D + if ((ParentPagingEntry->Pnle.Bits.ReadWrite =3D=3D 0) && (Mask->Bits.R= eadWrite =3D=3D 1) && (Attribute->Bits.ReadWrite =3D=3D 1)) {=0D + if (Modify) {=0D + ParentPagingEntry->Pnle.Bits.ReadWrite =3D 1;=0D + }=0D +=0D + ChildAttribute.Bits.ReadWrite =3D 0;=0D + ChildMask.Bits.ReadWrite =3D 1;=0D + }=0D +=0D + if ((ParentPagingEntry->Pnle.Bits.UserSupervisor =3D=3D 0) && (Mask->B= its.UserSupervisor =3D=3D 1) && (Attribute->Bits.UserSupervisor =3D=3D 1)) = {=0D + if (Modify) {=0D + ParentPagingEntry->Pnle.Bits.UserSupervisor =3D 1;=0D + }=0D +=0D + ChildAttribute.Bits.UserSupervisor =3D 0;=0D + ChildMask.Bits.UserSupervisor =3D 1;=0D + }=0D +=0D + if ((ParentPagingEntry->Pnle.Bits.Nx =3D=3D 1) && (Mask->Bits.Nx =3D= =3D 1) && (Attribute->Bits.Nx =3D=3D 0)) {=0D + if (Modify) {=0D + ParentPagingEntry->Pnle.Bits.Nx =3D 0;=0D + }=0D +=0D + ChildAttribute.Bits.Nx =3D 1;=0D + ChildMask.Bits.Nx =3D 1;=0D + }=0D +=0D + if (ChildMask.Uint64 !=3D 0) {=0D + if (Modify) {=0D + //=0D + // Update child entries to use restrictive attribute inherited fro= m parent.=0D + // e.g.: Set PDE[0-255].ReadWrite =3D 0=0D + //=0D + PagingEntry =3D (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_B= ASE_ADDRESS (&ParentPagingEntry->Pnle);=0D + for (Index =3D 0; Index < 512; Index++) {=0D + if (PagingEntry[Index].Pce.Present =3D=3D 0) {=0D + continue;=0D + }=0D +=0D + if (IsPle (&PagingEntry[Index], Level)) {=0D + PageTableLibSetPle (Level - 1, &PagingEntry[Index], 0, &ChildA= ttribute, &ChildMask);=0D + } else {=0D + PageTableLibSetPnle (&PagingEntry[Index].Pnle, &ChildAttribute= , &ChildMask);=0D + }=0D + }=0D + }=0D + }=0D }=0D =0D //=0D @@ -355,6 +444,8 @@ PageTableLibMapInLevel ( RegionMask =3D RegionLength - 1;=0D RegionStart =3D (LinearAddress + Offset) & ~RegionMask;=0D =0D + ParentAttribute->Uint64 =3D PageTableLibGetPnleMapAttribute (&ParentPagi= ngEntry->Pnle, ParentAttribute);=0D +=0D //=0D // Apply the attribute.=0D //=0D @@ -386,6 +477,7 @@ PageTableLibMapInLevel ( //=0D Status =3D PageTableLibMapInLevel (=0D CurrentPagingEntry,=0D + ParentAttribute,=0D Modify,=0D Buffer,=0D BufferSize,=0D @@ -449,12 +541,13 @@ PageTableMap ( IN IA32_MAP_ATTRIBUTE *Mask=0D )=0D {=0D - RETURN_STATUS Status;=0D - IA32_PAGING_ENTRY TopPagingEntry;=0D - INTN RequiredSize;=0D - UINT64 MaxLinearAddress;=0D - UINTN MaxLevel;=0D - UINTN MaxLeafLevel;=0D + RETURN_STATUS Status;=0D + IA32_PAGING_ENTRY TopPagingEntry;=0D + INTN RequiredSize;=0D + UINT64 MaxLinearAddress;=0D + UINTN MaxLevel;=0D + UINTN MaxLeafLevel;=0D + IA32_MAP_ATTRIBUTE ParentAttribute;=0D =0D if ((PagingMode =3D=3D Paging32bit) || (PagingMode =3D=3D PagingPae) || = (PagingMode >=3D PagingModeMax)) {=0D //=0D @@ -499,15 +592,26 @@ PageTableMap ( =0D TopPagingEntry.Uintn =3D *PageTable;=0D if (TopPagingEntry.Uintn !=3D 0) {=0D - TopPagingEntry.Pce.Present =3D 1;=0D + TopPagingEntry.Pce.Present =3D 1;=0D + TopPagingEntry.Pce.ReadWrite =3D 1;=0D + TopPagingEntry.Pce.UserSupervisor =3D 1;=0D + TopPagingEntry.Pce.Nx =3D 0;=0D }=0D =0D + ParentAttribute.Uint64 =3D 0;=0D + ParentAttribute.Bits.PageTableBaseAddress =3D 1;=0D + ParentAttribute.Bits.Present =3D 1;=0D + ParentAttribute.Bits.ReadWrite =3D 1;=0D + ParentAttribute.Bits.UserSupervisor =3D 1;=0D + ParentAttribute.Bits.Nx =3D 0;=0D +=0D //=0D // Query the required buffer size without modifying the page table.=0D //=0D RequiredSize =3D 0;=0D Status =3D PageTableLibMapInLevel (=0D &TopPagingEntry,=0D + &ParentAttribute,=0D FALSE,=0D NULL,=0D &RequiredSize,=0D @@ -537,8 +641,16 @@ PageTableMap ( //=0D // Update the page table when the supplied buffer is sufficient.=0D //=0D + ParentAttribute.Uint64 =3D 0;=0D + ParentAttribute.Bits.PageTableBaseAddress =3D 1;=0D + ParentAttribute.Bits.Present =3D 1;=0D + ParentAttribute.Bits.ReadWrite =3D 1;=0D + ParentAttribute.Bits.UserSupervisor =3D 1;=0D + ParentAttribute.Bits.Nx =3D 0;=0D +=0D Status =3D PageTableLibMapInLevel (=0D &TopPagingEntry,=0D + &ParentAttribute,=0D TRUE,=0D Buffer,=0D BufferSize,=0D --=20 2.35.1.windows.2