From patchwork Fri Mar 18 17:26:10 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Konrad Rzeszutek Wilk X-Patchwork-Id: 8622661 Return-Path: X-Original-To: patchwork-xen-devel@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 D03A79F44D for ; Fri, 18 Mar 2016 17:28:35 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3D1D62020F for ; Fri, 18 Mar 2016 17:28:34 +0000 (UTC) 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.kernel.org (Postfix) with ESMTPS id 78F5F2026C for ; Fri, 18 Mar 2016 17:28:32 +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 1agyAK-0000yr-RE; Fri, 18 Mar 2016 17:26:24 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1agyAJ-0000yl-Uz for xen-devel@lists.xenproject.org; Fri, 18 Mar 2016 17:26:24 +0000 Received: from [193.109.254.147] by server-9.bemta-14.messagelabs.com id 62/B9-02984-F3A3CE65; Fri, 18 Mar 2016 17:26:23 +0000 X-Env-Sender: konrad@char.us.oracle.com X-Msg-Ref: server-15.tower-27.messagelabs.com!1458321980!32342752!1 X-Originating-IP: [156.151.31.81] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogMTU2LjE1MS4zMS44MSA9PiAyODgzMzk=\n X-StarScan-Received: X-StarScan-Version: 8.11; banners=-,-,- X-VirusChecked: Checked Received: (qmail 25684 invoked from network); 18 Mar 2016 17:26:21 -0000 Received: from userp1040.oracle.com (HELO userp1040.oracle.com) (156.151.31.81) by server-15.tower-27.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 18 Mar 2016 17:26:21 -0000 Received: from aserv0021.oracle.com (aserv0021.oracle.com [141.146.126.233]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u2IHQDtf020606 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 18 Mar 2016 17:26:14 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserv0021.oracle.com (8.13.8/8.13.8) with ESMTP id u2IHQD26026777 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL); Fri, 18 Mar 2016 17:26:13 GMT Received: from abhmp0017.oracle.com (abhmp0017.oracle.com [141.146.116.23]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id u2IHQCZK017494; Fri, 18 Mar 2016 17:26:12 GMT Received: from char.us.oracle.com (/10.137.176.158) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 18 Mar 2016 10:26:11 -0700 Received: by char.us.oracle.com (Postfix, from userid 1000) id 662366A00B9; Fri, 18 Mar 2016 13:26:10 -0400 (EDT) Date: Fri, 18 Mar 2016 13:26:10 -0400 From: Konrad Rzeszutek Wilk To: Jan Beulich Message-ID: <20160318172610.GA9624@char.us.oracle.com> References: <1458064616-23101-1-git-send-email-konrad.wilk@oracle.com> <1458064616-23101-4-git-send-email-konrad.wilk@oracle.com> <56EBFADB02000078000DE4F6@prv-mh.provo.novell.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <56EBFADB02000078000DE4F6@prv-mh.provo.novell.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Source-IP: aserv0021.oracle.com [141.146.126.233] Cc: Wei Liu , Stefano Stabellini , andrew.cooper3@citrix.com, Ian Jackson , mpohlack@amazon.de, ross.lagerwall@citrix.com, xen-devel@lists.xenproject.org, Daniel De Graaf , sasha.levin@oracle.com Subject: Re: [Xen-devel] [PATCH v4 03/34] xsm/xen_version: Add XSM for the xen_version hypercall 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-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, 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 Fri, Mar 18, 2016 at 05:55:55AM -0600, Jan Beulich wrote: > >>> On 15.03.16 at 18:56, wrote: > > @@ -223,12 +224,15 @@ void __init do_initcalls(void) > > /* > > * Simple hypercalls. > > */ > > - > > DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > Please retain the blank line, as it relates to more than just this > one function. Done! (stray change). > > > { > > + bool_t deny = !!xsm_xen_version(XSM_OTHER, cmd); > > + > > switch ( cmd ) > > { > > case XENVER_version: > > + if ( deny ) > > + return 0; > > return (xen_major_version() << 16) | xen_minor_version(); > > To be honest, I'm now rather uncertain about this one: If a guest > can't figure out the hypervisor version, how would it be able to > adjust its behavior accordingly (e.g. use deprecated hypercalls as > needed)? IOW, other than for most/all other stuff here (the > get-features and platform-parameters sub-ops may be considered > similar to this one, see also below), I don't think allowing the > "permitted" default to be overridden makes sense here. I don't want to crash old guests or lead them astray. Removed the 'deny' here. Also removed the XSM checks for this sub-op (and the others below) as they are ignored. > > > @@ -274,6 +279,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > .virt_start = HYPERVISOR_VIRT_START > > }; > > > > + if ( deny ) > > + params.virt_start = 0; > > Guests may (validly imo) assume to get a valid address here. If you > mean to not expose the non-constant address in the compat mode > case, I could accept that. But you would then need to set the ABI > mandated __HYPERVISOR_COMPAT_VIRT_START (and retain the > constant value in the non-compat case). Our old 32-bit PV guests > would crash extremely early on boot if they got back zero here > (that's for 2.6.30 and later, and I think both you and Citrix had > derived some of their kernels from our 2.6.32 based one). OK. Let me also relax this one and always return a value. > > > @@ -302,6 +310,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > switch ( fi.submap_idx ) > > { > > case 0: > > + if ( deny ) > > + break; > > I think if to be put here at all, this should go ahead of the switch(), I am OK not acking on the XSM check. It really throws a wrench in Linux (upstream Linux hangs when initializing the XenBus frontend driver). > so that guests wouldn't be able to guess from the valid index values > which features may be available. And of course you should clear > fi.submap if you deny access, instead of leaving in it what has been > there before. > > > case XENVER_guest_handle: > > - if ( copy_to_guest(arg, current->domain->handle, > > - ARRAY_SIZE(current->domain->handle)) ) > > + { > > + xen_domain_handle_t hdl; > > + ssize_t len; > > + > > + if ( deny ) > > + { > > + len = sizeof(hdl); > > + memset(&hdl, 0, len); > > + } else > > + len = ARRAY_SIZE(current->domain->handle); > > + > > + if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, len ) ) > > return -EFAULT; > > return 0; > > What is this "len" handling here about? Aren't both the same type > and hence size? Perhaps, if you feel unsure about that, simply add > a respective BUILD_BUG_ON()? Yes they are. Used a BUILD_BUG_ON just in case somebody mucks around. > > > --- a/xen/include/xen/version.h > > +++ b/xen/include/xen/version.h > > @@ -12,5 +12,5 @@ unsigned int xen_minor_version(void); > > const char *xen_extra_version(void); > > const char *xen_changeset(void); > > const char *xen_banner(void); > > - > > +const char *xen_deny(void); > > #endif /* __XEN_VERSION_H__ */ > > Please retain the blank line. Yes. > > Jan > Inline is what the patch now looks like: From 0d5d62a9f15b8306e0c62fb00af193a733af435c Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Fri, 11 Mar 2016 21:40:43 -0500 Subject: [PATCH] xsm/xen_version: Add XSM for most of xen_version hypercall Most of XENVER_* have now an XSM check for their sub-ops. The subop for XENVER_commandline is now a priviliged operation. To not break guests we still return an string - but it is just '\0'. The XENVER_[version|parameters|get_features] - will always return an value to the guest. The rest: XENVER_[extraversion|capabilities|page_size| guest_handle|changeset| compile_info] behave as before - allowed by default for all guests if using the XSM default policy or with the dummy one. And if the system admin wants to curtail access to some of them - they can do that now with a non-default XSM policy. Also we add a local variable block. Signed-off-by: Konrad Rzeszutek Wilk --- Cc: Daniel De Graaf Cc: Ian Jackson Cc: Stefano Stabellini Cc: Wei Liu v2: Do XSM check for all the XENVER_ ops. - Add empty data conditions. - Return for priv subops. - Move extraversion from priv to normal. Drop the XSM check for the non-priv subops. v3: - Add +1 for strlen(xen_deny()) to include NULL. Move changeset, compile_info to non-priv subops. - Remove the \0 on xen_deny() - Add new XSM domain for xenver hypercall. Add all subops to it. - Remove the extra line, Add Ack from Daniel v4: - Rename the XSM from xen_version_op to xsm_xen_version. Prefix the types with 'xen' to distinguish it from another hypercall performing similar operation. Removed Ack from Daniel as it was so large. Add local variable block. v5: - Make XENVER_platform_parameters,get_features,version be excluded from the XSM check per Jan's review. Add BUILD_BUG_CHECK and fix odd line removals. --- tools/flask/policy/policy/modules/xen/xen.te | 14 +++++++++ xen/common/kernel.c | 43 +++++++++++++++++++++------- xen/common/version.c | 15 ++++++++++ xen/include/xen/version.h | 1 + xen/include/xsm/dummy.h | 24 ++++++++++++++++ xen/include/xsm/xsm.h | 5 ++++ xen/xsm/dummy.c | 1 + xen/xsm/flask/hooks.c | 39 +++++++++++++++++++++++++ xen/xsm/flask/policy/access_vectors | 25 ++++++++++++++++ xen/xsm/flask/policy/security_classes | 1 + 10 files changed, 157 insertions(+), 11 deletions(-) diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te index d35ae22..18f49b5 100644 --- a/tools/flask/policy/policy/modules/xen/xen.te +++ b/tools/flask/policy/policy/modules/xen/xen.te @@ -73,6 +73,14 @@ allow dom0_t xen_t:xen2 { pmu_ctrl get_symbol }; + +# Allow dom0 to use all XENVER_ subops that have checks. +# Note that dom0 is part of domain_type so this has duplicates. +allow dom0_t xen_t:version { + xen_extraversion xen_compile_info xen_capabilities + xen_changeset xen_pagesize xen_guest_handle xen_commandline +}; + allow dom0_t xen_t:mmu memorymap; # Allow dom0 to use these domctls on itself. For domctls acting on other @@ -137,6 +145,12 @@ if (guest_writeconsole) { # pmu_ctrl is for) allow domain_type xen_t:xen2 pmu_use; +# For normal guests all possible except XENVER_commandline. +allow domain_type xen_t:version { + xen_extraversion xen_compile_info xen_capabilities + xen_changeset xen_pagesize xen_guest_handle +}; + ############################################################################### # # Domain creation diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 0618da2..06ecf26 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -226,6 +227,8 @@ void __init do_initcalls(void) DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { + bool_t deny = !!xsm_xen_version(XSM_OTHER, cmd); + switch ( cmd ) { case XENVER_version: @@ -236,7 +239,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) xen_extraversion_t extraversion; memset(extraversion, 0, sizeof(extraversion)); - safe_strcpy(extraversion, xen_extra_version()); + safe_strcpy(extraversion, deny ? xen_deny() : xen_extra_version()); if ( copy_to_guest(arg, extraversion, ARRAY_SIZE(extraversion)) ) return -EFAULT; return 0; @@ -247,10 +250,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) xen_compile_info_t info; memset(&info, 0, sizeof(info)); - safe_strcpy(info.compiler, xen_compiler()); - safe_strcpy(info.compile_by, xen_compile_by()); - safe_strcpy(info.compile_domain, xen_compile_domain()); - safe_strcpy(info.compile_date, xen_compile_date()); + safe_strcpy(info.compiler, deny ? xen_deny() : xen_compiler()); + safe_strcpy(info.compile_by, deny ? xen_deny() : xen_compile_by()); + safe_strcpy(info.compile_domain, deny ? xen_deny() : xen_compile_domain()); + safe_strcpy(info.compile_date, deny ? xen_deny() : xen_compile_date()); if ( copy_to_guest(arg, &info, 1) ) return -EFAULT; return 0; @@ -261,7 +264,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) xen_capabilities_info_t info; memset(info, 0, sizeof(info)); - arch_get_xen_caps(&info); + if ( !deny ) + arch_get_xen_caps(&info); if ( copy_to_guest(arg, info, ARRAY_SIZE(info)) ) return -EFAULT; @@ -285,7 +289,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) xen_changeset_info_t chgset; memset(chgset, 0, sizeof(chgset)); - safe_strcpy(chgset, xen_changeset()); + safe_strcpy(chgset, deny ? xen_deny() : xen_changeset()); if ( copy_to_guest(arg, chgset, ARRAY_SIZE(chgset)) ) return -EFAULT; return 0; @@ -342,19 +346,36 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) } case XENVER_pagesize: + if ( deny ) + return 0; return (!guest_handle_is_null(arg) ? -EINVAL : PAGE_SIZE); case XENVER_guest_handle: - if ( copy_to_guest(arg, current->domain->handle, - ARRAY_SIZE(current->domain->handle)) ) + { + xen_domain_handle_t hdl; + + if ( deny ) + memset(&hdl, 0, ARRAY_SIZE(hdl)); + + BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl)); + + if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, + ARRAY_SIZE(hdl) ) ) return -EFAULT; return 0; - + } case XENVER_commandline: - if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) ) + { + size_t len = ARRAY_SIZE(saved_cmdline); + + if ( deny ) + len = strlen(xen_deny()) + 1; + + if ( copy_to_guest(arg, deny ? xen_deny() : saved_cmdline, len) ) return -EFAULT; return 0; } + } return -ENOSYS; } diff --git a/xen/common/version.c b/xen/common/version.c index b152e27..fc9bf42 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -55,3 +55,18 @@ const char *xen_banner(void) { return XEN_BANNER; } + +const char *xen_deny(void) +{ + return ""; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h index 81a3c7d..2015c0b 100644 --- a/xen/include/xen/version.h +++ b/xen/include/xen/version.h @@ -12,5 +12,6 @@ unsigned int xen_minor_version(void); const char *xen_extra_version(void); const char *xen_changeset(void); const char *xen_banner(void); +const char *xen_deny(void); #endif /* __XEN_VERSION_H__ */ diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 1d13826..87be9e5 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -727,3 +727,27 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int } #endif /* CONFIG_X86 */ + +#include +static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) +{ + XSM_ASSERT_ACTION(XSM_OTHER); + switch ( op ) + { + case XENVER_version: + case XENVER_platform_parameters: + case XENVER_get_features: + /* The sub-ops ignores the permission check and returns data. */ + return 0; + case XENVER_extraversion: + case XENVER_compile_info: + case XENVER_capabilities: + case XENVER_changeset: + case XENVER_pagesize: + case XENVER_guest_handle: + /* These MUST always be accessible to any guest by default. */ + return xsm_default_action(XSM_HOOK, current->domain, NULL); + default: + return xsm_default_action(XSM_PRIV, current->domain, NULL); + } +} diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 3afed70..db440f6 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -193,6 +193,7 @@ struct xsm_operations { int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow); int (*pmu_op) (struct domain *d, unsigned int op); #endif + int (*xen_version) (uint32_t cmd); }; #ifdef CONFIG_XSM @@ -731,6 +732,10 @@ static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int #endif /* CONFIG_X86 */ +static inline int xsm_xen_version (xsm_default_t def, uint32_t op) +{ + return xsm_ops->xen_version(op); +} #endif /* XSM_NO_WRAPPERS */ #ifdef CONFIG_MULTIBOOT diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index 0f32636..9791ad4 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -162,4 +162,5 @@ void xsm_fixup_ops (struct xsm_operations *ops) set_to_dummy_if_null(ops, ioport_mapping); set_to_dummy_if_null(ops, pmu_op); #endif + set_to_dummy_if_null(ops, xen_version); } diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 4813623..1a95689 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -26,6 +26,7 @@ #include #include #include +#include #include @@ -1620,6 +1621,43 @@ static int flask_pmu_op (struct domain *d, unsigned int op) } #endif /* CONFIG_X86 */ +static int flask_xen_version (uint32_t op) +{ + u32 dsid = domain_sid(current->domain); + + switch ( op ) + { + case XENVER_version: + case XENVER_platform_parameters: + case XENVER_get_features: + /* The sub-ops ignore the permission check and always return data. */ + return 0; + case XENVER_extraversion: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_EXTRAVERSION, NULL); + case XENVER_compile_info: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_COMPILE_INFO, NULL); + case XENVER_capabilities: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_CAPABILITIES, NULL); + case XENVER_changeset: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_CHANGESET, NULL); + case XENVER_pagesize: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_PAGESIZE, NULL); + case XENVER_guest_handle: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_GUEST_HANDLE, NULL); + case XENVER_commandline: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__XEN_COMMANDLINE, NULL); + default: + return -EPERM; + } +} + long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op); int compat_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op); @@ -1758,6 +1796,7 @@ static struct xsm_operations flask_ops = { .ioport_mapping = flask_ioport_mapping, .pmu_op = flask_pmu_op, #endif + .xen_version = flask_xen_version, }; static __init void flask_init(void) diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index effb59f..badcf1c 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -495,3 +495,28 @@ class security # remove ocontext label definitions for resources del_ocontext } + +# Class version is used to describe the XENVER_ hypercall. +# Almost all sub-ops are described here - in the default case all of them should +# be allowed except the XENVER_commandline. +# +# The ones that are omitted are XENVER_version, XENVER_platform_parameters, +# and XENVER_get_features - as they MUST always be returned to a guest. +# +class version +{ +# Extra informations (-unstable). + xen_extraversion +# Compile information of the hypervisor. + xen_compile_info +# Such as "xen-3.0-x86_64 xen-3.0-x86_32p hvm-3.0-x86_32 hvm-3.0-x86_32p hvm-3.0-x86_64". + xen_capabilities +# Source code changeset. + xen_changeset +# Page size the hypervisor uses. + xen_pagesize +# An value that the control stack can choose. + xen_guest_handle +# Xen command line. + xen_commandline +} diff --git a/xen/xsm/flask/policy/security_classes b/xen/xsm/flask/policy/security_classes index ca191db..cde4e1a 100644 --- a/xen/xsm/flask/policy/security_classes +++ b/xen/xsm/flask/policy/security_classes @@ -18,5 +18,6 @@ class shadow class event class grant class security +class version # FLASK