From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::343; helo=mail-wm1-x343.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x343.google.com (mail-wm1-x343.google.com [IPv6:2a00:1450:4864:20::343]) (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 E94A22117FD4C for ; Mon, 14 Jan 2019 07:07:03 -0800 (PST) Received: by mail-wm1-x343.google.com with SMTP id y8so9156701wmi.4 for ; Mon, 14 Jan 2019 07:07:03 -0800 (PST) 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=PGFEgpSf/2NTeOqnXfi4EkX40D4WJyrBmzgX6NpEdaQ=; b=gh1U4YYFqvg+ulQ0+cpoDyIl0z8M3jMmspqxhu3xSZ3hico2J81YzDXLUKCjcdQbvo jgXqJfzirHMP9AGq58Hm9WlAXpDQUxt084maZ0s64pqRAKCYMKcxE+xWiTSgz3reiuOB KMGVdYo2leMh+tcaj72vCq+b1lf55tJ4DEvyQ= 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=PGFEgpSf/2NTeOqnXfi4EkX40D4WJyrBmzgX6NpEdaQ=; b=illvtis+w7fN0rLnJURkkjt3CEi/foK7s8Lj+NqQxYpvPssB+V8yXxf4GG84Y3mdIl hDx2+BdcsA4hSMmtPr8aArrSzPK1g8yAekJvOsR2ESJscHafNrvZvq/Y4Q+tzOKjAvuf qUu8ijtbiok7kQk6UgsBCBD7S/+ARJ5I4CzTUyHK2vcLpY1XhRlJ0pplAWC8HDKv3gpE 6laDrea74zIs1ZY//4fQZvQLkqrpn39Z7a6gMwyDaJxkTfsxY6bBHhW/9wvEbgWzVQFF K4o18mflDHglxNygpNp7SL7Gywptr7nyMLV43IT+NPlPFJrexq5IHavGG88dlg0lzIM2 /Q4Q== X-Gm-Message-State: AJcUukeucoEpLQmCM5e2rOP5m6H7iLFBHBaR0/2Ki3B7DQbPOKGoftYb D1l0ncFgjASTrE82DoHrocz2yA== X-Google-Smtp-Source: ALg8bN69SGCxM8A9GDhR5IJLOKCSq/aVf+uTA+du8nRUnatE2oQTRpw2ky9PO0iNSE20Ca2y0l3+JA== X-Received: by 2002:a1c:e345:: with SMTP id a66mr12011580wmh.12.1547478422218; Mon, 14 Jan 2019 07:07:02 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id m4sm28310421wmi.3.2019.01.14.07.07.00 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 14 Jan 2019 07:07:01 -0800 (PST) Date: Mon, 14 Jan 2019 15:06:59 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: Charles Garcia-Tobin , "edk2-devel@lists.01.org" Message-ID: <20190114150659.tvvrqctsjjed6gfy@bivouac.eciton.net> References: <20190107071504.2431-1-ard.biesheuvel@linaro.org> <20190107071504.2431-4-ard.biesheuvel@linaro.org> <20190114142901.lkwgte6mlsv6bjyk@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 3/5] ArmPkg/ArmMmuLib AARCH64: implement support for EFI_MEMORY_RP permissions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Jan 2019 15:07:04 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jan 14, 2019 at 03:59:08PM +0100, Ard Biesheuvel wrote: > On Mon, 14 Jan 2019 at 15:29, Leif Lindholm wrote: > > > > On Mon, Jan 07, 2019 at 08:15:02AM +0100, Ard Biesheuvel wrote: > > > Wire up the access flag (AF) page table attribute to the EFI_MEMORY_RP > > > permission attribute, so that attempts to read from such a region will > > > trigger an access flag fault. > > > > > > Note that this is a stronger notion than just read protection, since > > > it now implies that any write or execute attempt is trapped as well. > > > However, this does not really matter in practice since we never assume > > > that a read protected page is writable or executable, and StackGuard > > > and HeapGuard (which are the primary users of this facility) certainly > > > don't care. > > > > So ... I'm cautiously positive to this patch. > > But this use does contradict the UEFI spec (2.7a, 2.3.6.1 Memory > > types), which says EFI_MEMORY_RP is "not used or defined" for AArch64. > > > > Charles? > > Not defined by the spec means we can use it do whatever we bloody want > with it, at least that is what a typical compiler engineer will tell > you :-) Not defined, yes. Not used, less so. For a reciprocal compiler engineer analogy, think pointer tagging :) > I think there was a pending ECR to update the AArch64 binding code to > reflect reality, but I don't think we included EFI_MEMORY_RP. Right. Anyway, I'm reasonably happy with stretching the rules slightly here, and I believe it to be safe, but I do want Charles' take on it. / Leif > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Ard Biesheuvel > > > --- > > > ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 5 +++-- > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 14 +++++++++++--- > > > 2 files changed, 14 insertions(+), 5 deletions(-) > > > > > > diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c > > > index 3e216c7cb235..e62e3fa87112 100644 > > > --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c > > > +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c > > > @@ -223,8 +223,9 @@ EfiAttributeToArmAttribute ( > > > ArmAttributes = TT_ATTR_INDX_MASK; > > > } > > > > > > - // Set the access flag to match the block attributes > > > - ArmAttributes |= TT_AF; > > > + if ((EfiAttributes & EFI_MEMORY_RP) == 0) { > > > + ArmAttributes |= TT_AF; > > > + } > > > > > > // Determine protection attributes > > > if (EfiAttributes & EFI_MEMORY_RO) { > > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > > index e1fabfcbea14..b59c081a7e49 100644 > > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > > @@ -102,6 +102,10 @@ PageAttributeToGcdAttribute ( > > > GcdAttributes |= EFI_MEMORY_XP; > > > } > > > > > > + if ((PageAttributes & TT_AF) == 0) { > > > + GcdAttributes |= EFI_MEMORY_RP; > > > + } > > > + > > > return GcdAttributes; > > > } > > > > > > @@ -451,7 +455,11 @@ GcdAttributeToPageAttribute ( > > > PageAttributes |= TT_AP_RO_RO; > > > } > > > > > > - return PageAttributes | TT_AF; > > > + if ((GcdAttributes & EFI_MEMORY_RP) == 0) { > > > + PageAttributes |= TT_AF; > > > + } > > > + > > > + return PageAttributes; > > > } > > > > > > EFI_STATUS > > > @@ -474,9 +482,9 @@ ArmSetMemoryAttributes ( > > > // No memory type was set in Attributes, so we are going to update the > > > // permissions only. > > > // > > > - PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK; > > > + PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF; > > > PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK | > > > - TT_PXN_MASK | TT_XN_MASK); > > > + TT_PXN_MASK | TT_XN_MASK | TT_AF); > > > } > > > > > > TranslationTable = ArmGetTTBR0BaseAddress (); > > > -- > > > 2.20.1 > > >