[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |