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

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

On 4/21/22 05:53, Roger Pau Monné wrote:
> On Wed, Apr 20, 2022 at 06:28:33PM -0400, Daniel P. Smith 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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  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();
> 
> Could we put depriv or dipriviledge somewhere here? 'transition' seem to
> ambiguous IMO (but I'm not a native speaker).
> 
> xsm_{depriv,demote}_current();

Let me say this explanation is not to say no but to give context to the
concerns. Forms of deprive/demote were considered though when
considering the concept proposed was to change the security model where
the hypervisor/idle domain were now explicitly being give a new security
context, is_privileged and xenboot_t, under which setup is being run.
This new xsm hook is to provide a transition point for the XSM policies
to set what the running security context should be for the
hypervisor/idle domain. The name xsm_transition_running() clearly
denotes when/where this hook should be used, where as the name
xsm_depriv_current() is more generic and another developer may attempt
to use it in a manner it was not intended.

It is possible to consider creating an xsm_depriv_current() that
functions in a more generic manner but will likely be more complicated
to support general usage, especially for flask where a flask specific
"lower" security context must be provided.

If there is still a preference towards xsm_depriv_current() while
maintaining the current mechanics as it makes more sense for the
majority, I have no issue with that.

>> +
>> +    /* 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");
> 
> Could you also add a check that d->is_privileged == true?

Are you thinking along the lines of,

    if ( (!d->is_privileged) || (d->domain_id != DOMID_IDLE)
        panic("some message\n");

or is your concern more of,

    if ( !d->is_privileged )
        return;

In my mind the former is legitimate because execution should only arrive
here with current->domain being the idle domain and is_privileged set to
true. The latter check I feel is extraneous because 1) this hook should
only ever be called under the idle domain, thus it should be checked
first and should absolutely panic if another domain context is in place.
Which leads to, 2) checking if it is not false before setting to false
is only protecting against resetting to false for which there could be
no side effects this guard would be protecting against.

v/r,
dps