From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6C602821BE for ; Tue, 28 Feb 2017 13:14:54 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0575B4E328; Tue, 28 Feb 2017 21:14:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-21.phx2.redhat.com [10.3.116.21]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1SLEr2D007186; Tue, 28 Feb 2017 16:14:54 -0500 To: Ard Biesheuvel , edk2-devel@ml01.01.org References: <1488290169-10701-1-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <650c64bd-ed0a-bd0e-5fce-3cd1ab0c6293@redhat.com> Date: Tue, 28 Feb 2017 22:14:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1488290169-10701-1-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 28 Feb 2017 21:14:55 +0000 (UTC) Subject: Re: [PATCH] ArmVirtPkg/HighMemDxe: preserve non-exec permissions on newly added regions 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, 28 Feb 2017 21:14:54 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 02/28/17 14:56, Ard Biesheuvel wrote: > Using DxeServices::SetMemorySpaceAttributes to set cacheability > attributes has the side effect of stripping permission attributes, > given that those are bits in the same bitfield, and so setting the > Attributes argument to EFI_MEMORY_WB implies not setting EFI_MEMORY_XP > or EFI_MEMORY_RO attributes. > > In fact, the situation is even worse, given that the descriptor returned > by DxeServices::GetMemorySpaceDescriptor does not reflect the permission > attributes that may have been set by the preceding call to > DxeServices::AddMemorySpace if PcdDxeNxMemoryProtectionPolicy has been > configured to map EfiConventionalMemory with non-executable permissions. > > Note that this applies equally to the non-executable stack and to PE/COFF > sections that may have been mapped with R-X or RW- permissions. This is > due to the ambiguity in the meaning of the EFI_MEMORY_RO/EFI_MEMORY_XP > attributes when used in the GCD memory map, i.e., between signifying > that an underlying RAM region has the controls to be configures as s/configures/configured/ > read-only or non-executable, and signifying that the contents of a > certain UEFI memory region allow them to be mapped with certain > restricted permissions. > > So let's check the policy in PcdDxeNxMemoryProtectionPolicy directly, > and set the EFI_MEMORY_XP attribute if appropriate for > EfiConventionalMemory regions. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/HighMemDxe/HighMemDxe.c | 18 ++++++++++++++++-- > ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 1 + > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > index 22f738279b20..853660437cb0 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c > @@ -37,6 +37,7 @@ InitializeHighMemDxe ( > UINTN AddressCells, SizeCells; > UINT64 CurBase; > UINT64 CurSize; > + UINT64 Attributes; > > Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, > (VOID **)&FdtClient); > @@ -77,8 +78,21 @@ InitializeHighMemDxe ( > continue; > } > > - Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, > - EFI_MEMORY_WB); > + // > + // Take care not to strip any permission attributes that will have been > + // set by DxeCore on the region we just added if a strict permission > + // policy is in effect for EfiConventionalMemory regions. > + // Unfortunately, we cannot interrogate the GCD memory space map for > + // those permissions, since they are not recorded there (for historical > + // reasons), so check the policy directly. > + // > + Attributes = EFI_MEMORY_WB; > + if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & > + (1UL << (UINT32)EfiConventionalMemory)) != 0) { I think you should be using 1ULL here as constant, for consistency, as in ARM and (somewhat uselessly here) in Ia32 builds, 1UL won't have type UINT64 (= unsigned long long). But then again, 1ULL cannot be portably shifted with << (we can only do that up to UINTN), so ultimately I suggest LShiftU64() here. Or else, if we're sure that it's going to fit in a UINT32, use "1U". With those fixed up: Reviewed-by: Laszlo Ersek Thanks, Laszlo > + Attributes |= EFI_MEMORY_XP; > + } > + > + Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes); > > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, > diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > index 3661cfd8c80c..89c743ebe058 100644 > --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf > @@ -45,6 +45,7 @@ [Protocols] > > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemoryBase > + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy > > [Depex] > gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid >