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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> +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?

Jan