Jian, 1. For protocol not defined in Spec, please use EDKII prefix, instead of EFI 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 <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; > Laszlo Ersek <ler...@redhat.com>; Ni, Ruiyu <ruiyu...@intel.com> > Subject: RE: [edk2] [PATCH v3 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Disable > page table protection > > Laszlo and Ruiyu, > > Could you please take a look at this part? There's a new protocol introduced. > > Thanks, > Jian > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-boun...@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 <jiewen....@intel.com>; Dong, Eric > > <eric.d...@intel.com> > > 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 <eric.d...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Suggested-by: Ayellet Wolman <ayellet.wol...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > > --- > > 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)) != 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 = { > > SmmWriteSaveState > > }; > > > > +/// > > +/// SMM Memory Attribute Protocol instance /// > > +EFI_SMM_MEMORY_ATTRIBUTE_PROTOCOL mSmmMemoryAttribute = > { > > + 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 = 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 <Protocol/SmmAccess2.h> > > #include <Protocol/SmmReadyToLock.h> > > #include <Protocol/SmmCpuService.h> > > +#include <Protocol/SmmMemoryAttribute.h> > > > > #include <Guid/AcpiS3Context.h> > > #include <Guid/PiSmmMemoryAttributesTable.h> > > @@ -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 address > > 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 > > combination of > > + attributes that cannot be set together. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range > > specified > > + by BaseAddress and Length. > > + The bit mask of attributes is not support > > 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 address > > 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 > > combination of > > + attributes that cannot be set together. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range > > specified > > + by BaseAddress and Length. > > + The bit mask of attributes is not support > > 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 address > > 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 region. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes is NULL. > > + @retval EFI_NO_MAPPING Attributes are not consistent cross the > > memory > > + region. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range > > specified > > + by BaseAddress and Length. > > + The bit mask of attributes is not support > > 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 ## HOB # > > 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 address > > 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 > > combination of > > + attributes that cannot be set together. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range > > specified > > + by BaseAddress and Length. > > + The bit mask of attributes is not support > > 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 address > > 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 > > combination of > > + attributes that cannot be set together. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range > > specified > > + by BaseAddress and Length. > > + The bit mask of attributes is not support > > 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 address > > 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 region. > > + @retval EFI_INVALID_PARAMETER Length is zero. > > + Attributes is NULL. > > + @retval EFI_NO_MAPPING Attributes are not consistent cross the > > memory > > + region. > > + @retval EFI_UNSUPPORTED The processor does not support one or > more > > + bytes of the memory resource range > > specified > > + by BaseAddress and Length. > > + The bit mask of attributes is not support > > 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 == NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Size = (INT64)Length; > > + MemAttr = (UINT64)-1; > > + > > + do { > > + > > + PageEntry = GetPageTableEntry (BaseAddress, &PageAttr); > > + if (PageEntry == NULL || PageAttr == PageNone) { > > + return EFI_UNSUPPORTED; > > + } > > + > > + // > > + // If the memory range is cross page table boundary, make sure they > > + // share the same attribute. Return EFI_NO_MAPPING if not. > > + // > > + *Attributes = GetAttributesFromPageEntry (PageEntry); > > + if (MemAttr != (UINT64)-1 && *Attributes != MemAttr) { > > + return EFI_NO_MAPPING; > > + } > > + > > + switch (PageAttr) { > > + case Page4K: > > + Address = *PageEntry & ~mAddressEncMask & > > PAGING_4K_ADDRESS_MASK_64; > > + Size -= (SIZE_4KB - (BaseAddress - Address)); > > + BaseAddress += (SIZE_4KB - (BaseAddress - Address)); > > + break; > > + > > + case Page2M: > > + Address = *PageEntry & ~mAddressEncMask & > > PAGING_2M_ADDRESS_MASK_64; > > + Size -= SIZE_2MB - (BaseAddress - Address); > > + BaseAddress += SIZE_2MB - (BaseAddress - Address); > > + break; > > + > > + case Page1G: > > + Address = *PageEntry & ~mAddressEncMask & > > PAGING_1G_ADDRESS_MASK_64; > > + Size -= SIZE_1GB - (BaseAddress - Address); > > + BaseAddress += SIZE_1GB - (BaseAddress - Address); > > + break; > > + > > + default: > > + return EFI_UNSUPPORTED; > > + } > > + > > + MemAttr = *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)) != 0) { > > return ; > > } > > > > -- > > 2.14.1.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
