[PATCH v3 0/2] Adds starting the idle domain privileged

[PATCH v3 0/2] 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.

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 (2):
  xsm: create idle domain privileged and demote after setup
  flask: implement xsm_set_system_active

 tools/flask/policy/modules/xen.if      |  6 ++++++
 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                   |  3 +++
 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                  | 29 +++++++++++++++++++++++++-
 xen/xsm/flask/policy/initial_sids      |  1 +
 11 files changed, 73 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH v3 1/2] xsm: create idle domain privileged and demote after setup

[Daniel P. Smith]

There are now instances where internal hypervisor logic needs to make resource
allocation calls that are protected by XSM checks. The internal hypervisor logic
is represented a number of system domains which by designed are represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit changes the idle
domain to be created as a privileged domain under the default policy, which is
inherited by the SILO 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.

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>
---
 xen/arch/arm/setup.c    |  3 +++
 xen/arch/x86/setup.c    |  3 +++
 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   | 21 +++++++++++++++++++++
 7 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d5d0792ed4..e71fa3f860 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1048,6 +1048,9 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Hide UART from DOM0 if we're using it */
     serial_endboot();
 
+    if ( xsm_set_system_active() != 0)
+        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
+
     system_state = SYS_STATE_active;
 
     for_each_domain( d )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 6f20e17892..a3ce288ef9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -621,6 +621,9 @@ static void noreturn init_done(void)
     void *va;
     unsigned long start, end;
 
+    if ( xsm_set_system_active() != 0)
+        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
+
     system_state = SYS_STATE_active;
 
     domain_unpause_by_systemcontroller(dom0);
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 19ab678181..22a619e260 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3021,7 +3021,12 @@ void __init scheduler_init(void)
         sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
     }
 
-    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
+    /*
+     * idle dom is created privileged to ensure unrestricted access during
+     * setup and will be demoted by xsm_transition_running 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..3291fb5396 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("xsm_set_system_active: should only be called by idle domain\n");
+        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 0bf63ffa84..8a62de2fd6 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -186,6 +186,26 @@ 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;
+
+    if ( d->domain_id != DOMID_IDLE )
+    {
+        printk("xsm_set_system_active should only be called by idle domain\n");
+        return -EPERM;
+    }
+
+    /*
+     * While is_privileged has no significant meaning under flask, set to false
+     * as there are times in hypervisor code privilege checks check this
+     * directly instead of going through XSM.
+     */
+    d->is_privileged = false;
+
+    return 0;
+}
+
 static void cf_check flask_domain_free_security(struct domain *d)
 {
     struct domain_security_struct *dsec = d->ssid;
@@ -1766,6 +1786,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 v3 2/2] 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>
---
 tools/flask/policy/modules/xen.if      | 6 ++++++
 tools/flask/policy/modules/xen.te      | 1 +
 tools/flask/policy/policy/initial_sids | 1 +
 xen/xsm/flask/hooks.c                  | 8 +++++++-
 xen/xsm/flask/policy/initial_sids      | 1 +
 5 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 5e2aa472b6..4ec676fff1 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -62,6 +62,12 @@ 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_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 8a62de2fd6..4732ca4019 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -168,7 +168,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;
@@ -188,8 +188,12 @@ 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( dsec->sid == SECINITSID_XENBOOT);
+
     if ( d->domain_id != DOMID_IDLE )
     {
         printk("xsm_set_system_active should only be called by idle domain\n");
@@ -203,6 +207,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

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

[Jason Andryuk]

On Fri, Apr 22, 2022 at 12:35 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> There are now instances where internal hypervisor logic needs to make resource
> allocation calls that are protected by XSM checks. The internal hypervisor logic
> is represented a number of system domains which by designed are represented by
> non-privileged struct domain instances. To enable these logic blocks to
> function correctly but in a controlled manner, this commit changes the idle
> domain to be created as a privileged domain under the default policy, which is
> inherited by the SILO 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.
>
> 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>

Re: [PATCH v3 2/2] flask: implement xsm_set_system_active

[Jason Andryuk]

On Fri, Apr 22, 2022 at 12:35 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> 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>
> ---

> @@ -188,8 +188,12 @@ 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( dsec->sid == SECINITSID_XENBOOT);

Extra space before dsec.

With that fixed,
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

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

[Roger Pau Monné]

On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
> There are now instances where internal hypervisor logic needs to make resource
> allocation calls that are protected by XSM checks. The internal hypervisor logic
> is represented a number of system domains which by designed are represented by

'Some of the hypervisor code can be executed in a system domain that's
represented by a per-CPU non-privileged struct domain. To enable...'

> non-privileged struct domain instances. To enable these logic blocks to
> function correctly but in a controlled manner, this commit changes the idle
> domain to be created as a privileged domain under the default policy, which is
> inherited by the SILO 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.
> 
> 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>
> ---
>  xen/arch/arm/setup.c    |  3 +++
>  xen/arch/x86/setup.c    |  3 +++
>  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   | 21 +++++++++++++++++++++
>  7 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d5d0792ed4..e71fa3f860 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1048,6 +1048,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>      /* Hide UART from DOM0 if we're using it */
>      serial_endboot();
>  
> +    if ( xsm_set_system_active() != 0)
> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
> +
>      system_state = SYS_STATE_active;
>  
>      for_each_domain( d )
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 6f20e17892..a3ce288ef9 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
>      void *va;
>      unsigned long start, end;
>  
> +    if ( xsm_set_system_active() != 0)
           ^ extra space.

Since the function returns an error code you might as well add it to
the panic message, or else just make the function return bool instead.

Or just make the function void and panic in the handler itself (like
in previous versions), as I don't think it's sensible to continue
normal execution if xsm_set_system_active fails.

> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
> +
>      system_state = SYS_STATE_active;
>  
>      domain_unpause_by_systemcontroller(dom0);
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 19ab678181..22a619e260 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -3021,7 +3021,12 @@ void __init scheduler_init(void)
>          sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
>      }
>  
> -    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
> +    /*
> +     * idle dom is created privileged to ensure unrestricted access during
> +     * setup and will be demoted by xsm_transition_running when setup is

s/xsm_transition_running/xsm_set_system_active/

> +     * complete

Nit: missing full stop according to CODING_STYLE.

> +     */
> +    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..3291fb5396 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("xsm_set_system_active: should only be called by idle domain\n");
> +        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 0bf63ffa84..8a62de2fd6 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -186,6 +186,26 @@ 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;

Nit: you should also add the assert for d->is_privileged, I don't see
a reason for the xsm and flask functions to differ in that regard.

Thanks, Roger.

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

[Jan Beulich]

On 25.04.2022 11:44, Roger Pau Monné wrote:
> On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
>>      void *va;
>>      unsigned long start, end;
>>  
>> +    if ( xsm_set_system_active() != 0)
>            ^ extra space.

Hmm, did you mean

                                         ^ missing space
?

>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -3021,7 +3021,12 @@ void __init scheduler_init(void)
>>          sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
>>      }
>>  
>> -    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
>> +    /*
>> +     * idle dom is created privileged to ensure unrestricted access during
>> +     * setup and will be demoted by xsm_transition_running when setup is
> 
> s/xsm_transition_running/xsm_set_system_active/
> 
>> +     * complete
> 
> Nit: missing full stop according to CODING_STYLE.

Not really: A single-sentence comment may omit the full stop (while
personally I agree a stop would better be there). Instead starting
with a capital letter is mandated. 

Jan

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

[Roger Pau Monné]

On Mon, Apr 25, 2022 at 12:53:02PM +0200, Jan Beulich wrote:
> On 25.04.2022 11:44, Roger Pau Monné wrote:
> > On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
> >>      void *va;
> >>      unsigned long start, end;
> >>  
> >> +    if ( xsm_set_system_active() != 0)
> >            ^ extra space.
> 
> Hmm, did you mean
> 
>                                          ^ missing space
> ?

Indeed.  I was switching from a different code context and got
confused.

> >> --- a/xen/common/sched/core.c
> >> +++ b/xen/common/sched/core.c
> >> @@ -3021,7 +3021,12 @@ void __init scheduler_init(void)
> >>          sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
> >>      }
> >>  
> >> -    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
> >> +    /*
> >> +     * idle dom is created privileged to ensure unrestricted access during
> >> +     * setup and will be demoted by xsm_transition_running when setup is
> > 
> > s/xsm_transition_running/xsm_set_system_active/
> > 
> >> +     * complete
> > 
> > Nit: missing full stop according to CODING_STYLE.
> 
> Not really: A single-sentence comment may omit the full stop (while
> personally I agree a stop would better be there). Instead starting
> with a capital letter is mandated. 

Right, it's a multi line comment but single sentence, and hence the
full stop is not mandatory. Sorry for the noise.

Roger.

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

[Daniel P. Smith]

On 4/25/22 05:44, Roger Pau Monné wrote:
> On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
>> There are now instances where internal hypervisor logic needs to make resource
>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>> is represented a number of system domains which by designed are represented by
> 
> 'Some of the hypervisor code can be executed in a system domain that's
> represented by a per-CPU non-privileged struct domain. To enable...'

Ack, will reword.

>> non-privileged struct domain instances. To enable these logic blocks to
>> function correctly but in a controlled manner, this commit changes the idle
>> domain to be created as a privileged domain under the default policy, which is
>> inherited by the SILO 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.
>>
>> 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>
>> ---
>>  xen/arch/arm/setup.c    |  3 +++
>>  xen/arch/x86/setup.c    |  3 +++
>>  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   | 21 +++++++++++++++++++++
>>  7 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index d5d0792ed4..e71fa3f860 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -1048,6 +1048,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      /* Hide UART from DOM0 if we're using it */
>>      serial_endboot();
>>  
>> +    if ( xsm_set_system_active() != 0)
>> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
>> +
>>      system_state = SYS_STATE_active;
>>  
>>      for_each_domain( d )
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 6f20e17892..a3ce288ef9 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
>>      void *va;
>>      unsigned long start, end;
>>  
>> +    if ( xsm_set_system_active() != 0)
>            ^ extra space.
> 
> Since the function returns an error code you might as well add it to
> the panic message, or else just make the function return bool instead.
> 
> Or just make the function void and panic in the handler itself (like
> in previous versions), as I don't think it's sensible to continue
> normal execution if xsm_set_system_active fails.

