From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f182.google.com (mail-pg1-f182.google.com [209.85.215.182]) by mx.groups.io with SMTP id smtpd.web10.2126.1684548008717577338 for ; Fri, 19 May 2023 19:00:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20221208 header.b=EPPZVvjE; spf=pass (domain: gmail.com, ip: 209.85.215.182, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f182.google.com with SMTP id 41be03b00d2f7-51f6461af24so2681099a12.2 for ; Fri, 19 May 2023 19:00:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684548008; x=1687140008; 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=RrteXdI89IjSkUjM4D8PSsYUEdEh3itmQ7uI+QGiaiA=; b=EPPZVvjEGr6aqhkCWSpp73MrIZ8PMC/O9yJowHnV+GobV19uydPOheYh64C4Q+57k1 iRaEA5XAQYibkPi/zYlUu0XSLfgPrix6NmSKX8Lnwb8vYyIGrqOSLZHrzFBVD7Qz6zqL Yda842J/N/gG2VGNOEDmZfMqCvrryHThReYVKajnsp4kDBxBbZ8ci2iiQCvUM1zvoyay 3ZGTixnH+RDYcD+GwH4xVky9sQ3LOL+NzmCGjd0bBMMuJXYhRXr3GndjRtBlWUMl33vJ 1WzWh7J0deLfjf55aQu6fuDHcLewIeABiPZv9athIj54/uxf8lmKQT7xnm38zpQlWJV4 4UgA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684548008; x=1687140008; 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=RrteXdI89IjSkUjM4D8PSsYUEdEh3itmQ7uI+QGiaiA=; b=VGzQdHHEYyFgbla1nHNSNSgeXYKKGqpVGlclWo3nN+rLqLZ8IP8BYqzkbo8xBSfqOV T3tygGM4CK8dE7x5CcVvamjBvycUWZZLmx+D6AqRy66/QYyLep50eO+2voc2fIqCwJNJ SPjdhhVmI8WbgcNy+zGRU9VfCXt7uxR5SKzxrNPOfZFHpWiP9+iDMSmBwmqiTdZMRYsQ zosmOFEPn3UA5SpOYL7EnKV6JV5MRzdLdjlKwChOzFrfzfz4xuxLABJb3R8hSnG98CVa 45eJu8TPZwtbtwqJWJFvjiw2erL7DmX7h3CC1JDZwPSEurqCAyMekaz7FjYC4BWjXAg5 aJtQ== X-Gm-Message-State: AC+VfDziOvr/rDLed036LITnmN2H95mQrQJaHK4p2FOO8/d5/9yP0hFA 0WIH5os+7NZSCkIBEX1oe83RbW2Q9TU= X-Google-Smtp-Source: ACHHUZ5zle7jBhQlxRctM5gm73L2EVGPn5q9feSX6mOuL+0Hr2tw2RShVrIhB4OOsfsjTEv7TuqWQw== X-Received: by 2002:a17:902:e889:b0:1ac:753a:7933 with SMTP id w9-20020a170902e88900b001ac753a7933mr4972812plg.39.1684548007770; Fri, 19 May 2023 19:00:07 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:cc20:9afe:1f43:2278? ([2001:4898:80e8:f:4c34:9afe:1f43:2278]) by smtp.gmail.com with ESMTPSA id d6-20020a170902c18600b001addf547a6esm301667pld.17.2023.05.19.19.00.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 19 May 2023 19:00:07 -0700 (PDT) Message-ID: <88ebdba4-c595-2b4e-885e-a0fef4beea2f@gmail.com> Date: Fri, 19 May 2023 19:00:06 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP To: devel@edk2.groups.io, dun.tan@intel.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20230516095932.1525-1-dun.tan@intel.com> <20230516095932.1525-8-dun.tan@intel.com> From: "Kun Qin" In-Reply-To: <20230516095932.1525-8-dun.tan@intel.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > 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; > }