From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 0A5362034CF8B for ; Thu, 26 Oct 2017 00:13:49 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Oct 2017 00:17:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,434,1503385200"; d="scan'208";a="167278012" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga005.fm.intel.com with ESMTP; 26 Oct 2017 00:17:35 -0700 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 26 Oct 2017 00:17:34 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 26 Oct 2017 00:17:34 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by shsmsx102.ccr.corp.intel.com ([169.254.2.175]) with mapi id 14.03.0319.002; Thu, 26 Oct 2017 15:17:32 +0800 From: "Ni, Ruiyu" To: "Wang, Jian J" , "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Dong, Eric" , Laszlo Ersek Thread-Topic: [edk2] [PATCH v3 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Disable page table protection Thread-Index: AQHTS5kQfbpThsyK6UqRAwpWg3FktqL1J6kAgACV1GA= Date: Thu, 26 Oct 2017 07:17:31 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BAA7891@SHSMSX104.ccr.corp.intel.com> References: <20171023005054.7528-1-jian.j.wang@intel.com> <20171023005054.7528-6-jian.j.wang@intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v3 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Disable page table protection 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: Thu, 26 Oct 2017 07:13:50 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Jian, 1. For protocol not defined in Spec, please use EDKII prefix, instead of EF= I prefix. 2. Could you please add more comments for BIT3 and BIT2 check? 3. Better to separate the protocol definition to a single commit. Thanks/Ray > -----Original Message----- > From: Wang, Jian J > Sent: Thursday, October 26, 2017 2:20 PM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen ; Dong, Eric ; > Laszlo Ersek ; Ni, Ruiyu > Subject: RE: [edk2] [PATCH v3 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Disable > page table protection >=20 > Laszlo and Ruiyu, >=20 > Could you please take a look at this part? There's a new protocol introdu= ced. >=20 > Thanks, > Jian >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Jian J Wang > > Sent: Monday, October 23, 2017 8:51 AM > > To: edk2-devel@lists.01.org > > Cc: Yao, Jiewen ; Dong, Eric > > > > Subject: [edk2] [PATCH v3 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Disable > page > > table protection > > > > > v3 > > > According to Jiewen's feedback, implement new protocol > > > gEdkiiSmmMemoryAttributeProtocolGuid > > > to change memory attributes. > > > > > v2 > > > According to Eric's feedback: > > > a. Enclose bit-or with parentheses > > > b. Add code in 32-bit code to bypass setting page table to read-only > > > > Heap guard feature will update page attributes frequently. The page > > table should not set to be read-only if heap guard feature is enabled > > for SMM mode. Otherwise this feature cannot work. > > > > Cc: Eric Dong > > Cc: Jiewen Yao > > Suggested-by: Ayellet Wolman > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang > > --- > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 7 + > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 20 +++ > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 98 > > +++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 2 + > > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 163 > > +++++++++++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 3 +- > > 6 files changed, 292 insertions(+), 1 deletion(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > index f295c2ebf2..27c11f1b8d 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > @@ -184,6 +184,13 @@ SetPageTableAttributes ( > > BOOLEAN IsSplitted; > > BOOLEAN PageTableSplitted; > > > > + // > > + // Don't mark page table as read-only if heap guard is enabled. > > + // > > + if ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) !=3D 0) { > > + return ; > > + } > > + > > DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n")); > > > > // > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > > index 282d2e6981..8635f2d2c8 100755 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > > @@ -76,6 +76,15 @@ EFI_SMM_CPU_PROTOCOL mSmmCpu =3D { > > SmmWriteSaveState > > }; > > > > +/// > > +/// SMM Memory Attribute Protocol instance /// > > +EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL mSmmMemoryAttribute =3D > { > > + EdkiiSmmGetMemoryAttributes, > > + EdkiiSmmSetMemoryAttributes, > > + EdkiiSmmClearMemoryAttributes > > +}; > > + > > EFI_CPU_INTERRUPT_HANDLER > > mExternalVectorTable[EXCEPTION_VECTOR_NUMBER]; > > > > // > > @@ -893,6 +902,17 @@ PiCpuSmmEntry ( > > ); > > ASSERT_EFI_ERROR (Status); > > > > + // > > + // Install the SMM Memory Attribute Protocol into SMM protocol > > + database // Status =3D gSmst->SmmInstallProtocolInterface ( > > + &mSmmCpuHandle, > > + &gEdkiiSmmMemoryAttributeProtocolGuid, > > + EFI_NATIVE_INTERFACE, > > + &mSmmMemoryAttribute > > + ); > > + ASSERT_EFI_ERROR (Status); > > + > > // > > // Expose address of CPU Hot Plug Data structure if CPU hot plug is > supported. > > // > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > index 1cf85c1481..e1c231e10b 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > @@ -25,6 +25,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF > ANY KIND, > > EITHER EXPRESS OR IMPLIED. > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -1068,4 +1069,101 @@ TransferApToSafeState ( > > IN UINTN NumberToFinishAddress > > ); > > > > +/** > > + This function set given attributes of the memory region specified > > +by > > + BaseAddress and Length. > > + > > + @param This The EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL > > instance. > > + @param BaseAddress The physical address that is the start add= ress of > > + a memory region. > > + @param Length The size in bytes of the memory region. > > + @param Attributes The bit mask of attributes to set for the = memory > > + region. > > + > > + @retval EFI_SUCCESS The attributes were set for the memory > region. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes specified an illegal combin= ation of > > + attributes that cannot be set together= . > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range spe= cified > > + by BaseAddress and Length. > > + The bit mask of attributes is not supp= ort for > > + the memory resource range specified by > > + BaseAddress and Length. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +EdkiiSmmSetMemoryAttributes ( > > + IN EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This, > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > + IN UINT64 Length, > > + IN UINT64 Attributes > > + ); > > + > > +/** > > + This function clears given attributes of the memory region > > +specified by > > + BaseAddress and Length. > > + > > + @param This The EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL > > instance. > > + @param BaseAddress The physical address that is the start add= ress of > > + a memory region. > > + @param Length The size in bytes of the memory region. > > + @param Attributes The bit mask of attributes to set for the = memory > > + region. > > + > > + @retval EFI_SUCCESS The attributes were set for the memory > region. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes specified an illegal combin= ation of > > + attributes that cannot be set together= . > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range spe= cified > > + by BaseAddress and Length. > > + The bit mask of attributes is not supp= ort for > > + the memory resource range specified by > > + BaseAddress and Length. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +EdkiiSmmClearMemoryAttributes ( > > + IN EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This, > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > + IN UINT64 Length, > > + IN UINT64 Attributes > > + ); > > + > > +/** > > + This function retrieve the attributes of the memory region > > +specified by > > + BaseAddress and Length. If different attributes are got from > > +different part > > + of the memory region, EFI_NO_MAPPING will be returned. > > + > > + @param This The EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL > > instance. > > + @param BaseAddress The physical address that is the start add= ress of > > + a memory region. > > + @param Length The size in bytes of the memory region. > > + @param Attributes Pointer to attributes returned. > > + > > + @retval EFI_SUCCESS The attributes got for the memory regi= on. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes is NULL. > > + @retval EFI_NO_MAPPING Attributes are not consistent cross th= e > > memory > > + region. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range spe= cified > > + by BaseAddress and Length. > > + The bit mask of attributes is not supp= ort for > > + the memory resource range specified by > > + BaseAddress and Length. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +EdkiiSmmGetMemoryAttributes ( > > + IN EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This, > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > + IN UINT64 Length, > > + IN UINT64 *Attributes > > + ); > > + > > #endif > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > index 099792e6ce..43d9edd0d5 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > @@ -129,6 +129,7 @@ > > gEfiSmmCpuProtocolGuid ## PRODUCES > > gEfiSmmReadyToLockProtocolGuid ## NOTIFY > > gEfiSmmCpuServiceProtocolGuid ## PRODUCES > > + gEdkiiSmmMemoryAttributeProtocolGuid ## PRODUCES > > > > [Guids] > > gEfiAcpiVariableGuid ## SOMETIMES_CONSUMES ## HO= B # it is > > used for S3 boot. > > @@ -159,6 +160,7 @@ > > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable ## > > CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## > CONSUMES > > > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM > ask > > ## CONSUMES > > + gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask > ## > > CONSUMES > > > > [Depex] > > gEfiMpServiceProtocolGuid > > diff --git > a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > > index 3ad5256f1e..eb5e639e4b 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > > @@ -1120,3 +1120,166 @@ IsSmmCommBufferForbiddenAddress ( > > } > > return FALSE; > > } > > + > > +/** > > + This function set given attributes of the memory region specified > > +by > > + BaseAddress and Length. > > + > > + @param This The EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL > > instance. > > + @param BaseAddress The physical address that is the start add= ress of > > + a memory region. > > + @param Length The size in bytes of the memory region. > > + @param Attributes The bit mask of attributes to set for the = memory > > + region. > > + > > + @retval EFI_SUCCESS The attributes were set for the memory > region. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes specified an illegal combin= ation of > > + attributes that cannot be set together= . > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range spe= cified > > + by BaseAddress and Length. > > + The bit mask of attributes is not supp= ort for > > + the memory resource range specified by > > + BaseAddress and Length. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +EdkiiSmmSetMemoryAttributes ( > > + IN EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This, > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > + IN UINT64 Length, > > + IN UINT64 Attributes > > + ) > > +{ > > + return SmmSetMemoryAttributes (BaseAddress, Length, Attributes); } > > + > > +/** > > + This function clears given attributes of the memory region > > +specified by > > + BaseAddress and Length. > > + > > + @param This The EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL > > instance. > > + @param BaseAddress The physical address that is the start add= ress of > > + a memory region. > > + @param Length The size in bytes of the memory region. > > + @param Attributes The bit mask of attributes to set for the = memory > > + region. > > + > > + @retval EFI_SUCCESS The attributes were set for the memory > region. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes specified an illegal combin= ation of > > + attributes that cannot be set together= . > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range spe= cified > > + by BaseAddress and Length. > > + The bit mask of attributes is not supp= ort for > > + the memory resource range specified by > > + BaseAddress and Length. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +EdkiiSmmClearMemoryAttributes ( > > + IN EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This, > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > + IN UINT64 Length, > > + IN UINT64 Attributes > > + ) > > +{ > > + return SmmClearMemoryAttributes (BaseAddress, Length, Attributes); > > +} > > + > > +/** > > + This function retrieve the attributes of the memory region > > +specified by > > + BaseAddress and Length. If different attributes are got from > > +different part > > + of the memory region, EFI_NO_MAPPING will be returned. > > + > > + @param This The EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL > > instance. > > + @param BaseAddress The physical address that is the start add= ress of > > + a memory region. > > + @param Length The size in bytes of the memory region. > > + @param Attributes Pointer to attributes returned. > > + > > + @retval EFI_SUCCESS The attributes got for the memory regi= on. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes is NULL. > > + @retval EFI_NO_MAPPING Attributes are not consistent cross th= e > > memory > > + region. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range spe= cified > > + by BaseAddress and Length. > > + The bit mask of attributes is not supp= ort for > > + the memory resource range specified by > > + BaseAddress and Length. > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +EdkiiSmmGetMemoryAttributes ( > > + IN EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This, > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > + IN UINT64 Length, > > + IN UINT64 *Attributes > > + ) > > +{ > > + EFI_PHYSICAL_ADDRESS Address; > > + UINT64 *PageEntry; > > + UINT64 MemAttr; > > + PAGE_ATTRIBUTE PageAttr; > > + INT64 Size; > > + > > + if (Length < SIZE_4KB || Attributes =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Size =3D (INT64)Length; > > + MemAttr =3D (UINT64)-1; > > + > > + do { > > + > > + PageEntry =3D GetPageTableEntry (BaseAddress, &PageAttr); > > + if (PageEntry =3D=3D NULL || PageAttr =3D=3D PageNone) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + // > > + // If the memory range is cross page table boundary, make sure the= y > > + // share the same attribute. Return EFI_NO_MAPPING if not. > > + // > > + *Attributes =3D GetAttributesFromPageEntry (PageEntry); > > + if (MemAttr !=3D (UINT64)-1 && *Attributes !=3D MemAttr) { > > + return EFI_NO_MAPPING; > > + } > > + > > + switch (PageAttr) { > > + case Page4K: > > + Address =3D *PageEntry & ~mAddressEncMask & > > PAGING_4K_ADDRESS_MASK_64; > > + Size -=3D (SIZE_4KB - (BaseAddress - Address)); > > + BaseAddress +=3D (SIZE_4KB - (BaseAddress - Address)); > > + break; > > + > > + case Page2M: > > + Address =3D *PageEntry & ~mAddressEncMask & > > PAGING_2M_ADDRESS_MASK_64; > > + Size -=3D SIZE_2MB - (BaseAddress - Address); > > + BaseAddress +=3D SIZE_2MB - (BaseAddress - Address); > > + break; > > + > > + case Page1G: > > + Address =3D *PageEntry & ~mAddressEncMask & > > PAGING_1G_ADDRESS_MASK_64; > > + Size -=3D SIZE_1GB - (BaseAddress - Address); > > + BaseAddress +=3D SIZE_1GB - (BaseAddress - Address); > > + break; > > + > > + default: > > + return EFI_UNSUPPORTED; > > + } > > + > > + MemAttr =3D *Attributes; > > + > > + } while (Size > 0); > > + > > + return EFI_SUCCESS; > > +} > > + > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > index 3dde80f9ba..4d4668a0c6 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > @@ -902,7 +902,8 @@ SetPageTableAttributes ( > > BOOLEAN IsSplitted; > > BOOLEAN PageTableSplitted; > > > > - if (!mCpuSmmStaticPageTable) { > > + if (!mCpuSmmStaticPageTable > > + || (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) !=3D 0) = { > > return ; > > } > > > > -- > > 2.14.1.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel