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

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

On 4/26/22 03:12, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 12:39:17PM -0400, Daniel P. Smith wrote:
>> On 4/25/22 05:44, Roger Pau Monné wrote:
>>> On Fri, Apr 22, 2022 at 12:34:57PM -0400, Daniel P. Smith wrote:
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index d5d0792ed4..e71fa3f860 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 ( xsm_set_system_active() != 0)
>>>> +        panic("xsm: unable to set hypervisor to SYSTEM_ACTIVE 
>>>> privilege\n");
>>>> +
>>>>      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..a3ce288ef9 100644
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -621,6 +621,9 @@ static void noreturn init_done(void)
>>>>      void *va;
>>>>      unsigned long start, end;
>>>>  
>>>> +    if ( xsm_set_system_active() != 0)
>>>            ^ extra space.
>>>
>>> Since the function returns an error code you might as well add it to
>>> the panic message, or else just make the function return bool instead.
>>>
>>> Or just make the function void and panic in the handler itself (like
>>> in previous versions), as I don't think it's sensible to continue
>>> normal execution if xsm_set_system_active fails.
>>
>> After reflecting on it, I believe that was not the correct action. The
>> policy should handle setting/checking all access control state and fail
>> with an error of why and then allow the hypervisor logic decided what to
>> do with that failure. For the policies that are present today, yes it is
>> an immediate panic. Ultimately this will future proof the interface
>> should a future policy type be introduced with a more varied result that
>> could allow the hypervisor to continue to boot, for instance to a
>> limited and/or debug state.
> 
> That's all fine, but if you return an error code, please print it as
> part of the panic message.  The more information we can add in case of
> panic, the better.

Ack.

>>>> 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..8a62de2fd6 100644
>>>> --- a/xen/xsm/flask/hooks.c
>>>> +++ b/xen/xsm/flask/hooks.c
>>>> @@ -186,6 +186,26 @@ 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;
>>>
>>> Nit: you should also add the assert for d->is_privileged, I don't see
>>> a reason for the xsm and flask functions to differ in that regard.
>>
>> This goes back to an issued I have raised before, is_privileged really
>> encompasses two properties of a domain. Whether the domain is filling
>> the special control domain role versus what accesses the domain has
>> based on the context under which is_control_domain() is called. For
>> instance the function init_domain_msr_policy() uses is_control_domain()
>> not to make an access control decision but configure behavior. Under
>> flask is_privileged no longer reflects the accesses a domain with it set
>> will have, thus whether it is cleared when flask is enabled is
>> irrelevant as far as flask is concerned. For the ASSERT, what matters is
>> that the label was set to xenboot_t on construction and that it was not
>> changed before reaching this point. Or in a short form, when under the
>> default policy the expected state is concerned with is_privilege while
>> for flask it is only the SID.
> 
> I certainly don't care that much, but you do set d->is_privileged =
> false in flask_set_system_active, hence it would seem logic to expect
> d->is_privileged == true also?

Yes, I did this just for consistency not because there is any
significance of is_privilege on the idle domain, in both contexts for
which is_privileged is used, when flask is the enforcing policy.

> If not for anything else, just to assert that the function is not
> called twice.

Under this patch flask_set_system_active() is effectively a no-op, so
calling it twice has no effect. In the next patch flask_set_system()
becomes a real check and there is an ASSERT on the SID as that is the
relevant context under flask and will ensure calling only once.

In the end I can add the ASSERT but it would be adding it for the sake
of adding it as it would not be protecting the hypervisor from moving
into an incorrect state.

v/r,
dps