* [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
@ 2018-04-11 7:29 Jian J Wang
2018-04-11 8:00 ` Zeng, Star
0 siblings, 1 reply; 3+ messages in thread
From: Jian J Wang @ 2018-04-11 7:29 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Eric Dong, Jiewen Yao, Ruiyu Ni, Michael D Kinney
This patch fixes an issue introduced by commit
5b91bf82c67b586b9588cbe4bbffa1588f6b5926
This issue will only happen if PcdDxeNxMemoryProtectionPolicy is
enabled for reserved memory, which will mark SMM RAM as NX (non-
executable) during DXE core initialization. SMM IPL driver will
unset the NX attribute for SMM RAM to allow loading and running
SMM core/drivers.
But above commit will fail the unset operation of the NX attribute
due to a fact that SMM RAM has zero cache attribute (MRC code always
sets 0 attribute for reserved memory), which will cause GCD internal
method ConverToCpuArchAttributes() to return 0 attribute which is
taken as invalid CPU paging attribute and skip the calling of
gCpu->SetMemoryAttributes().
Commit 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c tries to fix compatible
issue but not this one. The solution is to make use of existing
functionality in PiSmmIpl to make sure one cache attribute is set
for SMM RAM. For performance consideration, PiSmmIpl will always
try to set SMM RAM to write-back. But there's a hole in the code
which will fail the setting write-back attribute because of no
corresponding cache capabilities. This patch will add necessary
cache capabilities before setting corresponding attributes.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 94d671bd74..552220b4dd 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -1617,6 +1617,21 @@ SmmIplEntry (
GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase, &mSmramCacheSize);
//
+ // Make sure we can change the cache attributes.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
+ &MemDesc
+ );
+ if (!EFI_ERROR (Status) &&
+ (MemDesc.Capabilities & (EFI_MEMORY_WB | EFI_MEMORY_UC)) != (EFI_MEMORY_WB | EFI_MEMORY_UC)) {
+ gDS->SetMemorySpaceCapabilities (
+ mSmramCacheBase,
+ mSmramCacheSize,
+ MemDesc.Capabilities | EFI_MEMORY_WB | EFI_MEMORY_UC
+ );
+ }
+ //
// If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
// XP if it's set.
// Note that it is expected that cacheability of SMRAM has been set to WB if CPU AP
@@ -1626,7 +1641,7 @@ SmmIplEntry (
Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuArch);
if (!EFI_ERROR (Status)) {
Status = gDS->SetMemorySpaceAttributes(
- mSmramCacheBase,
+ mSmramCacheBase,
mSmramCacheSize,
EFI_MEMORY_WB
);
@@ -1634,16 +1649,17 @@ SmmIplEntry (
DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to EFI_MEMORY_WB\n"));
}
- Status = gDS->GetMemorySpaceDescriptor(
- mCurrentSmramRange->PhysicalStart,
+ Status = gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
&MemDesc
);
if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
- gDS->SetMemorySpaceAttributes (
- mCurrentSmramRange->PhysicalStart,
- mCurrentSmramRange->PhysicalSize,
- MemDesc.Attributes & (~EFI_MEMORY_XP)
- );
+ Status = gDS->SetMemorySpaceAttributes (
+ mSmramCacheBase,
+ mSmramCacheSize,
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ ASSERT_EFI_ERROR (Status);
}
}
//
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
2018-04-11 7:29 [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM Jian J Wang
@ 2018-04-11 8:00 ` Zeng, Star
2018-04-12 0:59 ` Wang, Jian J
0 siblings, 1 reply; 3+ messages in thread
From: Zeng, Star @ 2018-04-11 8:00 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Yao, Jiewen, Ni, Ruiyu, Kinney, Michael D, Zeng, Star
It should be exposed by 5b91bf82c67b586b9588cbe4bbffa1588f6b5926 + 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c.
With this information updated in commit log, Reviewed-by: Star Zeng <star.zeng@intel.com>.
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Wednesday, April 11, 2018 3:30 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
This patch fixes an issue introduced by commit
5b91bf82c67b586b9588cbe4bbffa1588f6b5926
This issue will only happen if PcdDxeNxMemoryProtectionPolicy is
enabled for reserved memory, which will mark SMM RAM as NX (non-
executable) during DXE core initialization. SMM IPL driver will
unset the NX attribute for SMM RAM to allow loading and running
SMM core/drivers.
But above commit will fail the unset operation of the NX attribute
due to a fact that SMM RAM has zero cache attribute (MRC code always
sets 0 attribute for reserved memory), which will cause GCD internal
method ConverToCpuArchAttributes() to return 0 attribute which is
taken as invalid CPU paging attribute and skip the calling of
gCpu->SetMemoryAttributes().
Commit 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c tries to fix compatible
issue but not this one. The solution is to make use of existing
functionality in PiSmmIpl to make sure one cache attribute is set
for SMM RAM. For performance consideration, PiSmmIpl will always
try to set SMM RAM to write-back. But there's a hole in the code
which will fail the setting write-back attribute because of no
corresponding cache capabilities. This patch will add necessary
cache capabilities before setting corresponding attributes.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 94d671bd74..552220b4dd 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -1617,6 +1617,21 @@ SmmIplEntry (
GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase, &mSmramCacheSize);
//
+ // Make sure we can change the cache attributes.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
+ &MemDesc
+ );
+ if (!EFI_ERROR (Status) &&
+ (MemDesc.Capabilities & (EFI_MEMORY_WB | EFI_MEMORY_UC)) != (EFI_MEMORY_WB | EFI_MEMORY_UC)) {
+ gDS->SetMemorySpaceCapabilities (
+ mSmramCacheBase,
+ mSmramCacheSize,
+ MemDesc.Capabilities | EFI_MEMORY_WB | EFI_MEMORY_UC
+ );
+ }
+ //
// If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
// XP if it's set.
// Note that it is expected that cacheability of SMRAM has been set to WB if CPU AP
@@ -1626,7 +1641,7 @@ SmmIplEntry (
Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuArch);
if (!EFI_ERROR (Status)) {
Status = gDS->SetMemorySpaceAttributes(
- mSmramCacheBase,
+ mSmramCacheBase,
mSmramCacheSize,
EFI_MEMORY_WB
);
@@ -1634,16 +1649,17 @@ SmmIplEntry (
DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to EFI_MEMORY_WB\n"));
}
- Status = gDS->GetMemorySpaceDescriptor(
- mCurrentSmramRange->PhysicalStart,
+ Status = gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
&MemDesc
);
if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
- gDS->SetMemorySpaceAttributes (
- mCurrentSmramRange->PhysicalStart,
- mCurrentSmramRange->PhysicalSize,
- MemDesc.Attributes & (~EFI_MEMORY_XP)
- );
+ Status = gDS->SetMemorySpaceAttributes (
+ mSmramCacheBase,
+ mSmramCacheSize,
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ ASSERT_EFI_ERROR (Status);
}
}
//
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
2018-04-11 8:00 ` Zeng, Star
@ 2018-04-12 0:59 ` Wang, Jian J
0 siblings, 0 replies; 3+ messages in thread
From: Wang, Jian J @ 2018-04-12 0:59 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org
Cc: Dong, Eric, Yao, Jiewen, Ni, Ruiyu, Kinney, Michael D
Yeah, you're right. I'll update the log text. Thanks for catching it.
Regards,
Jian
> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, April 11, 2018 4:00 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni,
> Ruiyu <ruiyu.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
>
> It should be exposed by 5b91bf82c67b586b9588cbe4bbffa1588f6b5926 +
> 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c.
>
> With this information updated in commit log, Reviewed-by: Star Zeng
> <star.zeng@intel.com>.
>
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Wednesday, April 11, 2018 3:30 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
>
> This patch fixes an issue introduced by commit
>
> 5b91bf82c67b586b9588cbe4bbffa1588f6b5926
>
> This issue will only happen if PcdDxeNxMemoryProtectionPolicy is
> enabled for reserved memory, which will mark SMM RAM as NX (non-
> executable) during DXE core initialization. SMM IPL driver will
> unset the NX attribute for SMM RAM to allow loading and running
> SMM core/drivers.
>
> But above commit will fail the unset operation of the NX attribute
> due to a fact that SMM RAM has zero cache attribute (MRC code always
> sets 0 attribute for reserved memory), which will cause GCD internal
> method ConverToCpuArchAttributes() to return 0 attribute which is
> taken as invalid CPU paging attribute and skip the calling of
> gCpu->SetMemoryAttributes().
>
> Commit 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c tries to fix compatible
> issue but not this one. The solution is to make use of existing
> functionality in PiSmmIpl to make sure one cache attribute is set
> for SMM RAM. For performance consideration, PiSmmIpl will always
> try to set SMM RAM to write-back. But there's a hole in the code
> which will fail the setting write-back attribute because of no
> corresponding cache capabilities. This patch will add necessary
> cache capabilities before setting corresponding attributes.
>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 32
> ++++++++++++++++++++++++--------
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 94d671bd74..552220b4dd 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -1617,6 +1617,21 @@ SmmIplEntry (
>
> GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase,
> &mSmramCacheSize);
> //
> + // Make sure we can change the cache attributes.
> + //
> + Status = gDS->GetMemorySpaceDescriptor (
> + mSmramCacheBase,
> + &MemDesc
> + );
> + if (!EFI_ERROR (Status) &&
> + (MemDesc.Capabilities & (EFI_MEMORY_WB | EFI_MEMORY_UC)) !=
> (EFI_MEMORY_WB | EFI_MEMORY_UC)) {
> + gDS->SetMemorySpaceCapabilities (
> + mSmramCacheBase,
> + mSmramCacheSize,
> + MemDesc.Capabilities | EFI_MEMORY_WB | EFI_MEMORY_UC
> + );
> + }
> + //
> // If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
> // XP if it's set.
> // Note that it is expected that cacheability of SMRAM has been set to WB if
> CPU AP
> @@ -1626,7 +1641,7 @@ SmmIplEntry (
> Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID
> **)&CpuArch);
> if (!EFI_ERROR (Status)) {
> Status = gDS->SetMemorySpaceAttributes(
> - mSmramCacheBase,
> + mSmramCacheBase,
> mSmramCacheSize,
> EFI_MEMORY_WB
> );
> @@ -1634,16 +1649,17 @@ SmmIplEntry (
> DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to
> EFI_MEMORY_WB\n"));
> }
>
> - Status = gDS->GetMemorySpaceDescriptor(
> - mCurrentSmramRange->PhysicalStart,
> + Status = gDS->GetMemorySpaceDescriptor (
> + mSmramCacheBase,
> &MemDesc
> );
> if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
> - gDS->SetMemorySpaceAttributes (
> - mCurrentSmramRange->PhysicalStart,
> - mCurrentSmramRange->PhysicalSize,
> - MemDesc.Attributes & (~EFI_MEMORY_XP)
> - );
> + Status = gDS->SetMemorySpaceAttributes (
> + mSmramCacheBase,
> + mSmramCacheSize,
> + MemDesc.Attributes & (~EFI_MEMORY_XP)
> + );
> + ASSERT_EFI_ERROR (Status);
> }
> }
> //
> --
> 2.16.2.windows.1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-04-12 0:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-11 7:29 [PATCH] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM Jian J Wang
2018-04-11 8:00 ` Zeng, Star
2018-04-12 0:59 ` Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox