Add two functions to disable/enable CR0.WP. These two unctions
will also be used in later commits. This commit doesn't change any
functionality.
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24 ++++++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
2 files changed, 90 insertions(+), 49 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index ba341cadc6..e0c4ca76dc 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
VOID
);
+/**
+ Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
+
+ @param[out] WpEnabled If Cr0.WP is enabled.
+ @param[out] CetEnabled If CET is enabled.
+**/
+VOID
+DisableReadOnlyPageWriteProtect (
+ OUT BOOLEAN *WpEnabled,
+ OUT BOOLEAN *CetEnabled
+ );
+
+/**
+ Enable Write Protect on pages marked as read-only.
+
+ @param[out] WpEnabled If Cr0.WP should be enabled.
+ @param[out] CetEnabled If CET should be enabled.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+ BOOLEAN WpEnabled,
+ BOOLEAN CetEnabled
+ );
+
#endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 2faee8f859..4b512edf68 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
//
BOOLEAN mIsReadOnlyPageTable = FALSE;
+/**
+ Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
+
+ @param[out] WpEnabled If Cr0.WP is enabled.
+ @param[out] CetEnabled If CET is enabled.
+**/
+VOID
+DisableReadOnlyPageWriteProtect (
+ OUT BOOLEAN *WpEnabled,
+ OUT BOOLEAN *CetEnabled
+ )
+{
+ IA32_CR0 Cr0;
+
+ *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ Cr0.UintN = AsmReadCr0 ();
+ *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
+ if (*WpEnabled) {
+ if (*CetEnabled) {
+ //
+ // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
+ //
+ DisableCet ();
+ }
+
+ Cr0.Bits.WP = 0;
+ AsmWriteCr0 (Cr0.UintN);
+ }
+}
+
+/**
+ Enable Write Protect on pages marked as read-only.
+
+ @param[out] WpEnabled If Cr0.WP should be enabled.
+ @param[out] CetEnabled If CET should be enabled.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+ BOOLEAN WpEnabled,
+ BOOLEAN CetEnabled
+ )
+{
+ IA32_CR0 Cr0;
+
+ if (WpEnabled) {
+ Cr0.UintN = AsmReadCr0 ();
+ Cr0.Bits.WP = 1;
+ AsmWriteCr0 (Cr0.UintN);
+
+ if (CetEnabled) {
+ //
+ // re-enable CET.
+ //
+ EnableCet ();
+ }
+ }
+}
+
/**
Initialize a buffer pool for page table use only.
@@ -62,10 +120,9 @@ InitializePageTablePool (
IN UINTN PoolPages
)
{
- VOID *Buffer;
- BOOLEAN CetEnabled;
- BOOLEAN WpEnabled;
- IA32_CR0 Cr0;
+ VOID *Buffer;
+ BOOLEAN WpEnabled;
+ BOOLEAN CetEnabled;
//
// Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
@@ -102,34 +159,9 @@ InitializePageTablePool (
// If page table memory has been marked as RO, mark the new pool pages as read-only.
//
if (mIsReadOnlyPageTable) {
- CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
- Cr0.UintN = AsmReadCr0 ();
- WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
- if (WpEnabled) {
- if (CetEnabled) {
- //
- // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
- //
- DisableCet ();
- }
-
- Cr0.Bits.WP = 0;
- AsmWriteCr0 (Cr0.UintN);
- }
-
+ DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
- if (WpEnabled) {
- Cr0.UintN = AsmReadCr0 ();
- Cr0.Bits.WP = 1;
- AsmWriteCr0 (Cr0.UintN);
-
- if (CetEnabled) {
- //
- // re-enable CET.
- //
- EnableCet ();
- }
- }
+ EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
}
return TRUE;
@@ -1782,6 +1814,7 @@ SetPageTableAttributes (
VOID
)
{
+ BOOLEAN WpEnabled;
BOOLEAN CetEnabled;
if (!IfReadOnlyPageTableNeeded ()) {
@@ -1794,15 +1827,7 @@ SetPageTableAttributes (
// Disable write protection, because we need mark page table to be write protected.
// We need *write* page table memory, to mark itself to be *read only*.
//
- CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
- if (CetEnabled) {
- //
- // CET must be disabled if WP is disabled.
- //
- DisableCet ();
- }
-
- AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
+ DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
// Set memory used by page table as Read Only.
DEBUG ((DEBUG_INFO, "Start...\n"));
@@ -1811,20 +1836,12 @@ SetPageTableAttributes (
//
// Enable write protection, after page table attribute updated.
//
- AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
+ EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
mIsReadOnlyPageTable = TRUE;
//
// Flush TLB after mark all page table pool as read only.
//
FlushTlbForAll ();
-
- if (CetEnabled) {
- //
- // re-enable CET.
- //
- EnableCet ();
- }
-
return;
}
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104927): https://edk2.groups.io/g/devel/message/104927
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, May 16, 2023 5:59 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul
> R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to
> disable/enable CR0.WP
>
> Add two functions to disable/enable CR0.WP. These two unctions
> will also be used in later commits. This commit doesn't change any
> functionality.
>
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24
> ++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +-------------------------------------------------
> 2 files changed, 90 insertions(+), 49 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index ba341cadc6..e0c4ca76dc 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
> VOID
> );
>
> +/**
> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> +
> + @param[out] WpEnabled If Cr0.WP is enabled.
> + @param[out] CetEnabled If CET is enabled.
> +**/
> +VOID
> +DisableReadOnlyPageWriteProtect (
> + OUT BOOLEAN *WpEnabled,
> + OUT BOOLEAN *CetEnabled
> + );
> +
> +/**
> + Enable Write Protect on pages marked as read-only.
> +
> + @param[out] WpEnabled If Cr0.WP should be enabled.
> + @param[out] CetEnabled If CET should be enabled.
> +**/
> +VOID
> +EnableReadOnlyPageWriteProtect (
> + BOOLEAN WpEnabled,
> + BOOLEAN CetEnabled
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 2faee8f859..4b512edf68 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> //
> BOOLEAN mIsReadOnlyPageTable = FALSE;
>
> +/**
> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> +
> + @param[out] WpEnabled If Cr0.WP is enabled.
> + @param[out] CetEnabled If CET is enabled.
> +**/
> +VOID
> +DisableReadOnlyPageWriteProtect (
> + OUT BOOLEAN *WpEnabled,
> + OUT BOOLEAN *CetEnabled
> + )
> +{
> + IA32_CR0 Cr0;
> +
> + *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + Cr0.UintN = AsmReadCr0 ();
> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> + if (*WpEnabled) {
> + if (*CetEnabled) {
> + //
> + // CET must be disabled if WP is disabled. Disable CET before clearing
> CR0.WP.
> + //
> + DisableCet ();
> + }
> +
> + Cr0.Bits.WP = 0;
> + AsmWriteCr0 (Cr0.UintN);
> + }
> +}
> +
> +/**
> + Enable Write Protect on pages marked as read-only.
> +
> + @param[out] WpEnabled If Cr0.WP should be enabled.
> + @param[out] CetEnabled If CET should be enabled.
> +**/
> +VOID
> +EnableReadOnlyPageWriteProtect (
> + BOOLEAN WpEnabled,
> + BOOLEAN CetEnabled
> + )
> +{
> + IA32_CR0 Cr0;
> +
> + if (WpEnabled) {
> + Cr0.UintN = AsmReadCr0 ();
> + Cr0.Bits.WP = 1;
> + AsmWriteCr0 (Cr0.UintN);
> +
> + if (CetEnabled) {
> + //
> + // re-enable CET.
> + //
> + EnableCet ();
> + }
> + }
> +}
> +
> /**
> Initialize a buffer pool for page table use only.
>
> @@ -62,10 +120,9 @@ InitializePageTablePool (
> IN UINTN PoolPages
> )
> {
> - VOID *Buffer;
> - BOOLEAN CetEnabled;
> - BOOLEAN WpEnabled;
> - IA32_CR0 Cr0;
> + VOID *Buffer;
> + BOOLEAN WpEnabled;
> + BOOLEAN CetEnabled;
>
> //
> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page
> for
> @@ -102,34 +159,9 @@ InitializePageTablePool (
> // If page table memory has been marked as RO, mark the new pool pages as
> read-only.
> //
> if (mIsReadOnlyPageTable) {
> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - Cr0.UintN = AsmReadCr0 ();
> - WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> - if (WpEnabled) {
> - if (CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled. Disable CET before clearing
> CR0.WP.
> - //
> - DisableCet ();
> - }
> -
> - Cr0.Bits.WP = 0;
> - AsmWriteCr0 (Cr0.UintN);
> - }
> -
> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - if (WpEnabled) {
> - Cr0.UintN = AsmReadCr0 ();
> - Cr0.Bits.WP = 1;
> - AsmWriteCr0 (Cr0.UintN);
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> - }
> + EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> }
>
> return TRUE;
> @@ -1782,6 +1814,7 @@ SetPageTableAttributes (
> VOID
> )
> {
> + BOOLEAN WpEnabled;
> BOOLEAN CetEnabled;
>
> if (!IfReadOnlyPageTableNeeded ()) {
> @@ -1794,15 +1827,7 @@ SetPageTableAttributes (
> // Disable write protection, because we need mark page table to be write
> protected.
> // We need *write* page table memory, to mark itself to be *read only*.
> //
> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - if (CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled.
> - //
> - DisableCet ();
> - }
> -
> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>
> // Set memory used by page table as Read Only.
> DEBUG ((DEBUG_INFO, "Start...\n"));
> @@ -1811,20 +1836,12 @@ SetPageTableAttributes (
> //
> // Enable write protection, after page table attribute updated.
> //
> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> + EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> mIsReadOnlyPageTable = TRUE;
>
> //
> // Flush TLB after mark all page table pool as read only.
> //
> FlushTlbForAll ();
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> -
> return;
> }
> --
> 2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105617): https://edk2.groups.io/g/devel/message/105617
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Dun,
Thanks for the notice on the other thread (v4 04/15).
I have a few more questions on this specific patch (and a few associated
commits related to it):
Why would we allow page table manipulation after `mIsReadOnlyPageTable`
is evaluated to TRUE?
As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside
`SetPageTableAttributes` function,
but then we also have code in `InitializePageTablePool` to expect more
page tables to be allocated.
Could you please let me when this would happen?
I thought there would not be any new page table memory (i.e. memory
attribute update) after ready
to lock with restricted memory access option? With these change, it
seems to be doable through
EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so, would
you mind shedding
some light on what other behavior changes there might be?
In addition, I might miss it in the patch series. If the newly allocated
page memory is marked as read only
after the above flag is set to TRUE, how would the callers able to use them?
Thanks in advance.
Regards,
Kun
On 5/16/2023 2:59 AM, duntan wrote:
> Add two functions to disable/enable CR0.WP. These two unctions
> will also be used in later commits. This commit doesn't change any
> functionality.
>
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24 ++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
> 2 files changed, 90 insertions(+), 49 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index ba341cadc6..e0c4ca76dc 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
> VOID
> );
>
> +/**
> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> +
> + @param[out] WpEnabled If Cr0.WP is enabled.
> + @param[out] CetEnabled If CET is enabled.
> +**/
> +VOID
> +DisableReadOnlyPageWriteProtect (
> + OUT BOOLEAN *WpEnabled,
> + OUT BOOLEAN *CetEnabled
> + );
> +
> +/**
> + Enable Write Protect on pages marked as read-only.
> +
> + @param[out] WpEnabled If Cr0.WP should be enabled.
> + @param[out] CetEnabled If CET should be enabled.
> +**/
> +VOID
> +EnableReadOnlyPageWriteProtect (
> + BOOLEAN WpEnabled,
> + BOOLEAN CetEnabled
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 2faee8f859..4b512edf68 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> //
> BOOLEAN mIsReadOnlyPageTable = FALSE;
>
> +/**
> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> +
> + @param[out] WpEnabled If Cr0.WP is enabled.
> + @param[out] CetEnabled If CET is enabled.
> +**/
> +VOID
> +DisableReadOnlyPageWriteProtect (
> + OUT BOOLEAN *WpEnabled,
> + OUT BOOLEAN *CetEnabled
> + )
> +{
> + IA32_CR0 Cr0;
> +
> + *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + Cr0.UintN = AsmReadCr0 ();
> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> + if (*WpEnabled) {
> + if (*CetEnabled) {
> + //
> + // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
> + //
> + DisableCet ();
> + }
> +
> + Cr0.Bits.WP = 0;
> + AsmWriteCr0 (Cr0.UintN);
> + }
> +}
> +
> +/**
> + Enable Write Protect on pages marked as read-only.
> +
> + @param[out] WpEnabled If Cr0.WP should be enabled.
> + @param[out] CetEnabled If CET should be enabled.
> +**/
> +VOID
> +EnableReadOnlyPageWriteProtect (
> + BOOLEAN WpEnabled,
> + BOOLEAN CetEnabled
> + )
> +{
> + IA32_CR0 Cr0;
> +
> + if (WpEnabled) {
> + Cr0.UintN = AsmReadCr0 ();
> + Cr0.Bits.WP = 1;
> + AsmWriteCr0 (Cr0.UintN);
> +
> + if (CetEnabled) {
> + //
> + // re-enable CET.
> + //
> + EnableCet ();
> + }
> + }
> +}
> +
> /**
> Initialize a buffer pool for page table use only.
>
> @@ -62,10 +120,9 @@ InitializePageTablePool (
> IN UINTN PoolPages
> )
> {
> - VOID *Buffer;
> - BOOLEAN CetEnabled;
> - BOOLEAN WpEnabled;
> - IA32_CR0 Cr0;
> + VOID *Buffer;
> + BOOLEAN WpEnabled;
> + BOOLEAN CetEnabled;
>
> //
> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
> @@ -102,34 +159,9 @@ InitializePageTablePool (
> // If page table memory has been marked as RO, mark the new pool pages as read-only.
> //
> if (mIsReadOnlyPageTable) {
> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - Cr0.UintN = AsmReadCr0 ();
> - WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> - if (WpEnabled) {
> - if (CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
> - //
> - DisableCet ();
> - }
> -
> - Cr0.Bits.WP = 0;
> - AsmWriteCr0 (Cr0.UintN);
> - }
> -
> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - if (WpEnabled) {
> - Cr0.UintN = AsmReadCr0 ();
> - Cr0.Bits.WP = 1;
> - AsmWriteCr0 (Cr0.UintN);
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> - }
> + EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> }
>
> return TRUE;
> @@ -1782,6 +1814,7 @@ SetPageTableAttributes (
> VOID
> )
> {
> + BOOLEAN WpEnabled;
> BOOLEAN CetEnabled;
>
> if (!IfReadOnlyPageTableNeeded ()) {
> @@ -1794,15 +1827,7 @@ SetPageTableAttributes (
> // Disable write protection, because we need mark page table to be write protected.
> // We need *write* page table memory, to mark itself to be *read only*.
> //
> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - if (CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled.
> - //
> - DisableCet ();
> - }
> -
> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>
> // Set memory used by page table as Read Only.
> DEBUG ((DEBUG_INFO, "Start...\n"));
> @@ -1811,20 +1836,12 @@ SetPageTableAttributes (
> //
> // Enable write protection, after page table attribute updated.
> //
> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> + EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> mIsReadOnlyPageTable = TRUE;
>
> //
> // Flush TLB after mark all page table pool as read only.
> //
> FlushTlbForAll ();
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> -
> return;
> }
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105090): https://edk2.groups.io/g/devel/message/105090
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Kun,
I've updated my answers in your original mail.
Thanks,
Dun
-----Original Message-----
From: Kun Qin <kuqin12@gmail.com>
Sent: Saturday, May 20, 2023 10:00 AM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
Hi Dun,
Thanks for the notice on the other thread (v4 04/15).
I have a few more questions on this specific patch (and a few associated commits related to it):
Why would we allow page table manipulation after `mIsReadOnlyPageTable` is evaluated to TRUE?
Dun: `mIsReadOnlyPageTable` is a flag to indicate that current page table has been marked as RO and the new allocated pool should also be RO. We only need to clear Cr0.WP before modify page table.
As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside `SetPageTableAttributes` function, but then we also have code in `InitializePageTablePool` to expect more page tables to be allocated.
Could you please let me when this would happen?
Dun: After `SetPageTableAttributes`, in 'SmmCpuFeaturesCompleteSmmReadyToLock()' API of different platform SmmCpuFeaturesLib, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL may be used to convert memory attribute. Also, in SMI handler after ReadyToLock, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL still may be used to convert memory attribute. During this process, if page split happens, new page table pool may be allocated.
I thought there would not be any new page table memory (i.e. memory attribute update) after ready to lock with restricted memory access option? With these change, it seems to be doable through EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so, would you mind shedding some light on what other behavior changes there might be?
Dun: Do you mean that we should check if ReadyToLock in EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL implementation to make sure that page table won't be modified after ReadyToLock?
If is, as I mentioned above, page table still may be modified after ReadyToLock.
In addition, I might miss it in the patch series. If the newly allocated page memory is marked as read only after the above flag is set to TRUE, how would the callers able to use them?
Dun: Caller can clear the Cr0.WP before modifying the page table.
Thanks in advance.
Regards,
Kun
On 5/16/2023 2:59 AM, duntan wrote:
> Add two functions to disable/enable CR0.WP. These two unctions will
> also be used in later commits. This commit doesn't change any
> functionality.
>
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24 ++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
> 2 files changed, 90 insertions(+), 49 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index ba341cadc6..e0c4ca76dc 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
> VOID
> );
>
> +/**
> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> +
> + @param[out] WpEnabled If Cr0.WP is enabled.
> + @param[out] CetEnabled If CET is enabled.
> +**/
> +VOID
> +DisableReadOnlyPageWriteProtect (
> + OUT BOOLEAN *WpEnabled,
> + OUT BOOLEAN *CetEnabled
> + );
> +
> +/**
> + Enable Write Protect on pages marked as read-only.
> +
> + @param[out] WpEnabled If Cr0.WP should be enabled.
> + @param[out] CetEnabled If CET should be enabled.
> +**/
> +VOID
> +EnableReadOnlyPageWriteProtect (
> + BOOLEAN WpEnabled,
> + BOOLEAN CetEnabled
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 2faee8f859..4b512edf68 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> //
> BOOLEAN mIsReadOnlyPageTable = FALSE;
>
> +/**
> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> +
> + @param[out] WpEnabled If Cr0.WP is enabled.
> + @param[out] CetEnabled If CET is enabled.
> +**/
> +VOID
> +DisableReadOnlyPageWriteProtect (
> + OUT BOOLEAN *WpEnabled,
> + OUT BOOLEAN *CetEnabled
> + )
> +{
> + IA32_CR0 Cr0;
> +
> + *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + Cr0.UintN = AsmReadCr0 ();
> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE; if (*WpEnabled) {
> + if (*CetEnabled) {
> + //
> + // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
> + //
> + DisableCet ();
> + }
> +
> + Cr0.Bits.WP = 0;
> + AsmWriteCr0 (Cr0.UintN);
> + }
> +}
> +
> +/**
> + Enable Write Protect on pages marked as read-only.
> +
> + @param[out] WpEnabled If Cr0.WP should be enabled.
> + @param[out] CetEnabled If CET should be enabled.
> +**/
> +VOID
> +EnableReadOnlyPageWriteProtect (
> + BOOLEAN WpEnabled,
> + BOOLEAN CetEnabled
> + )
> +{
> + IA32_CR0 Cr0;
> +
> + if (WpEnabled) {
> + Cr0.UintN = AsmReadCr0 ();
> + Cr0.Bits.WP = 1;
> + AsmWriteCr0 (Cr0.UintN);
> +
> + if (CetEnabled) {
> + //
> + // re-enable CET.
> + //
> + EnableCet ();
> + }
> + }
> +}
> +
> /**
> Initialize a buffer pool for page table use only.
>
> @@ -62,10 +120,9 @@ InitializePageTablePool (
> IN UINTN PoolPages
> )
> {
> - VOID *Buffer;
> - BOOLEAN CetEnabled;
> - BOOLEAN WpEnabled;
> - IA32_CR0 Cr0;
> + VOID *Buffer;
> + BOOLEAN WpEnabled;
> + BOOLEAN CetEnabled;
>
> //
> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including
> one page for @@ -102,34 +159,9 @@ InitializePageTablePool (
> // If page table memory has been marked as RO, mark the new pool pages as read-only.
> //
> if (mIsReadOnlyPageTable) {
> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - Cr0.UintN = AsmReadCr0 ();
> - WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> - if (WpEnabled) {
> - if (CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
> - //
> - DisableCet ();
> - }
> -
> - Cr0.Bits.WP = 0;
> - AsmWriteCr0 (Cr0.UintN);
> - }
> -
> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - if (WpEnabled) {
> - Cr0.UintN = AsmReadCr0 ();
> - Cr0.Bits.WP = 1;
> - AsmWriteCr0 (Cr0.UintN);
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> - }
> + EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> }
>
> return TRUE;
> @@ -1782,6 +1814,7 @@ SetPageTableAttributes (
> VOID
> )
> {
> + BOOLEAN WpEnabled;
> BOOLEAN CetEnabled;
>
> if (!IfReadOnlyPageTableNeeded ()) { @@ -1794,15 +1827,7 @@
> SetPageTableAttributes (
> // Disable write protection, because we need mark page table to be write protected.
> // We need *write* page table memory, to mark itself to be *read only*.
> //
> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE :
> FALSE;
> - if (CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled.
> - //
> - DisableCet ();
> - }
> -
> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>
> // Set memory used by page table as Read Only.
> DEBUG ((DEBUG_INFO, "Start...\n")); @@ -1811,20 +1836,12 @@
> SetPageTableAttributes (
> //
> // Enable write protection, after page table attribute updated.
> //
> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> + EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> mIsReadOnlyPageTable = TRUE;
>
> //
> // Flush TLB after mark all page table pool as read only.
> //
> FlushTlbForAll ();
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> -
> return;
> }
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105168): https://edk2.groups.io/g/devel/message/105168
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Dun,
Thanks for your reply. That was helpful!
Just a follow-up question, is there any plan to support heap guard with
PcdCpuSmmRestrictedMemoryAccess enabled after these changes? I think it
would be a great value prop for the developers to have both features
enabled during firmware validation process.
Thanks,
Kun
On 5/23/2023 2:14 AM, duntan wrote:
> Hi Kun,
>
> I've updated my answers in your original mail.
>
> Thanks,
> Dun
>
> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Saturday, May 20, 2023 10:00 AM
> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
>
> Hi Dun,
>
> Thanks for the notice on the other thread (v4 04/15).
>
> I have a few more questions on this specific patch (and a few associated commits related to it):
>
> Why would we allow page table manipulation after `mIsReadOnlyPageTable` is evaluated to TRUE?
> Dun: `mIsReadOnlyPageTable` is a flag to indicate that current page table has been marked as RO and the new allocated pool should also be RO. We only need to clear Cr0.WP before modify page table.
>
>
> As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside `SetPageTableAttributes` function, but then we also have code in `InitializePageTablePool` to expect more page tables to be allocated.
> Could you please let me when this would happen?
> Dun: After `SetPageTableAttributes`, in 'SmmCpuFeaturesCompleteSmmReadyToLock()' API of different platform SmmCpuFeaturesLib, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL may be used to convert memory attribute. Also, in SMI handler after ReadyToLock, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL still may be used to convert memory attribute. During this process, if page split happens, new page table pool may be allocated.
>
>
> I thought there would not be any new page table memory (i.e. memory attribute update) after ready to lock with restricted memory access option? With these change, it seems to be doable through EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so, would you mind shedding some light on what other behavior changes there might be?
> Dun: Do you mean that we should check if ReadyToLock in EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL implementation to make sure that page table won't be modified after ReadyToLock?
> If is, as I mentioned above, page table still may be modified after ReadyToLock.
>
>
> In addition, I might miss it in the patch series. If the newly allocated page memory is marked as read only after the above flag is set to TRUE, how would the callers able to use them?
> Dun: Caller can clear the Cr0.WP before modifying the page table.
>
>
> Thanks in advance.
>
> Regards,
> Kun
>
> On 5/16/2023 2:59 AM, duntan wrote:
>> Add two functions to disable/enable CR0.WP. These two unctions will
>> also be used in later commits. This commit doesn't change any
>> functionality.
>>
>> Signed-off-by: Dun Tan <dun.tan@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24 ++++++++++++++++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
>> 2 files changed, 90 insertions(+), 49 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index ba341cadc6..e0c4ca76dc 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
>> VOID
>> );
>>
>> +/**
>> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>> +
>> + @param[out] WpEnabled If Cr0.WP is enabled.
>> + @param[out] CetEnabled If CET is enabled.
>> +**/
>> +VOID
>> +DisableReadOnlyPageWriteProtect (
>> + OUT BOOLEAN *WpEnabled,
>> + OUT BOOLEAN *CetEnabled
>> + );
>> +
>> +/**
>> + Enable Write Protect on pages marked as read-only.
>> +
>> + @param[out] WpEnabled If Cr0.WP should be enabled.
>> + @param[out] CetEnabled If CET should be enabled.
>> +**/
>> +VOID
>> +EnableReadOnlyPageWriteProtect (
>> + BOOLEAN WpEnabled,
>> + BOOLEAN CetEnabled
>> + );
>> +
>> #endif
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> index 2faee8f859..4b512edf68 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> @@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
>> //
>> BOOLEAN mIsReadOnlyPageTable = FALSE;
>>
>> +/**
>> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>> +
>> + @param[out] WpEnabled If Cr0.WP is enabled.
>> + @param[out] CetEnabled If CET is enabled.
>> +**/
>> +VOID
>> +DisableReadOnlyPageWriteProtect (
>> + OUT BOOLEAN *WpEnabled,
>> + OUT BOOLEAN *CetEnabled
>> + )
>> +{
>> + IA32_CR0 Cr0;
>> +
>> + *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
>> + Cr0.UintN = AsmReadCr0 ();
>> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE; if (*WpEnabled) {
>> + if (*CetEnabled) {
>> + //
>> + // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
>> + //
>> + DisableCet ();
>> + }
>> +
>> + Cr0.Bits.WP = 0;
>> + AsmWriteCr0 (Cr0.UintN);
>> + }
>> +}
>> +
>> +/**
>> + Enable Write Protect on pages marked as read-only.
>> +
>> + @param[out] WpEnabled If Cr0.WP should be enabled.
>> + @param[out] CetEnabled If CET should be enabled.
>> +**/
>> +VOID
>> +EnableReadOnlyPageWriteProtect (
>> + BOOLEAN WpEnabled,
>> + BOOLEAN CetEnabled
>> + )
>> +{
>> + IA32_CR0 Cr0;
>> +
>> + if (WpEnabled) {
>> + Cr0.UintN = AsmReadCr0 ();
>> + Cr0.Bits.WP = 1;
>> + AsmWriteCr0 (Cr0.UintN);
>> +
>> + if (CetEnabled) {
>> + //
>> + // re-enable CET.
>> + //
>> + EnableCet ();
>> + }
>> + }
>> +}
>> +
>> /**
>> Initialize a buffer pool for page table use only.
>>
>> @@ -62,10 +120,9 @@ InitializePageTablePool (
>> IN UINTN PoolPages
>> )
>> {
>> - VOID *Buffer;
>> - BOOLEAN CetEnabled;
>> - BOOLEAN WpEnabled;
>> - IA32_CR0 Cr0;
>> + VOID *Buffer;
>> + BOOLEAN WpEnabled;
>> + BOOLEAN CetEnabled;
>>
>> //
>> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including
>> one page for @@ -102,34 +159,9 @@ InitializePageTablePool (
>> // If page table memory has been marked as RO, mark the new pool pages as read-only.
>> //
>> if (mIsReadOnlyPageTable) {
>> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
>> - Cr0.UintN = AsmReadCr0 ();
>> - WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
>> - if (WpEnabled) {
>> - if (CetEnabled) {
>> - //
>> - // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
>> - //
>> - DisableCet ();
>> - }
>> -
>> - Cr0.Bits.WP = 0;
>> - AsmWriteCr0 (Cr0.UintN);
>> - }
>> -
>> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
>> - if (WpEnabled) {
>> - Cr0.UintN = AsmReadCr0 ();
>> - Cr0.Bits.WP = 1;
>> - AsmWriteCr0 (Cr0.UintN);
>> -
>> - if (CetEnabled) {
>> - //
>> - // re-enable CET.
>> - //
>> - EnableCet ();
>> - }
>> - }
>> + EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
>> }
>>
>> return TRUE;
>> @@ -1782,6 +1814,7 @@ SetPageTableAttributes (
>> VOID
>> )
>> {
>> + BOOLEAN WpEnabled;
>> BOOLEAN CetEnabled;
>>
>> if (!IfReadOnlyPageTableNeeded ()) { @@ -1794,15 +1827,7 @@
>> SetPageTableAttributes (
>> // Disable write protection, because we need mark page table to be write protected.
>> // We need *write* page table memory, to mark itself to be *read only*.
>> //
>> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE :
>> FALSE;
>> - if (CetEnabled) {
>> - //
>> - // CET must be disabled if WP is disabled.
>> - //
>> - DisableCet ();
>> - }
>> -
>> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
>> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>>
>> // Set memory used by page table as Read Only.
>> DEBUG ((DEBUG_INFO, "Start...\n")); @@ -1811,20 +1836,12 @@
>> SetPageTableAttributes (
>> //
>> // Enable write protection, after page table attribute updated.
>> //
>> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
>> + EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
>> mIsReadOnlyPageTable = TRUE;
>>
>> //
>> // Flush TLB after mark all page table pool as read only.
>> //
>> FlushTlbForAll ();
>> -
>> - if (CetEnabled) {
>> - //
>> - // re-enable CET.
>> - //
>> - EnableCet ();
>> - }
>> -
>> return;
>> }
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105278): https://edk2.groups.io/g/devel/message/105278
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Kun,
Thanks for raising that up😊
We have some ideas. Will post them later.
Looking forward to work with community together.
Thanks,
Ray
> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Thursday, May 25, 2023 2:39 AM
> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm:
> Add 2 function to disable/enable CR0.WP
>
> Hi Dun,
>
> Thanks for your reply. That was helpful!
>
> Just a follow-up question, is there any plan to support heap guard with
> PcdCpuSmmRestrictedMemoryAccess enabled after these changes? I think it
> would be a great value prop for the developers to have both features
> enabled during firmware validation process.
>
> Thanks,
> Kun
>
> On 5/23/2023 2:14 AM, duntan wrote:
> > Hi Kun,
> >
> > I've updated my answers in your original mail.
> >
> > Thanks,
> > Dun
> >
> > -----Original Message-----
> > From: Kun Qin <kuqin12@gmail.com>
> > Sent: Saturday, May 20, 2023 10:00 AM
> > To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> > Subject: Re: [edk2-devel] [Patch V4 07/15]
> UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
> >
> > Hi Dun,
> >
> > Thanks for the notice on the other thread (v4 04/15).
> >
> > I have a few more questions on this specific patch (and a few associated
> commits related to it):
> >
> > Why would we allow page table manipulation after `mIsReadOnlyPageTable`
> is evaluated to TRUE?
> > Dun: `mIsReadOnlyPageTable` is a flag to indicate that current page table has
> been marked as RO and the new allocated pool should also be RO. We only
> need to clear Cr0.WP before modify page table.
> >
> >
> > As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside
> `SetPageTableAttributes` function, but then we also have code in
> `InitializePageTablePool` to expect more page tables to be allocated.
> > Could you please let me when this would happen?
> > Dun: After `SetPageTableAttributes`, in
> 'SmmCpuFeaturesCompleteSmmReadyToLock()' API of different platform
> SmmCpuFeaturesLib, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL may be
> used to convert memory attribute. Also, in SMI handler after ReadyToLock,
> EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL still may be used to convert
> memory attribute. During this process, if page split happens, new page table
> pool may be allocated.
> >
> >
> > I thought there would not be any new page table memory (i.e. memory
> attribute update) after ready to lock with restricted memory access option?
> With these change, it seems to be doable through
> EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so,
> would you mind shedding some light on what other behavior changes there
> might be?
> > Dun: Do you mean that we should check if ReadyToLock in
> EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL implementation to make sure
> that page table won't be modified after ReadyToLock?
> > If is, as I mentioned above, page table still may be modified after
> ReadyToLock.
> >
> >
> > In addition, I might miss it in the patch series. If the newly allocated page
> memory is marked as read only after the above flag is set to TRUE, how would
> the callers able to use them?
> > Dun: Caller can clear the Cr0.WP before modifying the page table.
> >
> >
> > Thanks in advance.
> >
> > Regards,
> > Kun
> >
> > On 5/16/2023 2:59 AM, duntan wrote:
> >> Add two functions to disable/enable CR0.WP. These two unctions will
> >> also be used in later commits. This commit doesn't change any
> >> functionality.
> >>
> >> Signed-off-by: Dun Tan <dun.tan@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> ---
> >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24
> ++++++++++++++++++++++++
> >> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++-------------------------------------------------
> >> 2 files changed, 90 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> >> index ba341cadc6..e0c4ca76dc 100644
> >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> >> @@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
> >> VOID
> >> );
> >>
> >> +/**
> >> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> >> +
> >> + @param[out] WpEnabled If Cr0.WP is enabled.
> >> + @param[out] CetEnabled If CET is enabled.
> >> +**/
> >> +VOID
> >> +DisableReadOnlyPageWriteProtect (
> >> + OUT BOOLEAN *WpEnabled,
> >> + OUT BOOLEAN *CetEnabled
> >> + );
> >> +
> >> +/**
> >> + Enable Write Protect on pages marked as read-only.
> >> +
> >> + @param[out] WpEnabled If Cr0.WP should be enabled.
> >> + @param[out] CetEnabled If CET should be enabled.
> >> +**/
> >> +VOID
> >> +EnableReadOnlyPageWriteProtect (
> >> + BOOLEAN WpEnabled,
> >> + BOOLEAN CetEnabled
> >> + );
> >> +
> >> #endif
> >> diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> >> index 2faee8f859..4b512edf68 100644
> >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> >> @@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> >> //
> >> BOOLEAN mIsReadOnlyPageTable = FALSE;
> >>
> >> +/**
> >> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> >> +
> >> + @param[out] WpEnabled If Cr0.WP is enabled.
> >> + @param[out] CetEnabled If CET is enabled.
> >> +**/
> >> +VOID
> >> +DisableReadOnlyPageWriteProtect (
> >> + OUT BOOLEAN *WpEnabled,
> >> + OUT BOOLEAN *CetEnabled
> >> + )
> >> +{
> >> + IA32_CR0 Cr0;
> >> +
> >> + *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE :
> FALSE;
> >> + Cr0.UintN = AsmReadCr0 ();
> >> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE; if (*WpEnabled) {
> >> + if (*CetEnabled) {
> >> + //
> >> + // CET must be disabled if WP is disabled. Disable CET before clearing
> CR0.WP.
> >> + //
> >> + DisableCet ();
> >> + }
> >> +
> >> + Cr0.Bits.WP = 0;
> >> + AsmWriteCr0 (Cr0.UintN);
> >> + }
> >> +}
> >> +
> >> +/**
> >> + Enable Write Protect on pages marked as read-only.
> >> +
> >> + @param[out] WpEnabled If Cr0.WP should be enabled.
> >> + @param[out] CetEnabled If CET should be enabled.
> >> +**/
> >> +VOID
> >> +EnableReadOnlyPageWriteProtect (
> >> + BOOLEAN WpEnabled,
> >> + BOOLEAN CetEnabled
> >> + )
> >> +{
> >> + IA32_CR0 Cr0;
> >> +
> >> + if (WpEnabled) {
> >> + Cr0.UintN = AsmReadCr0 ();
> >> + Cr0.Bits.WP = 1;
> >> + AsmWriteCr0 (Cr0.UintN);
> >> +
> >> + if (CetEnabled) {
> >> + //
> >> + // re-enable CET.
> >> + //
> >> + EnableCet ();
> >> + }
> >> + }
> >> +}
> >> +
> >> /**
> >> Initialize a buffer pool for page table use only.
> >>
> >> @@ -62,10 +120,9 @@ InitializePageTablePool (
> >> IN UINTN PoolPages
> >> )
> >> {
> >> - VOID *Buffer;
> >> - BOOLEAN CetEnabled;
> >> - BOOLEAN WpEnabled;
> >> - IA32_CR0 Cr0;
> >> + VOID *Buffer;
> >> + BOOLEAN WpEnabled;
> >> + BOOLEAN CetEnabled;
> >>
> >> //
> >> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including
> >> one page for @@ -102,34 +159,9 @@ InitializePageTablePool (
> >> // If page table memory has been marked as RO, mark the new pool
> pages as read-only.
> >> //
> >> if (mIsReadOnlyPageTable) {
> >> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> >> - Cr0.UintN = AsmReadCr0 ();
> >> - WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> >> - if (WpEnabled) {
> >> - if (CetEnabled) {
> >> - //
> >> - // CET must be disabled if WP is disabled. Disable CET before clearing
> CR0.WP.
> >> - //
> >> - DisableCet ();
> >> - }
> >> -
> >> - Cr0.Bits.WP = 0;
> >> - AsmWriteCr0 (Cr0.UintN);
> >> - }
> >> -
> >> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> >> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> >> - if (WpEnabled) {
> >> - Cr0.UintN = AsmReadCr0 ();
> >> - Cr0.Bits.WP = 1;
> >> - AsmWriteCr0 (Cr0.UintN);
> >> -
> >> - if (CetEnabled) {
> >> - //
> >> - // re-enable CET.
> >> - //
> >> - EnableCet ();
> >> - }
> >> - }
> >> + EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> >> }
> >>
> >> return TRUE;
> >> @@ -1782,6 +1814,7 @@ SetPageTableAttributes (
> >> VOID
> >> )
> >> {
> >> + BOOLEAN WpEnabled;
> >> BOOLEAN CetEnabled;
> >>
> >> if (!IfReadOnlyPageTableNeeded ()) { @@ -1794,15 +1827,7 @@
> >> SetPageTableAttributes (
> >> // Disable write protection, because we need mark page table to be write
> protected.
> >> // We need *write* page table memory, to mark itself to be *read only*.
> >> //
> >> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE :
> >> FALSE;
> >> - if (CetEnabled) {
> >> - //
> >> - // CET must be disabled if WP is disabled.
> >> - //
> >> - DisableCet ();
> >> - }
> >> -
> >> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> >> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> >>
> >> // Set memory used by page table as Read Only.
> >> DEBUG ((DEBUG_INFO, "Start...\n")); @@ -1811,20 +1836,12 @@
> >> SetPageTableAttributes (
> >> //
> >> // Enable write protection, after page table attribute updated.
> >> //
> >> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> >> + EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> >> mIsReadOnlyPageTable = TRUE;
> >>
> >> //
> >> // Flush TLB after mark all page table pool as read only.
> >> //
> >> FlushTlbForAll ();
> >> -
> >> - if (CetEnabled) {
> >> - //
> >> - // re-enable CET.
> >> - //
> >> - EnableCet ();
> >> - }
> >> -
> >> return;
> >> }
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105289): https://edk2.groups.io/g/devel/message/105289
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Thanks, Ray. Looking forward to seeing the ideas on this feature!
Regards,
Kun
On 5/24/2023 5:46 PM, Ni, Ray wrote:
> Kun,
> Thanks for raising that up😊
>
> We have some ideas. Will post them later.
> Looking forward to work with community together.
>
> Thanks,
> Ray
>
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Thursday, May 25, 2023 2:39 AM
>> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
>> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
>> Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm:
>> Add 2 function to disable/enable CR0.WP
>>
>> Hi Dun,
>>
>> Thanks for your reply. That was helpful!
>>
>> Just a follow-up question, is there any plan to support heap guard with
>> PcdCpuSmmRestrictedMemoryAccess enabled after these changes? I think it
>> would be a great value prop for the developers to have both features
>> enabled during firmware validation process.
>>
>> Thanks,
>> Kun
>>
>> On 5/23/2023 2:14 AM, duntan wrote:
>>> Hi Kun,
>>>
>>> I've updated my answers in your original mail.
>>>
>>> Thanks,
>>> Dun
>>>
>>> -----Original Message-----
>>> From: Kun Qin <kuqin12@gmail.com>
>>> Sent: Saturday, May 20, 2023 10:00 AM
>>> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
>>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
>> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
>>> Subject: Re: [edk2-devel] [Patch V4 07/15]
>> UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
>>> Hi Dun,
>>>
>>> Thanks for the notice on the other thread (v4 04/15).
>>>
>>> I have a few more questions on this specific patch (and a few associated
>> commits related to it):
>>> Why would we allow page table manipulation after `mIsReadOnlyPageTable`
>> is evaluated to TRUE?
>>> Dun: `mIsReadOnlyPageTable` is a flag to indicate that current page table has
>> been marked as RO and the new allocated pool should also be RO. We only
>> need to clear Cr0.WP before modify page table.
>>>
>>> As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside
>> `SetPageTableAttributes` function, but then we also have code in
>> `InitializePageTablePool` to expect more page tables to be allocated.
>>> Could you please let me when this would happen?
>>> Dun: After `SetPageTableAttributes`, in
>> 'SmmCpuFeaturesCompleteSmmReadyToLock()' API of different platform
>> SmmCpuFeaturesLib, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL may be
>> used to convert memory attribute. Also, in SMI handler after ReadyToLock,
>> EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL still may be used to convert
>> memory attribute. During this process, if page split happens, new page table
>> pool may be allocated.
>>>
>>> I thought there would not be any new page table memory (i.e. memory
>> attribute update) after ready to lock with restricted memory access option?
>> With these change, it seems to be doable through
>> EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so,
>> would you mind shedding some light on what other behavior changes there
>> might be?
>>> Dun: Do you mean that we should check if ReadyToLock in
>> EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL implementation to make sure
>> that page table won't be modified after ReadyToLock?
>>> If is, as I mentioned above, page table still may be modified after
>> ReadyToLock.
>>>
>>> In addition, I might miss it in the patch series. If the newly allocated page
>> memory is marked as read only after the above flag is set to TRUE, how would
>> the callers able to use them?
>>> Dun: Caller can clear the Cr0.WP before modifying the page table.
>>>
>>>
>>> Thanks in advance.
>>>
>>> Regards,
>>> Kun
>>>
>>> On 5/16/2023 2:59 AM, duntan wrote:
>>>> Add two functions to disable/enable CR0.WP. These two unctions will
>>>> also be used in later commits. This commit doesn't change any
>>>> functionality.
>>>>
>>>> Signed-off-by: Dun Tan <dun.tan@intel.com>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24
>> ++++++++++++++++++++++++
>>>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++-------------------------------------------------
>>>> 2 files changed, 90 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>>> index ba341cadc6..e0c4ca76dc 100644
>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>>> @@ -1565,4 +1565,28 @@ SmmWaitForApArrival (
>>>> VOID
>>>> );
>>>>
>>>> +/**
>>>> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>>>> +
>>>> + @param[out] WpEnabled If Cr0.WP is enabled.
>>>> + @param[out] CetEnabled If CET is enabled.
>>>> +**/
>>>> +VOID
>>>> +DisableReadOnlyPageWriteProtect (
>>>> + OUT BOOLEAN *WpEnabled,
>>>> + OUT BOOLEAN *CetEnabled
>>>> + );
>>>> +
>>>> +/**
>>>> + Enable Write Protect on pages marked as read-only.
>>>> +
>>>> + @param[out] WpEnabled If Cr0.WP should be enabled.
>>>> + @param[out] CetEnabled If CET should be enabled.
>>>> +**/
>>>> +VOID
>>>> +EnableReadOnlyPageWriteProtect (
>>>> + BOOLEAN WpEnabled,
>>>> + BOOLEAN CetEnabled
>>>> + );
>>>> +
>>>> #endif
>>>> diff --git
>> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>>> index 2faee8f859..4b512edf68 100644
>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>>> @@ -40,6 +40,64 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
>>>> //
>>>> BOOLEAN mIsReadOnlyPageTable = FALSE;
>>>>
>>>> +/**
>>>> + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>>>> +
>>>> + @param[out] WpEnabled If Cr0.WP is enabled.
>>>> + @param[out] CetEnabled If CET is enabled.
>>>> +**/
>>>> +VOID
>>>> +DisableReadOnlyPageWriteProtect (
>>>> + OUT BOOLEAN *WpEnabled,
>>>> + OUT BOOLEAN *CetEnabled
>>>> + )
>>>> +{
>>>> + IA32_CR0 Cr0;
>>>> +
>>>> + *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE :
>> FALSE;
>>>> + Cr0.UintN = AsmReadCr0 ();
>>>> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE; if (*WpEnabled) {
>>>> + if (*CetEnabled) {
>>>> + //
>>>> + // CET must be disabled if WP is disabled. Disable CET before clearing
>> CR0.WP.
>>>> + //
>>>> + DisableCet ();
>>>> + }
>>>> +
>>>> + Cr0.Bits.WP = 0;
>>>> + AsmWriteCr0 (Cr0.UintN);
>>>> + }
>>>> +}
>>>> +
>>>> +/**
>>>> + Enable Write Protect on pages marked as read-only.
>>>> +
>>>> + @param[out] WpEnabled If Cr0.WP should be enabled.
>>>> + @param[out] CetEnabled If CET should be enabled.
>>>> +**/
>>>> +VOID
>>>> +EnableReadOnlyPageWriteProtect (
>>>> + BOOLEAN WpEnabled,
>>>> + BOOLEAN CetEnabled
>>>> + )
>>>> +{
>>>> + IA32_CR0 Cr0;
>>>> +
>>>> + if (WpEnabled) {
>>>> + Cr0.UintN = AsmReadCr0 ();
>>>> + Cr0.Bits.WP = 1;
>>>> + AsmWriteCr0 (Cr0.UintN);
>>>> +
>>>> + if (CetEnabled) {
>>>> + //
>>>> + // re-enable CET.
>>>> + //
>>>> + EnableCet ();
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> /**
>>>> Initialize a buffer pool for page table use only.
>>>>
>>>> @@ -62,10 +120,9 @@ InitializePageTablePool (
>>>> IN UINTN PoolPages
>>>> )
>>>> {
>>>> - VOID *Buffer;
>>>> - BOOLEAN CetEnabled;
>>>> - BOOLEAN WpEnabled;
>>>> - IA32_CR0 Cr0;
>>>> + VOID *Buffer;
>>>> + BOOLEAN WpEnabled;
>>>> + BOOLEAN CetEnabled;
>>>>
>>>> //
>>>> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including
>>>> one page for @@ -102,34 +159,9 @@ InitializePageTablePool (
>>>> // If page table memory has been marked as RO, mark the new pool
>> pages as read-only.
>>>> //
>>>> if (mIsReadOnlyPageTable) {
>>>> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
>>>> - Cr0.UintN = AsmReadCr0 ();
>>>> - WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
>>>> - if (WpEnabled) {
>>>> - if (CetEnabled) {
>>>> - //
>>>> - // CET must be disabled if WP is disabled. Disable CET before clearing
>> CR0.WP.
>>>> - //
>>>> - DisableCet ();
>>>> - }
>>>> -
>>>> - Cr0.Bits.WP = 0;
>>>> - AsmWriteCr0 (Cr0.UintN);
>>>> - }
>>>> -
>>>> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>>>> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
>> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
>>>> - if (WpEnabled) {
>>>> - Cr0.UintN = AsmReadCr0 ();
>>>> - Cr0.Bits.WP = 1;
>>>> - AsmWriteCr0 (Cr0.UintN);
>>>> -
>>>> - if (CetEnabled) {
>>>> - //
>>>> - // re-enable CET.
>>>> - //
>>>> - EnableCet ();
>>>> - }
>>>> - }
>>>> + EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
>>>> }
>>>>
>>>> return TRUE;
>>>> @@ -1782,6 +1814,7 @@ SetPageTableAttributes (
>>>> VOID
>>>> )
>>>> {
>>>> + BOOLEAN WpEnabled;
>>>> BOOLEAN CetEnabled;
>>>>
>>>> if (!IfReadOnlyPageTableNeeded ()) { @@ -1794,15 +1827,7 @@
>>>> SetPageTableAttributes (
>>>> // Disable write protection, because we need mark page table to be write
>> protected.
>>>> // We need *write* page table memory, to mark itself to be *read only*.
>>>> //
>>>> - CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE :
>>>> FALSE;
>>>> - if (CetEnabled) {
>>>> - //
>>>> - // CET must be disabled if WP is disabled.
>>>> - //
>>>> - DisableCet ();
>>>> - }
>>>> -
>>>> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
>>>> + DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
>>>>
>>>> // Set memory used by page table as Read Only.
>>>> DEBUG ((DEBUG_INFO, "Start...\n")); @@ -1811,20 +1836,12 @@
>>>> SetPageTableAttributes (
>>>> //
>>>> // Enable write protection, after page table attribute updated.
>>>> //
>>>> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
>>>> + EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
>>>> mIsReadOnlyPageTable = TRUE;
>>>>
>>>> //
>>>> // Flush TLB after mark all page table pool as read only.
>>>> //
>>>> FlushTlbForAll ();
>>>> -
>>>> - if (CetEnabled) {
>>>> - //
>>>> - // re-enable CET.
>>>> - //
>>>> - EnableCet ();
>>>> - }
>>>> -
>>>> return;
>>>> }
>>>
>>>
>>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105329): https://edk2.groups.io/g/devel/message/105329
Mute This Topic: https://groups.io/mt/98922933/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2023 Red Hat, Inc.