MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 51 ++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 14 deletions(-)
> v2 changes:
> a. Remove redundant code and fill-up potential logic hole
> b. Code clean-up
> c. Fix error in commit log
This patch fixes an issue introduced by commit
5b91bf82c67b586b9588cbe4bbffa1588f6b5926
and
0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c
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 to 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().
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 | 51 ++++++++++++++++++++++++----------
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 94d671bd74..dee6e62bf4 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -42,6 +42,15 @@
#include "PiSmmCorePrivateData.h"
+#define SMRAM_CAPABILITIES \
+ (EFI_MEMORY_WB | EFI_MEMORY_UC | EFI_MEMORY_XP | EFI_MEMORY_RP | EFI_MEMORY_RO)
+
+#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+ EFI_MEMORY_WT | EFI_MEMORY_WB | \
+ EFI_MEMORY_WP | EFI_MEMORY_UCE)
+
+#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | EFI_MEMORY_RO)
+
//
// Function prototypes from produced protocols
//
@@ -1617,34 +1626,48 @@ SmmIplEntry (
GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase, &mSmramCacheSize);
//
+ // Make sure we can change the desired memory attributes.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
+ &MemDesc
+ );
+ ASSERT_EFI_ERROR (Status);
+ if ((MemDesc.Capabilities & SMRAM_CAPABILITIES) != SMRAM_CAPABILITIES) {
+ gDS->SetMemorySpaceCapabilities (
+ mSmramCacheBase,
+ mSmramCacheSize,
+ MemDesc.Capabilities | SMRAM_CAPABILITIES
+ );
+ }
+ //
// If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
- // XP if it's set.
+ // all paging attributes.
// Note that it is expected that cacheability of SMRAM has been set to WB if CPU AP
// is not available here.
//
CpuArch = NULL;
Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuArch);
if (!EFI_ERROR (Status)) {
- Status = gDS->SetMemorySpaceAttributes(
- mSmramCacheBase,
+ MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES | MEMORY_PAGE_ATTRIBUTES);
+ MemDesc.Attributes |= EFI_MEMORY_WB;
+ Status = gDS->SetMemorySpaceAttributes (
+ mSmramCacheBase,
mSmramCacheSize,
- EFI_MEMORY_WB
+ MemDesc.Attributes
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to EFI_MEMORY_WB\n"));
}
- Status = gDS->GetMemorySpaceDescriptor(
- mCurrentSmramRange->PhysicalStart,
- &MemDesc
- );
- if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
- gDS->SetMemorySpaceAttributes (
- mCurrentSmramRange->PhysicalStart,
- mCurrentSmramRange->PhysicalSize,
- MemDesc.Attributes & (~EFI_MEMORY_XP)
+ DEBUG_CODE (
+ gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
+ &MemDesc
);
- }
+ DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n", MemDesc.Attributes));
+ ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
+ );
}
//
// if Loading module at Fixed Address feature is enabled, save the SMRAM base to Load
--
2.16.2.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Has no need to set paging capability as the code is going to clear paging attribute.
With that refined, Reviewed-by: Star Zeng <star.zeng@intel.com>.
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Thursday, April 12, 2018 12:58 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 v2] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
> v2 changes:
> a. Remove redundant code and fill-up potential logic hole
> b. Code clean-up
> c. Fix error in commit log
This patch fixes an issue introduced by commit
5b91bf82c67b586b9588cbe4bbffa1588f6b5926
and
0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c
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 to 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().
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 | 51 ++++++++++++++++++++++++----------
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 94d671bd74..dee6e62bf4 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -42,6 +42,15 @@
#include "PiSmmCorePrivateData.h"
+#define SMRAM_CAPABILITIES \
+ (EFI_MEMORY_WB | EFI_MEMORY_UC | EFI_MEMORY_XP | EFI_MEMORY_RP |
+EFI_MEMORY_RO)
+
+#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+ EFI_MEMORY_WT | EFI_MEMORY_WB | \
+ EFI_MEMORY_WP | EFI_MEMORY_UCE)
+
+#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP |
+EFI_MEMORY_RO)
+
//
// Function prototypes from produced protocols // @@ -1617,34 +1626,48 @@ SmmIplEntry (
GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase, &mSmramCacheSize);
//
+ // Make sure we can change the desired memory attributes.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
+ &MemDesc
+ );
+ ASSERT_EFI_ERROR (Status);
+ if ((MemDesc.Capabilities & SMRAM_CAPABILITIES) != SMRAM_CAPABILITIES) {
+ gDS->SetMemorySpaceCapabilities (
+ mSmramCacheBase,
+ mSmramCacheSize,
+ MemDesc.Capabilities | SMRAM_CAPABILITIES
+ );
+ }
+ //
// If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
- // XP if it's set.
+ // all paging attributes.
// Note that it is expected that cacheability of SMRAM has been set to WB if CPU AP
// is not available here.
//
CpuArch = NULL;
Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuArch);
if (!EFI_ERROR (Status)) {
- Status = gDS->SetMemorySpaceAttributes(
- mSmramCacheBase,
+ MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES | MEMORY_PAGE_ATTRIBUTES);
+ MemDesc.Attributes |= EFI_MEMORY_WB;
+ Status = gDS->SetMemorySpaceAttributes (
+ mSmramCacheBase,
mSmramCacheSize,
- EFI_MEMORY_WB
+ MemDesc.Attributes
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to EFI_MEMORY_WB\n"));
}
- Status = gDS->GetMemorySpaceDescriptor(
- mCurrentSmramRange->PhysicalStart,
- &MemDesc
- );
- if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
- gDS->SetMemorySpaceAttributes (
- mCurrentSmramRange->PhysicalStart,
- mCurrentSmramRange->PhysicalSize,
- MemDesc.Attributes & (~EFI_MEMORY_XP)
+ DEBUG_CODE (
+ gDS->GetMemorySpaceDescriptor (
+ mSmramCacheBase,
+ &MemDesc
);
- }
+ DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n", MemDesc.Attributes));
+ ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
+ );
}
//
// if Loading module at Fixed Address feature is enabled, save the SMRAM base to Load
--
2.16.2.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Right, I checked the GCD code. Capabilities has only effect with "set" attributes.
Regards,
Jian
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, April 12, 2018 1:38 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 v2] MdeModulePkg/PiSmmIpl: fix non-executable SMM
> RAM
>
> Has no need to set paging capability as the code is going to clear paging
> attribute.
>
> With that refined, Reviewed-by: Star Zeng <star.zeng@intel.com>.
>
>
> Thanks,
> Star
>
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, April 12, 2018 12:58 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 v2] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
>
> > v2 changes:
> > a. Remove redundant code and fill-up potential logic hole
> > b. Code clean-up
> > c. Fix error in commit log
>
> This patch fixes an issue introduced by commit
>
> 5b91bf82c67b586b9588cbe4bbffa1588f6b5926
>
> and
>
> 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c
>
> 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 to
> 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().
>
> 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 | 51
> ++++++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 94d671bd74..dee6e62bf4 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -42,6 +42,15 @@
>
> #include "PiSmmCorePrivateData.h"
>
> +#define SMRAM_CAPABILITIES \
> + (EFI_MEMORY_WB | EFI_MEMORY_UC | EFI_MEMORY_XP |
> EFI_MEMORY_RP |
> +EFI_MEMORY_RO)
> +
> +#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC |
> EFI_MEMORY_WC | \
> + EFI_MEMORY_WT | EFI_MEMORY_WB | \
> + EFI_MEMORY_WP | EFI_MEMORY_UCE)
> +
> +#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP |
> +EFI_MEMORY_RO)
> +
> //
> // Function prototypes from produced protocols // @@ -1617,34 +1626,48
> @@ SmmIplEntry (
>
> GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase,
> &mSmramCacheSize);
> //
> + // Make sure we can change the desired memory attributes.
> + //
> + Status = gDS->GetMemorySpaceDescriptor (
> + mSmramCacheBase,
> + &MemDesc
> + );
> + ASSERT_EFI_ERROR (Status);
> + if ((MemDesc.Capabilities & SMRAM_CAPABILITIES) !=
> SMRAM_CAPABILITIES) {
> + gDS->SetMemorySpaceCapabilities (
> + mSmramCacheBase,
> + mSmramCacheSize,
> + MemDesc.Capabilities | SMRAM_CAPABILITIES
> + );
> + }
> + //
> // If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
> - // XP if it's set.
> + // all paging attributes.
> // Note that it is expected that cacheability of SMRAM has been set to WB if
> CPU AP
> // is not available here.
> //
> CpuArch = NULL;
> Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID
> **)&CpuArch);
> if (!EFI_ERROR (Status)) {
> - Status = gDS->SetMemorySpaceAttributes(
> - mSmramCacheBase,
> + MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES |
> MEMORY_PAGE_ATTRIBUTES);
> + MemDesc.Attributes |= EFI_MEMORY_WB;
> + Status = gDS->SetMemorySpaceAttributes (
> + mSmramCacheBase,
> mSmramCacheSize,
> - EFI_MEMORY_WB
> + MemDesc.Attributes
> );
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to
> EFI_MEMORY_WB\n"));
> }
>
> - Status = gDS->GetMemorySpaceDescriptor(
> - mCurrentSmramRange->PhysicalStart,
> - &MemDesc
> - );
> - if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
> - gDS->SetMemorySpaceAttributes (
> - mCurrentSmramRange->PhysicalStart,
> - mCurrentSmramRange->PhysicalSize,
> - MemDesc.Attributes & (~EFI_MEMORY_XP)
> + DEBUG_CODE (
> + gDS->GetMemorySpaceDescriptor (
> + mSmramCacheBase,
> + &MemDesc
> );
> - }
> + DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n",
> MemDesc.Attributes));
> + ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
> + );
> }
> //
> // if Loading module at Fixed Address feature is enabled, save the SMRAM
> base to Load
> --
> 2.16.2.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.