[PATCH] XSM/domctl: Fix permission checks on XEN_DOMCTL_createdomain

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 current permitted to create a a domain" check, to
flask_domain_create().

As a conseqeuence, 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
   volentary 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 remedate this, pass the ssidref into xsm_domctl() and at least
check that the calling domain privileged enough to create domains.

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

Reported-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Julien Grall <julien@xxxxxxx>
CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
---
 xen/arch/x86/mm/paging.c |  2 +-
 xen/common/domctl.c      |  4 +++-
 xen/include/xsm/dummy.h  |  2 +-
 xen/include/xsm/xsm.h    |  6 +++---
 xen/xsm/flask/hooks.c    | 13 +++++++++++--
 5 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index bca320fffabf..dd47bde5ce40 100644
--- 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 2c0331bb05ed..ea16b759109e 100644
--- 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 00d2cbebf25a..709de238e4ef 100644
--- 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, 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 8dad03fd3d45..4a6f31ab9c23 100644
--- 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, int cmd, uint32_t ssidref);
     int (*sysctl)(int cmd);
     int (*readconsole)(uint32_t clear);
 
@@ -248,9 +248,9 @@ 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, 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 5e88c71b8e22..3d228a6011f3 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -663,12 +663,21 @@ 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, 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:

base-commit: 8b5016e28737f140926619b14b8ca291dc4c5e62
-- 
2.39.2