[PATCH v9 0/3] Adds starting the idle domain privileged

[PATCH v9 0/3] Adds starting the idle domain privileged

[Daniel P. Smith]

This series makes it so that the idle domain is started privileged under the
default policy, which the SILO policy inherits, and under the flask policy. It
then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
by an XSM policy to transition the idle domain to its running privilege level.

Patch 3 is an important one, as first it addresses the issue raised under an
RFC late last year by Jason Andryuk regarding the awkward entanglement of
flask_domain_alloc_security() and flask_domain_create(). Second, it helps
articulate why it is that the hypervisor should go through the access control
checks, even when it is doing the action itself. The issue at hand is not that
the hypervisor could be influenced to go around these check. The issue is these
checks provides a configurable way to express the execution flow that the
hypervisor should enforce. Specifically with this change, it is now possible
for an owner of a dom0less or hyperlaunch system to express a policy where the
hypervisor will enforce that no dom0 will be constructed, regardless of what
boot construction details were provided to it. Likewise, an owner that does not
want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
will only construct a dom0 domain. This can all be accomplished without the
need to rebuild the hypervisor with these features enabled or disabled.

Changes in v9:
- added missing Rb/Tb to patch 1
- corrected the flask policy macro in patch 2 to allow domain create
- added patch 3 to address allowing the hypervisor create more than 1 domain

Changes in v8:
- adjusted panic messages in arm and x86 setup.c to be less than 80cols
- fixed comment line that went over 80col
- added line in patch #1 commit message to clarify the need is for domain
  creation

Changes in v7:
- adjusted error message in default and flask xsm_set_system_active hooks
- merged panic messages in arm and x86 setup.c to a single line

Changes in v6:
- readded the setting of is_privileged in flask_set_system_active()
- clarified comment on is_privileged in flask_set_system_active()
- added ASSERT on is_privileged and self_sid in flask_set_system_active()
- fixed err code returned on Arm for xsm_set_system_active() panic message

Changes in v5:
- dropped setting is_privileged in flask_set_system_active()
- added err code returned by xsm_set_system_active() to panic message

Changes in v4:
- reworded patch 1 commit messaged
- fixed whitespace to coding style
- fixed comment to coding style

Changes in v3:
- renamed *_transition_running() to *_set_system_active()
- changed the XSM hook set_system_active() from void to int return
- added ASSERT check for the expected privilege level each XSM policy expected
- replaced a check against is_privileged in each arch with checking the return
  value from the call to xsm_set_system_active()

Changes in v2:
- renamed flask_domain_runtime_security() to flask_transition_running()
- added the missed assignment of self_sid

Daniel P. Smith (3):
  xsm: create idle domain privileged and demote after setup
  flask: implement xsm_set_system_active
  xsm: refactor flask sid alloc and domain check

 tools/flask/policy/modules/dom0.te     |  3 ++
 tools/flask/policy/modules/domU.te     |  3 ++
 tools/flask/policy/modules/xen.if      |  7 +++
 tools/flask/policy/modules/xen.te      |  1 +
 tools/flask/policy/policy/initial_sids |  1 +
 xen/arch/arm/setup.c                   |  3 ++
 xen/arch/x86/setup.c                   |  4 ++
 xen/common/sched/core.c                |  7 ++-
 xen/include/xsm/dummy.h                | 17 +++++++
 xen/include/xsm/xsm.h                  |  6 +++
 xen/xsm/dummy.c                        |  1 +
 xen/xsm/flask/hooks.c                  | 66 ++++++++++++++++++++------
 xen/xsm/flask/policy/initial_sids      |  1 +
 13 files changed, 104 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH v9 1/3] xsm: create idle domain privileged and demote after setup

[Daniel P. Smith]

There are new capabilities, dom0less and hyperlaunch, that introduce internal
hypervisor logic, which needs to make resource allocation calls that are
protected by XSM access checks. The need for these resource allocations are
necessary for dom0less and hyperlaunch when they are constructing the initial
domain(s).  This creates an issue as a subset of the hypervisor code is
executed under a system domain, the idle domain, that is represented by a
per-CPU non-privileged struct domain. To enable these new capabilities to
function correctly but in a controlled manner, this commit changes the idle
system domain to be created as a privileged domain under the default policy and
demoted before transitioning to running. A new XSM hook,
xsm_set_system_active(), is introduced to allow each XSM policy type to demote
the idle domain appropriately for that policy type. In the case of SILO, it
inherits the default policy's hook for xsm_set_system_active().

For flask, a stub is added to ensure that flask policy system will function
correctly with this patch until flask is extended with support for starting the
idle domain privileged and properly demoting it on the call to
xsm_set_system_active().

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com> # arm
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/setup.c    |  3 +++
 xen/arch/x86/setup.c    |  4 ++++
 xen/common/sched/core.c |  7 ++++++-
 xen/include/xsm/dummy.h | 17 +++++++++++++++++
 xen/include/xsm/xsm.h   |  6 ++++++
 xen/xsm/dummy.c         |  1 +
 xen/xsm/flask/hooks.c   | 23 +++++++++++++++++++++++
 7 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 577c54e6fb..85ff956ec2 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1063,6 +1063,9 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Hide UART from DOM0 if we're using it */
     serial_endboot();
 
+    if ( (rc = xsm_set_system_active()) != 0 )
+        panic("xsm: unable to switch to SYSTEM_ACTIVE privilege: %d\n", rc);
+
     system_state = SYS_STATE_active;
 
     for_each_domain( d )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 53a73010e0..f08b07b8de 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -619,6 +619,10 @@ static void noreturn init_done(void)
 {
     void *va;
     unsigned long start, end;
+    int err;
+
+    if ( (err = xsm_set_system_active()) != 0 )
+        panic("xsm: unable to switch to SYSTEM_ACTIVE privilege: %d\n", err);
 
     system_state = SYS_STATE_active;
 
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 8c73489654..250207038e 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3033,7 +3033,12 @@ void __init scheduler_init(void)
         sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
     }
 
-    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
+    /*
+     * The idle dom is created privileged to ensure unrestricted access during
+     * setup and will be demoted by xsm_set_system_active() when setup is
+     * complete.
+     */
+    idle_domain = domain_create(DOMID_IDLE, NULL, CDF_privileged);
     BUG_ON(IS_ERR(idle_domain));
     BUG_ON(nr_cpu_ids > ARRAY_SIZE(idle_vcpu));
     idle_domain->vcpu = idle_vcpu;
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 58afc1d589..77f27e7163 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -101,6 +101,23 @@ static always_inline int xsm_default_action(
     }
 }
 
