Re: [PATCH v9 3/3] xsm: refactor flask sid alloc and domain check

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

On 6/30/22 02:14, Jan Beulich wrote:
> Just a two nits - while the change looks plausible, I'm afraid I'm
> not qualified to properly review it.
> 
> On 30.06.2022 04:21, Daniel P. Smith wrote:
>> The function flask_domain_alloc_security() is where a default sid should be
>> assigned to a domain under construction. For reasons unknown, the initial
>> domain would be assigned unlabeled_t and then fixed up under
>> flask_domain_create().  With the introduction of xenboot_t it is now possible
>> to distinguish when the hypervisor is in the boot state.
>>
>> This commit looks to correct this by using a check to see if the hypervisor 
>> is
>> under the xenboot_t context in flask_domain_alloc_security(). If it is, then 
>> it
> 
> While (or maybe because) I'm not a native speaker, the use of "looks"
> reads ambiguous to me. I think you mean it in the sense of e.g. "aims",
> but at first I read it in the sense of "seems", which made me think
> you're not certain whether it actually does.

Apologies, "look to" or "looks to" are forms of an American idiom, and
was used for its meaning of "expected to happen"[1]. I will reword to
provide a concise version of this statement.

[1] https://idioms.thefreedictionary.com/look+to

>> will inspect the domain's is_privileged field, and select the appropriate
>> default label, dom0_t or domU_t, for the domain. The logic for
>> flask_domain_create() was changed to allow the incoming sid to override the
>> default label.
>>
>> The base policy was adjusted to allow the idle domain under the xenboot_t
>> context to be able to construct domains of both types, dom0 and domU.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  tools/flask/policy/modules/dom0.te |  3 +++
>>  tools/flask/policy/modules/domU.te |  3 +++
>>  xen/xsm/flask/hooks.c              | 34 ++++++++++++++++++------------
>>  3 files changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/flask/policy/modules/dom0.te 
>> b/tools/flask/policy/modules/dom0.te
>> index 0a63ce15b6..2022bb9636 100644
>> --- a/tools/flask/policy/modules/dom0.te
>> +++ b/tools/flask/policy/modules/dom0.te
>> @@ -75,3 +75,6 @@ admin_device(dom0_t, ioport_t)
>>  admin_device(dom0_t, iomem_t)
>>  
>>  domain_comms(dom0_t, dom0_t)
>> +
>> +# Allow they hypervisor to build domains of type dom0_t
> 
> Since it repeats ...

Ack.

>> +xen_build_domain(dom0_t)
>> diff --git a/tools/flask/policy/modules/domU.te 
>> b/tools/flask/policy/modules/domU.te
>> index b77df29d56..73fc90c3c6 100644
>> --- a/tools/flask/policy/modules/domU.te
>> +++ b/tools/flask/policy/modules/domU.te
>> @@ -13,6 +13,9 @@ domain_comms(domU_t, domU_t)
>>  migrate_domain_out(dom0_t, domU_t)
>>  domain_self_comms(domU_t)
>>  
>> +# Allow they hypervisor to build domains of type domU_t
>> +xen_build_domain(domU_t)
> 
> ... here - s/they/the/ in both places?

Ack.

> Jan