Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore

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

On 14 March 2017 at 17:13, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> Hi Ard,
>
> On 14 March 2017 at 08:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> The recently introduced memory protection features inadvertently broke
>> the boot on all PrePi platforms, because the changes to explicitly use
>> EfiBootServicesCode for loading the DxeCore PE/COFF image need to be
>> applied in a different way for PrePi. So add a simple helper function
>> that sets the type of an allocation to EfiBootServicesCode, and invoke
>> it to allocate the space for DxeCore.
>>
>
> I'm not quite sure which issue this patch is supposed to fix.
>
> EDK2 is broken on Juno for me right now. I see this output at boot:
>
> EhcDriverBindingStart: failed to enable 64-bit DMA on 64-bit capable
> controller @ FD41B898 (Unsupported)
>

This bit is unrelated (unless you are booting from USB)

> ASSERT_EFI_ERROR (Status = Unsupported)
> ASSERT [ArmJunoDxe]
> /linaro/platforms/uefi/edk2/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c(361):
> !EFI_ERROR (Status)
>

... but this is obviously the culprit.

I'm afraid I made a thinko there: setting the XP bit in the GCD memory
space attributes is only supported if the memory has the XP capability
to begin with, and given that it does not propagate to the page tables
anyway, there is no point in setting this capability, and so we never
bother.

> Bisecting tells me this patch is to blame:
>
> $ git bisect bad
> e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 is the first bad commit
> commit e7b24ec9785d206f1d3faf8f646e63a1b540d6a5
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Tue Feb 28 12:13:12 2017 +0000
>
>     ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable
>
>     The primary use case for UncachedMemoryAllocationLib is non-coherent DMA,
>     which implies that such regions are not used to fetch instructions from.
>
>     So let's map them as non-executable, to avoid creating a security hole
>     when the rest of the platform may be enforcing strict memory permissions
>     on ordinary allocations.
>
>     Contributed-under: TianoCore Contribution Agreement 1.0
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> :040000 040000 6310e6a81c3b1e9eda315a9f4cad2bc918f74b25
> 12b11759ebd83f91a127c1f016b8e41cbfd097d4 M    ArmPkg
>
> Applying the patch from $subject and "[PATCH] Platforms/ARM: enable
> memory protection features" does not resolve the issue.
>
> AFAICT, only reverting e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 makes
> things work again. I suspect there are further Juno specific changes
> that could be applied instead of reverting the problematic patch.
>
> So, Juno and TC2 boot fine if my edk2 tree looks like this:
>
> 50e5735  2017-03-14  Revert "ArmPkg/UncachedMemoryAllocationLib: map
> uncached allocations non-executable"       [Ryan Harkin]
> 7e9c1b0  2017-03-14  EmbeddedPkg/PrePiLib: allocate code pages for
> DxeCore      [Ard Biesheuvel]
> c03f5b2  2017-03-10  BaseTools/UPT: Fix an issue in subst command
>  [Hess Chen]
>
> And OpenPlatformPkg looks like this:
>
> d6a743d  2017-03-07  Platforms/ARM: enable memory protection features
>  [Ard Biesheuvel]
> 408d600  2016-01-29  HACK: Platforms/ARM: TC2: set
> gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride TRUE  [Ryan
> Harkin]
> 17dbc2a  2017-02-24  Treewide: remove references to
> VirtualUncachedPages libraries and PCDs     [Ard Biesheuvel]
>
> If I go on to drop $subject patch, Juno and TC2 no longer boot, so I
> presume that it's supposed to fix the issue where OpenPlatformPkg has
> enabled the memory protection features. And that the upstream Juno
> problems are different.
>
> Cheers,
> Ryan.
>
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Tested-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
>> ---
>> v2: add missing MemoryAllocationLib.h include
>>     add Michael's T/b
>>
>>  EmbeddedPkg/Library/PrePiLib/PrePi.h    |  1 +
>>  EmbeddedPkg/Library/PrePiLib/PrePiLib.c | 34 +++++++++++++++++++-
>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/EmbeddedPkg/Library/PrePiLib/PrePi.h b/EmbeddedPkg/Library/PrePiLib/PrePi.h
>> index 607561cd2496..84eca23ec8a6 100644
>> --- a/EmbeddedPkg/Library/PrePiLib/PrePi.h
>> +++ b/EmbeddedPkg/Library/PrePiLib/PrePi.h
>> @@ -28,6 +28,7 @@
>>  #include <Library/CacheMaintenanceLib.h>
>>  #include <Library/TimerLib.h>
>>  #include <Library/PerformanceLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>>
>>  #include <Guid/MemoryAllocationHob.h>
>>
>> diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
>> index 9a1ef344df6e..bba8e7384edc 100644
>> --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
>> +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
>> @@ -28,6 +28,38 @@ SecWinNtPeiLoadFile (
>>    IN  EFI_PHYSICAL_ADDRESS    *EntryPoint
>>    );
>>
>> +STATIC
>> +VOID*
>> +EFIAPI
>> +AllocateCodePages (
>> +  IN  UINTN     Pages
>> +  )
>> +{
>> +  VOID                    *Alloc;
>> +  EFI_PEI_HOB_POINTERS    Hob;
>> +
>> +  Alloc = AllocatePages (Pages);
>> +  if (Alloc == NULL) {
>> +    return NULL;
>> +  }
>> +
>> +  // find the HOB we just created, and change the type to EfiBootServicesCode
>> +  Hob.Raw = GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION);
>> +  while (Hob.Raw != NULL) {
>> +    if (Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress == (UINTN)Alloc) {
>> +      Hob.MemoryAllocation->AllocDescriptor.MemoryType = EfiBootServicesCode;
>> +      return Alloc;
>> +    }
>> +    Hob.Raw = GET_NEXT_HOB (Hob);
>> +    Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, Hob.Raw);
>> +  }
>> +
>> +  ASSERT (FALSE);
>> +
>> +  FreePages (Alloc, Pages);
>> +  return NULL;
>> +}
>> +
>>
>>  EFI_STATUS
>>  EFIAPI
>> @@ -54,7 +86,7 @@ LoadPeCoffImage (
>>    //
>>    // Allocate Memory for the image
>>    //
>> -  Buffer = AllocatePages (EFI_SIZE_TO_PAGES((UINT32)ImageContext.ImageSize));
>> +  Buffer = AllocateCodePages (EFI_SIZE_TO_PAGES((UINT32)ImageContext.ImageSize));
>>    ASSERT (Buffer != 0);
>>
>>
>> --
>> 2.7.4
>>