public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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] MdeModulePkg/PiSmmIpl: fix non-executable SMM RAM
Date: Thu, 12 Apr 2018 00:59:13 +0000	[thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624D3BA13@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BAA9C60@shsmsx102.ccr.corp.intel.com>

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



      reply	other threads:[~2018-04-12  0:59 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 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=D827630B58408649ACB04F44C510003624D3BA13@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