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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Daniel,

> On 11 May 2022, at 12:30 pm, Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx> 
> 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@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx> # arm

Reviewed-by: Rahul Singh <rahul.singh@xxxxxxx>
Tested-by: Rahul Singh <rahul.singh@xxxxxxx>

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