After reflecting on it, I believe that was not the correct action. The
policy should handle setting/checking all access control state and fail
with an error of why and then allow the hypervisor logic decided what to
do with that failure. For the policies that are present today, yes it is
an immediate panic. Ultimately this will future proof the interface
should a future policy type be introduced with a more varied result that
could allow the hypervisor to continue to boot, for instance to a
limited and/or debug state.

>> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
>> +
>>      system_state = SYS_STATE_active;
>>  
>>      domain_unpause_by_systemcontroller(dom0);
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 19ab678181..22a619e260 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -3021,7 +3021,12 @@ void __init scheduler_init(void)
>>          sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
>>      }
>>  
>> -    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
>> +    /*
>> +     * idle dom is created privileged to ensure unrestricted access during
>> +     * setup and will be demoted by xsm_transition_running when setup is
> 
> s/xsm_transition_running/xsm_set_system_active/

I missed one, apologies.

>> +     * complete
> 
> Nit: missing full stop according to CODING_STYLE.

Ack.

>> +     */
>> +    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..3291fb5396 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("xsm_set_system_active: should only be called by idle domain\n");
>> +        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 0bf63ffa84..8a62de2fd6 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -186,6 +186,26 @@ 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;
> 
> Nit: you should also add the assert for d->is_privileged, I don't see
> a reason for the xsm and flask functions to differ in that regard.

This goes back to an issued I have raised before, is_privileged really
encompasses two properties of a domain. Whether the domain is filling
the special control domain role versus what accesses the domain has
based on the context under which is_control_domain() is called. For
instance the function init_domain_msr_policy() uses is_control_domain()
not to make an access control decision but configure behavior. Under
flask is_privileged no longer reflects the accesses a domain with it set
will have, thus whether it is cleared when flask is enabled is
irrelevant as far as flask is concerned. For the ASSERT, what matters is
that the label was set to xenboot_t on construction and that it was not
changed before reaching this point. Or in a short form, when under the
default policy the expected state is concerned with is_privilege while
for flask it is only the SID.

Re: [PATCH v3 2/2] flask: implement xsm_set_system_active

[Daniel P. Smith]

On 4/22/22 12:58, Jason Andryuk wrote:
> On Fri, Apr 22, 2022 at 12:35 PM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> 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>
>> ---
> 
>> @@ -188,8 +188,12 @@ 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( dsec->sid == SECINITSID_XENBOOT);
> 
> Extra space before dsec.

Ack.

> With that fixed,
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

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

[Roger Pau Monné]