+static XSM_INLINE int cf_check xsm_set_system_active(void)
+{
+    struct domain *d = current->domain;
+
+    ASSERT(d->is_privileged);
+
+    if ( d->domain_id != DOMID_IDLE )
+    {
+        printk("%s: should only be called by idle domain\n", __func__);
+        return -EPERM;
+    }
+
+    d->is_privileged = false;
+
+    return 0;
+}
+
 static XSM_INLINE void cf_check xsm_security_domaininfo(
     struct domain *d, struct xen_domctl_getdomaininfo *info)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 3e2b7fe3db..8dad03fd3d 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -52,6 +52,7 @@ typedef enum xsm_default xsm_default_t;
  * !!! WARNING !!!
  */
 struct xsm_ops {
+    int (*set_system_active)(void);
     void (*security_domaininfo)(struct domain *d,
                                 struct xen_domctl_getdomaininfo *info);
     int (*domain_create)(struct domain *d, uint32_t ssidref);
@@ -208,6 +209,11 @@ extern struct xsm_ops xsm_ops;
 
 #ifndef XSM_NO_WRAPPERS
 
+static inline int xsm_set_system_active(void)
+{
+    return alternative_call(xsm_ops.set_system_active);
+}
+
 static inline void xsm_security_domaininfo(
     struct domain *d, struct xen_domctl_getdomaininfo *info)
 {
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 8c044ef615..e6ffa948f7 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -14,6 +14,7 @@
 #include <xsm/dummy.h>
 
 static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
+    .set_system_active             = xsm_set_system_active,
     .security_domaininfo           = xsm_security_domaininfo,
     .domain_create                 = xsm_domain_create,
     .getdomaininfo                 = xsm_getdomaininfo,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 6ffafc2f44..c97c44f803 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -191,6 +191,28 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
     return 0;
 }
 
+static int cf_check flask_set_system_active(void)
+{
+    struct domain *d = current->domain;
+
+    ASSERT(d->is_privileged);
+
+    if ( d->domain_id != DOMID_IDLE )
+    {
+        printk("%s: should only be called by idle domain\n", __func__);
+        return -EPERM;
+    }
+
+    /*
+     * While is_privileged has no significant meaning under flask, set to false
+     * as is_privileged is not only used for a privilege check but also as a
+     * type of domain check, specifically if the domain is the control domain.
+     */
+    d->is_privileged = false;
+
+    return 0;
+}
+
 static void cf_check flask_domain_free_security(struct domain *d)
 {
     struct domain_security_struct *dsec = d->ssid;
@@ -1774,6 +1796,7 @@ static int cf_check flask_argo_send(
 #endif
 
 static const struct xsm_ops __initconst_cf_clobber flask_ops = {
+    .set_system_active = flask_set_system_active,
     .security_domaininfo = flask_security_domaininfo,
     .domain_create = flask_domain_create,
     .getdomaininfo = flask_getdomaininfo,
-- 
2.20.1

[PATCH v9 2/3] flask: implement xsm_set_system_active

[Daniel P. Smith]

This commit implements full support for starting the idle domain privileged by
introducing a new flask label xenboot_t which the idle domain is labeled with
at creation.  It then provides the implementation for the XSM hook
xsm_set_system_active to relabel the idle domain to the existing xen_t flask
label.

In the reference flask policy a new macro, xen_build_domain(target), is
introduced for creating policies for dom0less/hyperlaunch allowing the
hypervisor to create and assign the necessary resources for domain
construction.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
 tools/flask/policy/modules/xen.if      | 7 +++++++
 tools/flask/policy/modules/xen.te      | 1 +
 tools/flask/policy/policy/initial_sids | 1 +
 xen/xsm/flask/hooks.c                  | 9 ++++++++-
 xen/xsm/flask/policy/initial_sids      | 1 +
 5 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 5e2aa472b6..424daab6a0 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -62,6 +62,13 @@ define(`create_domain_common', `
 			setparam altp2mhvm altp2mhvm_op dm };
 ')
 
+# xen_build_domain(target)
+#   Allow a domain to be created at boot by the hypervisor
+define(`xen_build_domain', `
+	allow xenboot_t $1:domain create;
+	allow xenboot_t $1_channel:event create;
+')
+
 # create_domain(priv, target)
 #   Allow a domain to be created directly
 define(`create_domain', `
diff --git a/tools/flask/policy/modules/xen.te b/tools/flask/policy/modules/xen.te
index 3dbf93d2b8..de98206fdd 100644
--- a/tools/flask/policy/modules/xen.te
+++ b/tools/flask/policy/modules/xen.te
@@ -24,6 +24,7 @@ attribute mls_priv;
 ################################################################################
 
 # The hypervisor itself
+type xenboot_t, xen_type, mls_priv;
 type xen_t, xen_type, mls_priv;
 
 # Domain 0
diff --git a/tools/flask/policy/policy/initial_sids b/tools/flask/policy/policy/initial_sids
index 6b7b7eff21..ec729d3ba3 100644
--- a/tools/flask/policy/policy/initial_sids
+++ b/tools/flask/policy/policy/initial_sids
@@ -2,6 +2,7 @@
 # objects created before the policy is loaded or for objects that do not have a
 # label defined in some other manner.
 
+sid xenboot gen_context(system_u:system_r:xenboot_t,s0)
 sid xen gen_context(system_u:system_r:xen_t,s0)
 sid dom0 gen_context(system_u:system_r:dom0_t,s0)
 sid domxen gen_context(system_u:system_r:domxen_t,s0)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index c97c44f803..8c9cd0f297 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -173,7 +173,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
     switch ( d->domain_id )
     {
     case DOMID_IDLE:
-        dsec->sid = SECINITSID_XEN;
+        dsec->sid = SECINITSID_XENBOOT;
         break;
     case DOMID_XEN:
         dsec->sid = SECINITSID_DOMXEN;
@@ -193,9 +193,14 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
 
 static int cf_check flask_set_system_active(void)
 {
+    struct domain_security_struct *dsec;
     struct domain *d = current->domain;
 
+    dsec = d->ssid;
+
     ASSERT(d->is_privileged);
+    ASSERT(dsec->sid == SECINITSID_XENBOOT);
+    ASSERT(dsec->self_sid == SECINITSID_XENBOOT);
 
     if ( d->domain_id != DOMID_IDLE )
     {
@@ -210,6 +215,8 @@ static int cf_check flask_set_system_active(void)
      */
     d->is_privileged = false;
 
+    dsec->self_sid = dsec->sid = SECINITSID_XEN;
+
     return 0;
 }
 
diff --git a/xen/xsm/flask/policy/initial_sids b/xen/xsm/flask/policy/initial_sids
index 7eca70d339..e8b55b8368 100644
--- a/xen/xsm/flask/policy/initial_sids
+++ b/xen/xsm/flask/policy/initial_sids
@@ -3,6 +3,7 @@
 #
 # Define initial security identifiers 
 #
+sid xenboot
 sid xen
 sid dom0
 sid domio
-- 
2.20.1

[PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Daniel P. Smith]

The function flask_domain_alloc_security() is where a default sid should be
assigned to a domain under construction. For reasons unknown, the initial
domain would be assigned unlabeled_t and then fixed up under
flask_domain_create().  With the introduction of xenboot_t it is now possible
to distinguish when the hypervisor is in the boot state.

This commit looks to correct this by using a check to see if the hypervisor is
under the xenboot_t context in flask_domain_alloc_security(). If it is, then it
will inspect the domain's is_privileged field, and select the appropriate
default label, dom0_t or domU_t, for the domain. The logic for
flask_domain_create() was changed to allow the incoming sid to override the
default label.

The base policy was adjusted to allow the idle domain under the xenboot_t
context to be able to construct domains of both types, dom0 and domU.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 tools/flask/policy/modules/dom0.te |  3 +++
 tools/flask/policy/modules/domU.te |  3 +++
 xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 0a63ce15b6..2022bb9636 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
 admin_device(dom0_t, iomem_t)
 
 domain_comms(dom0_t, dom0_t)
+
+# Allow they hypervisor to build domains of type dom0_t
+xen_build_domain(dom0_t)
diff --git a/tools/flask/policy/modules/domU.te b/tools/flask/policy/modules/domU.te
index b77df29d56..73fc90c3c6 100644
--- a/tools/flask/policy/modules/domU.te
+++ b/tools/flask/policy/modules/domU.te
@@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
 migrate_domain_out(dom0_t, domU_t)
 domain_self_comms(domU_t)
 
+# Allow they hypervisor to build domains of type domU_t
+xen_build_domain(domU_t)
+
 # Device model for domU_t.  You can define distinct types for device models for
 # domains of other types, or add more make_device_model lines for this type.
 declare_domain(dm_dom_t)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 8c9cd0f297..caa0ae7d4c 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -182,7 +182,15 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
         dsec->sid = SECINITSID_DOMIO;
         break;
     default:
-        dsec->sid = SECINITSID_UNLABELED;
+        if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
+        {
+            if ( d->is_privileged )
+                dsec->sid = SECINITSID_DOM0;
+            else
+                dsec->sid = SECINITSID_DOMU;
+        }
+        else
+            dsec->sid = SECINITSID_UNLABELED;
     }
 
     dsec->self_sid = dsec->sid;
@@ -548,23 +556,21 @@ 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
+    /*
+     * If domain has not already been labeled or a valid new label is provided,
+     * then use the provided label, otherwise use the existing label.
+     */
+    if ( dsec->sid == SECINITSID_UNLABELED || ssidref > 0 )
     {
-        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
-                          DOMAIN__CREATE, NULL);
-        if ( rc )
-            return rc;
-
         dsec->sid = ssidref;
+        dsec->self_sid = dsec->sid;
     }
-    dsec->self_sid = dsec->sid;
+
+    rc = avc_current_has_perm(dsec->sid, SECCLASS_DOMAIN,
+                              DOMAIN__CREATE, NULL);
+    if ( rc )
+        return rc;
 
     rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
                                  &dsec->self_sid);
-- 
2.20.1

Re: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Jan Beulich]

Just a two nits - while the change looks plausible, I'm afraid I'm
not qualified to properly review it.

On 30.06.2022 04:21, Daniel P. Smith wrote:
> The function flask_domain_alloc_security() is where a default sid should be
> assigned to a domain under construction. For reasons unknown, the initial
> domain would be assigned unlabeled_t and then fixed up under
> flask_domain_create().  With the introduction of xenboot_t it is now possible
> to distinguish when the hypervisor is in the boot state.
> 
> This commit looks to correct this by using a check to see if the hypervisor is
> under the xenboot_t context in flask_domain_alloc_security(). If it is, then it

While (or maybe because) I'm not a native speaker, the use of "looks"
reads ambiguous to me. I think you mean it in the sense of e.g. "aims",
but at first I read it in the sense of "seems", which made me think
you're not certain whether it actually does.

> will inspect the domain's is_privileged field, and select the appropriate
> default label, dom0_t or domU_t, for the domain. The logic for
> flask_domain_create() was changed to allow the incoming sid to override the
> default label.
> 
> The base policy was adjusted to allow the idle domain under the xenboot_t
> context to be able to construct domains of both types, dom0 and domU.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  tools/flask/policy/modules/dom0.te |  3 +++
>  tools/flask/policy/modules/domU.te |  3 +++
>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>  3 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
> index 0a63ce15b6..2022bb9636 100644
> --- a/tools/flask/policy/modules/dom0.te
> +++ b/tools/flask/policy/modules/dom0.te
> @@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
>  admin_device(dom0_t, iomem_t)
>  
>  domain_comms(dom0_t, dom0_t)
> +
> +# Allow they hypervisor to build domains of type dom0_t

Since it repeats ...

> +xen_build_domain(dom0_t)
> diff --git a/tools/flask/policy/modules/domU.te b/tools/flask/policy/modules/domU.te
> index b77df29d56..73fc90c3c6 100644
> --- a/tools/flask/policy/modules/domU.te
> +++ b/tools/flask/policy/modules/domU.te
> @@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
>  migrate_domain_out(dom0_t, domU_t)
>  domain_self_comms(domU_t)
>  
> +# Allow they hypervisor to build domains of type domU_t
> +xen_build_domain(domU_t)

... here - s/they/the/ in both places?

Jan

RE: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Henry Wang]

Hi Daniel,

> -----Original Message-----
> Subject: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check
> 
> The function flask_domain_alloc_security() is where a default sid should be
> assigned to a domain under construction. For reasons unknown, the initial
> domain would be assigned unlabeled_t and then fixed up under
> flask_domain_create().  With the introduction of xenboot_t it is now possible
> to distinguish when the hypervisor is in the boot state.
> 
> This commit looks to correct this by using a check to see if the hypervisor is
> under the xenboot_t context in flask_domain_alloc_security(). If it is, then it
> will inspect the domain's is_privileged field, and select the appropriate
> default label, dom0_t or domU_t, for the domain. The logic for
> flask_domain_create() was changed to allow the incoming sid to override the
> default label.
> 
> The base policy was adjusted to allow the idle domain under the xenboot_t
> context to be able to construct domains of both types, dom0 and domU.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Same as what Jan has said, I don't think I am qualified to properly review
the series, but I did run a compile and runtime test on Arm64 platform with
the xsm and flask enabled and it looks like everything is fine.

Hence (also for the whole series):
Tested-by: Henry Wang <Henry.Wang@arm.com>

> ---
>  tools/flask/policy/modules/dom0.te |  3 +++
>  tools/flask/policy/modules/domU.te |  3 +++
>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>  3 files changed, 26 insertions(+), 14 deletions(-)

Re: [PATCH v9 1/3] xsm: create idle domain privileged and demote after setup

[Roger Pau Monné]

On Wed, Jun 29, 2022 at 10:21:08PM -0400, Daniel P. Smith wrote:
> There are new capabilities, dom0less and hyperlaunch, that introduce internal
> hypervisor logic, which needs to make resource allocation calls that are
> protected by XSM access checks. The need for these resource allocations are
> necessary for dom0less and hyperlaunch when they are constructing the initial
> domain(s).  This creates an issue as a subset of the hypervisor code is
> executed under a system domain, the idle domain, that is represented by a
> per-CPU non-privileged struct domain. To enable these new capabilities to
> function correctly but in a controlled manner, this commit changes the idle
> system domain to be created as a privileged domain under the default policy and
> demoted before transitioning to running. A new XSM hook,
> xsm_set_system_active(), is introduced to allow each XSM policy type to demote
> the idle domain appropriately for that policy type. In the case of SILO, it
> inherits the default policy's hook for xsm_set_system_active().
> 
> For flask, a stub is added to ensure that flask policy system will function
> correctly with this patch until flask is extended with support for starting the
> idle domain privileged and properly demoting it on the call to
> xsm_set_system_active().
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> Acked-by: Julien Grall <jgrall@amazon.com> # arm
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>

LGTM:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Daniel P. Smith]

On 6/30/22 02:14, Jan Beulich wrote:
> Just a two nits - while the change looks plausible, I'm afraid I'm
> not qualified to properly review it.
> 
> On 30.06.2022 04:21, Daniel P. Smith wrote:
>> The function flask_domain_alloc_security() is where a default sid should be
>> assigned to a domain under construction. For reasons unknown, the initial
>> domain would be assigned unlabeled_t and then fixed up under
>> flask_domain_create().  With the introduction of xenboot_t it is now possible
>> to distinguish when the hypervisor is in the boot state.
>>
>> This commit looks to correct this by using a check to see if the hypervisor is
>> under the xenboot_t context in flask_domain_alloc_security(). If it is, then it
> 
> While (or maybe because) I'm not a native speaker, the use of "looks"
> reads ambiguous to me. I think you mean it in the sense of e.g. "aims",
> but at first I read it in the sense of "seems", which made me think
> you're not certain whether it actually does.

Apologies, "look to" or "looks to" are forms of an American idiom, and
was used for its meaning of "expected to happen"[1]. I will reword to
provide a concise version of this statement.

[1] https://idioms.thefreedictionary.com/look+to

>> will inspect the domain's is_privileged field, and select the appropriate
>> default label, dom0_t or domU_t, for the domain. The logic for
>> flask_domain_create() was changed to allow the incoming sid to override the
>> default label.
>>
>> The base policy was adjusted to allow the idle domain under the xenboot_t
>> context to be able to construct domains of both types, dom0 and domU.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>  tools/flask/policy/modules/dom0.te |  3 +++
>>  tools/flask/policy/modules/domU.te |  3 +++
>>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>>  3 files changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
>> index 0a63ce15b6..2022bb9636 100644
>> --- a/tools/flask/policy/modules/dom0.te
>> +++ b/tools/flask/policy/modules/dom0.te
>> @@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
>>  admin_device(dom0_t, iomem_t)
>>  
>>  domain_comms(dom0_t, dom0_t)
>> +
>> +# Allow they hypervisor to build domains of type dom0_t
> 
> Since it repeats ...

Ack.

>> +xen_build_domain(dom0_t)
>> diff --git a/tools/flask/policy/modules/domU.te b/tools/flask/policy/modules/domU.te
>> index b77df29d56..73fc90c3c6 100644
>> --- a/tools/flask/policy/modules/domU.te
>> +++ b/tools/flask/policy/modules/domU.te
>> @@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
>>  migrate_domain_out(dom0_t, domU_t)
>>  domain_self_comms(domU_t)
>>  
>> +# Allow they hypervisor to build domains of type domU_t
>> +xen_build_domain(domU_t)
> 
> ... here - s/they/the/ in both places?

Ack.

> Jan

Re: [PATCH v9 1/3] xsm: create idle domain privileged and demote after setup

[Daniel P. Smith]

On 6/30/22 05:24, Roger Pau Monné wrote:
> On Wed, Jun 29, 2022 at 10:21:08PM -0400, Daniel P. Smith wrote:
>> There are new capabilities, dom0less and hyperlaunch, that introduce internal
>> hypervisor logic, which needs to make resource allocation calls that are
>> protected by XSM access checks. The need for these resource allocations are
>> necessary for dom0less and hyperlaunch when they are constructing the initial
>> domain(s).  This creates an issue as a subset of the hypervisor code is
>> executed under a system domain, the idle domain, that is represented by a
>> per-CPU non-privileged struct domain. To enable these new capabilities to
>> function correctly but in a controlled manner, this commit changes the idle
>> system domain to be created as a privileged domain under the default policy and
>> demoted before transitioning to running. A new XSM hook,
>> xsm_set_system_active(), is introduced to allow each XSM policy type to demote
>> the idle domain appropriately for that policy type. In the case of SILO, it
>> inherits the default policy's hook for xsm_set_system_active().
>>
>> For flask, a stub is added to ensure that flask policy system will function
>> correctly with this patch until flask is extended with support for starting the
>> idle domain privileged and properly demoting it on the call to
>> xsm_set_system_active().
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
>> Acked-by: Julien Grall <jgrall@amazon.com> # arm
>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>> Tested-by: Rahul Singh <rahul.singh@arm.com>
> 
> LGTM:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks, Roger.

Thank you.

Re: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Daniel P. Smith]

On 6/30/22 04:40, Henry Wang wrote:
> Hi Daniel,
> 
>> -----Original Message-----
>> Subject: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check
>>
>> The function flask_domain_alloc_security() is where a default sid should be
>> assigned to a domain under construction. For reasons unknown, the initial
>> domain would be assigned unlabeled_t and then fixed up under
>> flask_domain_create().  With the introduction of xenboot_t it is now possible
>> to distinguish when the hypervisor is in the boot state.
>>
>> This commit looks to correct this by using a check to see if the hypervisor is
>> under the xenboot_t context in flask_domain_alloc_security(). If it is, then it
>> will inspect the domain's is_privileged field, and select the appropriate
>> default label, dom0_t or domU_t, for the domain. The logic for
>> flask_domain_create() was changed to allow the incoming sid to override the
>> default label.
>>
>> The base policy was adjusted to allow the idle domain under the xenboot_t
>> context to be able to construct domains of both types, dom0 and domU.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Same as what Jan has said, I don't think I am qualified to properly review
> the series, but I did run a compile and runtime test on Arm64 platform with
> the xsm and flask enabled and it looks like everything is fine.

Ack.

> Hence (also for the whole series):
> Tested-by: Henry Wang <Henry.Wang@arm.com>

Thank you.

>> ---
>>  tools/flask/policy/modules/dom0.te |  3 +++
>>  tools/flask/policy/modules/domU.te |  3 +++
>>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>>  3 files changed, 26 insertions(+), 14 deletions(-)
> 

Re: [PATCH v9 0/3] Adds starting the idle domain privileged

[Stefano Stabellini]

On Wed, 29 Jun 2022, Daniel P. Smith wrote:
> This series makes it so that the idle domain is started privileged under the
> default policy, which the SILO policy inherits, and under the flask policy. It
> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
> by an XSM policy to transition the idle domain to its running privilege level.
> 
> Patch 3 is an important one, as first it addresses the issue raised under an
> RFC late last year by Jason Andryuk regarding the awkward entanglement of
> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
> articulate why it is that the hypervisor should go through the access control
> checks, even when it is doing the action itself. The issue at hand is not that
> the hypervisor could be influenced to go around these check. The issue is these
> checks provides a configurable way to express the execution flow that the
> hypervisor should enforce. Specifically with this change, it is now possible
> for an owner of a dom0less or hyperlaunch system to express a policy where the
> hypervisor will enforce that no dom0 will be constructed, regardless of what
> boot construction details were provided to it. Likewise, an owner that does not
> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
> will only construct a dom0 domain. This can all be accomplished without the
> need to rebuild the hypervisor with these features enabled or disabled.


It looks like this patch series is fully acked except:
- in theory we need an ack from Daniel for flask
- there is a very small change to sched that would need an ack from
  George/Dario


I think we should commit the series in a couple of days unless someone
spots an issue with it. Let me know if you have any concerns with this.

The minimal request to improve the in-code comment by Jan could be done
on commit.

Note that committing this series would also have the benefit of
unblocking the ARM gitlab-ci pipeline.

Re: [PATCH v9 0/3] Adds starting the idle domain privileged

[Daniel P. Smith]

On 6/30/22 18:35, Stefano Stabellini wrote:
> On Wed, 29 Jun 2022, Daniel P. Smith wrote:
>> This series makes it so that the idle domain is started privileged under the
>> default policy, which the SILO policy inherits, and under the flask policy. It
>> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
>> by an XSM policy to transition the idle domain to its running privilege level.
>>
>> Patch 3 is an important one, as first it addresses the issue raised under an
>> RFC late last year by Jason Andryuk regarding the awkward entanglement of
>> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
>> articulate why it is that the hypervisor should go through the access control
>> checks, even when it is doing the action itself. The issue at hand is not that
>> the hypervisor could be influenced to go around these check. The issue is these
>> checks provides a configurable way to express the execution flow that the
>> hypervisor should enforce. Specifically with this change, it is now possible
>> for an owner of a dom0less or hyperlaunch system to express a policy where the
>> hypervisor will enforce that no dom0 will be constructed, regardless of what
>> boot construction details were provided to it. Likewise, an owner that does not
>> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
>> will only construct a dom0 domain. This can all be accomplished without the
>> need to rebuild the hypervisor with these features enabled or disabled.
> 
> 
> It looks like this patch series is fully acked except:
> - in theory we need an ack from Daniel for flask
> - there is a very small change to sched that would need an ack from
>   George/Dario
> 
> 
> I think we should commit the series in a couple of days unless someone
> spots an issue with it. Let me know if you have any concerns with this.
> 
> The minimal request to improve the in-code comment by Jan could be done
> on commit.

I already have those changes ready to go. I was holding off with the
hope that Jason might find some time to read the last patch. I can try
pinging him directly tomorrow to see if he is even in town, as this is a
big holiday weekend here in the states.

> Note that committing this series would also have the benefit of
> unblocking the ARM gitlab-ci pipeline.

v/r,
dps

Re: [PATCH v9 0/3] Adds starting the idle domain privileged

[Jan Beulich]

On 01.07.2022 00:35, Stefano Stabellini wrote:
> On Wed, 29 Jun 2022, Daniel P. Smith wrote:
>> This series makes it so that the idle domain is started privileged under the
>> default policy, which the SILO policy inherits, and under the flask policy. It
>> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
>> by an XSM policy to transition the idle domain to its running privilege level.
>>
>> Patch 3 is an important one, as first it addresses the issue raised under an
>> RFC late last year by Jason Andryuk regarding the awkward entanglement of
>> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
>> articulate why it is that the hypervisor should go through the access control
>> checks, even when it is doing the action itself. The issue at hand is not that
>> the hypervisor could be influenced to go around these check. The issue is these
>> checks provides a configurable way to express the execution flow that the
>> hypervisor should enforce. Specifically with this change, it is now possible
>> for an owner of a dom0less or hyperlaunch system to express a policy where the
>> hypervisor will enforce that no dom0 will be constructed, regardless of what
>> boot construction details were provided to it. Likewise, an owner that does not
>> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
>> will only construct a dom0 domain. This can all be accomplished without the
>> need to rebuild the hypervisor with these features enabled or disabled.
> 
> 
> It looks like this patch series is fully acked except:
> - in theory we need an ack from Daniel for flask
> - there is a very small change to sched that would need an ack from
>   George/Dario

I don't think I've seen any R-b for the last patch.

Jan

Re: [PATCH v9 0/3] Adds starting the idle domain privileged

[Stefano Stabellini]

On Fri, 1 Jul 2022, Jan Beulich wrote:
> On 01.07.2022 00:35, Stefano Stabellini wrote:
> > On Wed, 29 Jun 2022, Daniel P. Smith wrote:
> >> This series makes it so that the idle domain is started privileged under the
> >> default policy, which the SILO policy inherits, and under the flask policy. It
> >> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
> >> by an XSM policy to transition the idle domain to its running privilege level.
> >>
> >> Patch 3 is an important one, as first it addresses the issue raised under an
> >> RFC late last year by Jason Andryuk regarding the awkward entanglement of
> >> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
> >> articulate why it is that the hypervisor should go through the access control
> >> checks, even when it is doing the action itself. The issue at hand is not that
> >> the hypervisor could be influenced to go around these check. The issue is these
> >> checks provides a configurable way to express the execution flow that the
> >> hypervisor should enforce. Specifically with this change, it is now possible
> >> for an owner of a dom0less or hyperlaunch system to express a policy where the
> >> hypervisor will enforce that no dom0 will be constructed, regardless of what
> >> boot construction details were provided to it. Likewise, an owner that does not
> >> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
> >> will only construct a dom0 domain. This can all be accomplished without the
> >> need to rebuild the hypervisor with these features enabled or disabled.
> > 
> > 
> > It looks like this patch series is fully acked except:
> > - in theory we need an ack from Daniel for flask
> > - there is a very small change to sched that would need an ack from
> >   George/Dario
> 
> I don't think I've seen any R-b for the last patch.

Good point. I hope you'll be happy to give your Reviewed-by or Acked-by
to the next version with the minor comments fixed.

Re: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Jason Andryuk]

On Wed, Jun 29, 2022 at 10:22 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> The function flask_domain_alloc_security() is where a default sid should be
> assigned to a domain under construction. For reasons unknown, the initial
> domain would be assigned unlabeled_t and then fixed up under
> flask_domain_create().  With the introduction of xenboot_t it is now possible
> to distinguish when the hypervisor is in the boot state.

There is no "default label" which is why unlabeled_t was used.
flask_domain_create() does permission checks before setting the label
from the config.

flask_domain_alloc_security()
- mallocs domain_security_struct
- sets sid for system domains xen_t, domxen_t, domio_t
- is called for all domains.

flask_domain_create()
- special cases labeling dom0_t so it can only be done once.
- otherwise, ensures current has permission to domain_create.
- sets sid for both cases.
- has the config->ssidref passed in.
- is only called for non-system, non-idle domains.

> This commit looks to correct this by using a check to see if the hypervisor is
> under the xenboot_t context in flask_domain_alloc_security(). If it is, then it
> will inspect the domain's is_privileged field, and select the appropriate
> default label, dom0_t or domU_t, for the domain. The logic for
> flask_domain_create() was changed to allow the incoming sid to override the
> default label.
>
> The base policy was adjusted to allow the idle domain under the xenboot_t
> context to be able to construct domains of both types, dom0 and domU.

