[xen master] XSM/domctl: Fix permission checks on XEN_DOMCTL_createdomain

[patchbot@xxxxxxx]

commit ee32b9b29af449d38aad0a1b3a81aaae586f5ea7
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Fri Jul 5 12:52:05 2024 +0100
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Tue Jul 30 17:42:17 2024 +0100

    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@xxxxxxxxxx>
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
    Acked-by: Daniel P. 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    |  7 ++++---
 xen/xsm/flask/hooks.c    | 14 ++++++++++++--
 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index bca320fffa..dd47bde5ce 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 2c0331bb05..ea16b75910 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 00d2cbebf2..7956f27a29 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, 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 8dad03fd3d..627c0d2731 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, 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 5e88c71b8e..278ad38c2a 100644
--- 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:
--
generated by git-patchbot for /home/xen/git/xen.git#master