Re: [PATCH v4 03/34] xsm/xen_version: Add XSM for the xen_version hypercall

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Mar 18, 2016 at 05:55:55AM -0600, Jan Beulich wrote:
> >>> On 15.03.16 at 18:56, <konrad.wilk@oracle.com> 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 <konrad.wilk@oracle.com>
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 '<denied>\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 <konrad.wilk@oracle.com>

---
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

v2: Do XSM check for all the XENVER_ ops.
 - Add empty data conditions.
 - Return <denied> 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 <xen/nmi.h>
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
+#include <xsm/xsm.h>
 #include <asm/current.h>
 #include <public/nmi.h>
 #include <public/version.h>
@@ -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 "<denied>";
+}
+
+/*
+ * 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 <public/version.h>
+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 <public/xen.h>
 #include <public/physdev.h>
 #include <public/platform.h>
+#include <public/version.h>
 
 #include <public/xsm/flask_op.h>
 
@@ -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
-- 
2.5.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel