Re: [RFC PATCH 0/4] RFC: increased memory protection

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

HI Ard
Thanks to protect more. :-)
We did consider the idea to remove EXEC attribute for Data page before. But we got compatibility issue.

We documented some gaps in white paper -
https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf

I am glad that some limitation is already resolved or we have solution to mitigate it. But there is still some other need consideration.


1)      We observe some 3rd part code allocating data page for code. - That is hardest part. (OS limitation #12)
We might also need consider ReservedMemory/AcpiNvs. There might be code there, too. (Firmware limitation #6 and #7).

If we want to apply the protection, we might need define a new PCD to disable the data protection for compatibility consideration.


2)      About DxeCore in data page. (Firmware limitation #4)
I am thinking if we can fix LoadFile implementation in PeiCore.

MdeModulePkg\Core\Pei\Image\Image.c:
LoadAndRelocatePeCoffImage()
{
ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
}

AllocatePages means to allocate data page.
I think we should use PeiAllocatePages(EfiBootServicesCode, EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);

Does that fix the problem?



3)      I am not worried about EBC. That can be handled separately.


4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to some unknown reason. Would you please send it again?


Thank you
Yao Jiewen


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, February 23, 2017 2:25 AM
> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [RFC PATCH 0/4] RFC: increased memory protection
>
> Hello all,
>
> This is a proof of concept implementation that removes all executable
> permissions from writable memory regions, which greatly enhances security.
> It is based on Jiewen's recent work, which is a step in the right direction,
> but still leaves most of memory exploitable due to the default R+W+X
> permissions.
>
> The idea is that the implementation of the CPU arch protocol goes over the
> memory map and removes exec permissions from all regions that are not already
> marked as 'code. This requires some preparatory work to ensure that the
> DxeCore
> itself is covered by a BootServicesCode region, not a BootServicesData region.
> Exec permissions are re-granted selectively, when the PE/COFF loader allocates
> the space for it. Combined with Jiewen's code/data split, this removes all
> RWX mapped regions.
>
> There is a caveat, though (and there are likely more of that kind): the EBC
> driver will need some work to ensure the thunk buffers have the noexec
> restriction lifted. This could be done in the EBC driver, but perhaps it is
> better to either
> a) modify the DXE core so it always removes noexec restrictions when allocating
>    code pages, or
> b) add AllocateExecPages/AllocateExecPool() functions to the
> MemoryAllocationLib
>    API
>
> Comments please!
>
> Ard Biesheuvel (4):
>   MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode
>     conversion
>   MdeModulePkg/DxeCore: convert the DxeCore memory region to
>     BootServicesCode
>   MdeModulePkg/DxeCore: lift non-exec permissions on loaded images
>   ArmPkg/CpuDxe: remap all data regions non-executable
>
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++
>  MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++
>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +
>  MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++
>  MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-
>  5 files changed, 111 insertions(+), 1 deletion(-)
>
> --
> 2.7.4