From patchwork Mon Jul 31 15:56:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: David Woodhouse X-Patchwork-Id: 9872389 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DECEA602F0 for ; Mon, 31 Jul 2017 15:58:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CE40626E75 for ; Mon, 31 Jul 2017 15:58:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C064827C05; Mon, 31 Jul 2017 15:58:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A599D26E75 for ; Mon, 31 Jul 2017 15:58:57 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dcD3m-0004b7-1V; Mon, 31 Jul 2017 15:56:46 +0000 Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dcD3l-0004b0-4r for xen-devel@lists.xenproject.org; Mon, 31 Jul 2017 15:56:45 +0000 Received: from [85.158.137.68] by server-2.bemta-3.messagelabs.com id 3D/81-22472-C335F795; Mon, 31 Jul 2017 15:56:44 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrJIsWRWlGSWpSXmKPExsUSNTvmoq51cH2 kwdfnYhbft0xmcmD0OPzhCksAYxRrZl5SfkUCa8aq7h7Ggm9xFf1X+lkbGK9HdDFycrAInGCT mPZUqouRi0NIYAOjxNpHl9lAErwCphKLv89lBrGFBaIk5t47DRZnE9CWOLDjJAuILSKgLPH5+ UqwGmYBH4nlTU3sIDangL3E+ysvGSGGnmKUeDRjCRtEUa3Eq7Oz2SE2q0pcXNUKZHMALROU+L tDGCQsIaAhseHmMSYIu41R4t5quwmMfLOQdM9C6IAIa0q0bv/NDmFrSyxb+JoZwraV2H91JZR tKvH66EdGCFtRYkr3Q/YFjOyrGDWKU4vKUot0jSz0kooy0zNKchMzc3QNDYz1clOLixPTU3MS k4r1kvNzNzECg7megYFxB2P7Cb9DjJIcTEqivGek6iOF+JLyUyozEosz4otKc1KLDzHKcHAoS fAKBQHlBItS01Mr0jJzgHEFk5bg4FES4e0ASfMWFyTmFmemQ6ROMepyvJrw/xuTEEtefl6qlD jEDAGQoozSPLgRsBi/xCgrJczLyMDAIMRTkFqUm1mCKv+KUZyDUUmYVxlkCk9mXgncpldARzA BHSFZWgtyREkiQkqqgVE3u0/7msb+Q2au0s/P/730zSjj8hPOA5V9C+c/qVi7VTajtC7i/hkv y1uXfh9czH54qavPAjauR16vsi3MheY/OFxf8LVvWnv4RnXtdfJFAS+Oyfaqxiw3CP44SYLzS saGIEPJgvdm/8S3suvGHXo0wSM4Rj/n7+Iis1uWm2p3c/WUlvVk8ymxFGckGmoxFxUnAgCmrn DQ7AIAAA== X-Env-Sender: BATV+f5297d26ad76b06bb258+5090+infradead.org+dwmw2@twosheds .srs.infradead.org X-Msg-Ref: server-16.tower-31.messagelabs.com!1501516603!100336335!1 X-Originating-IP: [90.155.92.209] X-SpamReason: No, hits=0.0 required=7.0 tests= X-StarScan-Received: X-StarScan-Version: 9.4.25; banners=-,-,- X-VirusChecked: Checked Received: (qmail 21365 invoked from network); 31 Jul 2017 15:56:43 -0000 Received: from twosheds.infradead.org (HELO twosheds.infradead.org) (90.155.92.209) by server-16.tower-31.messagelabs.com with AES256-GCM-SHA384 encrypted SMTP; 31 Jul 2017 15:56:43 -0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=twosheds.20170209; h=Mime-Version:Date:Content-Type: References:In-Reply-To:Cc:To:From:Subject:Message-ID:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=iY3wvckU5YXMikUhsV09MXPr0LXCHjuv41U8RIliArM=; b=rIN2DNFVQPcdof2QD0JmgkYex 9d6BcJosFDukJ27L1NL9i9Pg71byZFicHkQhARyLTI37f4V0Aa2KxInDEuepaoKghHaW3qwMvR+RV TivibnXevauv+4IQDJqSdYyyPtaP9VXEwTRne8VAVX6uojjaZeWEnQjglf5DZDDbKdaI4aZSSaCU4 PCLix3tilDILRhCkRMa3iN8dD9tzMAxjFmcvR0lb6WoFT1tl6mHQkrfy8QWRUb1Xl5LqMrKJzUJ7c nrSp9f1HyfPxddLRV4f8sFczVQyo9XPeyITd1dlxxdgBUXUBhZqXF8KH7fnDAkTr9qefuHBSMzR9h YRDg1CnUQ==; Received: from [2001:8b0:10b:1:8585:20a6:d7a7:d739] by twosheds.infradead.org with esmtpsa (Exim 4.87 #1 (Red Hat Linux)) id 1dcD3e-0005p7-Ad; Mon, 31 Jul 2017 15:56:38 +0000 Message-ID: <1501516597.4771.328.camel@infradead.org> From: David Woodhouse To: Jan Beulich In-Reply-To: <597F2D860200007800103049@prv-mh.provo.novell.com> References: <1500564043.4400.15.camel@infradead.org> <597D79BD0200007800102F92@prv-mh.provo.novell.com> <1501498940.4771.251.camel@infradead.org> <597F2D860200007800103049@prv-mh.provo.novell.com> Date: Mon, 31 Jul 2017 16:56:38 +0100 Mime-Version: 1.0 X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 X-SRS-Rewrite: SMTP reverse-path rewritten from by twosheds.infradead.org. See http://www.infradead.org/rpr.html Cc: xen-devel@lists.xenproject.org, jiewen.yao@intel.com, jeff.fan@intel.com Subject: Re: [Xen-devel] xen/link: Move .data.rel.ro sections into .rodata for final link X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Mon, 2017-07-31 at 07:15 -0600, Jan Beulich wrote: > > > > David Woodhouse 07/31/17 1:02 PM >>> > > On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote: > > > > > > David Woodhouse 07/20/17 5:22 PM >>> > > > > This includes stuff lke the hypercall tables which we really want > > > > to be read-only. And they were going into .data.read-mostly. > > > > > > Yes, we'd like them to be read-only, but what if EFI properly assigned r/o > > > permissions to the .rodata section when loading xen.efi? We'd then be > > > unable to apply relocations when switching from 1:1 to virtual mappings > > > (see efi_arch_relocate_image()). > > > > FWIW it does look like TianoCore has gained the ability to mark > > sections as read-only, in January of this year: > > https://github.com/tianocore/edk2/commit/d0e92aad46 > > > > It doesn't actually seem to be complete — even with subsequent fixes > > since that commit, it doesn't look like it catches the case of data > > sections without EFI_IMAGE_SCN_MEM_WRITE, such as .rodata.  > > > > And even if/when that gets fixed you'll note that the protection is > > deliberately torn down in ExitBootServices(), specifically for the case > > you're concerned about below — because you'll need to do the > > relocations. > > As said in an earlier reply, a first pass over relocations is being done > long before the call to ExitBootServices(). A minimal adjustment to > efi_arch_relocate_image() will be needed anyway, afaict. Ah, right. I think more "implied" than "said" but I understand now. :) At least, I understand what you're saying... I have no bloody clue what's actually going on though. There is a first call to efi_arch_relocate_image(0) before the ExitBootServices() call. However I'm missing something because I can't see how that call achieves anything *other* than to trigger the fault we're concerned about. There are three types of relocations — PE_BASE_RELOC_ABS, PE_BASE_RELOC_HIGHLOW, and PE_BASE_RELOC_DIR64. The first (ABS) doesn't seem to do anything, ever. And is never emitted by mkrelocs.c.  The second (HIGHLOW) does nothing if (!delta). The third (DIR64) simply adds 'delta' to the target address. We could potentially stop it faulting on that pointless '*addr += 0' by doing this... ... but then again, if the whole function is really doing nothing at all when invoked with delta==0 then perhaps it would just be easier to remove the first pass altogether. I feel sure I'm missing something, but what? Is it still supposed to be adding xen_phys_start in the PE_BASE_RELOC_HIGHLOW case even when delta==0? Because it isn't... Either way, this is still broken before my patch though, right? Especially if UEFI learns to do it for non-executable sections, but AFAICT even before that. These are the sections I see the PE section headers of a local build:  Name     Characteristics   Relocations .text    0xe0d00020 (WRX)    ✓ .rodata  0x40600040 ( R )    ✓ .buildid 0x40300040 ( R ) .init.te 0x60500020 ( RX)    ✓ .init.da 0xc0d00040 (WR )    ✓ .data.re 0xc0800040 (WR )    ✓ .data    0xc0d00040 (WR )    ✓ .bss     0xc1000080 (WR ) .reloc   0x42300040 ( R ) .pad     0xc0300080 (WR ) So there are (again, before my patch) relocations in .init.da(ta) and .rodata sections which UEFI *might* start marking read-only, and also in .init.te(xt) which is R+X and could be marked read-only today. And the .init.te(xt) relocations include PE_BASE_RELOC_DIR64 relocations, which *would* cause a fault in the !delta case. (All the relocations in .rodata both before and after my patch are also PE_BASE_RELOC_DIR64, FWIW) --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -87,7 +87,8 @@ static void __init efi_arch_relocate_image(unsigned long delta)              case PE_BASE_RELOC_DIR64:                  if ( in_page_tables(addr) )                      blexit(L"Unexpected relocation type"); -                *(u64 *)addr += delta; +                if ( delta ) +                    *(u64 *)addr += delta;                  break;              default:                  blexit(L"Unsupported relocation type");