xen/common/domain.c | 6 ++---- xen/common/domctl.c | 4 ++++ xen/include/xsm/dummy.h | 6 +++--- xen/include/xsm/xsm.h | 12 +++++------ xen/xsm/flask/hooks.c | 48 ++++++++++++++++------------------------- 5 files changed, 34 insertions(+), 42 deletions(-)
Untested!
This only really matters for flask, but all of xsm is updated.
flask_domain_create() and flask_domain_alloc_security() are a strange
pair.
flask_domain_create() serves double duty. It both assigns sid and
self_sid values and checks if the calling domain has permission to
create the target domain. It also has special casing for handling dom0.
Meanwhile flask_domain_alloc_security() assigns some special sids, but
waits for others to be assigned in flask_domain_create. This split
seems to have come about so that the structures are allocated before
calling flask_domain_create(). It also means flask_domain_create is
called in the middle of domain_create.
Re-arrange the two calls. Let flask_domain_create just check if current
has permission to create ssidref. Then it can be moved out to do_domctl
and gate entry into domain_create. This avoids doing partial domain
creation before the permission check.
Have flask_domain_alloc_security() take a ssidref argument. The ssidref
was already permission checked earlier, so it can just be assigned.
Then the self_sid can be calculated here as well rather than in
flask_domain_create().
The dom0 special casing is moved into flask_domain_alloc_security().
Maybe this should be just a fall-through for the dom0 already created
case. This code may not be needed any longer.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
xen/common/domain.c | 6 ++----
xen/common/domctl.c | 4 ++++
xen/include/xsm/dummy.h | 6 +++---
xen/include/xsm/xsm.h | 12 +++++------
xen/xsm/flask/hooks.c | 48 ++++++++++++++++-------------------------
5 files changed, 34 insertions(+), 42 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f748806a45..6b1f5ed59d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -407,7 +407,8 @@ struct domain *domain_create(domid_t domid,
lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
- if ( (err = xsm_alloc_security_domain(d)) != 0 )
+ if ( (err = xsm_alloc_security_domain(d, config ? config->ssidref :
+ 0)) != 0 )
goto fail;
atomic_set(&d->refcnt, 1);
@@ -470,9 +471,6 @@ struct domain *domain_create(domid_t domid,
if ( !d->iomem_caps || !d->irq_caps )
goto fail;
- if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 )
- goto fail;
-
d->controller_pause_count = 1;
atomic_inc(&d->pause_count);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af044e2eda..ffdc1a41cd 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -406,6 +406,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
domid_t dom;
static domid_t rover = 0;
+ ret = xsm_domain_create(XSM_HOOK, op->u.createdomain.ssidref);
+ if (ret)
+ break;
+
dom = op->domain;
if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
{
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 7ae3c40eb5..29c4ca9951 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -104,10 +104,10 @@ static XSM_INLINE void xsm_security_domaininfo(struct domain *d,
return;
}
-static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG struct domain *d, u32 ssidref)
+static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG u32 ssidref)
{
XSM_ASSERT_ACTION(XSM_HOOK);
- return xsm_default_action(action, current->domain, d);
+ return xsm_default_action(action, current->domain, NULL);
}
static XSM_INLINE int xsm_getdomaininfo(XSM_DEFAULT_ARG struct domain *d)
@@ -163,7 +163,7 @@ static XSM_INLINE int xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
return xsm_default_action(action, current->domain, NULL);
}
-static XSM_INLINE int xsm_alloc_security_domain(struct domain *d)
+static XSM_INLINE int xsm_alloc_security_domain(struct domain *d, uint32_t ssidref)
{
return 0;
}
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 358ec13ba8..c1d2ef5832 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -46,7 +46,7 @@ typedef enum xsm_default xsm_default_t;
struct xsm_operations {
void (*security_domaininfo) (struct domain *d,
struct xen_domctl_getdomaininfo *info);
- int (*domain_create) (struct domain *d, u32 ssidref);
+ int (*domain_create) (u32 ssidref);
int (*getdomaininfo) (struct domain *d);
int (*domctl_scheduler_op) (struct domain *d, int op);
int (*sysctl_scheduler_op) (int op);
@@ -71,7 +71,7 @@ struct xsm_operations {
int (*grant_copy) (struct domain *d1, struct domain *d2);
int (*grant_query_size) (struct domain *d1, struct domain *d2);
- int (*alloc_security_domain) (struct domain *d);
+ int (*alloc_security_domain) (struct domain *d, uint32_t ssidref);
void (*free_security_domain) (struct domain *d);
int (*alloc_security_evtchn) (struct evtchn *chn);
void (*free_security_evtchn) (struct evtchn *chn);
@@ -202,9 +202,9 @@ static inline void xsm_security_domaininfo (struct domain *d,
xsm_ops->security_domaininfo(d, info);
}
-static inline int xsm_domain_create (xsm_default_t def, struct domain *d, u32 ssidref)
+static inline int xsm_domain_create (xsm_default_t def, u32 ssidref)
{
- return xsm_ops->domain_create(d, ssidref);
+ return xsm_ops->domain_create(ssidref);
}
static inline int xsm_getdomaininfo (xsm_default_t def, struct domain *d)
@@ -305,9 +305,9 @@ static inline int xsm_grant_query_size (xsm_default_t def, struct domain *d1, st
return xsm_ops->grant_query_size(d1, d2);
}
-static inline int xsm_alloc_security_domain (struct domain *d)
+static inline int xsm_alloc_security_domain (struct domain *d, uint32_t ssidref)
{
- return xsm_ops->alloc_security_domain(d);
+ return xsm_ops->alloc_security_domain(d, ssidref);
}
static inline void xsm_free_security_domain (struct domain *d)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index de050cc9fe..719fe90f22 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -156,9 +156,11 @@ static int avc_unknown_permission(const char *name, int id)
return rc;
}
-static int flask_domain_alloc_security(struct domain *d)
+static int flask_domain_alloc_security(struct domain *d, u32 ssidref)
{
struct domain_security_struct *dsec;
+ static int dom0_created = 0;
+ int rc;
dsec = xzalloc(struct domain_security_struct);
if ( !dsec )
@@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain *d)
case DOMID_IO:
dsec->sid = SECINITSID_DOMIO;
break;
+ case 0:
+ if ( !dom0_created ) {
+ dsec->sid = SECINITSID_DOM0;
+ dom0_created = 1;
+ } else {
+ dsec->sid = SECINITSID_UNLABELED;
+ }
+ break;
default:
- dsec->sid = SECINITSID_UNLABELED;
+ dsec->sid = ssidref;
}
dsec->self_sid = dsec->sid;
- d->ssid = dsec;
- return 0;
+ rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
+ &dsec->self_sid);
+
+ return rc;
}
static void flask_domain_free_security(struct domain *d)
@@ -507,32 +519,10 @@ static void flask_security_domaininfo(struct domain *d,
info->ssidref = domain_sid(d);
}
-static int flask_domain_create(struct domain *d, u32 ssidref)
+static int flask_domain_create(u32 ssidref)
{
- int rc;
- struct domain_security_struct *dsec = d->ssid;
- static int dom0_created = 0;
-
- if ( is_idle_domain(current->domain) && !dom0_created )
- {
- dsec->sid = SECINITSID_DOM0;
- dom0_created = 1;
- }
- else
- {
- rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
- DOMAIN__CREATE, NULL);
- if ( rc )
- return rc;
-
- dsec->sid = ssidref;
- }
- dsec->self_sid = dsec->sid;
-
- rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
- &dsec->self_sid);
-
- return rc;
+ return avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE,
+ NULL);
}
static int flask_getdomaininfo(struct domain *d)
--
2.26.2
---- On Mon, 26 Oct 2020 09:46:51 -0400 Jason Andryuk <jandryuk@gmail.com> wrote ----
> Untested!
>
> This only really matters for flask, but all of xsm is updated.
>
> flask_domain_create() and flask_domain_alloc_security() are a strange
> pair.
>
> flask_domain_create() serves double duty. It both assigns sid and
> self_sid values and checks if the calling domain has permission to
> create the target domain. It also has special casing for handling dom0.
> Meanwhile flask_domain_alloc_security() assigns some special sids, but
> waits for others to be assigned in flask_domain_create. This split
> seems to have come about so that the structures are allocated before
> calling flask_domain_create(). It also means flask_domain_create is
> called in the middle of domain_create.
>
> Re-arrange the two calls. Let flask_domain_create just check if current
> has permission to create ssidref. Then it can be moved out to do_domctl
> and gate entry into domain_create. This avoids doing partial domain
> creation before the permission check.
>
> Have flask_domain_alloc_security() take a ssidref argument. The ssidref
> was already permission checked earlier, so it can just be assigned.
> Then the self_sid can be calculated here as well rather than in
> flask_domain_create().
>
> The dom0 special casing is moved into flask_domain_alloc_security().
> Maybe this should be just a fall-through for the dom0 already created
> case. This code may not be needed any longer.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> xen/common/domain.c | 6 ++----
> xen/common/domctl.c | 4 ++++
> xen/include/xsm/dummy.h | 6 +++---
> xen/include/xsm/xsm.h | 12 +++++------
> xen/xsm/flask/hooks.c | 48 ++++++++++++++++-------------------------
> 5 files changed, 34 insertions(+), 42 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index f748806a45..6b1f5ed59d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -407,7 +407,8 @@ struct domain *domain_create(domid_t domid,
>
> lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
>
> - if ( (err = xsm_alloc_security_domain(d)) != 0 )
> + if ( (err = xsm_alloc_security_domain(d, config ? config->ssidref :
> + 0)) != 0 )
> goto fail;
>
> atomic_set(&d->refcnt, 1);
> @@ -470,9 +471,6 @@ struct domain *domain_create(domid_t domid,
> if ( !d->iomem_caps || !d->irq_caps )
> goto fail;
>
> - if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 )
> - goto fail;
> -
> d->controller_pause_count = 1;
> atomic_inc(&d->pause_count);
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index af044e2eda..ffdc1a41cd 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -406,6 +406,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> domid_t dom;
> static domid_t rover = 0;
>
> + ret = xsm_domain_create(XSM_HOOK, op->u.createdomain.ssidref);
> + if (ret)
> + break;
> +
> dom = op->domain;
> if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
> {
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 7ae3c40eb5..29c4ca9951 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -104,10 +104,10 @@ static XSM_INLINE void xsm_security_domaininfo(struct domain *d,
> return;
> }
>
> -static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG struct domain *d, u32 ssidref)
> +static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG u32 ssidref)
> {
> XSM_ASSERT_ACTION(XSM_HOOK);
> - return xsm_default_action(action, current->domain, d);
> + return xsm_default_action(action, current->domain, NULL);
> }
>
> static XSM_INLINE int xsm_getdomaininfo(XSM_DEFAULT_ARG struct domain *d)
> @@ -163,7 +163,7 @@ static XSM_INLINE int xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
> return xsm_default_action(action, current->domain, NULL);
> }
>
> -static XSM_INLINE int xsm_alloc_security_domain(struct domain *d)
> +static XSM_INLINE int xsm_alloc_security_domain(struct domain *d, uint32_t ssidref)
> {
> return 0;
> }
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 358ec13ba8..c1d2ef5832 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -46,7 +46,7 @@ typedef enum xsm_default xsm_default_t;
> struct xsm_operations {
> void (*security_domaininfo) (struct domain *d,
> struct xen_domctl_getdomaininfo *info);
> - int (*domain_create) (struct domain *d, u32 ssidref);
> + int (*domain_create) (u32 ssidref);
> int (*getdomaininfo) (struct domain *d);
> int (*domctl_scheduler_op) (struct domain *d, int op);
> int (*sysctl_scheduler_op) (int op);
> @@ -71,7 +71,7 @@ struct xsm_operations {
> int (*grant_copy) (struct domain *d1, struct domain *d2);
> int (*grant_query_size) (struct domain *d1, struct domain *d2);
>
> - int (*alloc_security_domain) (struct domain *d);
> + int (*alloc_security_domain) (struct domain *d, uint32_t ssidref);
> void (*free_security_domain) (struct domain *d);
> int (*alloc_security_evtchn) (struct evtchn *chn);
> void (*free_security_evtchn) (struct evtchn *chn);
> @@ -202,9 +202,9 @@ static inline void xsm_security_domaininfo (struct domain *d,
> xsm_ops->security_domaininfo(d, info);
> }
>
> -static inline int xsm_domain_create (xsm_default_t def, struct domain *d, u32 ssidref)
> +static inline int xsm_domain_create (xsm_default_t def, u32 ssidref)
> {
> - return xsm_ops->domain_create(d, ssidref);
> + return xsm_ops->domain_create(ssidref);
> }
>
> static inline int xsm_getdomaininfo (xsm_default_t def, struct domain *d)
> @@ -305,9 +305,9 @@ static inline int xsm_grant_query_size (xsm_default_t def, struct domain *d1, st
> return xsm_ops->grant_query_size(d1, d2);
> }
>
> -static inline int xsm_alloc_security_domain (struct domain *d)
> +static inline int xsm_alloc_security_domain (struct domain *d, uint32_t ssidref)
> {
> - return xsm_ops->alloc_security_domain(d);
> + return xsm_ops->alloc_security_domain(d, ssidref);
> }
>
> static inline void xsm_free_security_domain (struct domain *d)
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index de050cc9fe..719fe90f22 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -156,9 +156,11 @@ static int avc_unknown_permission(const char *name, int id)
> return rc;
> }
>
> -static int flask_domain_alloc_security(struct domain *d)
> +static int flask_domain_alloc_security(struct domain *d, u32 ssidref)
> {
> struct domain_security_struct *dsec;
> + static int dom0_created = 0;
> + int rc;
>
> dsec = xzalloc(struct domain_security_struct);
> if ( !dsec )
> @@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain *d)
> case DOMID_IO:
> dsec->sid = SECINITSID_DOMIO;
> break;
> + case 0:
> + if ( !dom0_created ) {
> + dsec->sid = SECINITSID_DOM0;
> + dom0_created = 1;
> + } else {
> + dsec->sid = SECINITSID_UNLABELED;
> + }
While the handling of this case is not wrong, I have to wonder if there is a better way to handle the dom0 creation case.
> + break;
> default:
> - dsec->sid = SECINITSID_UNLABELED;
> + dsec->sid = ssidref;
> }
>
> dsec->self_sid = dsec->sid;
> - d->ssid = dsec;
I don't think you meant to deleted that, without it domains will have no ssid assigned to them.
> - return 0;
> + rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
> + &dsec->self_sid);
> +
> + return rc;
> }
>
> static void flask_domain_free_security(struct domain *d)
> @@ -507,32 +519,10 @@ static void flask_security_domaininfo(struct domain *d,
> info->ssidref = domain_sid(d);
> }
>
> -static int flask_domain_create(struct domain *d, u32 ssidref)
> +static int flask_domain_create(u32 ssidref)
> {
> - int rc;
> - struct domain_security_struct *dsec = d->ssid;
> - static int dom0_created = 0;
> -
> - if ( is_idle_domain(current->domain) && !dom0_created )
> - {
> - dsec->sid = SECINITSID_DOM0;
> - dom0_created = 1;
> - }
> - else
> - {
> - rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
> - DOMAIN__CREATE, NULL);
> - if ( rc )
> - return rc;
> -
> - dsec->sid = ssidref;
> - }
> - dsec->self_sid = dsec->sid;
> -
> - rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
> - &dsec->self_sid);
> -
> - return rc;
> + return avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE,
> + NULL);
> }
>
> static int flask_getdomaininfo(struct domain *d)
> --
> 2.26.2
>
V/r,
Daniel P. Smith
Apertus Solutions, LLC
On Mon, Oct 26, 2020 at 12:23 PM Daniel Smith
<dpsmith@apertussolutions.com> wrote:
>
> ---- On Mon, 26 Oct 2020 09:46:51 -0400 Jason Andryuk <jandryuk@gmail.com> wrote ----
>
> > Untested!
> >
> > This only really matters for flask, but all of xsm is updated.
> >
> > flask_domain_create() and flask_domain_alloc_security() are a strange
> > pair.
> >
> > flask_domain_create() serves double duty. It both assigns sid and
> > self_sid values and checks if the calling domain has permission to
> > create the target domain. It also has special casing for handling dom0.
> > Meanwhile flask_domain_alloc_security() assigns some special sids, but
> > waits for others to be assigned in flask_domain_create. This split
> > seems to have come about so that the structures are allocated before
> > calling flask_domain_create(). It also means flask_domain_create is
> > called in the middle of domain_create.
> >
> > Re-arrange the two calls. Let flask_domain_create just check if current
> > has permission to create ssidref. Then it can be moved out to do_domctl
> > and gate entry into domain_create. This avoids doing partial domain
> > creation before the permission check.
> >
> > Have flask_domain_alloc_security() take a ssidref argument. The ssidref
> > was already permission checked earlier, so it can just be assigned.
> > Then the self_sid can be calculated here as well rather than in
> > flask_domain_create().
> >
> > The dom0 special casing is moved into flask_domain_alloc_security().
> > Maybe this should be just a fall-through for the dom0 already created
> > case. This code may not be needed any longer.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > ---
<snip>
> > -static int flask_domain_alloc_security(struct domain *d)
> > +static int flask_domain_alloc_security(struct domain *d, u32 ssidref)
> > {
> > struct domain_security_struct *dsec;
> > + static int dom0_created = 0;
> > + int rc;
> >
> > dsec = xzalloc(struct domain_security_struct);
> > if ( !dsec )
> > @@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain *d)
> > case DOMID_IO:
> > dsec->sid = SECINITSID_DOMIO;
> > break;
> > + case 0:
> > + if ( !dom0_created ) {
> > + dsec->sid = SECINITSID_DOM0;
> > + dom0_created = 1;
> > + } else {
> > + dsec->sid = SECINITSID_UNLABELED;
> > + }
>
> While the handling of this case is not wrong, I have to wonder if there is a better way to handle the dom0 creation case.
dom0_cfg.ssidref could be set to SECINITSID_DOM0. I guess that would
need some xsm_ssid_dom0 wrapper. Then maybe this logic can go away
and the default case used.
pv-shim doesn't necessarily use domid 0, so this may be broken there.
dom0_cfg.ssidref would fix that, I think. But I'm not familiar with
pv-shim.
> > + break;
> > default:
> > - dsec->sid = SECINITSID_UNLABELED;
> > + dsec->sid = ssidref;
> > }
> >
> > dsec->self_sid = dsec->sid;
> > - d->ssid = dsec;
>
> I don't think you meant to deleted that, without it domains will have no ssid assigned to them.
Yes, this should be retained.
Thanks for looking.
-Jason
© 2016 - 2026 Red Hat, Inc.