On Wed, Jun 29, 2022 at 10:22 PM Daniel P. Smith
<dpsm...@apertussolutions.com> 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.
There is no "default label" which is why unlabeled_t was used.
flask_domain_create() does permission checks before setting the label
from the config.
flask_domain_alloc_security()
- mallocs domain_security_struct
- sets sid for system domains xen_t, domxen_t, domio_t
- is called for all domains.
flask_domain_create()
- special cases labeling dom0_t so it can only be done once.
- otherwise, ensures current has permission to domain_create.
- sets sid for both cases.
- has the config->ssidref passed in.
- is only called for non-system, non-idle domains.
> 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
> 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.
Your patch special cases flask_domain_alloc_security() to assign
dom0/domU. You already have a config and therefore config->ssidref,
so that shouldn't be necessary since flask_domain_create() can use
that ssidref.
I don't see a precise reason for why this change is needed in the
commit message. I know you are working on hyperlaunch, but I'm
guessing at the rationale for this change. And hyperlaunch should be
passing in a config with ssidref set instead of relying on the flask
code to auto assign, so I'm not sure of why this change would be
needed for hyperlaunch.
Is the issue with only having a single creation of dom0_t? Can you
change create_dom()'s dom0_cfg to specify the ssidref when creating
dom0. I guess that would need a new xsm_dom0_ssidref hook (or
something) to hide away the flask code. But that way there is less
special casing in the code. flask_domain_alloc_security() could be
left unchanged, and flask_domain_create could() be streamlined to just
check the passed in ssidref. How does that sound?
> Signed-off-by: Daniel P. Smith <dpsm...@apertussolutions.com>
> ---
> 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
> +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)
> +
> # Device model for domU_t. You can define distinct types for device models
> for
> # domains of other types, or add more make_device_model lines for this type.
> declare_domain(dm_dom_t)
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 8c9cd0f297..caa0ae7d4c 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -182,7 +182,15 @@ static int cf_check flask_domain_alloc_security(struct
> domain *d)
> dsec->sid = SECINITSID_DOMIO;
> break;
> default:
> - dsec->sid = SECINITSID_UNLABELED;
> + if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
> + {
> + if ( d->is_privileged )
> + dsec->sid = SECINITSID_DOM0;
> + else
> + dsec->sid = SECINITSID_DOMU;
> + }
> + else
> + dsec->sid = SECINITSID_UNLABELED;
> }
>
> dsec->self_sid = dsec->sid;
> @@ -548,23 +556,21 @@ static int cf_check flask_domain_create(struct domain
> *d, uint32_t ssidref)
> {
> int rc;
> struct domain_security_struct *dsec = d->ssid;
> - static int dom0_created = 0;
>
> - if ( is_idle_domain(current->domain) && !dom0_created )
> - {
> - dsec->sid = SECINITSID_DOM0;
> - dom0_created = 1;
> - }
> - else
> + /*
> + * If domain has not already been labeled or a valid new label is
> provided,
> + * then use the provided label, otherwise use the existing label.
> + */
> + if ( dsec->sid == SECINITSID_UNLABELED || ssidref > 0 )
> {
> - rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
> - DOMAIN__CREATE, NULL);
> - if ( rc )
> - return rc;
The old code returned here before...
> -
> dsec->sid = ssidref;
... setting the sid. That is more robust since if the function fails,
the sid is not changed. It's not a problem today as the
xsm_domain_create() return value is checked, but it is more robust to
restore that property.
Regards,
Jason
> + dsec->self_sid = dsec->sid;
> }
> - dsec->self_sid = dsec->sid;
> +
> + rc = avc_current_has_perm(dsec->sid, SECCLASS_DOMAIN,
> + DOMAIN__CREATE, NULL);
> + if ( rc )
> + return rc;
>
> rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
> &dsec->self_sid);
> --
> 2.20.1
>
