From patchwork Fri Aug 5 11:20:48 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jan Beulich X-Patchwork-Id: 9265249 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 707D760760 for ; Fri, 5 Aug 2016 11:23:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 516C628409 for ; Fri, 5 Aug 2016 11:23:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 41CC82843A; Fri, 5 Aug 2016 11:23:40 +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 7EB4D28409 for ; Fri, 5 Aug 2016 11:23:39 +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 1bVdBP-0004ta-1t; Fri, 05 Aug 2016 11:20:55 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bVdBO-0004tU-8h for xen-devel@lists.xenproject.org; Fri, 05 Aug 2016 11:20:54 +0000 Received: from [85.158.139.211] by server-13.bemta-5.messagelabs.com id 4A/1C-12874-59674A75; Fri, 05 Aug 2016 11:20:53 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrCIsWRWlGSWpSXmKPExsXS6fjDS3dK2ZJ wg6e7BS2+b5nM5MDocfjDFZYAxijWzLyk/IoE1owN9ycwFtx1r/jTtpG9gfGCWRcjJ4eQQJ7E krObmEFsXgE7iV+HephAbAkBQ4mn76+zgdgsAqoSkx/OYwex2QTUJdqebWftYuTgEBEwkDh3N KmLkYuDWWAtk8TFWXfBeoUFrCXaZ85kAanhFRCU+LtDGCTMDDR+w97tzBMYuWYhZGYhyYCEmY EWrJ8nBBGWl2jeOhsqLC2x/B8HRNhRovHGCiYIW1ti2cLXzBC2j8SdhiamBYwcqxjVi1OLylK LdC31kooy0zNKchMzc3QNDUz1clOLixPTU3MSk4r1kvNzNzECQ48BCHYwrm11PsQoycGkJMq7 LnFJuBBfUn5KZUZicUZ8UWlOavEhRhkODiUJXvlSoJxgUWp6akVaZg4wCmDSEhw8SiK8giBp3 uKCxNzizHSI1ClGRSlx3r0gCQGQREZpHlwbLPIuMcpKCfMyAh0ixFOQWpSbWYIq/4pRnINRSZ hXGmQKT2ZeCdz0V0CLmYAWf7QCW1ySiJCSamA0/1Fk73l9aula1T2Va5YaLDok1XvZRyvuzso Z8ziZe9Wmr+XbYa8g2H0j5+ahJquJB3vzdkqIN7M8fHgo3r0zVvj4gz8s9nNs/tXM2bfp0atb F27LMJzwOK+f7HzgV++mTQLNy3+mT2/e1yvweMG6N9uDdshEplr9ZH/NLPfmxW2VQl6m9Qzbl ViKMxINtZiLihMBC8JO8bcCAAA= X-Env-Sender: JBeulich@suse.com X-Msg-Ref: server-7.tower-206.messagelabs.com!1470396050!52929003!1 X-Originating-IP: [137.65.248.74] X-SpamReason: No, hits=0.5 required=7.0 tests=BODY_RANDOM_LONG X-StarScan-Received: X-StarScan-Version: 8.77; banners=-,-,- X-VirusChecked: Checked Received: (qmail 61540 invoked from network); 5 Aug 2016 11:20:51 -0000 Received: from prv-mh.provo.novell.com (HELO prv-mh.provo.novell.com) (137.65.248.74) by server-7.tower-206.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 5 Aug 2016 11:20:51 -0000 Received: from INET-PRV-MTA by prv-mh.provo.novell.com with Novell_GroupWise; Fri, 05 Aug 2016 05:20:49 -0600 Message-Id: <57A492B002000078001032C6@prv-mh.provo.novell.com> X-Mailer: Novell GroupWise Internet Agent 14.2.1 Date: Fri, 05 Aug 2016 05:20:48 -0600 From: "Jan Beulich" To: "xen-devel" Mime-Version: 1.0 Cc: Stefano Stabellini , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , Tim Deegan Subject: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions 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 Qemu needs access to this for the domain it controls, both due to it being used by xc_domain_memory_mapping() (which qemu calls) and the explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend permissions to that of any "ordinary" domctl: A domain controlling the targeted domain can invoke this operation for that target domain (which is being achieved by no longer passing NULL to xsm_domctl()). This at once avoids a for_each_domain() loop when the ID of an existing domain gets passed in. Reported-by: Marek Marczykowski-Górecki Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper Acked-by: Daniel De Graaf --- v2: Add a comment. Clarify description as to what additional permission is being granted. --- I know there had been an alternative patch suggestion, but that one doesn't seem have seen a formal submission so far, so here is my original proposal. I wonder what good the duplication of the returned domain ID does: I'm tempted to remove the one in the command-specific structure. Does anyone have insight into why it was done that way? I further wonder why we have XSM_OTHER: The respective conversion into other XSM_* values in xsm/dummy.h could as well move into the callers, making intentions more obvious when looking at the actual code. domctl: relax getdomaininfo permissions Qemu needs access to this for the domain it controls, both due to it being used by xc_domain_memory_mapping() (which qemu calls) and the explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend permissions to that of any "ordinary" domctl: A domain controlling the targeted domain can invoke this operation for that target domain (which is being achieved by no longer passing NULL to xsm_domctl()). This at once avoids a for_each_domain() loop when the ID of an existing domain gets passed in. Reported-by: Marek Marczykowski-Górecki Signed-off-by: Jan Beulich --- v2: Add a comment. Clarify description as to what additional permission is being granted. --- I know there had been an alternative patch suggestion, but that one doesn't seem have seen a formal submission so far, so here is my original proposal. I wonder what good the duplication of the returned domain ID does: I'm tempted to remove the one in the command-specific structure. Does anyone have insight into why it was done that way? I further wonder why we have XSM_OTHER: The respective conversion into other XSM_* values in xsm/dummy.h could as well move into the callers, making intentions more obvious when looking at the actual code. --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -149,7 +149,7 @@ define(`device_model', ` create_channel($2, $1, $2_channel) allow $1 $2_channel:event create; - allow $1 $2_target:domain shutdown; + allow $1 $2_target:domain { getdomaininfo shutdown }; allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; ') --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; default: d = rcu_lock_domain_by_id(op->domain); - if ( d == NULL ) + if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) return -ESRCH; } @@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_getdomaininfo: { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); + domid_t dom = DOMID_INVALID; - for_each_domain ( d ) - if ( d->domain_id >= dom ) + if ( !d ) + { + ret = -EINVAL; + if ( op->domain >= DOMID_FIRST_RESERVED ) break; + rcu_read_lock(&domlist_read_lock); + + dom = op->domain; + for_each_domain ( d ) + if ( d->domain_id >= dom ) + break; + } + ret = -ESRCH; if ( d == NULL ) goto getdomaininfo_out; @@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe copyback = 1; getdomaininfo_out: + /* When d was non-NULL upon entry, no cleanup is needed. */ + if ( dom == DOMID_INVALID ) + break; + rcu_read_unlock(&domlist_read_lock); d = NULL; break; --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -61,7 +61,12 @@ static always_inline int xsm_default_act return 0; case XSM_TARGET: if ( src == target ) + { return 0; + case XSM_XS_PRIV: + if ( src->is_xenstore ) + return 0; + } /* fall through */ case XSM_DM_PRIV: if ( target && src->target == target ) @@ -71,10 +76,6 @@ static always_inline int xsm_default_act if ( src->is_privileged ) return 0; return -EPERM; - case XSM_XS_PRIV: - if ( src->is_xenstore || src->is_privileged ) - return 0; - return -EPERM; default: LINKER_BUG_ON(1); return -EPERM; --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -149,7 +149,7 @@ define(`device_model', ` create_channel($2, $1, $2_channel) allow $1 $2_channel:event create; - allow $1 $2_target:domain shutdown; + allow $1 $2_target:domain { getdomaininfo shutdown }; allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; ') --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; default: d = rcu_lock_domain_by_id(op->domain); - if ( d == NULL ) + if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) return -ESRCH; } @@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_getdomaininfo: { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); + domid_t dom = DOMID_INVALID; - for_each_domain ( d ) - if ( d->domain_id >= dom ) + if ( !d ) + { + ret = -EINVAL; + if ( op->domain >= DOMID_FIRST_RESERVED ) break; + rcu_read_lock(&domlist_read_lock); + + dom = op->domain; + for_each_domain ( d ) + if ( d->domain_id >= dom ) + break; + } + ret = -ESRCH; if ( d == NULL ) goto getdomaininfo_out; @@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe copyback = 1; getdomaininfo_out: + /* When d was non-NULL upon entry, no cleanup is needed. */ + if ( dom == DOMID_INVALID ) + break; + rcu_read_unlock(&domlist_read_lock); d = NULL; break; --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -61,7 +61,12 @@ static always_inline int xsm_default_act return 0; case XSM_TARGET: if ( src == target ) + { return 0; + case XSM_XS_PRIV: + if ( src->is_xenstore ) + return 0; + } /* fall through */ case XSM_DM_PRIV: if ( target && src->target == target ) @@ -71,10 +76,6 @@ static always_inline int xsm_default_act if ( src->is_privileged ) return 0; return -EPERM; - case XSM_XS_PRIV: - if ( src->is_xenstore || src->is_privileged ) - return 0; - return -EPERM; default: LINKER_BUG_ON(1); return -EPERM;