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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  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.