From patchwork Fri Apr 8 15:56:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matt Fleming X-Patchwork-Id: 8784321 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 C97B19F71A for ; Fri, 8 Apr 2016 15:58:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 7236C2026D for ; Fri, 8 Apr 2016 15:58:24 +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 02FA420295 for ; Fri, 8 Apr 2016 15:58:23 +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 1aoYly-00084Z-7L; Fri, 08 Apr 2016 15:56:38 +0000 Received: from mail-wm0-x232.google.com ([2a00:1450:400c:c09::232]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1aoYlt-00080Q-V5 for linux-arm-kernel@lists.infradead.org; Fri, 08 Apr 2016 15:56:36 +0000 Received: by mail-wm0-x232.google.com with SMTP id u206so27963693wme.1 for ; Fri, 08 Apr 2016 08:56:11 -0700 (PDT) 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=PoGkcGtZITszBj1FrcfSg9REuScuL5/z7d3P6njnt/Q=; b=jBNdDPkKpfMfOFEFFNLLkZuZRuJaa9um7c/EUoYb1j1twjQJa6kATAecDTrQZum9IH tAq2tIPTmeAJzwFJwUgpO7e78Mhigv5VDcj+f8dYBtjDIsNVLvlRafUkvueenNnY6Lc1 2RR/N1DMMszCxj5uF8cyA+Jr9w68X2jdu/lUhMJI4107HdUyQTSLkBjb26X7j9LyeBQC 7JgKROVfhfsy7okbe5UJgnCPqsA7yQt7ItIAMmujtNfJA2CUQaVZ37C9rnrc9jBZqpwZ tJ8H2Ji/zVbDaaVAN066oHq834uJQ1AfiefIzPUK4Hd+mFs0byk5UW/lK/IVPbmeJxQT jkTw== 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=PoGkcGtZITszBj1FrcfSg9REuScuL5/z7d3P6njnt/Q=; b=YJrp/1nmsX4YZPxCayww1BGG7BCEbpwSZv+JNbJSIwlNUbaIoBsjVD2gPQX6waN8PY cQVWCsSHiIWC6QUe+QFwFKWo3hse40FUwOHsei2ZimyaLp20L11FiDZG/dg28UKSsZPZ p8rpZrx8o/+KRK0u1vRahRIoJVTxS+CY70boVvgmrO6hTce1LHrC+lK3szOdMoaiMiZM OaFnX0e5IAMO1s5AIEmrXr6khKC2Y7PF4bmuJZ+LTDEs19N1Im4xsdnhmBPVl4xI4r3W gr2CiZZwREUr4t9M6WqWKkmwWgtzh8rGEy1LncENhrPhgyV93fWO433Bo+n7gd+paA4F PCCw== X-Gm-Message-State: AD7BkJJQcciSLmYt+vGTTHRaGBEXZqbcKom6gq5M7xkM+gYdvdfx1c5dx8KJ5dnaR0mGTQ== X-Received: by 10.28.156.10 with SMTP id f10mr4799724wme.42.1460130970372; Fri, 08 Apr 2016 08:56:10 -0700 (PDT) Received: from localhost (bcdc58e5.skybroadband.com. [188.220.88.229]) by smtp.gmail.com with ESMTPSA id da5sm14091272wjb.25.2016.04.08.08.56.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Apr 2016 08:56:09 -0700 (PDT) Date: Fri, 8 Apr 2016 16:56:08 +0100 From: Matt Fleming To: Ard Biesheuvel Subject: Re: [PATCH v2 4/5] efi: implement generic support for the Memory Attributes table Message-ID: <20160408155608.GQ2701@codeblueprint.co.uk> References: <1459355933-13529-1-git-send-email-ard.biesheuvel@linaro.org> <1459355933-13529-5-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1459355933-13529-5-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-20160408_085634_397076_FFDBDDF8 X-CRM114-Status: GOOD ( 43.57 ) 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, catalin.marinas@arm.com, will.deacon@arm.com, 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=-5.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 Wed, 30 Mar, at 06:38:52PM, Ard Biesheuvel wrote: > This implements shared support for discovering the presence of the > Memory Attributes table, and for parsing and validating its contents. > > The table is validated against the construction rules in the UEFI spec. > Since this is a new table, it makes sense to complain if we encounter > a table that does not follow those rules. > > The parsing and validation routine takes a callback that can be specified > per architecture, that gets passed each unique validated region, with the > virtual address retrieved from the ordinary memory map. > > Signed-off-by: Ard Biesheuvel > --- > drivers/firmware/efi/Makefile | 2 +- > drivers/firmware/efi/memattr.c | 170 ++++++++++++++++++++ > include/linux/efi.h | 13 ++ > 3 files changed, 184 insertions(+), 1 deletion(-) This commit breaks ia64 because it doesn't provide the global 'memmap' variable, drivers/built-in.o: In function `efi_memattr_apply_permissions': (.init.text+0x1f2b0): undefined reference to `memmap' drivers/built-in.o: In function `efi_memattr_apply_permissions': (.init.text+0x1f2b1): undefined reference to `memmap' I was surprised that the kbuild bot didn't let you know, but then I noticed that LKML isn't on Cc. This is yet another reminder that we need to drop 'memmap' on the floor. I'll send out the two patches I have to do that. Apart from that, this looks pretty good. There are a few comments below, but they're trivial changes that, if you agree, I can just fold in when applying this patch. If not, feel free to rework it. > diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c > new file mode 100644 > index 000000000000..c55683937f4a > --- /dev/null > +++ b/drivers/firmware/efi/memattr.c > @@ -0,0 +1,170 @@ > +/* > + * Copyright (C) 2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#define pr_fmt(fmt) "efi: " fmt > + > +#include > +#include > +#include > +#include > + > +#include > + > +static int __initdata tbl_size; > + > +/* > + * Reserve the memory associated with the Memory Attributes configuration > + * table, if it exists. > + */ > +int __init efi_memattr_init(void) > +{ > + efi_memory_attributes_table_t *tbl; > + > + if (efi.mem_attr_table == EFI_INVALID_TABLE_ADDR) > + return 0; > + > + tbl = early_memremap(efi.mem_attr_table, sizeof(*tbl)); > + if (!tbl) { > + pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n", > + efi.mem_attr_table); > + return -ENOMEM; > + } > + > + if (tbl->version > 1) { > + pr_warn("Unexpected EFI Memory Attribute table version %d\n", > + tbl->version); > + tbl_size = 0; Isn't it unnecessary to reset 'tbl_size'? > + goto unmap; > + } We should check that the desc_size values match between efi_memory_attributes_table_t and efi_memory_desc_t while we're here validating the table anyway. > + > + tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size; > + memblock_reserve(efi.mem_attr_table, tbl_size); > + > +unmap: > + early_memunmap(tbl, sizeof(*tbl)); > + return 0; > +} > + > +/* > + * Returns a copy @out of the UEFI memory descriptor @in if it is covered > + * entirely by a UEFI memory map entry with matching attributes. The virtual > + * address of @out is set according to the matching entry that was found. > + */ > +static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out) > +{ > + u64 in_paddr = in->phys_addr; > + u64 in_size = in->num_pages << EFI_PAGE_SHIFT; > + efi_memory_desc_t *md; > + > + if (in->type != EFI_RUNTIME_SERVICES_CODE && > + in->type != EFI_RUNTIME_SERVICES_DATA) { > + pr_warn("MEMATTR table entry type should be RuntimeServiceCode/Data\n"); > + return false; > + } > + > + if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) { > + pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n"); > + return false; > + } > + > + if (PAGE_SIZE > EFI_PAGE_SIZE && > + (!PAGE_ALIGNED(in->phys_addr) || > + !PAGE_ALIGNED(in->num_pages << EFI_PAGE_SHIFT))) { > + /* > + * Since arm64 may execute with page sizes of up to 64 KB, the > + * UEFI spec mandates that RuntimeServices memory regions must > + * be 64 KB aligned. We need to validate this here since we will > + * not be able to tighten permissions on such regions without > + * affecting adjacent regions. > + */ > + pr_warn("MEMATTR table entry misaligned\n"); > + return false; > + } Out of curiosity did you check whether the compiler optimises this out when PAGE_SIZE <= EFI_PAGE_SIZE? I would expect so, and if you haven't already done it already I can check. > + > + for_each_efi_memory_desc(&memmap, md) { > + u64 md_paddr = md->phys_addr; > + u64 md_size = md->num_pages << EFI_PAGE_SHIFT; > + > + if (!(md->attribute & EFI_MEMORY_RUNTIME)) > + continue; > + if (md->virt_addr == 0) { > + /* no virtual mapping has been installed by the stub */ > + break; > + } > + > + if (md_paddr > in_paddr || (in_paddr - md_paddr) >= md_size) > + continue; > + > + /* > + * This entry covers the start of @in, check whether > + * it covers the end as well. > + */ > + if (md_paddr + md_size < in_paddr + in_size) { > + pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n"); > + return false; > + } Here's an interesting tidbit: We never use "UEFI" in strings on x86, we always use "EFI", and of particular relevance to this feature is that "EFI" is used to describe both the memory and memory attribute table in the spec. How do you feel about the patch at the end of this email? While I'm OK with breaking the 80 column rule in exceptional circumstances, it was pretty straight forward to update the pr_fmt() macro and cut out a few words from the strings to keep this all within 80 columns. > + > + if (md->type != in->type) { > + pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n"); > + return false; > + } > + > + *out = *in; > + out->virt_addr = in_paddr + > + (md->virt_addr - md->phys_addr); For the benefit of future search-and-replace patches, we should use md_paddr here instead of md->phys_addr. > + return true; > + } > + return false; > +} > + > +/* > + * To be called after the EFI page tables have been populated. If a memory > + * attributes table is available, its contents will be used to update the > + * mappings with tightened permissions as described by the table. > + * This requires the UEFI memory map to have already been populated with > + * virtual addresses. > + */ > +int __init efi_memattr_apply_permissions(struct mm_struct *mm, > + efi_memattr_perm_setter fn) > +{ > + efi_memory_attributes_table_t *tbl; > + int i, ret; > + > + if (tbl_size <= sizeof(*tbl)) > + return 0; We need to check efi_enabled(EFI_MEMMAP) before we start traversing the memory map in entry_is_valid(). > + > + tbl = memremap(efi.mem_attr_table, tbl_size, MEMREMAP_WB); > + if (!tbl) { > + pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n", > + efi.mem_attr_table); > + return -ENOMEM; > + } > + > + if (efi_enabled(EFI_DBG)) > + pr_info("Processing UEFI Memory Attributes table:\n"); > + > + for (i = ret = 0; ret == 0 && i < tbl->num_entries; i++) { > + efi_memory_desc_t md; > + unsigned long size; > + bool valid; > + char buf[64]; > + > + valid = entry_is_valid((void *)tbl->entry + i * tbl->desc_size, > + &md); > + size = md.num_pages << EFI_PAGE_SHIFT; > + if (efi_enabled(EFI_DBG) || !valid) > + pr_info(" 0x%012llx-0x%012llx %s\n", md.phys_addr, > + md.phys_addr + size - 1, > + efi_md_typeattr_format(buf, sizeof(buf), &md)); This needs to include some indication of whether we printed this because it was invalid or debug EFI_DBG is set. I've inserted a "!" at the start of the line if it's invalid but if you've got other ideas let me know. > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 346d01ad7cca..277a9bb4e587 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -970,6 +970,19 @@ extern void __init efi_fake_memmap(void); > static inline void efi_fake_memmap(void) { } > #endif > > +/* > + * efi_memattr_perm_setter - arch specific callback function passed into > + * efi_memattr_apply_permissions() that updates the > + * mapping permissions described by the second > + * argument in the page tables referrred to by the Too many 'r's in referred. diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c index 23973868e186..a983c1b63f07 100644 --- a/drivers/firmware/efi/memattr.c +++ b/drivers/firmware/efi/memattr.c @@ -6,7 +6,7 @@ * published by the Free Software Foundation. */ -#define pr_fmt(fmt) "efi: " fmt +#define pr_fmt(fmt) "efi: memattr: " fmt #include #include @@ -30,15 +30,20 @@ int __init efi_memattr_init(void) tbl = early_memremap(efi.mem_attr_table, sizeof(*tbl)); if (!tbl) { - pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n", + pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n", efi.mem_attr_table); return -ENOMEM; } if (tbl->version > 1) { - pr_warn("Unexpected EFI Memory Attribute table version %d\n", + pr_warn("Unexpected EFI Memory Attributes table version %d\n", tbl->version); - tbl_size = 0; + goto unmap; + } + + if (tbl->desc_size != efi.memmap->desc_size) { + pr_warn("Descriptor size different from EFI memory map %d\n", + tbl->desc_size); goto unmap; } @@ -65,12 +70,12 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out) if (in->type != EFI_RUNTIME_SERVICES_CODE && in->type != EFI_RUNTIME_SERVICES_DATA) { - pr_warn("MEMATTR table entry type should be RuntimeServiceCode/Data\n"); + pr_warn("Entry type should be RuntimeServiceCode/Data\n"); return false; } if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) { - pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n"); + pr_warn("Entry attributes invalid: RO and XP bits both cleared\n"); return false; } @@ -84,7 +89,7 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out) * not be able to tighten permissions on such regions without * affecting adjacent regions. */ - pr_warn("MEMATTR table entry misaligned\n"); + pr_warn("Entry address region misaligned\n"); return false; } @@ -107,22 +112,21 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out) * it covers the end as well. */ if (md_paddr + md_size < in_paddr + in_size) { - pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n"); + pr_warn("Entry covers multiple EFI memory map regions\n"); return false; } if (md->type != in->type) { - pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n"); + pr_warn("Entry type deviates from EFI memory map region type\n"); return false; } - out->virt_addr = in_paddr + - (md->virt_addr - md->phys_addr); + out->virt_addr = in_paddr + (md->virt_addr - md_paddr); + return true; } - pr_warn("MEMATTR table entry does not having a matching entry in the memory map\n"); - + pr_warn("No matching entry found in the EFI memory map\n"); return false; } @@ -142,15 +146,24 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm, if (tbl_size <= sizeof(*tbl)) return 0; + /* + * We need the EFI memory map to be setup so we can use it to + * lookup the virtual addresses of all entries in the of EFI + * Memory Attributes table. If it isn't available, this + * function should not be called. + */ + if (WARN_ON(!efi_enabled(EFI_MEMMAP))) + return 0; + tbl = memremap(efi.mem_attr_table, tbl_size, MEMREMAP_WB); if (!tbl) { - pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n", + pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n", efi.mem_attr_table); return -ENOMEM; } if (efi_enabled(EFI_DBG)) - pr_info("Processing UEFI Memory Attributes table:\n"); + pr_info("Processing EFI Memory Attributes table:\n"); for (i = ret = 0; ret == 0 && i < tbl->num_entries; i++) { efi_memory_desc_t md; @@ -162,7 +175,8 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm, &md); size = md.num_pages << EFI_PAGE_SHIFT; if (efi_enabled(EFI_DBG) || !valid) - pr_info(" 0x%012llx-0x%012llx %s\n", md.phys_addr, + pr_info("%s 0x%012llx-0x%012llx %s\n", + valid ? "" : "!", md.phys_addr, md.phys_addr + size - 1, efi_md_typeattr_format(buf, sizeof(buf), &md)); diff --git a/include/linux/efi.h b/include/linux/efi.h index bb404979f4bd..c80895e4e505 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -974,8 +974,8 @@ static inline void efi_fake_memmap(void) { } * efi_memattr_perm_setter - arch specific callback function passed into * efi_memattr_apply_permissions() that updates the * mapping permissions described by the second - * argument in the page tables referrred to by the - * first argument + * argument in the page tables referred to by the + * first argument. */ typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);