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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 February 2017 at 08:52, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> HI Ard
>
> Thanks to protect more. :-)
>

Of course! This is a very important topic for me.

> 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
>

Thanks for the link.

> 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).
>

OK

> 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?
>

Using PeiServicesAllocatePage() in the way that you describe does
indeed remove the problem, so I will use that instead.

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

OK


> 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?
>

https://lists.01.org/pipermail/edk2-devel/2017-February/007685.html

I will send out a v2 shortly which, as you suggest, moves the handling
to DXE core. The only problem is that ARM's SyncCacheConfig() removes
the noexec attributes again, so I need to fix that first. Then, the
arch CPU protocol installation event can iterate over the memory map
to set the permissions according to a policy PCD

Thanks,
Ard.