| Message ID | 20220706191325.440538-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] flask: Remove magic SID setting | expand |
On 7/6/22 15:13, Jason Andryuk wrote: > flask_domain_alloc_security and flask_domain_create has special code to > magically label dom0 as dom0_t. This can all be streamlined by making > create_dom0 set ssidref before creating dom0. Hmm, I wouldn't call it magical, it is the initialization policy for a domain labeling, which is specific to each policy module. I considered this approach already and my concern here is two fold. First, it now hard codes the concept of dom0 vs domU into the XSM API. There is an ever growing desire by solution providers to not have a dom0 and at most have a hardware domain if at all and this is a step backwards from that movement. Second, and related, is this now pushes the initial label policy up into the domain builder code away from the policy module and spreads it out. Hopefully Xen will evolve to have a richer set of initial domains and an appropriate initial label policy will be needed for this case. This approach will result in having to continually expand the XSM API for each new initial domain type. > create_domU is also extended to create domains with domU_t. > > xsm_ssidref_domU and xsm_ssidref_dom0 are introduced to abstract away > the details. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- > Untested on ARM. Minimally tested on x86. Needs your Flask permission > changes for xenboot_t to create dom0_t and domU_t. > > This is what I was thinking would be a better way to handle SID > assignment. > > Regards, > Jason > --- > xen/arch/arm/domain_build.c | 2 ++ > xen/arch/x86/setup.c | 1 + > xen/include/xsm/dummy.h | 10 ++++++++++ > xen/include/xsm/xsm.h | 12 ++++++++++++ > xen/xsm/dummy.c | 2 ++ > xen/xsm/flask/hooks.c | 31 +++++++++++++++++-------------- > 6 files changed, 44 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 3fd1186b53..a7e88944c2 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3281,6 +3281,7 @@ void __init create_domUs(void) > .max_grant_frames = -1, > .max_maptrack_frames = -1, > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), > + .ssidref = xsm_ssidref_domU(), > }; > unsigned int flags = 0U; > > @@ -3438,6 +3439,7 @@ void __init create_dom0(void) > .max_grant_frames = gnttab_dom0_frames(), > .max_maptrack_frames = -1, > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), > + .ssidref = xsm_ssidref_dom0(), > }; > > /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index f08b07b8de..5a6086cfe3 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -771,6 +771,7 @@ static struct domain *__init create_dom0(const module_t *image, > .arch = { > .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, > }, > + .ssidref = xsm_ssidref_dom0(), > }; > struct domain *d; > char *cmdline; > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 77f27e7163..12fbc224d0 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -124,6 +124,16 @@ static XSM_INLINE void cf_check xsm_security_domaininfo( > return; > } > > +static XSM_INLINE int cf_check xsm_ssidref_dom0(XSM_DEFAULT_VOID) > +{ > + return 0; > +} > + > +static XSM_INLINE int cf_check xsm_ssidref_domU(XSM_DEFAULT_VOID) > +{ > + return 0; > +} > + > static XSM_INLINE int cf_check xsm_domain_create( > XSM_DEFAULT_ARG struct domain *d, uint32_t ssidref) > { > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 8dad03fd3d..a6a4ffe05a 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -55,6 +55,8 @@ struct xsm_ops { > int (*set_system_active)(void); > void (*security_domaininfo)(struct domain *d, > struct xen_domctl_getdomaininfo *info); > + int (*ssidref_dom0)(void); > + int (*ssidref_domU)(void); > int (*domain_create)(struct domain *d, uint32_t ssidref); > int (*getdomaininfo)(struct domain *d); > int (*domctl_scheduler_op)(struct domain *d, int op); > @@ -220,6 +222,16 @@ static inline void xsm_security_domaininfo( > alternative_vcall(xsm_ops.security_domaininfo, d, info); > } > > +static inline int xsm_ssidref_dom0(void) > +{ > + return alternative_call(xsm_ops.ssidref_dom0); > +} > + > +static inline int xsm_ssidref_domU(void) > +{ > + return alternative_call(xsm_ops.ssidref_domU); > +} > + > static inline int xsm_domain_create( > xsm_default_t def, struct domain *d, uint32_t ssidref) > { > diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c > index e6ffa948f7..d46cfef0ec 100644 > --- a/xen/xsm/dummy.c > +++ b/xen/xsm/dummy.c > @@ -16,6 +16,8 @@ > static const struct xsm_ops __initconst_cf_clobber dummy_ops = { > .set_system_active = xsm_set_system_active, > .security_domaininfo = xsm_security_domaininfo, > + .ssidref_dom0 = xsm_ssidref_dom0, > + .ssidref_domU = xsm_ssidref_domU, > .domain_create = xsm_domain_create, > .getdomaininfo = xsm_getdomaininfo, > .domctl_scheduler_op = xsm_domctl_scheduler_op, > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 8c9cd0f297..d6f786ea84 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -544,26 +544,27 @@ static void cf_check flask_security_domaininfo( > info->ssidref = domain_sid(d); > } > > +static int cf_check flask_ssidref_dom0(void) > +{ > + return SECINITSID_DOM0; > +} > + > +static int cf_check flask_ssidref_domU(void) > +{ > + return SECINITSID_DOMU; > +} > + > static int cf_check flask_domain_create(struct domain *d, uint32_t 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; > + rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, > + DOMAIN__CREATE, NULL); > + if ( rc ) > + return rc; > > - dsec->sid = ssidref; > - } > + dsec->sid = ssidref; > dsec->self_sid = dsec->sid; > > rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN, > @@ -1805,6 +1806,8 @@ static int cf_check flask_argo_send( > static const struct xsm_ops __initconst_cf_clobber flask_ops = { > .set_system_active = flask_set_system_active, > .security_domaininfo = flask_security_domaininfo, > + .ssidref_dom0 = flask_ssidref_dom0, > + .ssidref_domU = flask_ssidref_domU, > .domain_create = flask_domain_create, > .getdomaininfo = flask_getdomaininfo, > .domctl_scheduler_op = flask_domctl_scheduler_op,
On Thu, Jul 7, 2022 at 6:14 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > On 7/6/22 15:13, Jason Andryuk wrote: > > flask_domain_alloc_security and flask_domain_create has special code to > > magically label dom0 as dom0_t. This can all be streamlined by making > > create_dom0 set ssidref before creating dom0. > > Hmm, I wouldn't call it magical, it is the initialization policy for a > domain labeling, which is specific to each policy module. I considered > this approach already and my concern here is two fold. First, it now > hard codes the concept of dom0 vs domU into the XSM API. There is an > ever growing desire by solution providers to not have a dom0 and at most > have a hardware domain if at all and this is a step backwards from that > movement. Second, and related, is this now pushes the initial label > policy up into the domain builder code away from the policy module and > spreads it out. Hopefully Xen will evolve to have a richer set of > initial domains and an appropriate initial label policy will be needed > for this case. This approach will result in having to continually expand > the XSM API for each new initial domain type. Yeah, adding dom0 vs. domU into the XSM API isn't nice. My original idea was just for dom0, but I added the domU hook after you basically said in your other email that dom0less had to work. There should not be any more of these since they are just to provide backwards compatibility. A dom0/domU flask policy is not interesting for dom0less/hyperlaunch. So I don't see why xen/flask needs support for determining sids for domains. If you have dom0less/hyperlaunch + flask, every domain should have a ssidref defined in its config when building. If you require ssidrefs for dom0less/hyperlaunch + flask, then there is less initial label policy. An unspecified ssidref defaulting to unlabeled_t is fine. I saw your other patch as adding more "initial label policy" since it adds more special cases. I see requiring an explicit ssidref or getting unlabeled_t as a feature. Automatic labeling seems like a misfeature to me. Regards, Jason
On 7/7/22 08:45, Jason Andryuk wrote: > On Thu, Jul 7, 2022 at 6:14 AM Daniel P. Smith > <dpsmith@apertussolutions.com> wrote: >> >> On 7/6/22 15:13, Jason Andryuk wrote: >>> flask_domain_alloc_security and flask_domain_create has special code to >>> magically label dom0 as dom0_t. This can all be streamlined by making >>> create_dom0 set ssidref before creating dom0. >> >> Hmm, I wouldn't call it magical, it is the initialization policy for a >> domain labeling, which is specific to each policy module. I considered >> this approach already and my concern here is two fold. First, it now >> hard codes the concept of dom0 vs domU into the XSM API. There is an >> ever growing desire by solution providers to not have a dom0 and at most >> have a hardware domain if at all and this is a step backwards from that >> movement. Second, and related, is this now pushes the initial label >> policy up into the domain builder code away from the policy module and >> spreads it out. Hopefully Xen will evolve to have a richer set of >> initial domains and an appropriate initial label policy will be needed >> for this case. This approach will result in having to continually expand >> the XSM API for each new initial domain type. > > Yeah, adding dom0 vs. domU into the XSM API isn't nice. My original > idea was just for dom0, but I added the domU hook after you basically > said in your other email that dom0less had to work. There should not > be any more of these since they are just to provide backwards > compatibility. Help me understand, why is it considered magic/undesirable to assign a label for Dom0/DomU in flask_domain_alloc_security() when the context is clearly discernible, yet it is acceptable to assign SECINITSID_XENBOOT, SECINITSID_DOMXEN, and SECINITSID_DOMIO? Specifically, when flask_domain_alloc_security() is called with the current domain labeled with SECINITSID_XENBOOT, we know it is creating either a system domain or a Dom0/DomU. At no time should there be a domain created at this time that needs to be labeled with SECINITSID_UNLABELED. When the current domain is no longer SECINITSID_XENBOOT, then the context is no longer understood and the only safe SID to initializat with is SECINITSID_UNLABELED. While I am more than open to listening as to why my opinion/approach may be flawed, codifying dom0 and/or domU into the XSM API is really a non-starter for me. > A dom0/domU flask policy is not interesting for dom0less/hyperlaunch. > So I don't see why xen/flask needs support for determining sids for > domains. If you have dom0less/hyperlaunch + flask, every domain > should have a ssidref defined in its config when building. If you > require ssidrefs for dom0less/hyperlaunch + flask, then there is less > initial label policy. An unspecified ssidref defaulting to > unlabeled_t is fine. Actually, a Dom0/DomU policy is very interesting for those of us that would like to see XSM/Flask to be the default policy regardless of the method of construction for the initial system. A specific test case I would run was a configuration containing a Dom0 and a DomU without XSM labels specified. This configuration should Just Work(tm). > I saw your other patch as adding more "initial label policy" since it > adds more special cases. I see requiring an explicit ssidref or > getting unlabeled_t as a feature. Automatic labeling seems like a > misfeature to me. This is the crux of the problem, you view the XSM API Expansion as label or fail while viewing the Default Initial Assignment as being automatic labeling. The reality is that this is not the case and that the end result between them is exactly the same, just with slightly different flows to get there. The difference being that the XSM API Expansion has codified Dom0/DomU into the XSM API and incurred an additional XSM call on each construction path. Consider the state sequence for struct domain_security_struct{} of the two under dom0less where labels cannot be specified, XSM API Expansion: 1. xsm_ssidref_{dom0,domU}() -> config->ssidref = SECINITSID_DOM0 or SECINITSID_DOMU respectively 2. xsm_alloc_security_domain() -> d->ssid->sid = SECINITSID_UNLABELED 3. xsm_domain_create() will always test config->ssidref, SECINITSID_DOM0 or SECINITSID_DOMU, because (1) will always set it and never as SECINITSID_UNLABELED Default Initial Assignment: 1. xsm_alloc_security_domain() -> d->ssid->sid = SECINITSID_DOM0 or SECINITSID_DOMU 2. config->ssidref = NULL 3. xsm_domain_create() will always test d->ssid->sid, SECINITSID_DOM0 or SECINITSID_DOMU because of (1) and never as SECINITSID_UNLABELED Hyperlaunch domain construction works differently, Dom0/DomU was not codified into the API and where possible the existing Dom0 API codifications were eliminated. The XSM API Expansion approach would result in a similar if statement that is in xsm_alloc_security_domain() under the Default Initial Assignment approach. It would likely occur in builder_create_domains(). There a check of the domain'a permissions and functions would occur to then call the appropriate xsm_ssid_{dom0,domu}() hook. Maybe some day it will be reasonable to expect labeling as a standard part of a domain's configuration, and thus acceptable to panic during boot when it is missing. Unfortunately, that is not today and no matter how it is dressed, the current model has to be a default label assignment based on the understanding that the context is boot-time domain construction. v/r, dps
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3fd1186b53..a7e88944c2 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3281,6 +3281,7 @@ void __init create_domUs(void) .max_grant_frames = -1, .max_maptrack_frames = -1, .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), + .ssidref = xsm_ssidref_domU(), }; unsigned int flags = 0U; @@ -3438,6 +3439,7 @@ void __init create_dom0(void) .max_grant_frames = gnttab_dom0_frames(), .max_maptrack_frames = -1, .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), + .ssidref = xsm_ssidref_dom0(), }; /* The vGIC for DOM0 is exactly emulating the hardware GIC */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f08b07b8de..5a6086cfe3 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -771,6 +771,7 @@ static struct domain *__init create_dom0(const module_t *image, .arch = { .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, }, + .ssidref = xsm_ssidref_dom0(), }; struct domain *d; char *cmdline; diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 77f27e7163..12fbc224d0 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -124,6 +124,16 @@ static XSM_INLINE void cf_check xsm_security_domaininfo( return; } +static XSM_INLINE int cf_check xsm_ssidref_dom0(XSM_DEFAULT_VOID) +{ + return 0; +} + +static XSM_INLINE int cf_check xsm_ssidref_domU(XSM_DEFAULT_VOID) +{ + return 0; +} + static XSM_INLINE int cf_check xsm_domain_create( XSM_DEFAULT_ARG struct domain *d, uint32_t ssidref) { diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 8dad03fd3d..a6a4ffe05a 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -55,6 +55,8 @@ struct xsm_ops { int (*set_system_active)(void); void (*security_domaininfo)(struct domain *d, struct xen_domctl_getdomaininfo *info); + int (*ssidref_dom0)(void); + int (*ssidref_domU)(void); int (*domain_create)(struct domain *d, uint32_t ssidref); int (*getdomaininfo)(struct domain *d); int (*domctl_scheduler_op)(struct domain *d, int op); @@ -220,6 +222,16 @@ static inline void xsm_security_domaininfo( alternative_vcall(xsm_ops.security_domaininfo, d, info); } +static inline int xsm_ssidref_dom0(void) +{ + return alternative_call(xsm_ops.ssidref_dom0); +} + +static inline int xsm_ssidref_domU(void) +{ + return alternative_call(xsm_ops.ssidref_domU); +} + static inline int xsm_domain_create( xsm_default_t def, struct domain *d, uint32_t ssidref) { diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index e6ffa948f7..d46cfef0ec 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -16,6 +16,8 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = { .set_system_active = xsm_set_system_active, .security_domaininfo = xsm_security_domaininfo, + .ssidref_dom0 = xsm_ssidref_dom0, + .ssidref_domU = xsm_ssidref_domU, .domain_create = xsm_domain_create, .getdomaininfo = xsm_getdomaininfo, .domctl_scheduler_op = xsm_domctl_scheduler_op, diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 8c9cd0f297..d6f786ea84 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -544,26 +544,27 @@ static void cf_check flask_security_domaininfo( info->ssidref = domain_sid(d); } +static int cf_check flask_ssidref_dom0(void) +{ + return SECINITSID_DOM0; +} + +static int cf_check flask_ssidref_domU(void) +{ + return SECINITSID_DOMU; +} + static int cf_check flask_domain_create(struct domain *d, uint32_t 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; + rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, + DOMAIN__CREATE, NULL); + if ( rc ) + return rc; - dsec->sid = ssidref; - } + dsec->sid = ssidref; dsec->self_sid = dsec->sid; rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN, @@ -1805,6 +1806,8 @@ static int cf_check flask_argo_send( static const struct xsm_ops __initconst_cf_clobber flask_ops = { .set_system_active = flask_set_system_active, .security_domaininfo = flask_security_domaininfo, + .ssidref_dom0 = flask_ssidref_dom0, + .ssidref_domU = flask_ssidref_domU, .domain_create = flask_domain_create, .getdomaininfo = flask_getdomaininfo, .domctl_scheduler_op = flask_domctl_scheduler_op,
flask_domain_alloc_security and flask_domain_create has special code to magically label dom0 as dom0_t. This can all be streamlined by making create_dom0 set ssidref before creating dom0. create_domU is also extended to create domains with domU_t. xsm_ssidref_domU and xsm_ssidref_dom0 are introduced to abstract away the details. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- Untested on ARM. Minimally tested on x86. Needs your Flask permission changes for xenboot_t to create dom0_t and domU_t. This is what I was thinking would be a better way to handle SID assignment. Regards, Jason --- xen/arch/arm/domain_build.c | 2 ++ xen/arch/x86/setup.c | 1 + xen/include/xsm/dummy.h | 10 ++++++++++ xen/include/xsm/xsm.h | 12 ++++++++++++ xen/xsm/dummy.c | 2 ++ xen/xsm/flask/hooks.c | 31 +++++++++++++++++-------------- 6 files changed, 44 insertions(+), 14 deletions(-)