Your patch special cases flask_domain_alloc_security() to assign
dom0/domU.  You already have a config and therefore config->ssidref,
so that shouldn't be necessary since flask_domain_create() can use
that ssidref.

I don't see a precise reason for why this change is needed in the
commit message.  I know you are working on hyperlaunch, but I'm
guessing at the rationale for this change.  And hyperlaunch should be
passing in a config with ssidref set instead of relying on the flask
code to auto assign, so I'm not sure of why this change would be
needed for hyperlaunch.

Is the issue with only having a single creation of dom0_t?  Can you
change create_dom()'s dom0_cfg to specify the ssidref when creating
dom0.  I guess that would need a new xsm_dom0_ssidref hook (or
something) to hide away the flask code.  But that way there is less
special casing in the code.  flask_domain_alloc_security() could be
left unchanged, and flask_domain_create could() be streamlined to just
check the passed in ssidref.  How does that sound?

> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  tools/flask/policy/modules/dom0.te |  3 +++
>  tools/flask/policy/modules/domU.te |  3 +++
>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>  3 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
> index 0a63ce15b6..2022bb9636 100644
> --- a/tools/flask/policy/modules/dom0.te
> +++ b/tools/flask/policy/modules/dom0.te
> @@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
>  admin_device(dom0_t, iomem_t)
>
>  domain_comms(dom0_t, dom0_t)
> +
> +# Allow they hypervisor to build domains of type dom0_t
> +xen_build_domain(dom0_t)
> diff --git a/tools/flask/policy/modules/domU.te b/tools/flask/policy/modules/domU.te
> index b77df29d56..73fc90c3c6 100644
> --- a/tools/flask/policy/modules/domU.te
> +++ b/tools/flask/policy/modules/domU.te
> @@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
>  migrate_domain_out(dom0_t, domU_t)
>  domain_self_comms(domU_t)
>
> +# Allow they hypervisor to build domains of type domU_t
> +xen_build_domain(domU_t)
> +
>  # Device model for domU_t.  You can define distinct types for device models for
>  # domains of other types, or add more make_device_model lines for this type.
>  declare_domain(dm_dom_t)
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 8c9cd0f297..caa0ae7d4c 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -182,7 +182,15 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
>          dsec->sid = SECINITSID_DOMIO;
>          break;
>      default:
> -        dsec->sid = SECINITSID_UNLABELED;
> +        if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
> +        {
> +            if ( d->is_privileged )
> +                dsec->sid = SECINITSID_DOM0;
> +            else
> +                dsec->sid = SECINITSID_DOMU;
> +        }
> +        else
> +            dsec->sid = SECINITSID_UNLABELED;
>      }
>
>      dsec->self_sid = dsec->sid;
> @@ -548,23 +556,21 @@ 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
> +    /*
> +     * If domain has not already been labeled or a valid new label is provided,
> +     * then use the provided label, otherwise use the existing label.
> +     */
> +    if ( dsec->sid == SECINITSID_UNLABELED || ssidref > 0 )
>      {
> -        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
> -                          DOMAIN__CREATE, NULL);
> -        if ( rc )
> -            return rc;

