From patchwork Thu Jun 1 11:22:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Cooper X-Patchwork-Id: 9759411 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 348AB602BF for ; Thu, 1 Jun 2017 11:25:26 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1D16220453 for ; Thu, 1 Jun 2017 11:25:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0FAFA26E1A; Thu, 1 Jun 2017 11:25:26 +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.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED 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 8479420453 for ; Thu, 1 Jun 2017 11:25:25 +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 1dGOC9-0003k9-5H; Thu, 01 Jun 2017 11:23:13 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dGOC7-0003jE-GH for xen-devel@lists.xen.org; Thu, 01 Jun 2017 11:23:11 +0000 Received: from [85.158.143.35] by server-6.bemta-6.messagelabs.com id A8/2C-03920-E19FF295; Thu, 01 Jun 2017 11:23:10 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrJIsWRWlGSWpSXmKPExsWyU9JRQlfup36 kwYHNrBZLPi5mcWD0OLr7N1MAYxRrZl5SfkUCa8bn9gfMBT81KtYu7WZsYDyi0MXIySEh4Cdx 5897NhBbWCBM4vuLJewgtoiAskTvr98sXYxcHEICbcwSex4cBkswC+RLTP5xH6yBTUBfYveLT 0xdjBwcvAK2Ejuu+4OEWQRUJN5/mwpWLioQLvG28QgLiM0rIChxcuYTMJtTwF6i+cgMJoiRBh JHFs1hhbDlJba/ncMMYgsJqElc67/EDnFnusTWv7+YJzDyz0IyahaS9llI2hcwMq9i1ChOLSp LLdI1MtFLKspMzyjJTczM0TU0MNPLTS0uTkxPzUlMKtZLzs/dxAgMQgYg2MG472PkIUZJDiYl Ud4KG71IIb6k/JTKjMTijPii0pzU4kOMMhwcShK8r77rRwoJFqWmp1akZeYA4wEmLcHBoyTCa /oDKM1bXJCYW5yZDpE6xagoJc77BKRPACSRUZoH1waLwUuMslLCvIxAhwjxFKQW5WaWoMq/Yh TnYFQS5rUFGc+TmVcCN/0V0GImoMUvtoEtLklESEk1MEaoPj0Z5bqA0ZOB5bCF4Z+nbEIllaE HXjeeDpsco24yaxrXYremQ4rX+488U13s9DbU9Mt/vibW+ZxXdrqZ8PxlY7FKP3VY8lOQHp+X dNWLlaJH9+7IXiXR9bBNoTbmi6TcuYxt/Wdfrfogfcf1UNI1mwVtPjc979lVbckt7qsoOLV5w efUT0osxRmJhlrMRcWJAJYIFjq8AgAA X-Env-Sender: prvs=3184c523e=Andrew.Cooper3@citrix.com X-Msg-Ref: server-8.tower-21.messagelabs.com!1496316189!71676173!1 X-Originating-IP: [185.25.65.24] X-SpamReason: No, hits=0.5 required=7.0 tests=BODY_RANDOM_LONG, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 9.4.19; banners=-,-,- X-VirusChecked: Checked Received: (qmail 35042 invoked from network); 1 Jun 2017 11:23:10 -0000 Received: from smtp.ctxuk.citrix.com (HELO SMTP.EU.CITRIX.COM) (185.25.65.24) by server-8.tower-21.messagelabs.com with RC4-SHA encrypted SMTP; 1 Jun 2017 11:23:10 -0000 X-IronPort-AV: E=Sophos;i="5.39,279,1493683200"; d="scan'208";a="47040426" To: Jan Beulich References: <1495818213-345-1-git-send-email-andrew.cooper3@citrix.com> <1495818213-345-3-git-send-email-andrew.cooper3@citrix.com> <592BFED4020000780015D347@prv-mh.provo.novell.com> <0e4ea8ae-be0c-4d69-61f8-ca75c3faa029@citrix.com> <592C02C9020000780015D371@prv-mh.provo.novell.com> <6c3c894f-c92c-a172-ab67-954cbba6220a@citrix.com> <59300DBD020000780015EA20@prv-mh.provo.novell.com> From: Andrew Cooper Message-ID: <0614feaa-5ea0-adce-106c-dc4745b4835d@citrix.com> Date: Thu, 1 Jun 2017 12:22:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <59300DBD020000780015EA20@prv-mh.provo.novell.com> X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To AMSPEX02CL02.citrite.net (10.69.22.126) Cc: George Dunlap , Tim Deegan , Xen-devel Subject: Re: [Xen-devel] [PATCH 2/2] x86/pagewalk: Fix pagewalk's handling of instruction fetches 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 01/06/17 11:51, Jan Beulich wrote: >>>> On 01.06.17 at 12:19, wrote: >> On 29/05/17 10:15, Jan Beulich wrote: >>>>>> On 29.05.17 at 11:03, wrote: >>>> On 29/05/2017 09:58, Jan Beulich wrote: >>>>>>>> On 26.05.17 at 19:03, wrote: >>>>>> --- a/xen/arch/x86/mm/guest_walk.c >>>>>> +++ b/xen/arch/x86/mm/guest_walk.c >>>>>> @@ -114,22 +114,18 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain >> *p2m, >>>>>> ASSERT(!(walk & PFEC_implicit) || >>>>>> !(walk & (PFEC_insn_fetch | PFEC_user_mode))); >>>>>> >>>>>> - /* >>>>>> - * PFEC_insn_fetch is only used as an input to pagetable walking if NX >> or >>>>>> - * SMEP are enabled. Otherwise, instruction fetches are >> indistinguishable >>>>>> - * from data reads. >>>>>> - * >>>>>> - * This property can be demonstrated on real hardware by having NX and >>>>>> - * SMEP inactive, but SMAP active, and observing that EFLAGS.AC >> determines >>>>>> - * whether a pagefault occures for supervisor execution on user >> mappings. >>>>>> - */ >>>>>> - if ( !(guest_nx_enabled(v) || guest_smep_enabled(v)) ) >>>>>> - walk &= ~PFEC_insn_fetch; >>>>>> - >>>>>> perfc_incr(guest_walk); >>>>>> memset(gw, 0, sizeof(*gw)); >>>>>> gw->va = va; >>>>>> - gw->pfec = walk & (PFEC_insn_fetch | PFEC_user_mode | PFEC_write_access); >>>>>> + gw->pfec = walk & (PFEC_user_mode | PFEC_write_access); >>>>>> + >>>>>> + /* >>>>>> + * PFEC_insn_fetch is only reported if NX or SMEP are enabled. >> Hardware >>>>>> + * still distingueses instruction fetches during determination of >> access >>>>>> + * rights. >>>>>> + */ >>>>>> + if ( guest_nx_enabled(v) || guest_smep_enabled(v) ) >>>>>> + gw->pfec |= (walk & PFEC_insn_fetch); >>>>>> >>>>>> #if GUEST_PAGING_LEVELS >= 3 /* PAE or 64... */ >>>>>> #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */ >>>>> Don't you another adjustment to >>>>> >>>>> if ( (walk & PFEC_insn_fetch) && (ar & _PAGE_NX_BIT) ) >>>>> /* Requested an instruction fetch and found NX? Fail. */ >>>>> goto out; >>>>> >>>>> I can't see anything that would keep _PAGE_NX_BIT out of >>>>> ar if NX is not enabled. >>>> _PAGE_NX_BIT is reserved if NX is not enabled, and is accounted for in >>>> guest_rsvd_bits() in guest_pt.h, and we never hit the access rights logic. >>> Ah, right. But perhaps worth having a respective ASSERT() >>> here, at once serving as documentation? >> I could, but it would feel be out of place. NX being incorrectly set is >> a translation failure, and by definition, the translation needs to have >> succeeded before permissions get considered. >> >> Would this clarification be acceptable? >> >> index 5c6a85b..6d6b454 100644 >> --- a/xen/arch/x86/mm/guest_walk.c >> +++ b/xen/arch/x86/mm/guest_walk.c >> @@ -360,8 +360,9 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, >> gw->pfec |= PFEC_page_present; >> >> /* >> - * The pagetable walk has returned a successful translation. Now check >> - * access rights to see whether the access should succeed. >> + * The pagetable walk has returned a successful translation (i.e. All >> + * PTEs are present and have no reserved bits set). Now check access >> + * rights to see whether the access should succeed. >> */ > While this perhaps is a worthwhile addition, my original request > really was to make more visible around the place where it matters > that the NX bit is part of the reserved ones when NX is off. Hence > I'm not sure the comment change is worthwhile, and if you dislike > adding the suggested ASSERT() I won't the patch be left as is. I presume you means something like you won't mind if the patch is left as-is? How about this? #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */ /* * If all access checks are thus far ok, check Protection Key for 64bit One problem I have with an ASSERT beside the "if ( (walk & PFEC_insn_fetch) && (ar & _PAGE_NX_BIT) )" is that it is mid-way through the permissions checks, rather than at the start, which is likely to get missed if future access checks get introduced ahead of the protection key checks. ~Andrew diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c index 972364f..6055fec 100644 --- a/xen/arch/x86/mm/guest_walk.c +++ b/xen/arch/x86/mm/guest_walk.c @@ -356,11 +356,19 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, gw->pfec |= PFEC_page_present; /* - * The pagetable walk has returned a successful translation. Now check - * access rights to see whether the access should succeed. + * The pagetable walk has returned a successful translation (i.e. All PTEs + * are present and have no reserved bits set). Now check access rights to + * see whether the access should succeed. */ ar = (ar_and & AR_ACCUM_AND) | (ar_or & AR_ACCUM_OR); + /* + * Sanity check. If EFER.NX is disabled, _PAGE_NX_BIT is reserved and + * should have caused a translation failure before we get here. + */ + if ( ar & _PAGE_NX_BIT ) + ASSERT(guest_nx_enabled(v)); +