public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "Tian, Feng" <feng.tian@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Fan, Jeff" <jeff.fan@intel.com>,
	"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH V3 0/4] DXE Memory Protection
Date: Fri, 10 Feb 2017 12:59:14 +0000	[thread overview]
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A8ECAA8@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <F8F18618-14AD-4E82-85C3-BDB142F5B1CB@linaro.org>

Thanks for the info.

Mike/Vincent also mentioned that FW does not own page tables after ExitBootServices(), so the OS would have to relax NX protection of RT code pages across SVA.
Or delay setting NX protections on RT code pages until after SVA.

I agree with you that we can mark RT region to be RW.
Here is the pseudo code I plan to put to CoreExitBootServices().

=======================
VOID
MemoryprotectionExitBootServicesCallback (
  VOID
  )
{
  EFI_RUNTIME_IMAGE_ENTRY       *RuntimeImage;
  LIST_ENTRY                    *Link;

  //
  // We need remove the RT protection, because RT relocation need write code segment
  // at SetVirtualAddressMap(). We cannot assume OS/Loader has taken over page table at that time.
  //
  // Firmware does not own page tables after ExitBootServices(), so the OS would
  // have to relax protection of RT code pages across SetVirtualAddressMap(), or
  // delay setting protections on RT code pages until after SetVirtualAddressMap().
  // OS may set protection on RT based upon EFI_MEMORY_ATTRIBUTES_TABLE later.
  //
  if (mImageProtectionPolicy != 0) {
    for (Link = gRuntime->ImageHead.ForwardLink; Link != &gRuntime->ImageHead; Link = Link->ForwardLink) {
      RuntimeImage = BASE_CR (Link, EFI_RUNTIME_IMAGE_ENTRY, Link);
      SetUefiImageMemoryAttributes ((UINT64)(UINTN)RuntimeImage->ImageBase, ALIGN_VALUE(RuntimeImage->ImageSize, EFI_PAGE_SIZE), 0);
    }
  }
}
=======================


From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Friday, February 10, 2017 3:43 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection


On 10 Feb 2017, at 11:32, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
Good to learn that ARM/ARCH64 behavior. That is interesting.

Yes, if that is the case, we need figure out a way to make it work.

IMHO, “Undo the protection” directly is also risky.
Maybe the protection is used or setup by OS purposely before EBS (WoW. ☺). Unprotecting them in BIOS might break the OS expectation.


Would you please provide more info on ARM Linux? When the OSLoader or OS takes over the page table? After EBS?

On ARM, we call SetVirtualAddressMap directly after EBS, in the OS loader. The caches are disabled much later. It is the OS itself that reenables the MMU, and install the UEFI virtual mapping as per-process mapping, which is only live when a runtime services call is in progress.

Therefore, we use a low virtual mapping for runtime services, which may conflict with the 1:1 mapping, so we never map any uefi regions 1:1 under the os.

This implies that the virtual mapping we install is not yet live when we install it, and so the easiest way to do that is to install it immediately after ebs, when the firmware's 1:1 mapping is still live.




Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, February 9, 2017 10:42 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Fan, Jeff <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection



> On 10 Feb 2017, at 06:34, Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> wrote:
>
>
>
>> On 10 Feb 2017, at 02:26, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>>
>> Very good question.
>>
>> 1)       Yes, I did test UEFI OS boot, which is mentioned in V1 summary:
>> ======
>> Tested OS: UEFI Win10, UEFI Ubuntu 16.04.
>> ======
>>
>> 2)       Star helps double confirm that OS already takes over the control of page table on SetVirtualAddressMap().
>> See below log on UEFI Win10.
>> ======
>> DXEIPL CR3 0x88140000
>> RUNTIMEDXE CR3 0x1AB000
>> ======
>>
>
> Not on AArch64/ARM linux, and the spec does not mandate it, so we need to deal with this imo
>

I think we should probably undo the protections for runtime drivers in EBS()


>> Thank you
>> Yao Jiewen
>>
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
>> Sent: Thursday, February 9, 2017 8:48 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>> Cc: Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Fan, Jeff <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>> Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection
>>
>> On 9 February 2017 at 16:30, Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> wrote:
>> > On 9 February 2017 at 16:29, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>> >> Very good point.
>> >>
>> >> Can ARCH64 set 4K paging for 64K aligned runtime memory?
>> >>
>> >
>> > UEFI always uses 4 KB, but the OS may use 64 KB, so to create the
>> > virtual address map it needs the runtime regions to be 64 KB aligned.
>> >
>> >>
>> >>
>> >> If yes, how about we use
>> >>
>> >> “ImageRecord->ImageSize = ALIGN_VALUE(LoadedImage->ImageSize,
>> >> EFI_PAGE_SIZE);”
>> >>
>> >
>>
>> Another question: did you try SetVirtualAddressMap()? It looks like we
>> need to lift read-only permissions to allow the runtime PE/COFF
>> relocation to apply the fixups
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

  reply	other threads:[~2017-02-10 12:59 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-09  7:20 [PATCH V3 0/4] DXE Memory Protection Jiewen Yao
2017-02-09  7:20 ` [PATCH V3 1/4] UefiCpuPkg/CpuDxe: Add memory attribute setting Jiewen Yao
2017-02-09  7:20 ` [PATCH V3 2/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage Jiewen Yao
2017-02-09  7:20 ` [PATCH V3 3/4] MdeModulePkg/dec: add PcdImageProtectionPolicy Jiewen Yao
2017-02-09  7:20 ` [PATCH V3 4/4] MdeModulePkg/DxeCore: Add UEFI image protection Jiewen Yao
2017-02-09  7:43 ` [PATCH V3 0/4] DXE Memory Protection Yao, Jiewen
2017-02-09  8:49   ` Ard Biesheuvel
2017-02-09  9:09     ` Ard Biesheuvel
2017-02-09  9:22       ` Ard Biesheuvel
2017-02-09 13:19         ` Yao, Jiewen
2017-02-09 13:51           ` Ard Biesheuvel
2017-02-09 14:08             ` Yao, Jiewen
2017-02-09 14:55               ` Ard Biesheuvel
2017-02-09 15:27                 ` Yao, Jiewen
2017-02-09 15:28                   ` Ard Biesheuvel
2017-02-09 16:21                     ` Ard Biesheuvel
2017-02-09 16:29                       ` Yao, Jiewen
2017-02-09 16:30                         ` Ard Biesheuvel
2017-02-09 16:48                           ` Ard Biesheuvel
2017-02-10  2:26                             ` Yao, Jiewen
2017-02-10  6:34                               ` Ard Biesheuvel
2017-02-10  6:41                                 ` Ard Biesheuvel
2017-02-10 11:32                                   ` Yao, Jiewen
2017-02-10 11:42                                     ` Ard Biesheuvel
2017-02-10 12:59                                       ` Yao, Jiewen [this message]
2017-02-10 14:16                                         ` Ard Biesheuvel
2017-02-09 14:23 ` Yao, Jiewen

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=74D8A39837DF1E4DA445A8C0B3885C503A8ECAA8@shsmsx102.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