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

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

On Thu, Apr 21, 2022 at 10:14:18AM -0400, Daniel P. Smith wrote:
> 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.

Hm, I see. I (wrongly) originally understood it was related to making
a transition in the running context, rather than the context being
changed to the running state.

Maybe xsm_{transition_,set_,}system_active() to better match the
system_state?

Albeit now that I understand it's purpose it doesn't feel so weird.

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

I was thinking about the former, maybe adding it as a separate
condition so you can print a specific panic message, or joined with
the other if the panic message can be adjusted to fit both conditions.

Thanks, Roger.