public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Kun Qin" <kuqin12@gmail.com>
To: devel@edk2.groups.io, dun.tan@intel.com
Cc: Eric Dong <eric.dong@intel.com>, Ray Ni <ray.ni@intel.com>,
	Rahul Kumar <rahul1.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
Date: Fri, 19 May 2023 19:00:06 -0700	[thread overview]
Message-ID: <88ebdba4-c595-2b4e-885e-a0fef4beea2f@gmail.com> (raw)
In-Reply-To: <20230516095932.1525-8-dun.tan@intel.com>

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;
>   }

  reply	other threads:[~2023-05-20  2:00 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-16  9:59 [Patch V4 00/15] Use CpuPageTableLib to create and update smm page table duntan
2023-05-16  9:59 ` [Patch V4 01/15] OvmfPkg: Add CpuPageTableLib required by PiSmmCpuDxe duntan
2023-05-16  9:59 ` [Patch V4 02/15] UefiPayloadPkg: " duntan
2023-05-16 10:01   ` Guo, Gua
2023-05-16  9:59 ` [Patch V4 03/15] OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry duntan
2023-05-16  9:59 ` [Patch V4 04/15] MdeModulePkg: Remove RO and NX protection when unset guard page duntan
2023-05-16 19:04   ` [edk2-devel] " Kun Qin
2023-05-17 10:16     ` duntan
2023-05-16  9:59 ` [Patch V4 05/15] UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute duntan
2023-06-01  1:09   ` Ni, Ray
2023-05-16  9:59 ` [Patch V4 06/15] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid setting non-present range to RO/NX duntan
2023-05-16  9:59 ` [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP duntan
2023-05-20  2:00   ` Kun Qin [this message]
2023-05-23  9:14     ` [edk2-devel] " duntan
2023-05-24 18:39       ` Kun Qin
2023-05-25  0:46         ` Ni, Ray
2023-05-26  2:48           ` Kun Qin
2023-06-02  3:09   ` Ni, Ray
2023-05-16  9:59 ` [Patch V4 08/15] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR0.WP before modify page table duntan
2023-06-02  3:12   ` [edk2-devel] " Ni, Ray
2023-05-16  9:59 ` [Patch V4 09/15] UefiCpuPkg: Extern mSmmShadowStackSize in PiSmmCpuDxeSmm.h duntan
2023-06-02  3:16   ` [edk2-devel] " Ni, Ray
2023-06-02  3:36     ` duntan
2023-05-16  9:59 ` [Patch V4 10/15] UefiCpuPkg: Add GenSmmPageTable() to create smm page table duntan
2023-06-02  3:23   ` [edk2-devel] " Ni, Ray
2023-06-02  3:36     ` duntan
2023-06-02  3:46       ` duntan
2023-06-02  5:08         ` Ni, Ray
2023-06-02  7:33           ` duntan
2023-05-16  9:59 ` [Patch V4 11/15] UefiCpuPkg: Use GenSmmPageTable() to create Smm S3 " duntan
2023-06-02  3:31   ` [edk2-devel] " Ni, Ray
2023-06-02  3:37     ` duntan
2023-05-16  9:59 ` [Patch V4 12/15] UefiCpuPkg: Sort mSmmCpuSmramRanges in FindSmramInfo duntan
2023-06-02  3:33   ` [edk2-devel] " Ni, Ray
2023-06-02  3:43     ` duntan
2023-05-16  9:59 ` [Patch V4 13/15] UefiCpuPkg: Sort mProtectionMemRange when ReadyToLock duntan
2023-06-02  3:34   ` Ni, Ray
2023-06-02  3:35     ` Ni, Ray
2023-06-02  3:55       ` duntan
2023-05-16  9:59 ` [Patch V4 14/15] UefiCpuPkg: Refinement to smm runtime InitPaging() code duntan
2023-06-02  3:54   ` [edk2-devel] " Ni, Ray
2023-06-02  3:59     ` duntan
2023-05-16  9:59 ` [Patch V4 15/15] UefiCpuPkg/PiSmmCpuDxeSmm: Remove unnecessary function duntan
2023-06-02  3:55   ` Ni, Ray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=88ebdba4-c595-2b4e-885e-a0fef4beea2f@gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox