| Message ID | 20220503111731.12642-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Adds starting the idle domain privileged | expand |
> On 3 May 2022, at 12:17, Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > There are new capabilities, dom0less and hyperlaunch, that introduce internal > hypervisor logic which needs to make resource allocation calls that are > protected by XSM access checks. This creates an issue as a subset of the > hypervisor code is executed under a system domain, the idle domain, that is > represented by a per-CPU non-privileged struct domain. To enable these new > capabilities to function correctly but in a controlled manner, this commit > changes the idle system domain to be created as a privileged domain under the > default policy and demoted before transitioning to running. A new XSM hook, > xsm_set_system_active(), is introduced to allow each XSM policy type to demote > the idle domain appropriately for that policy type. In the case of SILO, it > inherits the default policy's hook for xsm_set_system_active(). > > 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_set_system_active(). > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Cheers, Luca
Hi Daniel, > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 0bf63ffa84..b93101191e 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -186,6 +186,28 @@ static int cf_check flask_domain_alloc_security(struct domain *d) > return 0; > } > > +static int cf_check flask_set_system_active(void) > +{ > + struct domain *d = current->domain; > + > + ASSERT(d->is_privileged); > + > + if ( d->domain_id != DOMID_IDLE ) > + { > + printk("xsm_set_system_active should only be called by idle domain\n"); Sorry I spotted that now, here in the printk probably you mean “flask_set_system_active” instead of “xsm_set_system_active”, you can keep my R-by after this change. Cheers, Luca
On 03/05/2022 14:17, Luca Fancellu wrote: >> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c >> index 0bf63ffa84..b93101191e 100644 >> --- a/xen/xsm/flask/hooks.c >> +++ b/xen/xsm/flask/hooks.c >> @@ -186,6 +186,28 @@ static int cf_check flask_domain_alloc_security(struct domain *d) >> return 0; >> } >> >> +static int cf_check flask_set_system_active(void) >> +{ >> + struct domain *d = current->domain; >> + >> + ASSERT(d->is_privileged); >> + >> + if ( d->domain_id != DOMID_IDLE ) >> + { >> + printk("xsm_set_system_active should only be called by idle domain\n"); > > Sorry I spotted that now, here in the printk probably you mean “flask_set_system_active” > instead of “xsm_set_system_active”, you can keep my R-by after this change. I tend to use "%s: ...", __func__ so the name always name the function. Cheers,
Hi Daniel, On 03/05/2022 12:17, Daniel P. Smith wrote: > There are new capabilities, dom0less and hyperlaunch, that introduce internal > hypervisor logic which needs to make resource allocation calls that are > protected by XSM access checks. This creates an issue as a subset of the > hypervisor code is executed under a system domain, the idle domain, that is > represented by a per-CPU non-privileged struct domain. To enable these new > capabilities to function correctly but in a controlled manner, this commit > changes the idle system domain to be created as a privileged domain under the > default policy and demoted before transitioning to running. A new XSM hook, > xsm_set_system_active(), is introduced to allow each XSM policy type to demote > the idle domain appropriately for that policy type. In the case of SILO, it > inherits the default policy's hook for xsm_set_system_active(). > > 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_set_system_active(). > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > --- > xen/arch/arm/setup.c | 4 ++++ > xen/arch/x86/setup.c | 5 +++++ > xen/common/sched/core.c | 7 ++++++- > xen/include/xsm/dummy.h | 17 +++++++++++++++++ > xen/include/xsm/xsm.h | 6 ++++++ > xen/xsm/dummy.c | 1 + > xen/xsm/flask/hooks.c | 23 +++++++++++++++++++++++ > 7 files changed, 62 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed4..39a654926d 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -1048,6 +1048,10 @@ void __init start_xen(unsigned long boot_phys_offset, > /* Hide UART from DOM0 if we're using it */ > serial_endboot(); > > + if ( (rc = xsm_set_system_active()) != 0 ) > + panic("xsm(err=%d): " > + "unable to set hypervisor to SYSTEM_ACTIVE privilege\n", rc); We usually don't split error message over multiple lines (even if they are over 80 characters). > + > 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..36a60ce884 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -620,6 +620,11 @@ static void noreturn init_done(void) > { > void *va; > unsigned long start, end; > + int err; > + > + if ( (err = xsm_set_system_active()) != 0 ) > + panic("xsm(err=%d): " > + "unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err); Same here. Other than the two remarks above and Luca's one: Acked-by: Julien Grall <jgrall@amazon.com> # arm Cheers,
On 5/3/22 09:17, Luca Fancellu wrote: > Hi Daniel, > >> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c >> index 0bf63ffa84..b93101191e 100644 >> --- a/xen/xsm/flask/hooks.c >> +++ b/xen/xsm/flask/hooks.c >> @@ -186,6 +186,28 @@ static int cf_check flask_domain_alloc_security(struct domain *d) >> return 0; >> } >> >> +static int cf_check flask_set_system_active(void) >> +{ >> + struct domain *d = current->domain; >> + >> + ASSERT(d->is_privileged); >> + >> + if ( d->domain_id != DOMID_IDLE ) >> + { >> + printk("xsm_set_system_active should only be called by idle domain\n"); > > Sorry I spotted that now, here in the printk probably you mean “flask_set_system_active” > instead of “xsm_set_system_active”, you can keep my R-by after this change. That was intentional as that was the hook it came in as, but after you pointed it out I realized this may cause confusion since the default policy function name is the same as the hook. Though changing it I would do as Julien suggested and use __func__. v/r, dps
On 5/9/22 14:33, Julien Grall wrote: > > > On 03/05/2022 14:17, Luca Fancellu wrote: >>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c >>> index 0bf63ffa84..b93101191e 100644 >>> --- a/xen/xsm/flask/hooks.c >>> +++ b/xen/xsm/flask/hooks.c >>> @@ -186,6 +186,28 @@ static int cf_check >>> flask_domain_alloc_security(struct domain *d) >>> return 0; >>> } >>> >>> +static int cf_check flask_set_system_active(void) >>> +{ >>> + struct domain *d = current->domain; >>> + >>> + ASSERT(d->is_privileged); >>> + >>> + if ( d->domain_id != DOMID_IDLE ) >>> + { >>> + printk("xsm_set_system_active should only be called by idle >>> domain\n"); >> >> Sorry I spotted that now, here in the printk probably you mean >> “flask_set_system_active” >> instead of “xsm_set_system_active”, you can keep my R-by after this >> change. > > I tend to use "%s: ...", __func__ so the name always name the function. Ack. v/r, dps
On 5/9/22 14:38, Julien Grall wrote: > Hi Daniel, > > On 03/05/2022 12:17, Daniel P. Smith wrote: >> There are new capabilities, dom0less and hyperlaunch, that introduce >> internal >> hypervisor logic which needs to make resource allocation calls that are >> protected by XSM access checks. This creates an issue as a subset of the >> hypervisor code is executed under a system domain, the idle domain, >> that is >> represented by a per-CPU non-privileged struct domain. To enable these >> new >> capabilities to function correctly but in a controlled manner, this >> commit >> changes the idle system domain to be created as a privileged domain >> under the >> default policy and demoted before transitioning to running. A new XSM >> hook, >> xsm_set_system_active(), is introduced to allow each XSM policy type >> to demote >> the idle domain appropriately for that policy type. In the case of >> SILO, it >> inherits the default policy's hook for xsm_set_system_active(). >> >> 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_set_system_active(). >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >> --- >> xen/arch/arm/setup.c | 4 ++++ >> xen/arch/x86/setup.c | 5 +++++ >> xen/common/sched/core.c | 7 ++++++- >> xen/include/xsm/dummy.h | 17 +++++++++++++++++ >> xen/include/xsm/xsm.h | 6 ++++++ >> xen/xsm/dummy.c | 1 + >> xen/xsm/flask/hooks.c | 23 +++++++++++++++++++++++ >> 7 files changed, 62 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index d5d0792ed4..39a654926d 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -1048,6 +1048,10 @@ void __init start_xen(unsigned long >> boot_phys_offset, >> /* Hide UART from DOM0 if we're using it */ >> serial_endboot(); >> + if ( (rc = xsm_set_system_active()) != 0 ) >> + panic("xsm(err=%d): " >> + "unable to set hypervisor to SYSTEM_ACTIVE >> privilege\n", rc); > > We usually don't split error message over multiple lines (even if they > are over 80 characters). > >> + >> 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..36a60ce884 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -620,6 +620,11 @@ static void noreturn init_done(void) >> { >> void *va; >> unsigned long start, end; >> + int err; >> + >> + if ( (err = xsm_set_system_active()) != 0 ) >> + panic("xsm(err=%d): " >> + "unable to set hypervisor to SYSTEM_ACTIVE >> privilege\n", err); > > Same here. > > Other than the two remarks above and Luca's one: > > Acked-by: Julien Grall <jgrall@amazon.com> # arm Ack. v/r, dps
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index d5d0792ed4..39a654926d 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -1048,6 +1048,10 @@ void __init start_xen(unsigned long boot_phys_offset, /* Hide UART from DOM0 if we're using it */ serial_endboot(); + if ( (rc = xsm_set_system_active()) != 0 ) + panic("xsm(err=%d): " + "unable to set hypervisor to SYSTEM_ACTIVE privilege\n", rc); + 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..36a60ce884 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -620,6 +620,11 @@ static void noreturn init_done(void) { void *va; unsigned long start, end; + int err; + + if ( (err = xsm_set_system_active()) != 0 ) + panic("xsm(err=%d): " + "unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err); system_state = SYS_STATE_active; diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index 19ab678181..7b1c03a0e1 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); + /* + * The idle dom is created privileged to ensure unrestricted access during + * setup and will be demoted by xsm_set_system_active() 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..3291fb5396 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -101,6 +101,23 @@ static always_inline int xsm_default_action( } } +static XSM_INLINE int cf_check xsm_set_system_active(void) +{ + struct domain *d = current->domain; + + ASSERT(d->is_privileged); + + if ( d->domain_id != DOMID_IDLE ) + { + printk("xsm_set_system_active: should only be called by idle domain\n"); + return -EPERM; + } + + d->is_privileged = false; + + return 0; +} + 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..8dad03fd3d 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 { + int (*set_system_active)(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 int xsm_set_system_active(void) +{ + return alternative_call(xsm_ops.set_system_active); +} + 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..e6ffa948f7 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 = { + .set_system_active = xsm_set_system_active, .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..b93101191e 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -186,6 +186,28 @@ static int cf_check flask_domain_alloc_security(struct domain *d) return 0; } +static int cf_check flask_set_system_active(void) +{ + struct domain *d = current->domain; + + ASSERT(d->is_privileged); + + if ( d->domain_id != DOMID_IDLE ) + { + printk("xsm_set_system_active should only be called by idle domain\n"); + return -EPERM; + } + + /* + * While is_privileged has no significant meaning under flask, set to false + * as is_privileged is not only used for a privilege check but also as a type + * of domain check, specifically if the domain is the control domain. + */ + d->is_privileged = false; + + return 0; +} + static void cf_check flask_domain_free_security(struct domain *d) { struct domain_security_struct *dsec = d->ssid; @@ -1766,6 +1788,7 @@ static int cf_check flask_argo_send( #endif static const struct xsm_ops __initconst_cf_clobber flask_ops = { + .set_system_active = flask_set_system_active, .security_domaininfo = flask_security_domaininfo, .domain_create = flask_domain_create, .getdomaininfo = flask_getdomaininfo,