From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f171.google.com (mail-pg1-f171.google.com [209.85.215.171]) by mx.groups.io with SMTP id smtpd.web11.411.1685069318843987374 for ; Thu, 25 May 2023 19:48:38 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20221208 header.b=GzYkwgHe; spf=pass (domain: gmail.com, ip: 209.85.215.171, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f171.google.com with SMTP id 41be03b00d2f7-5346d150972so233897a12.3 for ; Thu, 25 May 2023 19:48:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685069318; x=1687661318; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=VaboCcMYtSMo+9Z7fNRzxvwaUZ7I8+LA1PQcJ1PNqzw=; b=GzYkwgHeY4Ryp1JOhl7L7TjRciqO1PSA/xdMgR+xuZuL7xPMSg31tpx40klpwfzJnf N/gRrIVt0j9uBJXKJ0xdafOej/B/DGCkwu+BYITUfGPwUwgpFil6tLaR4NbHTosuJdJS UwHz03wCkgG/zt5V1cV2sqD3mH4yQIVqC25XdMw10PKjlEIMr5R1+7yozyt3pbeT6TKJ Zbz1ogfOHIOhWz/SPO+EatbIjjcLi876FYFfHJypmOCMFupw7YBmwuKGweeVQX5hFucl /4Nbso00bqRQfNFpsXcCNqO4P/Algori1715UGOvhkeRyJHJbq0EE6BIpMhv4LbNhlMG TxoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685069318; x=1687661318; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VaboCcMYtSMo+9Z7fNRzxvwaUZ7I8+LA1PQcJ1PNqzw=; b=OcIOCCHUV8SSdmIETMUURyOWoX0iNNAZ0TzLneLedYoqTd1xlzYetUAKO+TNQtFdQb keXc7livQBJsWtCR1L9BwSs/78Savq1CYObHX8Lf/dJn8WSo1FBhkMdmPmpTlQxAxtTu 5/gG5rkCB5edadYrR2ZuXhF+UvHsJzXvrfzsFVwWBsUk7mEpLcpRJOQMWLXj2rHF3xOG WJ0v4NJ0kA2yQjnE+BvcC1YPH5HVve1H1/WQZr6aKBGTwFOEIdDDeRzD61LDwleMt0zb 3w0RH35mrpAnsmywXvTuJ8sv8XSFidcN4EGHD2hdszwYoF617PZrZM+EFkNL5cYNxY3J WrHw== X-Gm-Message-State: AC+VfDzjnE6S/lJJdHgGiUwrKaQoNJ8wJu+TeDsKivxE/8jAfg4lRGLs mU9W7TK0yFDtL2bT49No3SI= X-Google-Smtp-Source: ACHHUZ7oJYOshCHoWc5ntpg2AItDKvInNbpFCkEBl7QdzqYCpyNDXiC2oU6FkGodXi8NlUSFrti+vg== X-Received: by 2002:a17:902:9308:b0:1ae:14d:8d0a with SMTP id bc8-20020a170902930800b001ae014d8d0amr799416plb.29.1685069317902; Thu, 25 May 2023 19:48:37 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:2444:9e53:f77f:bb92? ([2001:4898:80e8:8:a45e:9e53:f77f:bb92]) by smtp.gmail.com with ESMTPSA id ji19-20020a170903325300b001a804b16e38sm2074942plb.150.2023.05.25.19.48.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 25 May 2023 19:48:37 -0700 (PDT) Message-ID: <99afadcd-12f0-8b86-621f-fa74129e140d@gmail.com> Date: Thu, 25 May 2023 19:48:36 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP To: "Ni, Ray" , "devel@edk2.groups.io" , "Tan, Dun" Cc: "Dong, Eric" , "Kumar, Rahul R" , Gerd Hoffmann References: <20230516095932.1525-1-dun.tan@intel.com> <20230516095932.1525-8-dun.tan@intel.com> <88ebdba4-c595-2b4e-885e-a0fef4beea2f@gmail.com> From: "Kun Qin" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >> Sent: Thursday, May 25, 2023 2:39 AM >> To: devel@edk2.groups.io; Tan, Dun >> Cc: Dong, Eric ; Ni, Ray ; Kumar, >> Rahul R ; Gerd Hoffmann >> 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 >>> Sent: Saturday, May 20, 2023 10:00 AM >>> To: devel@edk2.groups.io; Tan, Dun >>> Cc: Dong, Eric ; Ni, Ray ; Kumar, >> Rahul R ; Gerd Hoffmann >>> 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 >>>> Cc: Eric Dong >>>> Cc: Ray Ni >>>> Cc: Rahul Kumar >>>> Cc: Gerd Hoffmann >>>> --- >>>> 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; >>>> } >>> >>> >>>