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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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@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).

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