Re: [XEN PATCH] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Fri, Jul 23, 2021 at 12:47 AM Scott Davis <scottwd@xxxxxxxxx> wrote:
>
> This adds an option to the xl domain configuration syntax for specifying
> a build-time XSM security label for device-model stubdomains separate from
> the run-time label specified by 'device_model_stubdomain_seclabel'. Fields
> are also added to the 'libxl_domain_build_info' struct to contain the new
> information, and a new call to 'xc_flask_relabel_domain' inserted to
> affect the change at the appropriate time.
>
> The implementation mirrors that of the 'seclabel' and 'init_seclabel'
> options for user domains. When all used in concert, this enables the
> creation of security policies that minimize run-time privileges between
> the toolstack domain, device-model stubdomains, and user domains.

Cool stuff!

> Signed-off-by: Scott Davis <scott.davis@xxxxxxxxxx>
> ---

> @@ -1935,7 +1953,13 @@ static void domcreate_complete(libxl__egc *egc,
>      libxl__domain_build_state_dispose(&dcs->build_state);
>
>      if (!rc && d_config->b_info.exec_ssidref)
> -        rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, 
> d_config->b_info.exec_ssidref);
> +        rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid,
> +                                     d_config->b_info.exec_ssidref);
> +
> +    if (!rc && dcs->sdss.pvqemu.guest_domid != INVALID_DOMID &&
> +        d_config->b_info.device_model_exec_ssidref)
> +        rc = xc_flask_relabel_domain(CTX->xch, dcs->sdss.pvqemu.guest_domid,
> +                                     
> d_config->b_info.device_model_exec_ssidref);

The build/create logic is complicated, so I'm asking the question in
case you already know.  This looks like domcreate_complete runs once
and relabels both the guest domain and the stubdom.  I thought it
would get called for each of stubdom and guest, so they would be
labeled according to exec_ssidref which you set for the stubdom b_info
below.  I looked around some and it seems like domcreate_complete is
only called for the guest.

Sort of relatedly, is stubdom unpaused before the guest gets
relabeled?  Quickly looking, I think stubdom is unpaused.  I would
think you want them both relabeled before either is unpaused.  If the
stubdom starts with the exec_label, but it sees the guest with the
init_label, it may get an unexpected denial?  On the other hand,
delayed unpausing of stubdom would slow down booting.

With the stubdom getting unpaused before relabel, do you have to give
the stubdom some extra flask policy permissions to handle that case?

>      bool retain_domain = !rc || rc == ERROR_ABORTED;
>
> diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> index dbd3c7f278..2b69b207c4 100644
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -2300,20 +2300,24 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
> libxl__stub_dm_spawn_state *sdss)
>      sdss->pvqemu.guest_domid = INVALID_DOMID;
>
>      libxl_domain_create_info_init(&dm_config->c_info);
> +    libxl_domain_build_info_init(&dm_config->b_info);
> +    libxl_domain_build_info_init_type(&dm_config->b_info, 
> LIBXL_DOMAIN_TYPE_PV);
> +

Is there a particular need for moving these lines here?

>      dm_config->c_info.type = LIBXL_DOMAIN_TYPE_PV;
>      dm_config->c_info.name = libxl__stub_dm_name(gc,
>                                      libxl__domid_to_name(gc, guest_domid));
> -    /* When we are here to launch stubdom, ssidref is a valid value
> -     * already, no need to parse it again.
> +
> +    /* When we are here to launch stubdom, ssidrefs are valid values already,
> +     * no need to parse them again.
>       */
>      dm_config->c_info.ssidref = guest_config->b_info.device_model_ssidref;
>      dm_config->c_info.ssid_label = NULL;
> +    dm_config->b_info.exec_ssidref =
> +        guest_config->b_info.device_model_exec_ssidref;
> +    dm_config->b_info.exec_ssid_label = NULL;

At first glance, it seems only these additions are strictly necessary.
But if only domcreate_complete is doing the relabel, then they are
unused?

Thanks,
Jason