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(-)
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@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Daniel Smith <dpsmith@apertussolutions.com>
---
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---- On Mon, 29 Jul 2024 12:26:51 -0400 Andrew Cooper wrote --- > 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@citrix.com> > Signed-off-by: Andrew Cooper andrew.cooper3@citrix.com> > --- Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
On 29.07.2024 18:26, Andrew Cooper wrote: > 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 Nit: Double "a". > 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@citrix.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> However, a remark and a nit: > --- 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) Might be a reasonable thing to also convert type of "cmd" here and elsewhere, as you're touching all relevant places anyway: The struct field passed in is uint32_t, so the caller needlessly does a signed-ness conversion. > @@ -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) This line is getting a little too long now. Jan
© 2016 - 2024 Red Hat, Inc.