The old code returned here before...

> -
>          dsec->sid = ssidref;

... setting the sid.  That is more robust since if the function fails,
the sid is not changed.  It's not a problem today as the
xsm_domain_create() return value is checked, but it is more robust to
restore that property.

Regards,
Jason

> +        dsec->self_sid = dsec->sid;
>      }
> -    dsec->self_sid = dsec->sid;
> +
> +    rc = avc_current_has_perm(dsec->sid, SECCLASS_DOMAIN,
> +                              DOMAIN__CREATE, NULL);
> +    if ( rc )
> +        return rc;
>
>      rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
>                                   &dsec->self_sid);
> --
> 2.20.1
>

Re: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

[Daniel P. Smith]

On 7/5/22 09:03, Jason Andryuk wrote:
> On Wed, Jun 29, 2022 at 10:22 PM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> The function flask_domain_alloc_security() is where a default sid should be
>> assigned to a domain under construction. For reasons unknown, the initial
>> domain would be assigned unlabeled_t and then fixed up under
>> flask_domain_create().  With the introduction of xenboot_t it is now possible
>> to distinguish when the hypervisor is in the boot state.
> 
> There is no "default label" which is why unlabeled_t was used.
> flask_domain_create() does permission checks before setting the label
> from the config.

The call to flask_domain_alloc_security() is in a shared code path for
both the hypervisor and toolstack domain creation. As such, there is no
label provided since it is not desirable to codify the domain label in
the general hypervisor code for when calling that shared path by the
hypervisor during setup. Since the function is responsible for
allocating the struct domain_security_struct{}, it is unwise to leave
the value uninitialized, zero/0, as that in a non-existent sid.
Therefore it implements a rudimentary assignment policy where if the
current domain is the idle domain, it assumes it is constructing one of
the system domains otherwise by default it assigns unlabeled_t.
Additionally, if a sid of 0 is provided by the toolstack, either by not
setting the sid or incorrectly setting the sid to 0, the result will be
that the security server will assign the label unlabeled_t to the
domain. This is what is meant by referring to it as the default label.

