From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x236.google.com (mail-wm0-x236.google.com [IPv6:2a00:1450:400c:c09::236]) (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 7B64E803DA for ; Tue, 14 Mar 2017 13:20:32 -0700 (PDT) Received: by mail-wm0-x236.google.com with SMTP id 196so15668313wmm.1 for ; Tue, 14 Mar 2017 13:20:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=z0k/GLPIxUpqBsQDw8yy8ru/0DN1Sxk1pwfu1JK46Lk=; b=Ii1fjS7HWaKMxWGi0pAjD2Cb8dxRQB56W5MtYeuGDOVfM14IyNjz+GWh3MCqfKTKGC ZMdI1cMdh3eYgmwX0I4sZOkW3w6w7CjxgwPaQreUqD7YuskpCv3y9eQ26pDKwD+3jzQF +XIQ2jPNrt/UGtJpNk+X1JGsAjdMmmiE3utc4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=z0k/GLPIxUpqBsQDw8yy8ru/0DN1Sxk1pwfu1JK46Lk=; b=O0daYImrXfYGqvP3bQ2NExnF8DEKgEsXsIXOZd6W4MujlSasllGoCHPs9mVXZLatgi 19xYQaGBuhfopujxMlrk4o58B7o89MRhVDsGsZYlaUIX8k9e4Z78seuYYVj94XLbTd34 fIdiuegmOCY9R7BXWNjtcYnke5U+8e/uqt1GPqTg5biK1fpm6GUPBUZoA83aX0L1hDz3 mJp6U6G9vai3LS7mhRAE1jvsnkoBksEwat8IX4zYdLsQkMxEg7Yin6SMWTHPlW7dsXpV iTltmjeuadjpW+FFpE3GOXV2Nl5T1MEtE/mMD061Pa3y8p7cfkNZjhu3bsBQ+ozonLD8 j55g== X-Gm-Message-State: AFeK/H14a/jDiyvetSF2TtZJ52Nzfocoh9lMmkLsKdRXmDRhKFwpL+Ak8d6ul9NuMaX2En4K X-Received: by 10.28.187.213 with SMTP id l204mr15719408wmf.104.1489522830655; Tue, 14 Mar 2017 13:20:30 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id q12sm16785799wmd.8.2017.03.14.13.20.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 14 Mar 2017 13:20:29 -0700 (PDT) Date: Tue, 14 Mar 2017 20:20:28 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org Message-ID: <20170314202028.GX16034@bivouac.eciton.net> References: <1489521495-2961-1-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1489521495-2961-1-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH] ArmPkg/UncachedMemoryAllocationLib: set XP bit via CPU arch protocol 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 20:20:32 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Mar 14, 2017 at 07:58:15PM +0000, Ard Biesheuvel wrote: > Commit e7b24ec9785d ("ArmPkg/UncachedMemoryAllocationLib: map uncached > allocations non-executable") adds code that manipulates the GCD memory > space attributes of a newly allocated uncached region without checking > whether this region expose these attributes in its capabilities mask. > > Given that the intent is to remove executable permissions from the region, > this is a fairly pointless exercise to begin with, regardless of whether > it is correct or not. The reason is that RO/XP memory attributes in the > GCD memory space map or the UEFI memory map are completely disconnected > from the actual mapping permissions used in the page tables. > > So instead, invoke the CPU arch protocol directly, and add the non-exec > attributes in the page tables directly. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 36 ++++++++++++++++---- > ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf | 8 ++++- > 2 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c > index b4fbfbcb362b..2530175bab7a 100644 > --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c > +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c > @@ -27,6 +27,10 @@ > #include > #include > > +#include > + > +STATIC EFI_CPU_ARCH_PROTOCOL *mCpu; > + > VOID * > UncachedInternalAllocatePages ( > IN EFI_MEMORY_TYPE MemoryType, > @@ -150,15 +154,19 @@ AllocatePagesFromList ( > > Status = gDS->GetMemorySpaceDescriptor (Memory, &Descriptor); > if (EFI_ERROR (Status)) { > - gBS->FreePages (Memory, Pages); > - return Status; > + goto FreePages; > } > > Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages), > - EFI_MEMORY_WC | EFI_MEMORY_XP); > + EFI_MEMORY_WC); > if (EFI_ERROR (Status)) { > - gBS->FreePages (Memory, Pages); > - return Status; > + goto FreePages; > + } > + > + Status = mCpu->SetMemoryAttributes (mCpu, Memory, EFI_PAGES_TO_SIZE (Pages), > + EFI_MEMORY_XP); So, all other changes here are straightforward, but this one relies on the fact that we don't care about EFI_MEMORY_RO being cleared when we flip to _XP. As discussed on irc, if you can add a small note on that (and wait for Ryan's Tested-by): Reviewed-by: Leif Lindholm > + if (EFI_ERROR (Status)) { > + goto FreePages; > } > > InvalidateDataCacheRange ((VOID *)(UINTN)Memory, EFI_PAGES_TO_SIZE (Pages)); > @@ -166,8 +174,8 @@ AllocatePagesFromList ( > NewNode = AllocatePool (sizeof (FREE_PAGE_NODE)); > if (NewNode == NULL) { > ASSERT (FALSE); > - gBS->FreePages (Memory, Pages); > - return EFI_OUT_OF_RESOURCES; > + Status = EFI_OUT_OF_RESOURCES; > + goto FreePages; > } > > NewNode->Base = Memory; > @@ -181,6 +189,10 @@ AllocatePagesFromList ( > > *Allocation = NewNode->Allocation; > return EFI_SUCCESS; > + > +FreePages: > + gBS->FreePages (Memory, Pages); > + return Status; > } > > /** > @@ -238,6 +250,16 @@ FreePagesFromList ( > */ > EFI_STATUS > EFIAPI > +UncachedMemoryAllocationLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu); > +} > + > +EFI_STATUS > +EFIAPI > UncachedMemoryAllocationLibDestructor ( > IN EFI_HANDLE ImageHandle, > IN EFI_SYSTEM_TABLE *SystemTable > diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf > index d7a0f2f792a1..c637430c9020 100644 > --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf > +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf > @@ -22,7 +22,7 @@ [Defines] > MODULE_TYPE = DXE_DRIVER > VERSION_STRING = 1.0 > LIBRARY_CLASS = UncachedMemoryAllocationLib > - > + CONSTRUCTOR = UncachedMemoryAllocationLibConstructor > DESTRUCTOR = UncachedMemoryAllocationLibDestructor > > [Sources.common] > @@ -42,3 +42,9 @@ [LibraryClasses] > > [Pcd] > gArmTokenSpaceGuid.PcdArmFreeUncachedMemorySizeThreshold > + > +[Protocols] > + gEfiCpuArchProtocolGuid > + > +[Depex] > + gEfiCpuArchProtocolGuid > -- > 2.7.4 >