| Message ID | 20240729162651.571991-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | XSM/domctl: Fix permission checks on XEN_DOMCTL_createdomain | expand |
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
---- 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>
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:
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(-) base-commit: 8b5016e28737f140926619b14b8ca291dc4c5e62