From patchwork Thu Sep 10 14:51:05 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 53384 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-lb0-f199.google.com (mail-lb0-f199.google.com [209.85.217.199]) by patches.linaro.org (Postfix) with ESMTPS id AEE5122B26 for ; Thu, 10 Sep 2015 14:59:21 +0000 (UTC) Received: by lbcjc2 with SMTP id jc2sf14856898lbc.0 for ; Thu, 10 Sep 2015 07:59:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:precedence:list-id:list-unsubscribe :list-archive:list-post:list-help:list-subscribe:cc:content-type :content-transfer-encoding:sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list; bh=HJm8/pzYznOUDtWsSChfV/n1xnSbQYDkHd5BVE67nT4=; b=G5e4biCdB9tbbrck/ommBBKblEte2aL9fNO7gemOq4xjhzWNndMH8vfAbg1KbIoxep Xx4HVmNEV60EkRlFhyd0L2/tIorkCKPBjb3kXP/RDbCxHis1jqF2JXo0FP07Hf1dZ3rq 9CJuOlKPPpQGXagycNpTNWkMxzhW+Cpc2GtfeDJi3xNqS9fc/X1F9MhwisvVIhjk1qfT Sb97iKAYNgaQL7Ujj+Uu81vz1Rh8Qov16YZQyN3MR5oi+XoQc5B07pns3NrgXzF9vtUF WjHDmbFdrP6IW3QOOHK4wY1JqYZMpr8p3t25kBWSuw2L+QWddoR8vpBH7NneTn0SAxAB FYcA== X-Gm-Message-State: ALoCoQmPVHvnrTdTS7e17tUk2zE9ImzXuu9tHMSb3eI26WW/3ddYU32XD4bC3FAiRsdb9JorJT/v X-Received: by 10.152.27.10 with SMTP id p10mr9886836lag.1.1441897160311; Thu, 10 Sep 2015 07:59:20 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.19.231 with SMTP id i7ls214715lae.1.gmail; Thu, 10 Sep 2015 07:59:20 -0700 (PDT) X-Received: by 10.152.19.34 with SMTP id b2mr12093520lae.79.1441897160156; Thu, 10 Sep 2015 07:59:20 -0700 (PDT) Received: from mail-lb0-f169.google.com (mail-lb0-f169.google.com. [209.85.217.169]) by mx.google.com with ESMTPS id 3si10669413lay.95.2015.09.10.07.59.19 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Sep 2015 07:59:19 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.169 as permitted sender) client-ip=209.85.217.169; Received: by lbcjc2 with SMTP id jc2so24675945lbc.0 for ; Thu, 10 Sep 2015 07:59:19 -0700 (PDT) X-Received: by 10.112.169.66 with SMTP id ac2mr35783099lbc.32.1441897159814; Thu, 10 Sep 2015 07:59:19 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.59.35 with SMTP id w3csp1028652lbq; Thu, 10 Sep 2015 07:59:18 -0700 (PDT) X-Received: by 10.66.233.97 with SMTP id tv1mr74013152pac.110.1441897158505; Thu, 10 Sep 2015 07:59:18 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id t5si19917084pbs.119.2015.09.10.07.59.17 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Sep 2015 07:59:18 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; 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 1Za3IS-0007I0-Um; Thu, 10 Sep 2015 14:57:56 +0000 Received: from mail-io0-f176.google.com ([209.85.223.176]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Za3IP-00075Z-5x for linux-arm-kernel@lists.infradead.org; Thu, 10 Sep 2015 14:57:54 +0000 Received: by ioiz6 with SMTP id z6so63503131ioi.2 for ; Thu, 10 Sep 2015 07:57:32 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.107.46.158 with SMTP id u30mr68312601iou.56.1441896665901; Thu, 10 Sep 2015 07:51:05 -0700 (PDT) Received: by 10.36.138.69 with HTTP; Thu, 10 Sep 2015 07:51:05 -0700 (PDT) In-Reply-To: <20150910140419.GH29293@leverpostej> References: <1441371986-4554-1-git-send-email-ard.biesheuvel@linaro.org> <1441782414-16284-1-git-send-email-ard.biesheuvel@linaro.org> <20150910132211.GF29293@leverpostej> <20150910140419.GH29293@leverpostej> Date: Thu, 10 Sep 2015 16:51:05 +0200 Message-ID: Subject: Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions From: Ard Biesheuvel To: Mark Rutland X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150910_075753_290642_B118071F X-CRM114-Status: GOOD ( 35.96 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.223.176 listed in list.dnswl.org] -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.223.176 listed in wl.mailspike.net] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 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: "linux-efi@vger.kernel.org" , Catalin Marinas , Will Deacon , "leif.lindholm@linaro.org" , "matt.fleming@intel.com" , "msalter@redhat.com" , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ard.biesheuvel@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.169 as permitted sender) smtp.mailfrom=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 On 10 September 2015 at 16:04, Mark Rutland wrote: >> >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c >> >> index e8ca6eaedd02..13671a9cf016 100644 >> >> --- a/arch/arm64/kernel/efi.c >> >> +++ b/arch/arm64/kernel/efi.c >> >> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void) >> >> */ >> >> if (!is_normal_ram(md)) >> >> prot = __pgprot(PROT_DEVICE_nGnRE); >> >> - else if (md->type == EFI_RUNTIME_SERVICES_CODE) >> >> + else if (md->type == EFI_RUNTIME_SERVICES_CODE || >> >> + !PAGE_ALIGNED(md->phys_addr)) >> >> prot = PAGE_KERNEL_EXEC; >> > >> > This looks coarser than necessary. For memory organised like: >> > >> > 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE >> > 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA >> > >> > We should be able to make the last 64K non-executable, but with this all >> > 128K is executable, unless I've missed something? >> > >> >> In theory, yes. But considering that >> >> a) this only affects 64 KB pages kernels, and >> b) this patch is intended for -stable >> >> I chose to keep it simple and ignore this, and just relax the >> permissions for any region that is not aligned to 64 KB. >> >> Since these regions are only mapped during Runtime Services calls, the >> window for abuse is not that large. > > Ok, that does sound reasonable. > >> > Maybe we could do a two-step pass, first mapping the data as >> > not-executable, then mapping any code pages executable (overriding any >> > overlapping portions, but only for the overlapping parts). >> > >> >> Let me have a go at that. > > Cheers! > OK so what we could do is the following: ------------8<-------------- ------------8<-------------- This will ensure that only the pages that are shared between 2 or more regions may have their permissions upgraded, but only if any of these regions requires it. I prefer the much simpler previous version, though, and I think it is more suitable for -stable. I can always follow up with an improvement like this for v4.3-late. >> >> else >> >> prot = PAGE_KERNEL; >> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c >> >> index e29560e6b40b..cb4e9c4de952 100644 >> >> --- a/drivers/firmware/efi/libstub/arm-stub.c >> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c >> >> @@ -13,6 +13,7 @@ >> >> */ >> >> >> >> #include >> >> +#include >> > >> > Sort isn't an inline in this header. I thought it wasn't safe to call >> > arbitary kernel functions from the stub? >> > >> >> We call string functions, cache maintenance functions, libfdt >> functions etc etc so it seems not everyone got the memo :-) >> >> I agree that treating vmlinux both as a static library and as a >> payload from the stub's pov is a bit sloppy, and I do remember >> discussing this, but for the life of me, I can't remember the exact >> issue, other than the use of adrp/add and adrp/ldr pairs, which we >> fixed by setting the PE/COFF section alignment to 4 KB. > > I only had a vague recollection that there was a problem, which I > thought was more to do with potential use of absolute kernel virtual > addresses, which would be incorrect in the context of an EFI > application. > That was it, of course. Unlike the x86 stub, which is built with -fPIC (as is the ARM decompressor, btw), the arm64 kernel is position dependent. Fortunately, the small code model is mostly position independent by default, but it would be good if we could spot any problems at build time. > Digging a bit, the stub code itself is safe due to commit > f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that libstub is linked into vmlinux so that does not make a different at all > isn't necessarily true of anything it calls (libfdt uses callbacks in > several places). I think the cache functions we call are all raw asm > which is position-oblivious. > I remember looking into this when doing the BE port. > We do seem to be ok so far, however. Maybe we just need to keep an eye > out. > I'd much rather restrict the code that goes into the stub somehow than deal with any absolute references. Perhaps we could reuse some of the section mismatch code in some way to tag certain code as stub-safe and do a verification pass on the binary. diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index e8ca6eaedd02..39fa2a70a7f1 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -233,6 +233,7 @@ void __init efi_init(void) static bool __init efi_virtmap_init(void) { efi_memory_desc_t *md; + u64 prev_end = 0; for_each_efi_memory_desc(&memmap, md) { u64 paddr, npages, size; @@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void) * executable, everything else can be mapped with the XN bits * set. */ - if (!is_normal_ram(md)) + if (!is_normal_ram(md)) { prot = __pgprot(PROT_DEVICE_nGnRE); - else if (md->type == EFI_RUNTIME_SERVICES_CODE) + } else if (md->type == EFI_RUNTIME_SERVICES_CODE) { prot = PAGE_KERNEL_EXEC; - else + } else { + /* + * If we are running with >4 KB pages and the current + * region shares a page frame with the preceding one, + * we should not map the leading page again since doing + * so may take its executable permissions away. + */ + if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) { + paddr += PAGE_SIZE; + size -= PAGE_SIZE; + if (!size) + continue; + } prot = PAGE_KERNEL; - + } + prev_end = paddr + size; create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); } return true;