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(-)
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(-)
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,
--
2.36.1On 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
© 2016 - 2024 Red Hat, Inc.