It should be noted that under the default policy, unlabeled_t is not a
valid type for a domain to run under. The result will be a failure as
there are no rules to allow the construction of domains with this type.

> flask_domain_alloc_security()
> - mallocs domain_security_struct
> - sets sid for system domains xen_t, domxen_t, domio_t
> - is called for all domains.

and you missed,
 - sets sid to unlabeled_t for all other domains.

The missed action along with the second action is what goes to the point
I was making in the commit message. This is because prior to the
introduction of xenboot_t, the only means to distinguish between
boot/setup time and runtime is if the domain id is a system domain id.
The basis for that limited distinction comes from the assumption that
domain_create() will block the use of a domain id from the reserved
range if the current domain is not the idle domain. For which, it should
be noted that there may be some unintended consequences. Whereas, with
the introduction of the xenboot_t label, there is a slightly stronger
binding to what state the construction of the domain is occurring.

> flask_domain_create()
> - special cases labeling dom0_t so it can only be done once.
> - otherwise, ensures current has permission to domain_create.
> - sets sid for both cases.
> - has the config->ssidref passed in.
> - is only called for non-system, non-idle domains.

I would clarify that last statement, 'is only called for domains which
require construction and assignment of resources', where assignment of
resources refers to resources that will be labeled based on domin's sid,
eg. domU_t_channel.

