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

[Ryan Harkin <ryan.harkin@linaro.org>]

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)

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

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
>