XSM/domctl: Fix permission checks on XEN_DOMCTL_createdomain

The XSM checks for XEN_DOMCTL_createdomain are problematic.  There's a split
between xsm_domctl() called early, and flask_domain_create() called quite late
during domain construction.

All XSM implementations except Flask have a simple IS_PRIV check in
xsm_domctl(), and operate as expected when an unprivileged domain tries to
make a hypercall.

Flask however foregoes any action in xsm_domctl() and defers everything,
including the simple "is the caller permitted to create a domain" check, to
flask_domain_create().

As a consequence, when XSM Flask is active, and irrespective of the policy
loaded, all domains irrespective of privilege can:

 * Mutate the global 'rover' variable, used to track the next free domid.
   Therefore, all domains can cause a domid wraparound, and combined with a
   voluntary reboot, choose their own domid.

 * Cause a reasonable amount of a domain to be constructed before ultimately
   failing for permission reasons, including the use of settings outside of
   supported limits.

In order to remediate this, pass the ssidref into xsm_domctl() and at least
check that the calling domain privileged enough to create domains.

Take the opportunity to also fix the sign of the cmd parameter to be unsigned.

This issue has not been assigned an XSA, because Flask is experimental and not
security supported.

Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index bca320fffabf6a61aee69ee2809ee5550e0083d6..dd47bde5ce40141c7168611c7c4713c39f7abe17 100644 (file)
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -767,7 +767,7 @@ long do_paging_domctl_cont(
     if ( d == NULL )
         return -ESRCH;
 
-    ret = xsm_domctl(XSM_OTHER, d, op.cmd);
+    ret = xsm_domctl(XSM_OTHER, d, op.cmd, 0 /* SSIDref not applicable */);
     if ( !ret )
     {
         if ( domctl_lock_acquire() )

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 2c0331bb05ed18fc857d9758363254bd03c416b4..ea16b759109e5071667522bb63c5a5dd13772119 100644 (file)
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -322,7 +322,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         break;
     }
 
-    ret = xsm_domctl(XSM_OTHER, d, op->cmd);
+    ret = xsm_domctl(XSM_OTHER, d, op->cmd,
+                     /* SSIDRef only applicable for cmd == createdomain */
+                     op->u.createdomain.ssidref);
     if ( ret )
         goto domctl_out_unlock_domonly;
 

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 00d2cbebf25aa021ce2f2cd11b7757afa81b390f..7956f27a291f73e6277c51ddf8170625fefa955b 100644 (file)
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -162,7 +162,7 @@ static XSM_INLINE int cf_check xsm_set_target(
 }
 
 static XSM_INLINE int cf_check xsm_domctl(
-    XSM_DEFAULT_ARG struct domain *d, int cmd)
+    XSM_DEFAULT_ARG struct domain *d, unsigned int cmd, uint32_t ssidref)
 {
     XSM_ASSERT_ACTION(XSM_OTHER);
     switch ( cmd )

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 8dad03fd3d45d853866ec19ecc9ea1e3c9ed6f95..627c0d2731af2c7ddfcf6ab449ae3b6d467f6e66 100644 (file)
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -60,7 +60,7 @@ struct xsm_ops {
     int (*domctl_scheduler_op)(struct domain *d, int op);
     int (*sysctl_scheduler_op)(int op);
     int (*set_target)(struct domain *d, struct domain *e);
-    int (*domctl)(struct domain *d, int cmd);
+    int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
     int (*sysctl)(int cmd);
     int (*readconsole)(uint32_t clear);
 
@@ -248,9 +248,10 @@ static inline int xsm_set_target(
     return alternative_call(xsm_ops.set_target, d, e);
 }
 
-static inline int xsm_domctl(xsm_default_t def, struct domain *d, int cmd)
+static inline int xsm_domctl(xsm_default_t def, struct domain *d,
+                             unsigned int cmd, uint32_t ssidref)
 {
-    return alternative_call(xsm_ops.domctl, d, cmd);
+    return alternative_call(xsm_ops.domctl, d, cmd, ssidref);
 }
 
 static inline int xsm_sysctl(xsm_default_t def, int cmd)

diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 5e88c71b8e225e86b48fba7f204ac1ebf7235401..278ad38c2af324aa1e35119305abf9eb9c7c724a 100644 (file)
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -663,12 +663,22 @@ static int cf_check flask_set_target(struct domain *d, struct domain *t)
     return rc;
 }
 
-static int cf_check flask_domctl(struct domain *d, int cmd)
+static int cf_check flask_domctl(struct domain *d, unsigned int cmd,
+                                 uint32_t ssidref)
 {
     switch ( cmd )
     {
-    /* These have individual XSM hooks (common/domctl.c) */
     case XEN_DOMCTL_createdomain:
+        /*
+         * There is a later hook too, but at this early point simply check
+         * that the calling domain is privileged enough to create a domain.
+         *
+         * Note that d is NULL because we haven't even allocated memory for it
+         * this early in XEN_DOMCTL_createdomain.
+         */
+        return avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE, NULL);
+
+    /* These have individual XSM hooks (common/domctl.c) */
     case XEN_DOMCTL_getdomaininfo:
     case XEN_DOMCTL_scheduler_op:
     case XEN_DOMCTL_irq_permission: