Re: [PATCH v2 2/2] flask: implement xsm_transtion_running

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

On 4/21/22 05:22, Jan Beulich wrote:
> On 21.04.2022 00:28, Daniel P. Smith wrote:
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -168,7 +168,7 @@ static int cf_check flask_domain_alloc_security(struct 
>> domain *d)
>>      switch ( d->domain_id )
>>      {
>>      case DOMID_IDLE:
>> -        dsec->sid = SECINITSID_XEN;
>> +        dsec->sid = SECINITSID_XENBOOT;
>>          break;
>>      case DOMID_XEN:
>>          dsec->sid = SECINITSID_DOMXEN;
>> @@ -188,6 +188,7 @@ static int cf_check flask_domain_alloc_security(struct 
>> domain *d)
>>  
>>  static void cf_check flask_transition_running(void)
>>  {
>> +    struct domain_security_struct *dsec;
>>      struct domain *d = current->domain;
>>  
>>      if ( d->domain_id != DOMID_IDLE )
>> @@ -198,6 +199,10 @@ static void cf_check flask_transition_running(void)
>>       * set to false for the consistency check(s) in the setup code.
>>       */
>>      d->is_privileged = false;
>> +
>> +    dsec = d->ssid;
>> +    dsec->sid = SECINITSID_XEN;
>> +    dsec->self_sid = dsec->sid;
>>  }
> 
> If replacing SIDs is an okay thing to do, perhaps assert that the
> values haven't changed from SECINITSID_XENBOOT prior to replacing
> them?

Yes, changing a domain's SID is a legitimate action that could be done
today via the FLASK_RELABEL_DOMAIN subop of xsm_op hypercall that ends
up calling flask_relabel_domain(), when using flask policy. This is
where Jason was concerned if I was going to be using that call to change
the SID, which would require a policy rule to allow xenboot_t to relabel
itself as xen_t. As flask works today, the system domains use initial
SIDs which are effectively compile-time labels, which means the policy
rule is a static, fixed rule, i.e. it is not possible to use a different
set of labels, that must always be present. This also introduces the
risk for a custom policy writer to inadvertently leave the xenboot_t to
xen_t transitional rule out resulting in a failed access attempt which
would lead to a panic. This is unnecessary pain when we can just handle
the transition internal to the hypervisor as that where it is all really
occurring.

As for the ASSERT, that is a good point since there is a specific state
we are expecting to enter the hook. Pair that with some thinking I have
had to do in answering Jason, Roger, and yourself, I am going to rewire
the hook to return a success/error return value and move the panic
outside the check.

v/r,
dps