[edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
Kun Qin
kuqin12 at gmail.com
Fri May 26 02:48:36 UTC 2023
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 at gmail.com>
>> Sent: Thursday, May 25, 2023 2:39 AM
>> To: devel at edk2.groups.io; Tan, Dun <dun.tan at intel.com>
>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar,
>> Rahul R <rahul.r.kumar at intel.com>; Gerd Hoffmann <kraxel at 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 at gmail.com>
>>> Sent: Saturday, May 20, 2023 10:00 AM
>>> To: devel at edk2.groups.io; Tan, Dun <dun.tan at intel.com>
>>> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar,
>> Rahul R <rahul.r.kumar at intel.com>; Gerd Hoffmann <kraxel at 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 at intel.com>
>>>> Cc: Eric Dong <eric.dong at intel.com>
>>>> Cc: Ray Ni <ray.ni at intel.com>
>>>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
>>>> Cc: Gerd Hoffmann <kraxel at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list