UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

Posted by Ni, Ray 4 years, 9 months ago

Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
* UefiCpuPkg/SmmCpu: Block access-out only when static paging is used

updated page fault handler to treat SMM access-out as allowed
address when static paging is not used.

But that commit is not complete because the page table is still
updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM
code accesses non-SMRAM memory, page fault is still generated.

This patch skips to update page table for non-SMRAM memory and
page table itself.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 2f7d777ee7..f75e75f55c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -1103,6 +1103,9 @@ FindSmramInfo (
   *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
   *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
 
+  //
+  // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges
+  //
   do {
     Found = FALSE;
     for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) {
@@ -1414,14 +1417,20 @@ PerformRemainingTasks (
     SetMemMapAttributes ();
 
     //
-    // For outside SMRAM, we only map SMM communication buffer or MMIO.
+    // Do not protect memory outside SMRAM when SMM static page table is not enabled.
     //
-    SetUefiMemMapAttributes ();
+    if (mCpuSmmStaticPageTable) {
 
-    //
-    // Set page table itself to be read-only
-    //
-    SetPageTableAttributes ();
+      //
+      // For outside SMRAM, we only map SMM communication buffer or MMIO.
+      //
+      SetUefiMemMapAttributes ();
+
+      //
+      // Set page table itself to be read-only
+      //
+      SetPageTableAttributes ();
+    }
 
     //
     // Configure SMM Code Access Check feature if available.
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43920): https://edk2.groups.io/g/devel/message/43920
Mute This Topic: https://groups.io/mt/32512605/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

Posted by Laszlo Ersek 4 years, 9 months ago

On 07/18/19 08:58, Ni, Ray wrote:
> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
> 
> updated page fault handler to treat SMM access-out as allowed
> address when static paging is not used.
> 
> But that commit is not complete because the page table is still
> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM
> code accesses non-SMRAM memory, page fault is still generated.
> 
> This patch skips to update page table for non-SMRAM memory and
> page table itself.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 2f7d777ee7..f75e75f55c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1103,6 +1103,9 @@ FindSmramInfo (
>    *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
>    *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
>  
> +  //
> +  // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges
> +  //
>    do {
>      Found = FALSE;
>      for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) {
> @@ -1414,14 +1417,20 @@ PerformRemainingTasks (
>      SetMemMapAttributes ();
>  
>      //
> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +    // Do not protect memory outside SMRAM when SMM static page table is not enabled.
>      //
> -    SetUefiMemMapAttributes ();
> +    if (mCpuSmmStaticPageTable) {
>  
> -    //
> -    // Set page table itself to be read-only
> -    //
> -    SetPageTableAttributes ();
> +      //
> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +      //
> +      SetUefiMemMapAttributes ();
> +
> +      //
> +      // Set page table itself to be read-only
> +      //
> +      SetPageTableAttributes ();
> +    }
>  
>      //
>      // Configure SMM Code Access Check feature if available.
> 

This patch is a no-op for platforms with PcdCpuSmmStaticPageTable=TRUE
(which is both the UefiCpuPkg default and what OVMF uses), so, from my side:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43947): https://edk2.groups.io/g/devel/message/43947
Mute This Topic: https://groups.io/mt/32512605/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

Posted by Dong, Eric 4 years, 9 months ago

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> Ray
> Sent: Thursday, July 18, 2019 2:58 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-
> out when static paging is OFF
> 
> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
> 
> updated page fault handler to treat SMM access-out as allowed address
> when static paging is not used.
> 
> But that commit is not complete because the page table is still updated in
> SetUefiMemMapAttributes() for non-SMRAM memory. When SMM code
> accesses non-SMRAM memory, page fault is still generated.
> 
> This patch skips to update page table for non-SMRAM memory and page
> table itself.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21
> +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 2f7d777ee7..f75e75f55c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1103,6 +1103,9 @@ FindSmramInfo (
>    *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
>    *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
> 
> +  //
> +  // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges  //
>    do {
>      Found = FALSE;
>      for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) { @@ -
> 1414,14 +1417,20 @@ PerformRemainingTasks (
>      SetMemMapAttributes ();
> 
>      //
> -    // For outside SMRAM, we only map SMM communication buffer or
> MMIO.
> +    // Do not protect memory outside SMRAM when SMM static page table is
> not enabled.
>      //
> -    SetUefiMemMapAttributes ();
> +    if (mCpuSmmStaticPageTable) {
> 
> -    //
> -    // Set page table itself to be read-only
> -    //
> -    SetPageTableAttributes ();
> +      //
> +      // For outside SMRAM, we only map SMM communication buffer or
> MMIO.
> +      //
> +      SetUefiMemMapAttributes ();
> +
> +      //
> +      // Set page table itself to be read-only
> +      //
> +      SetPageTableAttributes ();
> +    }
> 
>      //
>      // Configure SMM Code Access Check feature if available.
> --
> 2.21.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44022): https://edk2.groups.io/g/devel/message/44022
Mute This Topic: https://groups.io/mt/32512605/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

Posted by Laszlo Ersek 4 years, 8 months ago

Ray,

On 07/18/19 08:58, Ni, Ray wrote:
> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
>
> updated page fault handler to treat SMM access-out as allowed
> address when static paging is not used.
>
> But that commit is not complete because the page table is still
> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM
> code accesses non-SMRAM memory, page fault is still generated.
>
> This patch skips to update page table for non-SMRAM memory and
> page table itself.
>
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 2f7d777ee7..f75e75f55c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1103,6 +1103,9 @@ FindSmramInfo (
>    *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
>    *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
>
> +  //
> +  // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges
> +  //
>    do {
>      Found = FALSE;
>      for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) {
> @@ -1414,14 +1417,20 @@ PerformRemainingTasks (
>      SetMemMapAttributes ();
>
>      //
> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +    // Do not protect memory outside SMRAM when SMM static page table is not enabled.
>      //
> -    SetUefiMemMapAttributes ();
> +    if (mCpuSmmStaticPageTable) {
>
> -    //
> -    // Set page table itself to be read-only
> -    //
> -    SetPageTableAttributes ();
> +      //
> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +      //
> +      SetUefiMemMapAttributes ();
> +
> +      //
> +      // Set page table itself to be read-only
> +      //
> +      SetPageTableAttributes ();
> +    }
>
>      //
>      // Configure SMM Code Access Check feature if available.
>

Commit 30f6148546c7 causes a build failure, when building for IA32:

> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function 'PerformRemainingTasks':
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: 'mCpuSmmStaticPageTable' undeclared (first use in this function)
>      if (mCpuSmmStaticPageTable) {

"mCpuSmmStaticPageTable" is an X64-only variable. It is defined in
"X64/PageTbl.c", which is not linked into the IA32 binary. We must not
reference the variable in such code that is linked into both IA32 and
X64 builds, such as "PiSmmCpuDxeSmm.c".

We have encountered the same challenge at least once in the past:

- https://bugzilla.tianocore.org/show_bug.cgi?id=1593
- commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand
  paging in SMM", 2019-04-04)

The right approach is to declare a new function in "PiSmmCpuDxeSmm.h",
and to provide two definitions for the function, one in
"Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32
implementation should return a constant value. The X64 implementation
should return "mCpuSmmStaticPageTable". (In the example named above, the
functions were SaveCr2() and RestoreCr2().)

--*--

I'm going to revert commit 30f6148546c7 immediately, because it breaks
the build, and because catching this issue in advance would have been
trivial, if you had attempted to build for IA32. (To be sure, the
reviewers on this patch are not responsible; reviewers are welcome, but
not required, to catch such errors that the compiler/linker is supposed
to catch.) With this build breakage, I wouldn't be able to test Eric's
series

  [edk2-devel] [Patch v3 0/6] UefiCpuPkg: Enable Edkii Mp Services2 Ppi

and I'd like to proceed with that.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44451): https://edk2.groups.io/g/devel/message/44451
Mute This Topic: https://groups.io/mt/32512605/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

Posted by Laszlo Ersek 4 years, 8 months ago

On 07/26/19 11:58, Laszlo Ersek wrote:
> Ray,
> 
> On 07/18/19 08:58, Ni, Ray wrote:
>> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
>> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
>>
>> updated page fault handler to treat SMM access-out as allowed
>> address when static paging is not used.
>>
>> But that commit is not complete because the page table is still
>> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM
>> code accesses non-SMRAM memory, page fault is still generated.
>>
>> This patch skips to update page table for non-SMRAM memory and
>> page table itself.
>>
>> Signed-off-by: Ray Ni <ray.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index 2f7d777ee7..f75e75f55c 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -1103,6 +1103,9 @@ FindSmramInfo (
>>    *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
>>    *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
>>
>> +  //
>> +  // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges
>> +  //
>>    do {
>>      Found = FALSE;
>>      for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) {
>> @@ -1414,14 +1417,20 @@ PerformRemainingTasks (
>>      SetMemMapAttributes ();
>>
>>      //
>> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> +    // Do not protect memory outside SMRAM when SMM static page table is not enabled.
>>      //
>> -    SetUefiMemMapAttributes ();
>> +    if (mCpuSmmStaticPageTable) {
>>
>> -    //
>> -    // Set page table itself to be read-only
>> -    //
>> -    SetPageTableAttributes ();
>> +      //
>> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> +      //
>> +      SetUefiMemMapAttributes ();
>> +
>> +      //
>> +      // Set page table itself to be read-only
>> +      //
>> +      SetPageTableAttributes ();
>> +    }
>>
>>      //
>>      // Configure SMM Code Access Check feature if available.
>>
> 
> Commit 30f6148546c7 causes a build failure, when building for IA32:
> 
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function 'PerformRemainingTasks':
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: 'mCpuSmmStaticPageTable' undeclared (first use in this function)
>>      if (mCpuSmmStaticPageTable) {
> 
> "mCpuSmmStaticPageTable" is an X64-only variable. It is defined in
> "X64/PageTbl.c", which is not linked into the IA32 binary. We must not
> reference the variable in such code that is linked into both IA32 and
> X64 builds, such as "PiSmmCpuDxeSmm.c".
> 
> We have encountered the same challenge at least once in the past:
> 
> - https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> - commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand
>   paging in SMM", 2019-04-04)
> 
> The right approach is to declare a new function in "PiSmmCpuDxeSmm.h",
> and to provide two definitions for the function, one in
> "Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32
> implementation should return a constant value. The X64 implementation
> should return "mCpuSmmStaticPageTable". (In the example named above, the
> functions were SaveCr2() and RestoreCr2().)
> 
> --*--
> 
> I'm going to revert commit 30f6148546c7 immediately, because it breaks
> the build, and because catching this issue in advance would have been
> trivial, if you had attempted to build for IA32. (To be sure, the
> reviewers on this patch are not responsible; reviewers are welcome, but
> not required, to catch such errors that the compiler/linker is supposed
> to catch.) With this build breakage, I wouldn't be able to test Eric's
> series
> 
>   [edk2-devel] [Patch v3 0/6] UefiCpuPkg: Enable Edkii Mp Services2 Ppi
> 
> and I'd like to proceed with that.

The revert has commit hash d47b85a621ad.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44452): https://edk2.groups.io/g/devel/message/44452
Mute This Topic: https://groups.io/mt/32512605/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-