>> This commit looks to correct this by using a check to see if the hypervisor is
>> under the xenboot_t context in flask_domain_alloc_security(). If it is, then it
>> will inspect the domain's is_privileged field, and select the appropriate
>> default label, dom0_t or domU_t, for the domain. The logic for
>> flask_domain_create() was changed to allow the incoming sid to override the
>> default label.
>>
>> The base policy was adjusted to allow the idle domain under the xenboot_t
>> context to be able to construct domains of both types, dom0 and domU.
> 
> Your patch special cases flask_domain_alloc_security() to assign
> dom0/domU.  You already have a config and therefore config->ssidref,
> so that shouldn't be necessary since flask_domain_create() can use
> that ssidref.

As I stated above, flask_domain_alloc_security() contains an existing
policy for setting an initial value for the struct
security_domain_struct{} it is allocating based on assumptions that are
believed to be true. With the introduction of the xenboot_t transition,
a more solid set of assumptions can be made, which allows for a more
refined assignment policy.

Second, you are assuming there is a config that has a valid ssidref set.
Currently, dom0less does not support passing a sid in their
configuration. As I have reasoned the code and explained above, I do not
see how dom0less could construct a validly labeled domU, or more than
one domU with FLASK in enforcing mode. Based on my review, three
scenarios would play out for dom0less using FLASK,

1. The first domain to be constructed was a dom0.

The first domain would be created, labeled as dom0_t, the flag
dom0_created would be set to one, and then constructed. All subsequent
domains would fail creation because,
 - they would be labeled as unlabeled_t by flask_domain_alloc_security()
 - the parameter ssidref to flask_domain_create() would be 0
 - the !dom0_created check would fail
 - the call to avc_current_has_perm() would fail

With avc_current_has_perm() failing because ssidref's value of 0 will be
converted to unlabeled_t and the default/reference policy will deny
xenboot_t domain create of unlabeled_t.

The system will panic when the first domain creation failure occurrs.

2. The first domain is a domU and remaining may be either dom0 or domU.

The first domain would be created, incorrectly labeled as dom0_t, the
flag dom0_created would be set to one, and then constructed. All
subsequent domains will fail as they did in case 1, and again the system
will panic on the first domain creation failure.

3. A system with only a single domU.

The domain would be created, incorrectly labeled as dom0_t, the flag
dom0_created would be set to one, and then constructed. The system will
then finish and transition to running with the single domain, except
that domain will be fully privileged and not a domU as intended or expected.

> I don't see a precise reason for why this change is needed in the
> commit message.  I know you are working on hyperlaunch, but I'm
> guessing at the rationale for this change.  And hyperlaunch should be
> passing in a config with ssidref set instead of relying on the flask
> code to auto assign, so I'm not sure of why this change would be
> needed for hyperlaunch.

Apologies, I was attempting to be concise and hit the main points versus
a long diatribe.

