Remove mInternalCr3 in PiSmmCpuDxeSmm

[edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag

Posted by duntan 3 years, 6 months ago

Add a new IsShadowStack flag to identify whether current memory is
shadow stack. The dirty bit in page table entry for this memory will
be set if IsShadowStack is TRUE, instead of depending on mInternalCr3.

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 1f7cc15727..b369c0c435 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE  mPageAttributeTable[] = {
 };
 
 UINTN  mInternalCr3;
+UINTN  IsShadowStack = FALSE;
 
 /**
   Set the internal page table base address.
@@ -249,7 +250,7 @@ ConvertPageEntryAttribute (
   if ((Attributes & EFI_MEMORY_RO) != 0) {
     if (IsSet) {
       NewPageEntry &= ~(UINT64)IA32_PG_RW;
-      if (mInternalCr3 != 0) {
+      if (IsShadowStack) {
         // Environment setup
         // ReadOnly page need set Dirty bit for shadow stack
         NewPageEntry |= IA32_PG_D;
@@ -734,10 +735,11 @@ SetShadowStack (
   EFI_STATUS  Status;
 
   SetPageTableBase (Cr3);
-
-  Status = SmmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
+  IsShadowStack = TRUE;
+  Status        = SmmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
 
   SetPageTableBase (0);
+  IsShadowStack = FALSE;
 
   return Status;
 }
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92268): https://edk2.groups.io/g/devel/message/92268
Mute This Topic: https://groups.io/mt/92928945/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag

Posted by Ni, Ray 3 years, 6 months ago

Dun,
Can you please update the commit message to explain it's a code refactoring and
doesn't change any functionality? Also explain why such refactoring is needed.

IsShadowStack: the name doesn't follow EDKII coding style.
You need to use "mIsShadowStack".

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Wednesday, August 10, 2022 9:46 AM
> 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: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a
> new IsShadowStack flag
> 
> Add a new IsShadowStack flag to identify whether current memory is
> shadow stack. The dirty bit in page table entry for this memory will
> be set if IsShadowStack is TRUE, instead of depending on mInternalCr3.
> 
> 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 | 8
> +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 1f7cc15727..b369c0c435 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE  mPageAttributeTable[] = {
>  };
> 
>  UINTN  mInternalCr3;
> +UINTN  IsShadowStack = FALSE;
> 
>  /**
>    Set the internal page table base address.
> @@ -249,7 +250,7 @@ ConvertPageEntryAttribute (
>    if ((Attributes & EFI_MEMORY_RO) != 0) {
>      if (IsSet) {
>        NewPageEntry &= ~(UINT64)IA32_PG_RW;
> -      if (mInternalCr3 != 0) {
> +      if (IsShadowStack) {
>          // Environment setup
>          // ReadOnly page need set Dirty bit for shadow stack
>          NewPageEntry |= IA32_PG_D;
> @@ -734,10 +735,11 @@ SetShadowStack (
>    EFI_STATUS  Status;
> 
>    SetPageTableBase (Cr3);
> -
> -  Status = SmmSetMemoryAttributes (BaseAddress, Length,
> EFI_MEMORY_RO);
> +  IsShadowStack = TRUE;
> +  Status        = SmmSetMemoryAttributes (BaseAddress, Length,
> EFI_MEMORY_RO);
> 
>    SetPageTableBase (0);
> +  IsShadowStack = FALSE;
> 
>    return Status;
>  }
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92273): https://edk2.groups.io/g/devel/message/92273
Mute This Topic: https://groups.io/mt/92928945/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag

Posted by duntan 3 years, 6 months ago

Ok, I'll send the V2 patch set soon.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, August 10, 2022 11:52 AM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag

Dun,
Can you please update the commit message to explain it's a code refactoring and doesn't change any functionality? Also explain why such refactoring is needed.

IsShadowStack: the name doesn't follow EDKII coding style.
You need to use "mIsShadowStack".

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Wednesday, August 10, 2022 9:46 AM
> 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: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new 
> IsShadowStack flag
> 
> Add a new IsShadowStack flag to identify whether current memory is 
> shadow stack. The dirty bit in page table entry for this memory will 
> be set if IsShadowStack is TRUE, instead of depending on mInternalCr3.
> 
> 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 | 8
> +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 1f7cc15727..b369c0c435 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE  mPageAttributeTable[] = {  };
> 
>  UINTN  mInternalCr3;
> +UINTN  IsShadowStack = FALSE;
> 
>  /**
>    Set the internal page table base address.
> @@ -249,7 +250,7 @@ ConvertPageEntryAttribute (
>    if ((Attributes & EFI_MEMORY_RO) != 0) {
>      if (IsSet) {
>        NewPageEntry &= ~(UINT64)IA32_PG_RW;
> -      if (mInternalCr3 != 0) {
> +      if (IsShadowStack) {
>          // Environment setup
>          // ReadOnly page need set Dirty bit for shadow stack
>          NewPageEntry |= IA32_PG_D;
> @@ -734,10 +735,11 @@ SetShadowStack (
>    EFI_STATUS  Status;
> 
>    SetPageTableBase (Cr3);
> -
> -  Status = SmmSetMemoryAttributes (BaseAddress, Length, 
> EFI_MEMORY_RO);
> +  IsShadowStack = TRUE;
> +  Status        = SmmSetMemoryAttributes (BaseAddress, Length,
> EFI_MEMORY_RO);
> 
>    SetPageTableBase (0);
> +  IsShadowStack = FALSE;
> 
>    return Status;
>  }
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92275): https://edk2.groups.io/g/devel/message/92275
Mute This Topic: https://groups.io/mt/92928945/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-