From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22e.google.com (mail-it0-x22e.google.com [IPv6:2607:f8b0:4001:c0b::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BB87D8039D for ; Tue, 14 Mar 2017 12:23:13 -0700 (PDT) Received: by mail-it0-x22e.google.com with SMTP id w124so12805963itb.0 for ; Tue, 14 Mar 2017 12:23:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=bN3F7XoOv1sq4dueyp+waFMZNq1bowrfUitYInm9mDA=; b=gDTu0XghrrAQtkDKvKB/rzqm2WiVll0xqd+vg+NJvvkC+F6S3WQ3RpoYK7EOVszdUH sKHseezMG++/IeYpR3WRu2wqZwGliHhDFMXZ7BWxzLWjCEKwL2DexF4gthVBJ9O7afmo erLmLgNCuli1wBTtY8hWuoFPrgZ4dIH1T2eko= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=bN3F7XoOv1sq4dueyp+waFMZNq1bowrfUitYInm9mDA=; b=EC7T5xs6o0yqupoP7L6Nu16gZJu64rxTza8G2yJEzRpPtgNon0hE8vgsY7aVxwk+Tu xHdH8luq6HFHUCZ6NmmlfBuUgOpHb0JJGgSYUhLiAiRT5KduzZKPLITwJSB5s0eFJ4Nt 23qF+LEd+fODTkegTXiCK/e+4ceplZH4hyV//6LSJVfDfn1Op9oLqzShnt/IWCqM8Lk1 urnmj9gnE1Eek+2+4xWVEtij465mUl0/hDkW0lmjgr4A+YT9ym/i8vvJoAF96icL1Uh3 3uYR/DUqfjpoAMiOkkMeSsyU+EOrj/G1r8DCF7BJ57jptlcx0Fm3/qg/6bqUBvQZnQSN CzBg== X-Gm-Message-State: AFeK/H3dXNn4CsQvlCsGHuqaAPEXzxyhtnO+N+4mG0De+9WjTkumejKwVONfL817eTDpFEu6/aCsxVguDHiptuJ0 X-Received: by 10.36.169.69 with SMTP id x5mr1548918iti.37.1489519392984; Tue, 14 Mar 2017 12:23:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 14 Mar 2017 12:23:12 -0700 (PDT) In-Reply-To: References: <1489478464-5737-1-git-send-email-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Tue, 14 Mar 2017 19:23:12 +0000 Message-ID: To: Ryan Harkin Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Michael Zimmermann , Laszlo Ersek Subject: Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Mar 2017 19:23:14 -0000 Content-Type: text/plain; charset=UTF-8 On 14 March 2017 at 17:13, Ryan Harkin wrote: > Hi Ard, > > On 14 March 2017 at 08:01, Ard Biesheuvel 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 > 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 > Reviewed-by: Leif Lindholm > > :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 >> Tested-by: Michael Zimmermann >> --- >> 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 >> #include >> #include >> +#include >> >> #include >> >> 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 >>