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
