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

[PATCH v7 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 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 (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                   |  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                  | 32 +++++++++++++++++++++++++-
 xen/xsm/flask/policy/initial_sids      |  1 +
 11 files changed, 77 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH v7 1/2] 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. 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
---
 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 d5d0792ed4..7f3f00aa6a 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 ( (rc = xsm_set_system_active()) != 0 )
+        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\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 6f20e17892..57ee6cc407 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -620,6 +620,10 @@ static void noreturn init_done(void)
 {
     void *va;
     unsigned long start, end;
+    int err;
+
+    if ( (err = xsm_set_system_active()) != 0 )
+        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err);
 
     system_state = SYS_STATE_active;
 
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 19ab678181..7b1c03a0e1 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);
+    /*
+     * 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 0bf63ffa84..54745e6c6a 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -186,6 +186,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;
@@ -1766,6 +1788,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

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

[Rahul Singh]

Hi Daniel,

> On 11 May 2022, at 12:30 pm, Daniel P. Smith <dpsmith@apertussolutions.com> 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. 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>

Regards,
Rahul
> ---
> 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 d5d0792ed4..7f3f00aa6a 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 ( (rc = xsm_set_system_active()) != 0 )
> +        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\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 6f20e17892..57ee6cc407 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -620,6 +620,10 @@ static void noreturn init_done(void)
> {
>     void *va;
>     unsigned long start, end;
> +    int err;
> +
> +    if ( (err = xsm_set_system_active()) != 0 )
> +        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err);
> 
>     system_state = SYS_STATE_active;
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 19ab678181..7b1c03a0e1 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);
> +    /*
> +     * 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 0bf63ffa84..54745e6c6a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -186,6 +186,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;
> @@ -1766,6 +1788,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
> 
> 

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

[Roger Pau Monné]

On Wed, May 11, 2022 at 07:30:34AM -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. 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.

Should you mention that this subset of hypervisor code that requires
extended privileges but executed in the idle vCPU context strictly
only happens during initial domain(s) creation?

> 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
> ---
>  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 d5d0792ed4..7f3f00aa6a 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 ( (rc = xsm_set_system_active()) != 0 )
> +        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\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 6f20e17892..57ee6cc407 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -620,6 +620,10 @@ static void noreturn init_done(void)
>  {
>      void *va;
>      unsigned long start, end;
> +    int err;
> +
> +    if ( (err = xsm_set_system_active()) != 0 )
> +        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err);

Can you place err on a new line to make the line length no longer than
strictly necessary.

I think you could also reduce the printed message to:

"unable to switch to SYSTEM_ACTIVE privilege: %d\n"

Which could likely fit in a line (seeing as others are fine with the
longer message I'm not going to insist).

> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 0bf63ffa84..54745e6c6a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -186,6 +186,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

Nit: I think this line is over 80 cols.

Thanks, Roger.

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

[Daniel P. Smith]

On 5/31/22 03:56, Roger Pau Monné wrote:
> On Wed, May 11, 2022 at 07:30:34AM -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. 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.
> 
> Should you mention that this subset of hypervisor code that requires
> extended privileges but executed in the idle vCPU context strictly
> only happens during initial domain(s) creation?

Sure, I will work in some wording to clarify that point.

>> 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
>> ---
>>  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 d5d0792ed4..7f3f00aa6a 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 ( (rc = xsm_set_system_active()) != 0 )
>> +        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\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 6f20e17892..57ee6cc407 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -620,6 +620,10 @@ static void noreturn init_done(void)
>>  {
>>      void *va;
>>      unsigned long start, end;
>> +    int err;
>> +
>> +    if ( (err = xsm_set_system_active()) != 0 )
>> +        panic("xsm(err=%d): unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err);
> 
> Can you place err on a new line to make the line length no longer than
> strictly necessary.
> 
> I think you could also reduce the printed message to:
> 
> "unable to switch to SYSTEM_ACTIVE privilege: %d\n"
> 
> Which could likely fit in a line (seeing as others are fine with the
> longer message I'm not going to insist).

Nope, I am with you on this. I would prefer to have less than 80 or
wrap. I like the suggestion, it will get it below 80 without any loss of
meaning.

>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index 0bf63ffa84..54745e6c6a 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -186,6 +186,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
> 
> Nit: I think this line is over 80 cols.

Ugh, probably spell check pushed it over, and I didn't catch it. Will fix.


v/r,
dps

[PATCH v7 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>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.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                  | 9 ++++++++-
 xen/xsm/flask/policy/initial_sids      | 1 +
 5 files changed, 17 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 54745e6c6a..80b36cc2d8 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,9 +188,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 )
     {
@@ -205,6 +210,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 v7 2/2] flask: implement xsm_set_system_active

[Rahul Singh]

Hi Daniel,

> On 11 May 2022, at 12:30 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>
> 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>

Regards,
Rahul
> ---
> 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                  | 9 ++++++++-
> xen/xsm/flask/policy/initial_sids      | 1 +
> 5 files changed, 17 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 54745e6c6a..80b36cc2d8 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,9 +188,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 )
>     {
> @@ -205,6 +210,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
> 
> 

PING: [PATCH v7 0/2] Adds starting the idle domain privileged

[Daniel P. Smith]

On 5/11/22 07:30, 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.
> 
> 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
> 

Pinging to see if there are open issues I need to address or is the
series acceptable? IIU I am missing an ACK on patch 1 of the series from
x86 and sched maintainers. If it is already in your queues, apologies
for the nag.

V/r,
Daniel P. Smith

Re: PING: [PATCH v7 0/2] Adds starting the idle domain privileged

[Jan Beulich]

On 30.05.2022 19:15, Daniel P. Smith wrote:
> 
> On 5/11/22 07:30, 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.
>>
>> 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
>>
> 
> Pinging to see if there are open issues I need to address or is the
> series acceptable? IIU I am missing an ACK on patch 1 of the series from
> x86 and sched maintainers.

And a REST maintainer ack (to stand in for the designated maintainer continuing
to play dead).

Also I notice your ping did include neither x86 nor sched maintainers as (at
least) Cc.

Jan