Re: [PATCH V3 0/4] DXE Memory Protection

["Yao, Jiewen" <jiewen.yao@intel.com>]

1)       That is great. I appreciate your quick response and help.
I will drop my patch for ARM 2/4, and wait for yours.


2)       For ImageEnd alignment issue, I agree with you.
I plan to round up with:
ImageRecord->ImageSize = ALIGN_VALUE(LoadedImage->ImageSize, SectionAlignment);
before SetUefiImageProtectionAttributes (ImageRecord, Protect);

I will update it at V4.

Thank you
Yao Jiewen


From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, February 9, 2017 6:56 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 9 February 2017 at 14:08, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> For X86 CPU, the memory protection attribute goes to page table, the cache
> attribute goes to MTRR register.
>
> Those are 2 difference resource, and can be set separately.
>
>
>
> The high level pseudo code is below:
>
> =======================
>
> CacheAttribute = Attribute & CACHE_ATTRIBUTE_MASK
>
> MemoryProtectionAttribute = Attribute & MEMORY_PROTECTION_ATTRIBUTE_MASK
>
> If (CacheAttribute != 0) {
>
>      SetCacheAttribute(CacheAttribute)
>
> }
>
> SetMemoryProtectionAttribute(MemoryProtectionAttribute)
>
> =======================
>
>
>
> NOTE: we need compare CacheAttribute == 0, because the cache attribute is an
> individual mask. 0x1 means UN_CACHE, 0x8 means WRITE_BACK. 0 is meaningless.
>
> We do not compare MemoryProtectionAttribute == 0, because 0 is a valid
> memory protection attribute, which means to disable protection.
>
>
>
> Before GCD sync, the DxeCore does not know the cache attribute, so that it
> can only set memory attribute. The CPU driver only modifies page table based
> upon MemoryProtectionAttribute and keep cache attribute untouched.
>

OK, I think we should be able to work around this, although it is not
pretty. I will send it out as a separate patch.

I do have one other question though: would it be possible to round up
the end of the image to page size in this code? (in
SetUefiImageProtectionAttributes()) Otherwise, we may end up calling
SetMemoryAttributes() with a length that is not page aligned, which
hits an EFI_UNSUPPORTED on ARM. Given that the PE/COFF loader always
allocates full pages, we know the space after the image is not used,
so it is possible (and even more secure!) to clear the exec bit on it.

"""
  //
  // Last DATA
  //
  ASSERT (CurrentBase <= ImageEnd);
  if (CurrentBase < ImageEnd) {
    //
    // DATA
    //
    if (Protect) {
      Attribute = EFI_MEMORY_XP;
    } else {
      Attribute = 0;
    }
    SetUefiImageMemoryAttributes (
      CurrentBase,
      ImageEnd - CurrentBase,
      Attribute
      );
  }
"""
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel