From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22a.google.com (mail-it0-x22a.google.com [IPv6:2607:f8b0:4001:c0b::22a]) (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 6A21E821B9 for ; Wed, 1 Mar 2017 03:44:39 -0800 (PST) Received: by mail-it0-x22a.google.com with SMTP id 203so28511315ith.0 for ; Wed, 01 Mar 2017 03:44:39 -0800 (PST) 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=NYigOLOU2CYEvVSPEGY0wLa5PnB6BlFX35ZcfFszaP0=; b=hwaHgO85ovfTkcqdu5kdhOT5iKKJN1wv4viq7jrAQ9XNIACWZNEpvkj9dJWKA8Zlb7 3pqfrpb3yF3NXuPD4Yi8WeKWnkq+l1FMOWuAhCj6mhWzp3cgB3/rLq+pdWR544YHEHsZ jPDWuLP2b+Der0PnmjxaH2OhjKSP5V4lqJymA= 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=NYigOLOU2CYEvVSPEGY0wLa5PnB6BlFX35ZcfFszaP0=; b=gBVv1k5StNmI5QYhBp3ICJqc0MyehBSd/ILXGD2PIlYK3COauzeS0f6J3bfg00YEYF 3lIbxuTpgfW5ADZoFsKTPhDHaacaXnPXqN+NrpsRwGHR/hvHzl9Cy4ZiQcQBlJsg8tZb wKEHsvRogvQEhSSUKFIOVDvQLljPOQ0lYF21sZhOqMtT6QSfMbNRpstPMVtbbUTHwAD9 /P0vmJ59xbujZbdjQIm0t+fT3vRkrVbOtxcwc2rJKq7JbxVTi8hveQvK+EcbB5dLbM1O ohiuz61w2LD7gj5I6XmiKr4jL+vr5RKkIhRRBMCw9VkM55TE72OzrMZPKNUDyQ0ndiDX wNog== X-Gm-Message-State: AMke39mn+zk5q3qzHTSMlgvB3RR7SEaJtQNdCSf1UB+yzCbYH4CnIt9FqeUj8sgPsZ4vhPf71gSJtsooAj3KKNMh X-Received: by 10.36.77.10 with SMTP id l10mr3657554itb.59.1488368678653; Wed, 01 Mar 2017 03:44:38 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Wed, 1 Mar 2017 03:44:38 -0800 (PST) In-Reply-To: <650c64bd-ed0a-bd0e-5fce-3cd1ab0c6293@redhat.com> References: <1488290169-10701-1-git-send-email-ard.biesheuvel@linaro.org> <650c64bd-ed0a-bd0e-5fce-3cd1ab0c6293@redhat.com> From: Ard Biesheuvel Date: Wed, 1 Mar 2017 11:44:38 +0000 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" 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: Wed, 01 Mar 2017 11:44:39 -0000 Content-Type: text/plain; charset=UTF-8 On 28 February 2017 at 21:14, Laszlo Ersek wrote: > 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". > I opted for this solution, given that the RHS is a constant < 32 (EfiConventionalMemory) > With those fixed up: > > Reviewed-by: Laszlo Ersek > Pushed, thanks.