On Mon, Apr 25, 2022 at 12:39:17PM -0400, Daniel P. Smith wrote:
> On 4/25/22 05:44, Roger Pau Monné wrote:
> > On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> >> index d5d0792ed4..e71fa3f860 100644
> >> --- a/xen/arch/arm/setup.c
> >> +++ b/xen/arch/arm/setup.c
> >> @@ -1048,6 +1048,9 @@ void __init start_xen(unsigned long boot_phys_offset,
> >>      /* Hide UART from DOM0 if we're using it */
> >>      serial_endboot();
> >>  
> >> +    if ( xsm_set_system_active() != 0)
> >> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
> >> +
> >>      system_state = SYS_STATE_active;
> >>  
> >>      for_each_domain( d )
> >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> >> index 6f20e17892..a3ce288ef9 100644
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
> >>      void *va;
> >>      unsigned long start, end;
> >>  
> >> +    if ( xsm_set_system_active() != 0)
> >            ^ extra space.
> > 
> > Since the function returns an error code you might as well add it to
> > the panic message, or else just make the function return bool instead.
> > 
> > Or just make the function void and panic in the handler itself (like
> > in previous versions), as I don't think it's sensible to continue
> > normal execution if xsm_set_system_active fails.
> 
> After reflecting on it, I believe that was not the correct action. The
> policy should handle setting/checking all access control state and fail
> with an error of why and then allow the hypervisor logic decided what to
> do with that failure. For the policies that are present today, yes it is
> an immediate panic. Ultimately this will future proof the interface
> should a future policy type be introduced with a more varied result that
> could allow the hypervisor to continue to boot, for instance to a
> limited and/or debug state.

That's all fine, but if you return an error code, please print it as
part of the panic message.  The more information we can add in case of
panic, the better.

> >> 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 0bf63ffa84..8a62de2fd6 100644
> >> --- a/xen/xsm/flask/hooks.c
> >> +++ b/xen/xsm/flask/hooks.c
> >> @@ -186,6 +186,26 @@ 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;
> > 
> > Nit: you should also add the assert for d->is_privileged, I don't see
> > a reason for the xsm and flask functions to differ in that regard.
> 
> This goes back to an issued I have raised before, is_privileged really
> encompasses two properties of a domain. Whether the domain is filling
> the special control domain role versus what accesses the domain has
> based on the context under which is_control_domain() is called. For
> instance the function init_domain_msr_policy() uses is_control_domain()
> not to make an access control decision but configure behavior. Under
> flask is_privileged no longer reflects the accesses a domain with it set
> will have, thus whether it is cleared when flask is enabled is
> irrelevant as far as flask is concerned. For the ASSERT, what matters is
> that the label was set to xenboot_t on construction and that it was not
> changed before reaching this point. Or in a short form, when under the
> default policy the expected state is concerned with is_privilege while
> for flask it is only the SID.

I certainly don't care that much, but you do set d->is_privileged =
false in flask_set_system_active, hence it would seem logic to expect
d->is_privileged == true also?

If not for anything else, just to assert that the function is not
called twice.

Thanks, Roger.

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

[Daniel P. Smith]

On 4/26/22 03:12, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 12:39:17PM -0400, Daniel P. Smith wrote:
>> On 4/25/22 05:44, Roger Pau Monné wrote:
>>> On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index d5d0792ed4..e71fa3f860 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -1048,6 +1048,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>>      /* Hide UART from DOM0 if we're using it */
>>>>      serial_endboot();
>>>>  
>>>> +    if ( xsm_set_system_active() != 0)
>>>> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE privilege\n");
>>>> +
>>>>      system_state = SYS_STATE_active;
>>>>  
>>>>      for_each_domain( d )
>>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>> index 6f20e17892..a3ce288ef9 100644
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
>>>>      void *va;
>>>>      unsigned long start, end;
>>>>  
>>>> +    if ( xsm_set_system_active() != 0)
>>>            ^ extra space.
>>>
>>> Since the function returns an error code you might as well add it to
>>> the panic message, or else just make the function return bool instead.
>>>
>>> Or just make the function void and panic in the handler itself (like
>>> in previous versions), as I don't think it's sensible to continue
>>> normal execution if xsm_set_system_active fails.
>>
>> After reflecting on it, I believe that was not the correct action. The
>> policy should handle setting/checking all access control state and fail
>> with an error of why and then allow the hypervisor logic decided what to
>> do with that failure. For the policies that are present today, yes it is
>> an immediate panic. Ultimately this will future proof the interface
>> should a future policy type be introduced with a more varied result that
>> could allow the hypervisor to continue to boot, for instance to a
>> limited and/or debug state.
> 
> That's all fine, but if you return an error code, please print it as
> part of the panic message.  The more information we can add in case of
> panic, the better.

Ack.

>>>> 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 0bf63ffa84..8a62de2fd6 100644
>>>> --- a/xen/xsm/flask/hooks.c
>>>> +++ b/xen/xsm/flask/hooks.c
>>>> @@ -186,6 +186,26 @@ 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;
>>>
>>> Nit: you should also add the assert for d->is_privileged, I don't see
>>> a reason for the xsm and flask functions to differ in that regard.
>>
>> This goes back to an issued I have raised before, is_privileged really
>> encompasses two properties of a domain. Whether the domain is filling
>> the special control domain role versus what accesses the domain has
>> based on the context under which is_control_domain() is called. For
>> instance the function init_domain_msr_policy() uses is_control_domain()
>> not to make an access control decision but configure behavior. Under
>> flask is_privileged no longer reflects the accesses a domain with it set
>> will have, thus whether it is cleared when flask is enabled is
>> irrelevant as far as flask is concerned. For the ASSERT, what matters is
>> that the label was set to xenboot_t on construction and that it was not
>> changed before reaching this point. Or in a short form, when under the
>> default policy the expected state is concerned with is_privilege while
>> for flask it is only the SID.
> 
> I certainly don't care that much, but you do set d->is_privileged =
> false in flask_set_system_active, hence it would seem logic to expect
> d->is_privileged == true also?

Yes, I did this just for consistency not because there is any
significance of is_privilege on the idle domain, in both contexts for
which is_privileged is used, when flask is the enforcing policy.

> If not for anything else, just to assert that the function is not
> called twice.

Under this patch flask_set_system_active() is effectively a no-op, so
calling it twice has no effect. In the next patch flask_set_system()
becomes a real check and there is an ASSERT on the SID as that is the
relevant context under flask and will ensure calling only once.

In the end I can add the ASSERT but it would be adding it for the sake
of adding it as it would not be protecting the hypervisor from moving
into an incorrect state.

v/r,
dps

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

[Roger Pau Monné]

On Thu, Apr 28, 2022 at 10:57:42AM -0400, Daniel P. Smith wrote:
> On 4/26/22 03:12, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 12:39:17PM -0400, Daniel P. Smith wrote:
> >> On 4/25/22 05:44, Roger Pau Monné wrote:
> >>> On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
> >>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> >>>> index 0bf63ffa84..8a62de2fd6 100644
> >>>> --- a/xen/xsm/flask/hooks.c
> >>>> +++ b/xen/xsm/flask/hooks.c
> >>>> @@ -186,6 +186,26 @@ 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;
> >>>
> >>> Nit: you should also add the assert for d->is_privileged, I don't see
> >>> a reason for the xsm and flask functions to differ in that regard.
> >>
> >> This goes back to an issued I have raised before, is_privileged really
> >> encompasses two properties of a domain. Whether the domain is filling
> >> the special control domain role versus what accesses the domain has
> >> based on the context under which is_control_domain() is called. For
> >> instance the function init_domain_msr_policy() uses is_control_domain()
> >> not to make an access control decision but configure behavior. Under
> >> flask is_privileged no longer reflects the accesses a domain with it set
> >> will have, thus whether it is cleared when flask is enabled is
> >> irrelevant as far as flask is concerned. For the ASSERT, what matters is
> >> that the label was set to xenboot_t on construction and that it was not
> >> changed before reaching this point. Or in a short form, when under the
> >> default policy the expected state is concerned with is_privilege while
> >> for flask it is only the SID.
> > 
> > I certainly don't care that much, but you do set d->is_privileged =
> > false in flask_set_system_active, hence it would seem logic to expect
> > d->is_privileged == true also?
> 
> Yes, I did this just for consistency not because there is any
> significance of is_privilege on the idle domain, in both contexts for
> which is_privileged is used, when flask is the enforcing policy.
> 
> > If not for anything else, just to assert that the function is not
> > called twice.
> 
> Under this patch flask_set_system_active() is effectively a no-op, so
> calling it twice has no effect. In the next patch flask_set_system()
> becomes a real check and there is an ASSERT on the SID as that is the
> relevant context under flask and will ensure calling only once.
> 
> In the end I can add the ASSERT but it would be adding it for the sake
> of adding it as it would not be protecting the hypervisor from moving
> into an incorrect state.

If flask_set_system_active() is really a no-op then just adding a
comment in that regard and not touching is_privileged would be OK to
me, as otherwise I think it's confusing.

In any case I would leave that to the flask maintainers to decide.

Thanks, Roger.