Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP

["Kun Qin" <kuqin12@gmail.com>]

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