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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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();

> +
> +    /* 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?

Thanks, Roger.