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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx> # 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.