* [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code
@ 2023-01-03 5:56 duntan
2023-01-03 6:29 ` Ni, Ray
2023-01-03 6:34 ` Ni, Ray
0 siblings, 2 replies; 3+ messages in thread
From: duntan @ 2023-01-03 5:56 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar
When setting new page table pool to RO, only disable/enable WP when
Cr0.WP has been set to 1 to fix potential PF caused by b822be1a20
(UefiCpuPkg/PiSmmCpuDxeSmm: Introduce page table pool mechanism).
With previous code, if someone want to modify the page table and
Cr0.WP has been cleared before modify page table, Cr0.WP may be set
to 1 again since new pool may be generated during this process
Then PF fault may happens.
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>
---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 4bb23f6920..bab7f1887b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -67,8 +67,10 @@ InitializePageTablePool (
IN UINTN PoolPages
)
{
- VOID *Buffer;
- BOOLEAN CetEnabled;
+ VOID *Buffer;
+ BOOLEAN CetEnabled;
+ BOOLEAN WpEnabled;
+ IA32_CR0 Cr0;
//
// Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
@@ -106,21 +108,35 @@ InitializePageTablePool (
//
if (mIsReadOnlyPageTable) {
CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
- if (CetEnabled) {
+ 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 ();
+ }
+
//
- // CET must be disabled if WP is disabled.
+ // Only disable/enable WP when Cr0.Bits.WP has been set to 1.
//
- DisableCet ();
+ Cr0.Bits.WP = 0;
+ AsmWriteCr0 (Cr0.UintN);
}
- AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
- AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
- if (CetEnabled) {
- //
- // re-enable CET.
- //
- EnableCet ();
+ if (WpEnabled) {
+ Cr0.UintN = AsmReadCr0 ();
+ Cr0.Bits.WP = 1;
+ AsmWriteCr0 (Cr0.UintN);
+
+ if (CetEnabled) {
+ //
+ // re-enable CET.
+ //
+ EnableCet ();
+ }
}
}
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code
2023-01-03 5:56 [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code duntan
@ 2023-01-03 6:29 ` Ni, Ray
2023-01-03 6:34 ` Ni, Ray
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ray @ 2023-01-03 6:29 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R
Since it fixes a critical boot hang issue, I created PR for push @ https://github.com/tianocore/edk2/pull/3845
I removed following confusing comments.
+ // Only disable/enable WP when Cr0.Bits.WP has been set to 1.
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, January 3, 2023 1:56 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>
> Subject: [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code
>
> When setting new page table pool to RO, only disable/enable WP when
> Cr0.WP has been set to 1 to fix potential PF caused by b822be1a20
> (UefiCpuPkg/PiSmmCpuDxeSmm: Introduce page table pool mechanism).
> With previous code, if someone want to modify the page table and
> Cr0.WP has been cleared before modify page table, Cr0.WP may be set
> to 1 again since new pool may be generated during this process
> Then PF fault may happens.
>
> 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>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 40 ++++++++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 4bb23f6920..bab7f1887b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -67,8 +67,10 @@ InitializePageTablePool (
> IN UINTN PoolPages
> )
> {
> - VOID *Buffer;
> - BOOLEAN CetEnabled;
> + VOID *Buffer;
> + BOOLEAN CetEnabled;
> + BOOLEAN WpEnabled;
> + IA32_CR0 Cr0;
>
> //
> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
> @@ -106,21 +108,35 @@ InitializePageTablePool (
> //
> if (mIsReadOnlyPageTable) {
> CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - if (CetEnabled) {
> + 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 ();
> + }
> +
> //
> - // CET must be disabled if WP is disabled.
> + // Only disable/enable WP when Cr0.Bits.WP has been set to 1.
> //
> - DisableCet ();
> + Cr0.Bits.WP = 0;
> + AsmWriteCr0 (Cr0.UintN);
> }
>
> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages),
> EFI_MEMORY_RO);
> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> + if (WpEnabled) {
> + Cr0.UintN = AsmReadCr0 ();
> + Cr0.Bits.WP = 1;
> + AsmWriteCr0 (Cr0.UintN);
> +
> + if (CetEnabled) {
> + //
> + // re-enable CET.
> + //
> + EnableCet ();
> + }
> }
> }
>
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code
2023-01-03 5:56 [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code duntan
2023-01-03 6:29 ` Ni, Ray
@ 2023-01-03 6:34 ` Ni, Ray
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ray @ 2023-01-03 6:34 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, January 3, 2023 1:56 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>
> Subject: [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code
>
> When setting new page table pool to RO, only disable/enable WP when
> Cr0.WP has been set to 1 to fix potential PF caused by b822be1a20
> (UefiCpuPkg/PiSmmCpuDxeSmm: Introduce page table pool mechanism).
> With previous code, if someone want to modify the page table and
> Cr0.WP has been cleared before modify page table, Cr0.WP may be set
> to 1 again since new pool may be generated during this process
> Then PF fault may happens.
>
> 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>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 40 ++++++++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 4bb23f6920..bab7f1887b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -67,8 +67,10 @@ InitializePageTablePool (
> IN UINTN PoolPages
> )
> {
> - VOID *Buffer;
> - BOOLEAN CetEnabled;
> + VOID *Buffer;
> + BOOLEAN CetEnabled;
> + BOOLEAN WpEnabled;
> + IA32_CR0 Cr0;
>
> //
> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
> @@ -106,21 +108,35 @@ InitializePageTablePool (
> //
> if (mIsReadOnlyPageTable) {
> CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - if (CetEnabled) {
> + 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 ();
> + }
> +
> //
> - // CET must be disabled if WP is disabled.
> + // Only disable/enable WP when Cr0.Bits.WP has been set to 1.
> //
> - DisableCet ();
> + Cr0.Bits.WP = 0;
> + AsmWriteCr0 (Cr0.UintN);
> }
>
> - AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages),
> EFI_MEMORY_RO);
> - AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> + if (WpEnabled) {
> + Cr0.UintN = AsmReadCr0 ();
> + Cr0.Bits.WP = 1;
> + AsmWriteCr0 (Cr0.UintN);
> +
> + if (CetEnabled) {
> + //
> + // re-enable CET.
> + //
> + EnableCet ();
> + }
> }
> }
>
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-03 6:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-03 5:56 [Patch V4] UefiCpuPkg/PiSmmCpuDxeSmm:Fix PF issue caused by smm page table code duntan
2023-01-03 6:29 ` Ni, Ray
2023-01-03 6:34 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox