* [PATCH v2 0/2] Adds starting the idle domain privileged @ 2022-04-20 22:28 Daniel P. Smith 2022-04-20 22:28 ` [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup Daniel P. Smith 2022-04-20 22:28 ` [PATCH v2 2/2] flask: implement xsm_transtion_running Daniel P. Smith 0 siblings, 2 replies; 10+ messages in thread From: Daniel P. Smith @ 2022-04-20 22:28 UTC (permalink / raw) To: xen-devel; +Cc: Daniel P. Smith, scott.davis, jandryuk This series makes it so that the idle domain is started privileged under the default policy, which the SILO policy inherits, and under the flask policy. It then introduces a new one-way XSM hook, xsm_transition_running, that is hooked by an XSM policy to transition the idle domain to its running privilege level. Changes in v2: - renamed flask_domain_runtime_security() to flask_transition_running() - added the missed assignment of self_sid Daniel P. Smith (2): xsm: create idle domain privieged and demote after setup flask: implement xsm_transtion_running tools/flask/policy/modules/xen.if | 6 ++++++ tools/flask/policy/modules/xen.te | 1 + tools/flask/policy/policy/initial_sids | 1 + 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 | 22 +++++++++++++++++++++- xen/xsm/flask/policy/initial_sids | 1 + 11 files changed, 67 insertions(+), 2 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup 2022-04-20 22:28 [PATCH v2 0/2] Adds starting the idle domain privileged Daniel P. Smith @ 2022-04-20 22:28 ` Daniel P. Smith 2022-04-21 9:20 ` Jan Beulich 2022-04-21 9:53 ` Roger Pau Monné 2022-04-20 22:28 ` [PATCH v2 2/2] flask: implement xsm_transtion_running Daniel P. Smith 1 sibling, 2 replies; 10+ messages in thread From: Daniel P. Smith @ 2022-04-20 22:28 UTC (permalink / raw) To: xen-devel, Volodymyr Babchuk, Wei Liu, Daniel P. Smith Cc: scott.davis, jandryuk, Stefano Stabellini, Julien Grall, Bertrand Marquis, Jan Beulich, Andrew Cooper, Roger Pau Monné, George Dunlap, Dario Faggioli, Daniel De Graaf 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@apertussolutions.com> --- 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(); + + /* 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"); + + d->is_privileged = false; + + return; +} + 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..a5c06804ab 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 { + void (*transition_running)(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 void xsm_transition_running(void) +{ + alternative_vcall(xsm_ops.transition_running); +} + 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..66f26c6909 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 = { + .transition_running = xsm_transition_running, .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..decebc8231 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -186,6 +186,20 @@ static int cf_check flask_domain_alloc_security(struct domain *d) return 0; } +static void cf_check flask_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"); + + /* + * While is_privileged has no significant meaning under flask, + * set to false for the consistency check(s) in the setup code. + */ + d->is_privileged = false; +} + static void cf_check flask_domain_free_security(struct domain *d) { struct domain_security_struct *dsec = d->ssid; @@ -1766,6 +1780,7 @@ static int cf_check flask_argo_send( #endif static const struct xsm_ops __initconst_cf_clobber flask_ops = { + .transition_running = flask_transition_running, .security_domaininfo = flask_security_domaininfo, .domain_create = flask_domain_create, .getdomaininfo = flask_getdomaininfo, -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup 2022-04-20 22:28 ` [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup Daniel P. Smith @ 2022-04-21 9:20 ` Jan Beulich 2022-04-21 13:05 ` Daniel P. Smith 2022-04-21 9:53 ` Roger Pau Monné 1 sibling, 1 reply; 10+ messages in thread From: Jan Beulich @ 2022-04-21 9:20 UTC (permalink / raw) To: Daniel P. Smith Cc: scott.davis, jandryuk, Stefano Stabellini, Julien Grall, Bertrand Marquis, Andrew Cooper, Roger Pau Monné, George Dunlap, Dario Faggioli, Daniel De Graaf, xen-devel, Volodymyr Babchuk, Wei Liu On 21.04.2022 00:28, 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@apertussolutions.com> Looks okay to me, but I'm not sure in how far agreement was reached on taking this route. Just one nit: > --- 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"); > + > + d->is_privileged = false; > + > + return; > +} Please omit such return statements. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup 2022-04-21 9:20 ` Jan Beulich @ 2022-04-21 13:05 ` Daniel P. Smith 0 siblings, 0 replies; 10+ messages in thread From: Daniel P. Smith @ 2022-04-21 13:05 UTC (permalink / raw) To: Jan Beulich Cc: scott.davis, jandryuk, Stefano Stabellini, Julien Grall, Bertrand Marquis, Andrew Cooper, Roger Pau Monné, George Dunlap, Dario Faggioli, Daniel De Graaf, xen-devel, Volodymyr Babchuk, Wei Liu On 4/21/22 05:20, Jan Beulich wrote: > On 21.04.2022 00:28, 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@apertussolutions.com> > > Looks okay to me, but I'm not sure in how far agreement was reached on > taking this route. Just one nit: Thank you. As for the approach, Jason suggested it and Roger stated that if this approach was adopted it would resolve his concerns over __init. If you have a concern with this approach, please let me know so I can ensure it is addressed to the best of my ability. >> --- 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"); >> + >> + d->is_privileged = false; >> + >> + return; >> +} > > Please omit such return statements. Ack. v/r, dps ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup 2022-04-20 22:28 ` [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup Daniel P. Smith 2022-04-21 9:20 ` Jan Beulich @ 2022-04-21 9:53 ` Roger Pau Monné 2022-04-21 14:14 ` Daniel P. Smith 1 sibling, 1 reply; 10+ messages in thread From: Roger Pau Monné @ 2022-04-21 9:53 UTC (permalink / raw) To: Daniel P. Smith Cc: xen-devel, Volodymyr Babchuk, Wei Liu, scott.davis, jandryuk, Stefano Stabellini, Julien Grall, Bertrand Marquis, Jan Beulich, Andrew Cooper, George Dunlap, Dario Faggioli, Daniel De Graaf 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@apertussolutions.com> > --- > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup 2022-04-21 9:53 ` Roger Pau Monné @ 2022-04-21 14:14 ` Daniel P. Smith 2022-04-21 14:56 ` Roger Pau Monné 0 siblings, 1 reply; 10+ messages in thread From: Daniel P. Smith @ 2022-04-21 14:14 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Volodymyr Babchuk, Wei Liu, scott.davis, jandryuk, Stefano Stabellini, Julien Grall, Bertrand Marquis, Jan Beulich, Andrew Cooper, George Dunlap, Dario Faggioli, Daniel De Graaf 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@apertussolutions.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup 2022-04-21 14:14 ` Daniel P. Smith @ 2022-04-21 14:56 ` Roger Pau Monné 0 siblings, 0 replies; 10+ messages in thread From: Roger Pau Monné @ 2022-04-21 14:56 UTC (permalink / raw) To: Daniel P. Smith Cc: xen-devel, Volodymyr Babchuk, Wei Liu, scott.davis, jandryuk, Stefano Stabellini, Julien Grall, Bertrand Marquis, Jan Beulich, Andrew Cooper, George Dunlap, Dario Faggioli, Daniel De Graaf 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@apertussolutions.com> > >> --- > >> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] flask: implement xsm_transtion_running 2022-04-20 22:28 [PATCH v2 0/2] Adds starting the idle domain privileged Daniel P. Smith 2022-04-20 22:28 ` [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup Daniel P. Smith @ 2022-04-20 22:28 ` Daniel P. Smith 2022-04-21 9:22 ` Jan Beulich 1 sibling, 1 reply; 10+ messages in thread From: Daniel P. Smith @ 2022-04-20 22:28 UTC (permalink / raw) To: xen-devel, Daniel P. Smith Cc: scott.davis, jandryuk, Daniel De Graaf, Wei Liu, Anthony PERARD This commit implements full support for starting the idle domain privileged by introducing a new flask label xenboot_t which the idle domain is labeled with at creation. It then provides the implementation for the XSM hook xsm_transition_running to relabel the idle domain to the existing xen_t flask label. In the reference flask policy a new macro, xen_build_domain(target), is introduced for creating policies for dom0less/hyperlaunch allowing the hypervisor to create and assign the necessary resources for domain construction. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- tools/flask/policy/modules/xen.if | 6 ++++++ tools/flask/policy/modules/xen.te | 1 + tools/flask/policy/policy/initial_sids | 1 + xen/xsm/flask/hooks.c | 7 ++++++- xen/xsm/flask/policy/initial_sids | 1 + 5 files changed, 15 insertions(+), 1 deletion(-) diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if index 5e2aa472b6..4ec676fff1 100644 --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -62,6 +62,12 @@ define(`create_domain_common', ` setparam altp2mhvm altp2mhvm_op dm }; ') +# xen_build_domain(target) +# Allow a domain to be created at boot by the hypervisor +define(`xen_build_domain', ` + allow xenboot_t $1_channel:event create; +') + # create_domain(priv, target) # Allow a domain to be created directly define(`create_domain', ` diff --git a/tools/flask/policy/modules/xen.te b/tools/flask/policy/modules/xen.te index 3dbf93d2b8..de98206fdd 100644 --- a/tools/flask/policy/modules/xen.te +++ b/tools/flask/policy/modules/xen.te @@ -24,6 +24,7 @@ attribute mls_priv; ################################################################################ # The hypervisor itself +type xenboot_t, xen_type, mls_priv; type xen_t, xen_type, mls_priv; # Domain 0 diff --git a/tools/flask/policy/policy/initial_sids b/tools/flask/policy/policy/initial_sids index 6b7b7eff21..ec729d3ba3 100644 --- a/tools/flask/policy/policy/initial_sids +++ b/tools/flask/policy/policy/initial_sids @@ -2,6 +2,7 @@ # objects created before the policy is loaded or for objects that do not have a # label defined in some other manner. +sid xenboot gen_context(system_u:system_r:xenboot_t,s0) sid xen gen_context(system_u:system_r:xen_t,s0) sid dom0 gen_context(system_u:system_r:dom0_t,s0) sid domxen gen_context(system_u:system_r:domxen_t,s0) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index decebc8231..0643654aba 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -168,7 +168,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d) switch ( d->domain_id ) { case DOMID_IDLE: - dsec->sid = SECINITSID_XEN; + dsec->sid = SECINITSID_XENBOOT; break; case DOMID_XEN: dsec->sid = SECINITSID_DOMXEN; @@ -188,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d) static void cf_check flask_transition_running(void) { + struct domain_security_struct *dsec; struct domain *d = current->domain; if ( d->domain_id != DOMID_IDLE ) @@ -198,6 +199,10 @@ static void cf_check flask_transition_running(void) * set to false for the consistency check(s) in the setup code. */ d->is_privileged = false; + + dsec = d->ssid; + dsec->sid = SECINITSID_XEN; + dsec->self_sid = dsec->sid; } static void cf_check flask_domain_free_security(struct domain *d) diff --git a/xen/xsm/flask/policy/initial_sids b/xen/xsm/flask/policy/initial_sids index 7eca70d339..e8b55b8368 100644 --- a/xen/xsm/flask/policy/initial_sids +++ b/xen/xsm/flask/policy/initial_sids @@ -3,6 +3,7 @@ # # Define initial security identifiers # +sid xenboot sid xen sid dom0 sid domio -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] flask: implement xsm_transtion_running 2022-04-20 22:28 ` [PATCH v2 2/2] flask: implement xsm_transtion_running Daniel P. Smith @ 2022-04-21 9:22 ` Jan Beulich 2022-04-21 14:39 ` Daniel P. Smith 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2022-04-21 9:22 UTC (permalink / raw) To: Daniel P. Smith Cc: scott.davis, jandryuk, Daniel De Graaf, Wei Liu, Anthony PERARD, xen-devel On 21.04.2022 00:28, Daniel P. Smith wrote: > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -168,7 +168,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d) > switch ( d->domain_id ) > { > case DOMID_IDLE: > - dsec->sid = SECINITSID_XEN; > + dsec->sid = SECINITSID_XENBOOT; > break; > case DOMID_XEN: > dsec->sid = SECINITSID_DOMXEN; > @@ -188,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d) > > static void cf_check flask_transition_running(void) > { > + struct domain_security_struct *dsec; > struct domain *d = current->domain; > > if ( d->domain_id != DOMID_IDLE ) > @@ -198,6 +199,10 @@ static void cf_check flask_transition_running(void) > * set to false for the consistency check(s) in the setup code. > */ > d->is_privileged = false; > + > + dsec = d->ssid; > + dsec->sid = SECINITSID_XEN; > + dsec->self_sid = dsec->sid; > } If replacing SIDs is an okay thing to do, perhaps assert that the values haven't changed from SECINITSID_XENBOOT prior to replacing them? Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] flask: implement xsm_transtion_running 2022-04-21 9:22 ` Jan Beulich @ 2022-04-21 14:39 ` Daniel P. Smith 0 siblings, 0 replies; 10+ messages in thread From: Daniel P. Smith @ 2022-04-21 14:39 UTC (permalink / raw) To: Jan Beulich Cc: scott.davis, jandryuk, Daniel De Graaf, Wei Liu, Anthony PERARD, xen-devel On 4/21/22 05:22, Jan Beulich wrote: > On 21.04.2022 00:28, Daniel P. Smith wrote: >> --- a/xen/xsm/flask/hooks.c >> +++ b/xen/xsm/flask/hooks.c >> @@ -168,7 +168,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d) >> switch ( d->domain_id ) >> { >> case DOMID_IDLE: >> - dsec->sid = SECINITSID_XEN; >> + dsec->sid = SECINITSID_XENBOOT; >> break; >> case DOMID_XEN: >> dsec->sid = SECINITSID_DOMXEN; >> @@ -188,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d) >> >> static void cf_check flask_transition_running(void) >> { >> + struct domain_security_struct *dsec; >> struct domain *d = current->domain; >> >> if ( d->domain_id != DOMID_IDLE ) >> @@ -198,6 +199,10 @@ static void cf_check flask_transition_running(void) >> * set to false for the consistency check(s) in the setup code. >> */ >> d->is_privileged = false; >> + >> + dsec = d->ssid; >> + dsec->sid = SECINITSID_XEN; >> + dsec->self_sid = dsec->sid; >> } > > If replacing SIDs is an okay thing to do, perhaps assert that the > values haven't changed from SECINITSID_XENBOOT prior to replacing > them? Yes, changing a domain's SID is a legitimate action that could be done today via the FLASK_RELABEL_DOMAIN subop of xsm_op hypercall that ends up calling flask_relabel_domain(), when using flask policy. This is where Jason was concerned if I was going to be using that call to change the SID, which would require a policy rule to allow xenboot_t to relabel itself as xen_t. As flask works today, the system domains use initial SIDs which are effectively compile-time labels, which means the policy rule is a static, fixed rule, i.e. it is not possible to use a different set of labels, that must always be present. This also introduces the risk for a custom policy writer to inadvertently leave the xenboot_t to xen_t transitional rule out resulting in a failed access attempt which would lead to a panic. This is unnecessary pain when we can just handle the transition internal to the hypervisor as that where it is all really occurring. As for the ASSERT, that is a good point since there is a specific state we are expecting to enter the hook. Pair that with some thinking I have had to do in answering Jason, Roger, and yourself, I am going to rewire the hook to return a success/error return value and move the panic outside the check. v/r, dps ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-04-21 14:57 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-20 22:28 [PATCH v2 0/2] Adds starting the idle domain privileged Daniel P. Smith 2022-04-20 22:28 ` [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup Daniel P. Smith 2022-04-21 9:20 ` Jan Beulich 2022-04-21 13:05 ` Daniel P. Smith 2022-04-21 9:53 ` Roger Pau Monné 2022-04-21 14:14 ` Daniel P. Smith 2022-04-21 14:56 ` Roger Pau Monné 2022-04-20 22:28 ` [PATCH v2 2/2] flask: implement xsm_transtion_running Daniel P. Smith 2022-04-21 9:22 ` Jan Beulich 2022-04-21 14:39 ` Daniel P. Smith
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.