From patchwork Wed Mar 2 12:10:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matt Fleming X-Patchwork-Id: 8480501 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C84DF9F314 for ; Wed, 2 Mar 2016 12:12:55 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BF1192037C for ; Wed, 2 Mar 2016 12:12:54 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ACE1620173 for ; Wed, 2 Mar 2016 12:12:53 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ab5cM-000429-83; Wed, 02 Mar 2016 12:11:02 +0000 Received: from mail-wm0-x235.google.com ([2a00:1450:400c:c09::235]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ab5cJ-00040D-I0 for linux-arm-kernel@lists.infradead.org; Wed, 02 Mar 2016 12:11:00 +0000 Received: by mail-wm0-x235.google.com with SMTP id n186so82295549wmn.1 for ; Wed, 02 Mar 2016 04:10:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codeblueprint-co-uk.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=bbP/P9qwdoqkJBQrqtR6hJWWPI9sE5A8obQOMIV/6A4=; b=PD0P1S6KGruZhuiG6KoYNWSb5T2QC+zIc/XJ+lAhbz5Jd2kV6XHa/DaCS4ZAI75efP ZE/thEiqmxsD5j9R5IMAxXx1H290mz1cIdmLYfmYkmDJFbTYcd5gCcW7otSNSzdB3g1/ kuh/rCVKI36gJ6n2NhwMUXAa3Z0zEh4AqhrK5dssXt1TuRk1q5Zo3Bcm58QS6ZmYzRyx JkjRaAZ0cHCGVVuyDY9aaIYXG8yWxuzjuc7FBTczZgUcUTCpQJuHdLFsbEYwwhUArOc0 nqPUiBnx2iouVUu5p1xPoQUzu/0/9srHzyYR4NBFSOz5+ED5xJcbGdy0coHUN3Vi6yfC pvIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=bbP/P9qwdoqkJBQrqtR6hJWWPI9sE5A8obQOMIV/6A4=; b=TmQb02GUZhr6gmr1VFqnk9s4AAxTxSVxfkSIx66GcGqMVVBTwH5jt167yJSLvaQjrH 54MPOQVu3yGuJqzNn3FfJ+wUNS357gdkBgeY40du/HV+yozuZjTcWVpy/xsBOkBRgcQd Kt2rbGsYtmVgwnNMJSD4yMqjq9AVOVG0AA469dbwJHf6aO8TGUkzPaRlDzOktVBJx79u nbr0RzXzwZmeC+qwYZtOgKax/NEn3o9DlETWShnvpeEN6i+wBGrLQfoLGqG5GPFdH0U5 w7Y/NmaNljo8PugQOTkJmWe4zQ1hB/BvIHmT6ZCPJhYs7raIbODhUh3AU3EyGYlzIefZ lFpg== X-Gm-Message-State: AD7BkJLpb24zMPncQQ9JhRRLNEXTmOkD0tMGnHqx4IRMHPs1FVbWh7AF9ige6BCBF7AopA== X-Received: by 10.28.184.137 with SMTP id i131mr4097902wmf.96.1456920638133; Wed, 02 Mar 2016 04:10:38 -0800 (PST) Received: from localhost (bcdc58e5.skybroadband.com. [188.220.88.229]) by smtp.gmail.com with ESMTPSA id hx10sm27905346wjb.25.2016.03.02.04.10.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 Mar 2016 04:10:37 -0800 (PST) Date: Wed, 2 Mar 2016 12:10:36 +0000 From: Matt Fleming To: Ard Biesheuvel Subject: Re: [PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions Message-ID: <20160302121036.GD2649@codeblueprint.co.uk> References: <1456151158-25849-1-git-send-email-ard.biesheuvel@linaro.org> <1456151158-25849-3-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1456151158-25849-3-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.24+41 (02bc14ed1569) (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160302_041059_764082_F278EDCE X-CRM114-Status: GOOD ( 25.45 ) X-Spam-Score: -2.6 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, linux-efi@vger.kernel.org, linux@arm.linux.org.uk, leif.lindholm@linaro.org, sai.praneeth.prakhya@intel.com, pjones@redhat.com, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, 22 Feb, at 03:25:55PM, Ard Biesheuvel wrote: > Recent UEFI versions expose permission attributes for runtime services > memory regions, either in the UEFI memory map or in the separate memory > attributes table. This allows the kernel to map these regions with > stricter permissions, rather than the RWX permissions that are used by > default. So wire this up in our mapping routine. > > Note that in the absence of permission attributes, we still only map > regions of type EFI_RUNTIME_SERVICE_CODE with the executable bit set. > Also, we base the mapping attributes of EFI_MEMORY_MAPPED_IO on the > type directly rather than on the absence of the EFI_MEMORY_WB attribute. > This is more correct, but is also required for compatibility with the > upcoming support for the Memory Attributes Table, which only carries > permission attributes, not memory type attributes. > > Signed-off-by: Ard Biesheuvel > --- > arch/arm64/kernel/efi.c | 27 ++++++++++++++++---- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index b6abc852f2a1..3364408c154f 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -24,15 +24,32 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) > /* > * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be > * executable, everything else can be mapped with the XN bits > - * set. > + * set. Also take the new (optional) RO/XP bits into account. > */ > - if ((md->attribute & EFI_MEMORY_WB) == 0) > + if (md->type == EFI_MEMORY_MAPPED_IO) > prot_val = PROT_DEVICE_nGnRE; > - else if (md->type == EFI_RUNTIME_SERVICES_CODE || > - !PAGE_ALIGNED(md->phys_addr)) > + else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), > + "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?")) > + /* > + * If the region is not aligned to the page size of the OS, we > + * can not use strict permissions, since that would also affect > + * the mapping attributes of the adjacent regions. > + */ > prot_val = pgprot_val(PAGE_KERNEL_EXEC); > - else > + else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) == > + (EFI_MEMORY_XP | EFI_MEMORY_RO)) > + /* R-- */ > + prot_val = pgprot_val(PAGE_KERNEL_RO); > + else if (md->attribute & EFI_MEMORY_RO) > + /* R-X */ > + prot_val = pgprot_val(PAGE_KERNEL_ROX); > + else if (md->attribute & EFI_MEMORY_XP || > + md->type != EFI_RUNTIME_SERVICES_CODE) > + /* RW- */ > prot_val = pgprot_val(PAGE_KERNEL); > + else > + /* RWX */ > + prot_val = pgprot_val(PAGE_KERNEL_EXEC); > > create_pgd_mapping(mm, md->phys_addr, md->virt_addr, > md->num_pages << EFI_PAGE_SHIFT, The actual logic looks fine but it seems like there's quite a lot going on in this function which is fairly difficult to decipher with the if/else if clauses. Would you be open to splitting this out a little? It's just a suggestion, but maybe something like this, diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 3364408c154f..33a6da160a50 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -17,39 +17,48 @@ #include -int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) +/* + * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be + * executable, everything else can be mapped with the XN bits + * set. Also take the new (optional) RO/XP bits into account. + */ +static __init pteval_t create_mapping_protection(efi_memory_desc_t *md) { - pteval_t prot_val; + u64 attr = md->attribute; + u32 type = md->type; - /* - * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be - * executable, everything else can be mapped with the XN bits - * set. Also take the new (optional) RO/XP bits into account. - */ - if (md->type == EFI_MEMORY_MAPPED_IO) - prot_val = PROT_DEVICE_nGnRE; - else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), - "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?")) + if (type == EFI_MEMORY_MAPPED_IO) + return PROT_DEVICE_nGnRE; + + if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), + "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?")) /* * If the region is not aligned to the page size of the OS, we * can not use strict permissions, since that would also affect * the mapping attributes of the adjacent regions. */ - prot_val = pgprot_val(PAGE_KERNEL_EXEC); - else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) == - (EFI_MEMORY_XP | EFI_MEMORY_RO)) - /* R-- */ - prot_val = pgprot_val(PAGE_KERNEL_RO); - else if (md->attribute & EFI_MEMORY_RO) - /* R-X */ - prot_val = pgprot_val(PAGE_KERNEL_ROX); - else if (md->attribute & EFI_MEMORY_XP || - md->type != EFI_RUNTIME_SERVICES_CODE) - /* RW- */ - prot_val = pgprot_val(PAGE_KERNEL); - else - /* RWX */ - prot_val = pgprot_val(PAGE_KERNEL_EXEC); + return pgprot_val(PAGE_KERNEL_EXEC); + + /* R-- */ + if ((attr & (EFI_MEMORY_XP | EFI_MEMORY_RO)) == + (EFI_MEMORY_XP | EFI_MEMORY_RO)) + return pgprot_val(PAGE_KERNEL_RO); + + /* R-X */ + if (attr & EFI_MEMORY_RO) + return pgprot_val(PAGE_KERNEL_ROX); + + /* RW- */ + if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE) + return pgprot_val(PAGE_KERNEL); + + /* RWX */ + return pgprot_val(PAGE_KERNEL_EXEC); +} + +int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) +{ + pteval_t prot_val = create_mapping_protection(md); create_pgd_mapping(mm, md->phys_addr, md->virt_addr, md->num_pages << EFI_PAGE_SHIFT,