Thanks for the feedback. I'll change them in v4.
> -----Original Message----- > From: Ni, Ruiyu > Sent: Thursday, October 26, 2017 3:18 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; > Laszlo Ersek <ler...@redhat.com> > Subject: RE: [edk2] [PATCH v3 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Disable > page table protection > > 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
