From: "Wang, Jian J" <jian.j.wang@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <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>
Subject: Re: [PATCH v2] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
Date: Thu, 12 Apr 2018 05:44:48 +0000 [thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624D3CD0F@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BAAC2E8@shsmsx102.ccr.corp.intel.com>
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
prev parent reply other threads:[~2018-04-12 5:44 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-12 4:58 [PATCH v2] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM Jian J Wang
2018-04-12 5:37 ` Zeng, Star
2018-04-12 5:44 ` Wang, Jian J [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D827630B58408649ACB04F44C510003624D3CD0F@SHSMSX103.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox