| Message ID | 20220511113035.27070-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Adds starting the idle domain privileged | expand |
Hi Daniel, > On 11 May 2022, at 12:30 pm, 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> > Acked-by: Julien Grall <jgrall@amazon.com> # arm Reviewed-by: Rahul Singh <rahul.singh@arm.com> Tested-by: Rahul Singh <rahul.singh@arm.com> Regards, Rahul > --- > xen/arch/arm/setup.c | 3 +++ > xen/arch/x86/setup.c | 4 ++++ > 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, 60 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed4..7f3f00aa6a 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -1048,6 +1048,9 @@ 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..57ee6cc407 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -620,6 +620,10 @@ 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..77f27e7163 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("%s: should only be called by idle domain\n", __func__); > + 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..54745e6c6a 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("%s: should only be called by idle domain\n", __func__); > + 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, > -- > 2.20.1 > >
On Wed, May 11, 2022 at 07:30:34AM -0400, 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. Should you mention that this subset of hypervisor code that requires extended privileges but executed in the idle vCPU context strictly only happens during initial domain(s) creation? > 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> > Acked-by: Julien Grall <jgrall@amazon.com> # arm > --- > xen/arch/arm/setup.c | 3 +++ > xen/arch/x86/setup.c | 4 ++++ > 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, 60 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed4..7f3f00aa6a 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -1048,6 +1048,9 @@ 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..57ee6cc407 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -620,6 +620,10 @@ 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); Can you place err on a new line to make the line length no longer than strictly necessary. I think you could also reduce the printed message to: "unable to switch to SYSTEM_ACTIVE privilege: %d\n" Which could likely fit in a line (seeing as others are fine with the longer message I'm not going to insist). > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 0bf63ffa84..54745e6c6a 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("%s: should only be called by idle domain\n", __func__); > + 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 Nit: I think this line is over 80 cols. Thanks, Roger.
On 5/31/22 03:56, Roger Pau Monné wrote: > On Wed, May 11, 2022 at 07:30:34AM -0400, 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. > > Should you mention that this subset of hypervisor code that requires > extended privileges but executed in the idle vCPU context strictly > only happens during initial domain(s) creation? Sure, I will work in some wording to clarify that point. >> 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> >> Acked-by: Julien Grall <jgrall@amazon.com> # arm >> --- >> xen/arch/arm/setup.c | 3 +++ >> xen/arch/x86/setup.c | 4 ++++ >> 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, 60 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index d5d0792ed4..7f3f00aa6a 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -1048,6 +1048,9 @@ 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..57ee6cc407 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -620,6 +620,10 @@ 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); > > Can you place err on a new line to make the line length no longer than > strictly necessary. > > I think you could also reduce the printed message to: > > "unable to switch to SYSTEM_ACTIVE privilege: %d\n" > > Which could likely fit in a line (seeing as others are fine with the > longer message I'm not going to insist). Nope, I am with you on this. I would prefer to have less than 80 or wrap. I like the suggestion, it will get it below 80 without any loss of meaning. >> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c >> index 0bf63ffa84..54745e6c6a 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("%s: should only be called by idle domain\n", __func__); >> + 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 > > Nit: I think this line is over 80 cols. Ugh, probably spell check pushed it over, and I didn't catch it. Will fix. v/r, dps
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index d5d0792ed4..7f3f00aa6a 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -1048,6 +1048,9 @@ 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..57ee6cc407 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -620,6 +620,10 @@ 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..77f27e7163 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("%s: should only be called by idle domain\n", __func__); + 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..54745e6c6a 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("%s: should only be called by idle domain\n", __func__); + 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,