[PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[Ruiyu Ni]

From: Jiewen Yao <jiewen.yao@intel.com>

Today's implementation blocks SMM read-out no matter static paging
is enabled or not. But certain platform may need to read non-SMM
content from SMM code. These platforms don't have a way to disable
the read-out blocking.

The patch updates the policy to only block SMM read-out when static
paging is enabled. So that the static paging can be disabled for
those platforms that want SMM read-out.

Setting PcdCpuSmmStaticPageTable to FALSE can disable the static
paging.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 5bb7d57238..117502dafa 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -1,7 +1,7 @@
 /** @file
 Page Fault (#PF) handler for X64 processors
 
-Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
 This program and the accompanying materials
@@ -890,7 +890,7 @@ SmiPFHandler (
       CpuDeadLoop ();
     }
 
-    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
+    if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFAddress)) {
       DumpCpuContext (InterruptType, SystemContext);
       DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress));
       DEBUG_CODE (
-- 
2.16.1.windows.1

Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[Laszlo Ersek]

On 11/06/18 03:59, Ruiyu Ni wrote:
> From: Jiewen Yao <jiewen.yao@intel.com>
> 
> Today's implementation blocks SMM read-out no matter static paging
> is enabled or not. But certain platform may need to read non-SMM
> content from SMM code. These platforms don't have a way to disable
> the read-out blocking.
> 
> The patch updates the policy to only block SMM read-out when static
> paging is enabled. So that the static paging can be disabled for
> those platforms that want SMM read-out.
> 
> Setting PcdCpuSmmStaticPageTable to FALSE can disable the static
> paging.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 5bb7d57238..117502dafa 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Page Fault (#PF) handler for X64 processors
>  
> -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  
>  This program and the accompanying materials
> @@ -890,7 +890,7 @@ SmiPFHandler (
>        CpuDeadLoop ();
>      }
>  
> -    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
> +    if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFAddress)) {
>        DumpCpuContext (InterruptType, SystemContext);
>        DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress));
>        DEBUG_CODE (
> 

OVMF inherits the default TRUE value for PcdCpuSmmStaticPageTable, from
"UefiCpuPkg.dec", and that's intentional. Therefore this patch should be
a no-op from OVMF's perspective.

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

More generally, is the use of PcdCpuSmmStaticPageTable for controlling
this kind of read-out just a convenience / simplification (in which case
I don't think it's great!), or are these topics inherently connected
somehow?

I remember that Jiewen said earlier that with "static paging" enabled
(i.e., building the page tables used in SMM all in advance), we provide
more page protection.

Also, I see that PcdCpuSmmProfileEnable can only be enabled with
PcdCpuSmmStaticPageTable set to FALSE.

So it seems that with PcdCpuSmmStaticPageTable set to TRUE, our page
fault handling in SMM is generally strict(er). This patch looks
consistent with that, but it would be nice if the commit message spelled
out why *exactly* it makes sense to use PcdCpuSmmStaticPageTable for
this new purpose as well.

(I hope my question makes sense. :) )

Thanks
Laszlo

Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[Yao, Jiewen]

Good suggestion Laszlo. 
Current static paging will force:
1) only valid smm comm buffer is present. The OS memory is not present.
2) non smram is NX (no matter static or dynamic paging)
3) code region in Smm is RO (if pe image is page aligned)
4) data region in Smm is NX (if pe image is page aligned)

thank you!
Yao, Jiewen


> 在 2018年11月7日，上午12:13，Laszlo Ersek <lersek@redhat.com> 写道：
> 
>> On 11/06/18 03:59, Ruiyu Ni wrote:
>> From: Jiewen Yao <jiewen.yao@intel.com>
>> 
>> Today's implementation blocks SMM read-out no matter static paging
>> is enabled or not. But certain platform may need to read non-SMM
>> content from SMM code. These platforms don't have a way to disable
>> the read-out blocking.
>> 
>> The patch updates the policy to only block SMM read-out when static
>> paging is enabled. So that the static paging can be disabled for
>> those platforms that want SMM read-out.
>> 
>> Setting PcdCpuSmmStaticPageTable to FALSE can disable the static
>> paging.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> ---
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> index 5bb7d57238..117502dafa 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> @@ -1,7 +1,7 @@
>> /** @file
>> Page Fault (#PF) handler for X64 processors
>> 
>> -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>> Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>> 
>> This program and the accompanying materials
>> @@ -890,7 +890,7 @@ SmiPFHandler (
>>       CpuDeadLoop ();
>>     }
>> 
>> -    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
>> +    if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFAddress)) {
>>       DumpCpuContext (InterruptType, SystemContext);
>>       DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress));
>>       DEBUG_CODE (
>> 
> 
> OVMF inherits the default TRUE value for PcdCpuSmmStaticPageTable, from
> "UefiCpuPkg.dec", and that's intentional. Therefore this patch should be
> a no-op from OVMF's perspective.
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> More generally, is the use of PcdCpuSmmStaticPageTable for controlling
> this kind of read-out just a convenience / simplification (in which case
> I don't think it's great!), or are these topics inherently connected
> somehow?
> 
> I remember that Jiewen said earlier that with "static paging" enabled
> (i.e., building the page tables used in SMM all in advance), we provide
> more page protection.
> 
> Also, I see that PcdCpuSmmProfileEnable can only be enabled with
> PcdCpuSmmStaticPageTable set to FALSE.
> 
> So it seems that with PcdCpuSmmStaticPageTable set to TRUE, our page
> fault handling in SMM is generally strict(er). This patch looks
> consistent with that, but it would be nice if the commit message spelled
> out why *exactly* it makes sense to use PcdCpuSmmStaticPageTable for
> this new purpose as well.
> 
> (I hope my question makes sense. :) )
> 
> Thanks
> Laszlo

Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[Yao, Jiewen]

If Static paging is not used, then the SMM uses dynamic paging.

Here dynamic paging means the page fault request on-demand.

2 special examples here:
1) above 4G MMIO. By default, SMM only setup paging table for 4G memory. If MMIO above 4G, then SMM need use #PF handler to grant MMIO access.
2) server RAS. By default, SMM only setup paging table for SMM communication buffer. For server RAS, memory hotplug may request direct OS memory access. If so, we also rely on #PF handler to grant OS access.

This patch fixed the second issue in the second case.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, November 7, 2018 6:45 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric
> <eric.dong@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when
> static paging is used
> 
> Good suggestion Laszlo.
> Current static paging will force:
> 1) only valid smm comm buffer is present. The OS memory is not present.
> 2) non smram is NX (no matter static or dynamic paging)
> 3) code region in Smm is RO (if pe image is page aligned)
> 4) data region in Smm is NX (if pe image is page aligned)
> 
> thank you!
> Yao, Jiewen
> 
> 
> > 在 2018年11月7日，上午12:13，Laszlo Ersek <lersek@redhat.com> 写
> 道：
> >
> >> On 11/06/18 03:59, Ruiyu Ni wrote:
> >> From: Jiewen Yao <jiewen.yao@intel.com>
> >>
> >> Today's implementation blocks SMM read-out no matter static paging
> >> is enabled or not. But certain platform may need to read non-SMM
> >> content from SMM code. These platforms don't have a way to disable
> >> the read-out blocking.
> >>
> >> The patch updates the policy to only block SMM read-out when static
> >> paging is enabled. So that the static paging can be disabled for
> >> those platforms that want SMM read-out.
> >>
> >> Setting PcdCpuSmmStaticPageTable to FALSE can disable the static
> >> paging.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> >> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >> index 5bb7d57238..117502dafa 100644
> >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> >> @@ -1,7 +1,7 @@
> >> /** @file
> >> Page Fault (#PF) handler for X64 processors
> >>
> >> -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
> >> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> >> Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >>
> >> This program and the accompanying materials
> >> @@ -890,7 +890,7 @@ SmiPFHandler (
> >>       CpuDeadLoop ();
> >>     }
> >>
> >> -    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
> >> +    if (mCpuSmmStaticPageTable &&
> IsSmmCommBufferForbiddenAddress (PFAddress)) {
> >>       DumpCpuContext (InterruptType, SystemContext);
> >>       DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden
> address (0x%lx)!\n", PFAddress));
> >>       DEBUG_CODE (
> >>
> >
> > OVMF inherits the default TRUE value for PcdCpuSmmStaticPageTable,
> from
> > "UefiCpuPkg.dec", and that's intentional. Therefore this patch should be
> > a no-op from OVMF's perspective.
> >
> > Acked-by: Laszlo Ersek <lersek@redhat.com>
> >
> > More generally, is the use of PcdCpuSmmStaticPageTable for controlling
> > this kind of read-out just a convenience / simplification (in which case
> > I don't think it's great!), or are these topics inherently connected
> > somehow?
> >
> > I remember that Jiewen said earlier that with "static paging" enabled
> > (i.e., building the page tables used in SMM all in advance), we provide
> > more page protection.
> >
> > Also, I see that PcdCpuSmmProfileEnable can only be enabled with
> > PcdCpuSmmStaticPageTable set to FALSE.
> >
> > So it seems that with PcdCpuSmmStaticPageTable set to TRUE, our page
> > fault handling in SMM is generally strict(er). This patch looks
> > consistent with that, but it would be nice if the commit message spelled
> > out why *exactly* it makes sense to use PcdCpuSmmStaticPageTable for
> > this new purpose as well.
> >
> > (I hope my question makes sense. :) )
> >
> > Thanks
> > Laszlo

Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[Laszlo Ersek]

On 11/07/18 09:27, Yao, Jiewen wrote:
> If Static paging is not used, then the SMM uses dynamic paging.
> 
> Here dynamic paging means the page fault request on-demand.
> 
> 2 special examples here:
> 1) above 4G MMIO. By default, SMM only setup paging table for 4G memory. If MMIO above 4G, then SMM need use #PF handler to grant MMIO access.
> 2) server RAS. By default, SMM only setup paging table for SMM communication buffer. For server RAS, memory hotplug may request direct OS memory access. If so, we also rely on #PF handler to grant OS access.
> 
> This patch fixed the second issue in the second case.

This is very interesting -- if you have a bit of time, can you please
describe in more detail what "For server RAS, memory hotplug may request
direct OS memory access" means? What agents take what steps?

(I don't mean to hold this patch -- I've given my A-b, so this is for my
own education only. Although, adding this very useful info to the commit
message, or at least to the associated BZ, would be extremely useful.)

Thanks!
Laszlo

Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when static paging is used

[Yao, Jiewen]

Yeah, there is some good resource on the web.

ACPI spec defined APEI for error handling.
https://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf

Below ppt introduced basic info on memory technology such as spare or mirror.
ftp://ftp.hp.com/pub/c-products/servers/options/c00256943.pdf

Below ppt introduced basic info on RAS/HotPlug in Linux.
https://events.static.linuxfound.org/sites/events/files/lcjp13_chen.pdf
https://events.static.linuxfound.org/sites/events/files/lcjp13_ishimatsu.pdf

Put all things together: when memory error happens, a platform may generate SMI or SCI redirect to SMI.
Then SMI code may choose to analyze, recovery, or backup DIMM for OS memory. As such, RAS SMI handler may need access OS memory, and/or interactive with OS handler via ASL code.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 7, 2018 9:37 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric
> <eric.dong@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/SmmCpu: Block SMM read-out only when
> static paging is used
> 
> On 11/07/18 09:27, Yao, Jiewen wrote:
> > If Static paging is not used, then the SMM uses dynamic paging.
> >
> > Here dynamic paging means the page fault request on-demand.
> >
> > 2 special examples here:
> > 1) above 4G MMIO. By default, SMM only setup paging table for 4G
> memory. If MMIO above 4G, then SMM need use #PF handler to grant MMIO
> access.
> > 2) server RAS. By default, SMM only setup paging table for SMM
> communication buffer. For server RAS, memory hotplug may request direct
> OS memory access. If so, we also rely on #PF handler to grant OS access.
> >
> > This patch fixed the second issue in the second case.
> 
> This is very interesting -- if you have a bit of time, can you please
> describe in more detail what "For server RAS, memory hotplug may request
> direct OS memory access" means? What agents take what steps?
> 
> (I don't mean to hold this patch -- I've given my A-b, so this is for my
> own education only. Although, adding this very useful info to the commit
> message, or at least to the associated BZ, would be extremely useful.)
> 
> Thanks!
> Laszlo