diff mbox

[v3,1/3] xsm/xen_version: Add XSM for the xen_version hypercall (v6).

Message ID 1452219920-14043-2-git-send-email-konrad.wilk@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Jan. 8, 2016, 2:25 a.m. UTC
All of XENVER_* have now an XSM check.

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 rest: XENVER_[version|extraversion|capabilities|
parameters|get_features|page_size|guest_handle|changeset|
compile_info] behave as before - allowed by default for all guests.

This is with the XSM default (and non-default) policy and with
the dummy ones.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Do XSM check for all the XENVER_ ops.
v3: Add empty data conditions.
v4: Return <denied> for priv subops.
v5: Move extraversion from priv to normal. Drop the XSM check
    for the non-priv subops.
v6: Add +1 for strlen(xen_deny()) to include NULL. Move changeset,
    compile_info to non-priv subops.
---
 tools/flask/policy/policy/modules/xen/xen.te |  4 ++++
 xen/common/kernel.c                          | 13 +++++++++++--
 xen/common/version.c                         |  5 +++++
 xen/include/xen/version.h                    |  1 +
 xen/include/xsm/dummy.h                      | 21 +++++++++++++++++++++
 xen/include/xsm/xsm.h                        |  5 +++++
 xen/xsm/dummy.c                              |  1 +
 xen/xsm/flask/hooks.c                        | 24 ++++++++++++++++++++++++
 xen/xsm/flask/policy/access_vectors          |  2 ++
 9 files changed, 74 insertions(+), 2 deletions(-)

Comments

Daniel De Graaf Jan. 8, 2016, 5:57 p.m. UTC | #1
On 07/01/16 21:25, Konrad Rzeszutek Wilk wrote:
> All of XENVER_* have now an XSM check.
>
> 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 rest: XENVER_[version|extraversion|capabilities|
> parameters|get_features|page_size|guest_handle|changeset|
> compile_info] behave as before - allowed by default for all guests.
>
> This is with the XSM default (and non-default) policy and with
> the dummy ones.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Jan Beulich Jan. 12, 2016, 4:08 p.m. UTC | #2
>>> On 08.01.16 at 03:25, <konrad.wilk@oracle.com> wrote:
> @@ -226,9 +227,10 @@ void __init do_initcalls(void)
>  /*
>   * Simple hypercalls.
>   */
> -
>  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +    bool_t deny = !!xsm_version_op(XSM_OTHER, cmd);
> +
>      switch ( cmd )
>      {
>      case XENVER_version:
> @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          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;
>  }

As said before, I don't think it is appropriate for "deny" to be
ignored for all other sub-ops when there is a designated policy.

> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -55,3 +55,8 @@ const char *xen_banner(void)
>  {
>      return XEN_BANNER;
>  }
> +
> +const char *xen_deny(void)
> +{
> +    return "<denied>\0";
> +}

There's still this strange extra NUL character here.

> @@ -1621,6 +1622,28 @@ static int flask_pmu_op (struct domain *d, unsigned int op)
>  }
>  #endif /* CONFIG_X86 */
>  
> +static int flask_version_op (uint32_t op)
> +{
> +    u32 dsid = domain_sid(current->domain);
> +
> +    switch ( op )
> +    {
> +    case XENVER_version:
> +    case XENVER_extraversion:
> +    case XENVER_compile_info:
> +    case XENVER_capabilities:
> +    case XENVER_changeset:
> +    case XENVER_platform_parameters:
> +    case XENVER_get_features:
> +    case XENVER_pagesize:
> +    case XENVER_guest_handle:
> +        return 0; /* These MUST always be accessible to guests. */
> +    default:
> +        return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_XEN2,
> +                            XEN2__VERSION_PRIV, NULL);
> +    }
> +}

And along with the comment above, I don't think there should be
a switch statement here, but instead "op" should be subjected to
policy restrictions.

Jan
diff mbox

Patch

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index d35ae22..17f304e 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -73,6 +73,10 @@  allow dom0_t xen_t:xen2 {
     pmu_ctrl
     get_symbol
 };
+
+# Allow dom0 to use XENVER_commandline
+allow dom0_t xen_t:xen2 version_priv;
+
 allow dom0_t xen_t:mmu memorymap;
 
 # Allow dom0 to use these domctls on itself. For domctls acting on other
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 6a3196a..2b3ccc4 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,9 +227,10 @@  void __init do_initcalls(void)
 /*
  * Simple hypercalls.
  */
-
 DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
+    bool_t deny = !!xsm_version_op(XSM_OTHER, cmd);
+
     switch ( cmd )
     {
     case XENVER_version:
@@ -354,10 +356,17 @@  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         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..95332a0 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -55,3 +55,8 @@  const char *xen_banner(void)
 {
     return XEN_BANNER;
 }
+
+const char *xen_deny(void)
+{
+    return "<denied>\0";
+}
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 55b84f0..3f3614e 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -721,3 +721,24 @@  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_version_op (XSM_DEFAULT_ARG uint32_t op)
+{
+    XSM_ASSERT_ACTION(XSM_OTHER);
+    switch ( op )
+    {
+    case XENVER_version:
+    case XENVER_extraversion:
+    case XENVER_compile_info:
+    case XENVER_capabilities:
+    case XENVER_changeset:
+    case XENVER_platform_parameters:
+    case XENVER_get_features:
+    case XENVER_pagesize:
+    case XENVER_guest_handle:
+        return 0; /* These MUST always be accessible to any guest. */
+    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 2c365cd..64f1fa3 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -192,6 +192,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 (*version_op) (uint32_t cmd);
 };
 
 #ifdef CONFIG_XSM
@@ -730,6 +731,10 @@  static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int
 
 #endif /* CONFIG_X86 */
 
+static inline int xsm_version_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->version_op(op);
+}
 #endif /* XSM_NO_WRAPPERS */
 
 #ifdef CONFIG_MULTIBOOT
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 0f32636..1469dce 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, version_op);
 }
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 9b7de30..0c49804 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>
 
@@ -1621,6 +1622,28 @@  static int flask_pmu_op (struct domain *d, unsigned int op)
 }
 #endif /* CONFIG_X86 */
 
+static int flask_version_op (uint32_t op)
+{
+    u32 dsid = domain_sid(current->domain);
+
+    switch ( op )
+    {
+    case XENVER_version:
+    case XENVER_extraversion:
+    case XENVER_compile_info:
+    case XENVER_capabilities:
+    case XENVER_changeset:
+    case XENVER_platform_parameters:
+    case XENVER_get_features:
+    case XENVER_pagesize:
+    case XENVER_guest_handle:
+        return 0; /* These MUST always be accessible to guests. */
+    default:
+        return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_XEN2,
+                            XEN2__VERSION_PRIV, NULL);
+    }
+}
+
 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);
 
@@ -1759,6 +1782,7 @@  static struct xsm_operations flask_ops = {
     .ioport_mapping = flask_ioport_mapping,
     .pmu_op = flask_pmu_op,
 #endif
+    .version_op = flask_version_op,
 };
 
 static __init void flask_init(void)
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index effb59f..44a106e 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -93,6 +93,8 @@  class xen2
     pmu_ctrl
 # PMU use (domains, including unprivileged ones, will be using this operation)
     pmu_use
+# XENVER_commandline usage.
+    version_priv
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on