Re: [PATCH 2/2] flask: implement xsm_transtion_running

Re: [PATCH 2/2] flask: implement xsm_transtion_running

[Jason Andryuk]

On Wed, Apr 20, 2022 at 1:03 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_transition_running 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,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
>
>  static void cf_check flask_domain_runtime_security(void)
>  {
> +    struct domain_security_struct *dsec;
>      struct domain *d = current->domain;
>
>      if ( d->domain_id != DOMID_IDLE )
> @@ -198,6 +199,9 @@ static void cf_check flask_domain_runtime_security(void)
>       * set to false for the consistency check(s) in the setup code.
>       */
>      d->is_privileged = false;
> +
> +    dsec = d->ssid;
> +    dsec->sid = SECINITSID_XEN;

I think you also want
   dsec->self_sid = dsec->sid;
so self also changes to xen_t.

Otherwise I think it looks good,

I was wondering if you were going to require xenboot_t -> xen_t
permissions, but manually setting the sid fields side-steps that.
That seems nicer than requiring policy rules for the transition.

Hmmm, cross referencing other flask code, often after assigning
self_sid there is this call to potentially re-calculate it:
    security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
&dsec->self_sid);

But it isn't used for system domains, so omitting it seems fine.

Regards,
Jason

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

[Jason Andryuk]

On Wed, Apr 20, 2022 at 1:02 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> There are now instances where internal hypervisor logic needs to make resource
> allocation calls that are protectd 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_transition_running, 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_transtion_running.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 6f20e17892..72695dcb07 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -621,6 +621,12 @@ static void noreturn init_done(void)
>      void *va;
>      unsigned long start, end;
>
> +    xsm_transition_running();
> +
> +    /* Ensure idle domain was not left privileged */
> +    if ( current->domain->is_privileged )
> +        panic("idle domain did not properly transition from setup privilege\n");

Checking immediately after the XSM hook seems redundant, though I
guess having a sanity check isn't harmful.

>  static void cf_check flask_domain_free_security(struct domain *d)
>  {
>      struct domain_security_struct *dsec = d->ssid;
> @@ -1766,6 +1780,7 @@ static int cf_check flask_argo_send(
>  #endif
>
>  static const struct xsm_ops __initconst_cf_clobber flask_ops = {
> +    .transition_running = flask_domain_runtime_security,

I'd prefer flask_transition_running.  That way grep for the hook name
also finds the flask implementation.

Regards,
Jason

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

[Daniel P. Smith]

On 4/20/22 14:31, Jason Andryuk wrote:
> On Wed, Apr 20, 2022 at 1:02 PM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> There are now instances where internal hypervisor logic needs to make resource
>> allocation calls that are protectd 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_transition_running, 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_transtion_running.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
> 
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 6f20e17892..72695dcb07 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -621,6 +621,12 @@ static void noreturn init_done(void)
>>      void *va;
>>      unsigned long start, end;
>>
>> +    xsm_transition_running();
>> +
>> +    /* Ensure idle domain was not left privileged */
>> +    if ( current->domain->is_privileged )
>> +        panic("idle domain did not properly transition from setup privilege\n");
> 
> Checking immediately after the XSM hook seems redundant, though I
> guess having a sanity check isn't harmful.

I was back and forth on this, so I threw it in and figured if there was
strong opinions against it I could easily remove and respin the series.

>>  static void cf_check flask_domain_free_security(struct domain *d)
>>  {
>>      struct domain_security_struct *dsec = d->ssid;
>> @@ -1766,6 +1780,7 @@ static int cf_check flask_argo_send(
>>  #endif
>>
>>  static const struct xsm_ops __initconst_cf_clobber flask_ops = {
>> +    .transition_running = flask_domain_runtime_security,
> 
> I'd prefer flask_transition_running.  That way grep for the hook name
> also finds the flask implementation.

Sure.

v/r,
dps

Re: [PATCH 2/2] flask: implement xsm_transtion_running

[Daniel P. Smith]

On 4/20/22 14:07, Jason Andryuk wrote:
> On Wed, Apr 20, 2022 at 1:03 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_transition_running 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,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
>>
>>  static void cf_check flask_domain_runtime_security(void)
>>  {
>> +    struct domain_security_struct *dsec;
>>      struct domain *d = current->domain;
>>
>>      if ( d->domain_id != DOMID_IDLE )
>> @@ -198,6 +199,9 @@ static void cf_check flask_domain_runtime_security(void)
>>       * set to false for the consistency check(s) in the setup code.
>>       */
>>      d->is_privileged = false;
>> +
>> +    dsec = d->ssid;
>> +    dsec->sid = SECINITSID_XEN;
> 
> I think you also want
>    dsec->self_sid = dsec->sid;
> so self also changes to xen_t.

Erg, thanks for the catch.

> Otherwise I think it looks good,
> 
> I was wondering if you were going to require xenboot_t -> xen_t
> permissions, but manually setting the sid fields side-steps that.
> That seems nicer than requiring policy rules for the transition.

I was considering it but as I was reflecting on the discussions that
were had, this is a one-time, one-way transition. Combine that with the
fact that xenboot_t has to be an initial sid (fixed/permnant type) for
Xen's flask policy, there is no need to require a transition rule in the
policy that can never be changed. And yes, it helps makes things much
simpler.( ^_^)

> Hmmm, cross referencing other flask code, often after assigning
> self_sid there is this call to potentially re-calculate it:
>     security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
> &dsec->self_sid);
> 
> But it isn't used for system domains, so omitting it seems fine.

Hmm, now you have me concerned about decisions residing in the avc from
accesses made during domain creation. Let me double check that, but I
think it will be needed. I believe the reason it was not needed for the
system domains because prior to this no access decisions were made
before the domains were labeled.

v/r,
dps

[PATCH 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.

Daniel P. Smith (2):
  xsm: create idle domain privieged and demote after setup
  flask: implement xsm_transtion_running

 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                   |  6 ++++++
 xen/arch/x86/setup.c                   |  6 ++++++
 xen/common/sched/core.c                |  7 ++++++-
 xen/include/xsm/dummy.h                | 12 ++++++++++++
 xen/include/xsm/xsm.h                  |  6 ++++++
 xen/xsm/dummy.c                        |  1 +
 xen/xsm/flask/hooks.c                  | 21 ++++++++++++++++++++-
 xen/xsm/flask/policy/initial_sids      |  1 +
 11 files changed, 66 insertions(+), 2 deletions(-)

-- 
2.20.1

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

[Daniel P. Smith]

There are now instances where internal hypervisor logic needs to make resource
allocation calls that are protectd 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_transition_running, 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_transtion_running.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/arm/setup.c    |  6 ++++++
 xen/arch/x86/setup.c    |  6 ++++++
 xen/common/sched/core.c |  7 ++++++-
 xen/include/xsm/dummy.h | 12 ++++++++++++
 xen/include/xsm/xsm.h   |  6 ++++++
 xen/xsm/dummy.c         |  1 +
 xen/xsm/flask/hooks.c   | 15 +++++++++++++++
 7 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d5d0792ed4..763835aeb5 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1048,6 +1048,12 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Hide UART from DOM0 if we're using it */
     serial_endboot();
 
+    xsm_transition_running();
+
+    /* Ensure idle domain was not left privileged */
+    if ( current->domain->is_privileged )
+        panic("idle domain did not properly transition from setup 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..72695dcb07 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -621,6 +621,12 @@ static void noreturn init_done(void)
     void *va;
     unsigned long start, end;
 
+    xsm_transition_running();
+
+    /* Ensure idle domain was not left privileged */
+    if ( current->domain->is_privileged )
+        panic("idle domain did not properly transition from setup 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..b33f0ec672 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -101,6 +101,18 @@ static always_inline int xsm_default_action(
     }
 }
 
+static XSM_INLINE void cf_check xsm_transition_running(void)
+{
+    struct domain *d = current->domain;
+
+    if ( d->domain_id != DOMID_IDLE )
+        panic("xsm_transition_running should only be called by idle domain\n");
+
+    d->is_privileged = false;
+
+    return;
+}
+
 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..a5c06804ab 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 {
+    void (*transition_running)(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 void xsm_transition_running(void)
+{
+    alternative_vcall(xsm_ops.transition_running);
+}
+
 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..66f26c6909 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 = {
+    .transition_running            = xsm_transition_running,
     .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..51c896d9f7 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -186,6 +186,20 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
     return 0;
 }
 
+static void cf_check flask_domain_runtime_security(void)
+{
+    struct domain *d = current->domain;
+
+    if ( d->domain_id != DOMID_IDLE )
+        panic("xsm_transition_running should only be called by idle domain\n");
+
+    /*
+     * While is_privileged has no significant meaning under flask,
+     * set to false for the consistency check(s) in the setup code.
+     */
+    d->is_privileged = false;
+}
+
 static void cf_check flask_domain_free_security(struct domain *d)
 {
     struct domain_security_struct *dsec = d->ssid;
@@ -1766,6 +1780,7 @@ static int cf_check flask_argo_send(
 #endif
 
 static const struct xsm_ops __initconst_cf_clobber flask_ops = {
+    .transition_running = flask_domain_runtime_security,
     .security_domaininfo = flask_security_domaininfo,
     .domain_create = flask_domain_create,
     .getdomaininfo = flask_getdomaininfo,
-- 
2.20.1

[PATCH 2/2] flask: implement xsm_transtion_running

[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_transition_running 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                  | 6 +++++-
 xen/xsm/flask/policy/initial_sids      | 1 +
 5 files changed, 14 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 51c896d9f7..7e840af6fc 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,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
 
 static void cf_check flask_domain_runtime_security(void)
 {
+    struct domain_security_struct *dsec;
     struct domain *d = current->domain;
 
     if ( d->domain_id != DOMID_IDLE )
@@ -198,6 +199,9 @@ static void cf_check flask_domain_runtime_security(void)
      * set to false for the consistency check(s) in the setup code.
      */
     d->is_privileged = false;
+
+    dsec = d->ssid;
+    dsec->sid = SECINITSID_XEN;
 }
 
 static void cf_check flask_domain_free_security(struct domain *d)
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