While FLASK is not security supported, we should still ensure that
dom0less works completely as expected. Additionaly it should support
traditional intial domain, dom0less, and the upcoming hyperlaunch
without having to be special cased for each configuration. While
hyperlaunch will provide the ability to specify a sid in its
configuration, the initial domain and dom0less do not. Changing the XSM
interface to suit the single instance of the former versus the two
instances of the latter would seem slightly biased.

To summarize in another way,

FLASK has an internal sid assignement policy for when the hypervisor is
contstructing domains during boot. This is so the main hypervisor code
does not have to be made FLASK aware and potentially create a layering
violation. The current sid assignment policy is split between allocation
of the domain's security structure and the domain creation policy check.
The hypervisor has evolved since this policy was incorporated, seeing
the introduction of dom0less and soon hyperlaunch. This latest change
introduces a new security context for the hypervisor during setup along
with a domain transition for when the hypervisor moves to the running
state. With this new distinction is now possible to construct a more
informed initial sid assignment policy for domains conrstructed by the
hypervisor. The new assignment policy exapnds the existing policy to say
that if domain is not a system domain and the hypervisor is currently in
the boot/setup state, inspect the domain config to see if it is a
privileged domain, and set the label either dom0_t or domU_t accordingly.

The new initial sid assignment policy results in all of its logic being
centralized under the function flask_domain_alloc_security(). This
allows for the logic of flask_domain_create() to morph, were by the
parameter ssidref may now be treated simply as an override of the
initial sid assignment that a toolstack or a hypervisor domain builder,
such as hyperlaunch, may use to set the appropriate label.


> Is the issue with only having a single creation of dom0_t?  Can you
> change create_dom()'s dom0_cfg to specify the ssidref when creating
> dom0.  I guess that would need a new xsm_dom0_ssidref hook (or
> something) to hide away the flask code.  But that way there is less
> special casing in the code.  flask_domain_alloc_security() could be
> left unchanged, and flask_domain_create could() be streamlined to just
> check the passed in ssidref.  How does that sound?

As I noted above, coding in a flask label into main hypervisor code
would be a layering violation as it would assume the only XSM that would
ever use the struct domain_security_struct{}. This would defeat the
purpose of the introduction of struct domain_security_struct{} as it
previouly was assumed to be a flask security label.

Also previously mentioned, it goes beyond just dom0 config. Currently
dom0less does not have a way to specify the sid in a domU domain config.
This would mean another place with a flask label, domU_t, would have to
be codified into main hypervisor code.

IMHO, with a possible minor adjustment (see  below), the approach here
is the correct one. The two functions have distinct roles,
flask_domain_alloc_security() is for allocating the structure and
ensuring it is properly initialized. While flask_domain_create() is
where the check occurs if a domain of either the initially assigned type
or of the requested type, ssidref parameter, are allowed to be
constructed by the current domain. That is what is acheived here without
having to introduce a layering violation.

>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>  tools/flask/policy/modules/dom0.te |  3 +++
>>  tools/flask/policy/modules/domU.te |  3 +++
>>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>>  3 files changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
>> index 0a63ce15b6..2022bb9636 100644
>> --- a/tools/flask/policy/modules/dom0.te
>> +++ b/tools/flask/policy/modules/dom0.te
>> @@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
>>  admin_device(dom0_t, iomem_t)
>>
>>  domain_comms(dom0_t, dom0_t)
>> +
>> +# Allow they hypervisor to build domains of type dom0_t
>> +xen_build_domain(dom0_t)
>> diff --git a/tools/flask/policy/modules/domU.te b/tools/flask/policy/modules/domU.te
>> index b77df29d56..73fc90c3c6 100644
>> --- a/tools/flask/policy/modules/domU.te
>> +++ b/tools/flask/policy/modules/domU.te
>> @@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
>>  migrate_domain_out(dom0_t, domU_t)
>>  domain_self_comms(domU_t)
>>
>> +# Allow they hypervisor to build domains of type domU_t
>> +xen_build_domain(domU_t)
>> +
>>  # Device model for domU_t.  You can define distinct types for device models for
>>  # domains of other types, or add more make_device_model lines for this type.
>>  declare_domain(dm_dom_t)
>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index 8c9cd0f297..caa0ae7d4c 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -182,7 +182,15 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
>>          dsec->sid = SECINITSID_DOMIO;
>>          break;
>>      default:
>> -        dsec->sid = SECINITSID_UNLABELED;
>> +        if ( domain_sid(current->domain) == SECINITSID_XENBOOT )

After some thinking while responding to the review, this check should
move to the top and be negated. When requesting an allocation, if that
request was not by the idle domain during startup, then it should
immediately set the sid to SECINITSID_UNLABELED and return immediately.
Making it past that check will mean that the request is from the idle
domain during setup and should apply the initial sid policy.

>> +        {
>> +            if ( d->is_privileged )
>> +                dsec->sid = SECINITSID_DOM0;
>> +            else
>> +                dsec->sid = SECINITSID_DOMU;
>> +        }
>> +        else
>> +            dsec->sid = SECINITSID_UNLABELED;
>>      }
>>
>>      dsec->self_sid = dsec->sid;
>> @@ -548,23 +556,21 @@ 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
>> +    /*
>> +     * If domain has not already been labeled or a valid new label is provided,
>> +     * then use the provided label, otherwise use the existing label.
>> +     */
>> +    if ( dsec->sid == SECINITSID_UNLABELED || ssidref > 0 )
>>      {
>> -        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
>> -                          DOMAIN__CREATE, NULL);
>> -        if ( rc )
>> -            return rc;
> 
> The old code returned here before...
> 
>> -
>>          dsec->sid = ssidref;
> 
> ... setting the sid.  That is more robust since if the function fails,
> the sid is not changed.  It's not a problem today as the
> xsm_domain_create() return value is checked, but it is more robust to
> restore that property.

You are right, my intent/assumption was to rely on the code calling an
access check to correctly handle a failure response. I should reverse
the logic and use ssidref as the temporary holder of the value to check,
and only place that value into the struct on a successful check.

> Regards,
> Jason

Thank you for looking at this.

v/r,
dps

[RFC PATCH] flask: Remove magic SID setting

[Jason Andryuk]

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.1

Re: [RFC PATCH] flask: Remove magic SID setting

[Daniel P. Smith]

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,

Re: [RFC PATCH] flask: Remove magic SID setting

[Jason Andryuk]

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

Re: [RFC PATCH] flask: Remove magic SID setting

[Daniel P. Smith]

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