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

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

[Scott Davis]

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.

Signed-off-by: Scott Davis <scott.davis@starlab.io>
---
 docs/man/xl.cfg.5.pod.in             | 10 ++++++++++
 tools/golang/xenlight/helpers.gen.go |  5 +++++
 tools/golang/xenlight/types.gen.go   |  2 ++
 tools/include/libxl.h                | 10 ++++++++++
 tools/libs/light/libxl_create.c      | 28 ++++++++++++++++++++++++++--
 tools/libs/light/libxl_dm.c          | 14 +++++++++-----
 tools/libs/light/libxl_types.idl     |  2 ++
 tools/xl/xl_parse.c                  | 12 +++++++++++-
 8 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 56370a37db..3458d357fc 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2762,6 +2762,16 @@ you have selected.
 
 Assign an XSM security label to the device-model stubdomain.
 
+=item B<device_model_stubdomain_init_seclabel="LABEL">
+
+Specify a temporary XSM security label for the device-model stubdomain used
+during creation of it and its associated guest. The stubdomain's XSM label will
+then be changed to the execution seclabel (as specified by
+B<device_model_stubdomain_seclabel>) once creation is complete, prior to
+unpausing the stubdomain's guest. With proper (re)labeling, a security policy
+can be constructed that minimizes run-time privileges between the toolstack
+domain, device-model stubdomains, and user domains.
+
 =item B<device_model_args=[ "ARG", "ARG", ...]>
 
 Pass additional arbitrary options on the device-model command
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index db82537b42..e961cb5f75 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1022,6 +1022,8 @@ x.StubdomainRamdisk = C.GoString(xc.stubdomain_ramdisk)
 x.DeviceModel = C.GoString(xc.device_model)
 x.DeviceModelSsidref = uint32(xc.device_model_ssidref)
 x.DeviceModelSsidLabel = C.GoString(xc.device_model_ssid_label)
+x.DeviceModelExecSsidref = uint32(xc.device_model_exec_ssidref)
+x.DeviceModelExecSsidLabel = C.GoString(xc.device_model_exec_ssid_label)
 x.DeviceModelUser = C.GoString(xc.device_model_user)
 if err := x.Extra.fromC(&xc.extra);err != nil {
 return fmt.Errorf("converting field Extra: %v", err)
@@ -1351,6 +1353,9 @@ xc.device_model = C.CString(x.DeviceModel)}
 xc.device_model_ssidref = C.uint32_t(x.DeviceModelSsidref)
 if x.DeviceModelSsidLabel != "" {
 xc.device_model_ssid_label = C.CString(x.DeviceModelSsidLabel)}
+xc.device_model_exec_ssidref = C.uint32_t(x.DeviceModelExecSsidref)
+if x.DeviceModelExecSsidLabel != "" {
+xc.device_model_exec_ssid_label = C.CString(x.DeviceModelExecSsidLabel)}
 if x.DeviceModelUser != "" {
 xc.device_model_user = C.CString(x.DeviceModelUser)}
 if err := x.Extra.toC(&xc.extra); err != nil {
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index a214dd9df6..45061d1afa 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -487,6 +487,8 @@ StubdomainRamdisk string
 DeviceModel string
 DeviceModelSsidref uint32
 DeviceModelSsidLabel string
+DeviceModelExecSsidref uint32
+DeviceModelExecSsidLabel string
 DeviceModelUser string
 Extra StringList
 ExtraPv StringList
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ae7fe27c1f..62b69222f6 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1069,6 +1069,16 @@ typedef struct libxl__ctx libxl_ctx;
  */
 #define LIBXL_HAVE_SSID_LABEL 1
 
+/*
+ * LIBXL_HAVE_BUILDINFO_DEVICE_MODEL_STUBDOMAIN_EXEC_SSID
+ *
+ * If this is defined, then the libxl_domain_build_info structure will
+ * contain 'device_model_exec_ssidref' and 'device_model_exec_ssid_label' for
+ * specifying a run-time XSM security label separate from the build-time label
+ * specified in 'device_model_ssidref' and 'device_model_ssid_label'.
+ */
+#define LIBXL_HAVE_BUILDINFO_DEVICE_MODEL_STUBDOMAIN_EXEC_SSID 1
+
 /*
  * LIBXL_HAVE_CPUPOOL_NAME
  *
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index e356b2106d..a12da5531d 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1060,13 +1060,31 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         char *s = d_config->b_info.device_model_ssid_label;
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
                                          &d_config->b_info.device_model_ssidref);
+        if (ret) {
+            if (errno == ENOSYS) {
+                LOGD(WARN, domid,
+                     "XSM Disabled: device_model_stubdomain_init_seclabel not supported");
+                ret = 0;
+            } else {
+                LOGD(ERROR, domid,
+                     "Invalid device_model_stubdomain_init_seclabel: %s", s);
+                goto error_out;
+            }
+        }
+    }
+
+    if (d_config->b_info.device_model_exec_ssid_label) {
+        char *s = d_config->b_info.device_model_exec_ssid_label;
+        ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
+                                         &d_config->b_info.device_model_exec_ssidref);
         if (ret) {
             if (errno == ENOSYS) {
                 LOGD(WARN, domid,
                      "XSM Disabled: device_model_stubdomain_seclabel not supported");
                 ret = 0;
             } else {
-                LOGD(ERROR, domid, "Invalid device_model_stubdomain_seclabel: %s", s);
+                LOGD(ERROR, domid,
+                     "Invalid device_model_stubdomain_seclabel: %s", s);
                 goto error_out;
             }
         }
@@ -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);
 
     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);
+
     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;
 
     libxl_uuid_generate(&dm_config->c_info.uuid);
 
-    libxl_domain_build_info_init(&dm_config->b_info);
-    libxl_domain_build_info_init_type(&dm_config->b_info, LIBXL_DOMAIN_TYPE_PV);
-
     dm_config->b_info.shadow_memkb = 0;
     dm_config->b_info.max_vcpus = 1;
     dm_config->b_info.max_memkb = guest_config->b_info.stubdomain_memkb;
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index f45adddab0..b483729b9c 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -528,6 +528,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("device_model",     string),
     ("device_model_ssidref", uint32),
     ("device_model_ssid_label", string),
+    ("device_model_exec_ssidref",    uint32),
+    ("device_model_exec_ssid_label", string),
     ("device_model_user", string),
 
     # extra parameters pass directly to qemu, NULL terminated
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 9fb0791429..236f8b2fc0 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2523,10 +2523,20 @@ skip_usbdev:
     xlu_cfg_get_defbool (config, "device_model_stubdomain_override",
                          &b_info->device_model_stubdomain, 0);
 
-    if (!xlu_cfg_get_string (config, "device_model_stubdomain_seclabel",
+    if (!xlu_cfg_get_string (config, "device_model_stubdomain_init_seclabel",
                              &buf, 0))
+        xlu_cfg_replace_string(config, "device_model_stubdomain_init_seclabel",
+                               &b_info->device_model_ssid_label, 0);
+
+    if (!xlu_cfg_get_string (config, "device_model_stubdomain_seclabel",
+                             &buf, 0)) {
+        if (b_info->device_model_ssid_label)
+            xlu_cfg_replace_string(config, "device_model_stubdomain_seclabel",
+                                   &b_info->device_model_exec_ssid_label, 0);
+        else
             xlu_cfg_replace_string(config, "device_model_stubdomain_seclabel",
                                    &b_info->device_model_ssid_label, 0);
+    }
 
     xlu_cfg_replace_string(config, "device_model_user",
                            &b_info->device_model_user, 0);
-- 
2.25.1

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

[Jason Andryuk]

On Fri, Jul 23, 2021 at 12:47 AM Scott Davis <scottwd@gmail.com> 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@starlab.io>
> ---

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

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

[Andrew Cooper]

On 23/07/2021 05:47, Scott Davis wrote:
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index e356b2106d..a12da5531d 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -1060,13 +1060,31 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>          char *s = d_config->b_info.device_model_ssid_label;
>          ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
>                                           &d_config->b_info.device_model_ssidref);
> +        if (ret) {
> +            if (errno == ENOSYS) {
> +                LOGD(WARN, domid,
> +                     "XSM Disabled: device_model_stubdomain_init_seclabel not supported");
> +                ret = 0;

Surely this wants to be a hard error?

Not specifying a label is one thing, but specifying a label and having
it not take effect because code was compiled out of the hypervisor
sounds like a security hole.

I see this is a pattern copied from elsewhere, but it seems very short
signed.

~Andrew

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 881 bytes --]

On Mon, Jul 26, 2021 at 09:07:03AM -0400, Jason Andryuk wrote:
> 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.

Some parts of the stubdomain setup are done after it's unpaused (but
before the guest is unpaused). Especially, PCI devices are hot-plugged
only when QEMU is already running (not sure why).

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

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ian Jackson]

Andrew Cooper writes ("Re: [XEN PATCH] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg"):
> On 23/07/2021 05:47, Scott Davis wrote:
...
> >          ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
> >                                           &d_config->b_info.device_model_ssidref);
> > +        if (ret) {
> > +            if (errno == ENOSYS) {
> > +                LOGD(WARN, domid,
> > +                     "XSM Disabled: device_model_stubdomain_init_seclabel not supported");
> > +                ret = 0;
> 
> Surely this wants to be a hard error?
> 
> Not specifying a label is one thing, but specifying a label and having
> it not take effect because code was compiled out of the hypervisor
> sounds like a security hole.
> 
> I see this is a pattern copied from elsewhere, but it seems very short
> signed.

I wonder if this is to try to make it possible to boot a system whose
config specifies XSM labels but with XSM disabled.

Marek, or someone, can you advise ?

My initial thoughts are to agree with Andrew that ignoring this error
seems to me to be a bad plan, but maybe there is a good reason.

If we do want to improve this, maybe we need to update all the
corresponding call sites.

Thanks,
Ian.

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

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [XEN PATCH] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg"):
> On Mon, Jul 26, 2021 at 09:07:03AM -0400, Jason Andryuk wrote:
> > 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.
> 
> Some parts of the stubdomain setup are done after it's unpaused (but
> before the guest is unpaused). Especially, PCI devices are hot-plugged
> only when QEMU is already running (not sure why).

I think the PCI hotplug involves interaction with QEMU, and providing
only hotplug simplifies the code in libxl.  Anthony, do I have that
righgt ?

Naively, it seems to me that the security risks are limited because
until the guest is unpaused it doesn't have the ability to do
anything, so cannot yet mount an attack on qemu.  So I'm expecting
that qemu is still trustworthy until the guest is unpaused.

Ian.

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

[Jason Andryuk]

On Tue, Jul 27, 2021 at 9:32 AM Ian Jackson <iwj@xenproject.org> wrote:
>
> Marek Marczykowski-Górecki writes ("Re: [XEN PATCH] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg"):
> > On Mon, Jul 26, 2021 at 09:07:03AM -0400, Jason Andryuk wrote:
> > > 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.
> >
> > Some parts of the stubdomain setup are done after it's unpaused (but
> > before the guest is unpaused). Especially, PCI devices are hot-plugged
> > only when QEMU is already running (not sure why).

Thanks, Marek.

> I think the PCI hotplug involves interaction with QEMU, and providing
> only hotplug simplifies the code in libxl.  Anthony, do I have that
> righgt ?
>
> Naively, it seems to me that the security risks are limited because
> until the guest is unpaused it doesn't have the ability to do
> anything, so cannot yet mount an attack on qemu.  So I'm expecting
> that qemu is still trustworthy until the guest is unpaused.

I was looking at it from the other direction - protecting and guest
and stubdom from dom0.  The nice thing you can do is prevent dom0 from
mapping the guest's memory after the relabeling.

The relabeling placement in this patch may be okay.  The stubdom
itself is a dom0-supplied kernel & ramdisk.  So a window of time where
it's running before being relabeled isn't that big of a deal.  i.e.
instead of dom0 modifying the stubdom in that window, it could just
supply modified kernel and ramdisk initially.

Relabeling guest & stubdom prior to unpausing the guest ensures they
both have their desired labels before the guest is unpaused.  Like you
said, that seems to be the important part - both domains have their
desired label before the guest starts running.  It's when the guest
starts running that it may have sensitive contents in its memory.

I am curious if the relabeling in this patch requires more flask
permissions since the running init_label stubdom sees the paused
init_label guest.

Regards,
Jason

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

[Anthony PERARD]

On Tue, Jul 27, 2021 at 02:32:22PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("Re: [XEN PATCH] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg"):
> > On Mon, Jul 26, 2021 at 09:07:03AM -0400, Jason Andryuk wrote:
> > > 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.
> > 
> > Some parts of the stubdomain setup are done after it's unpaused (but
> > before the guest is unpaused). Especially, PCI devices are hot-plugged
> > only when QEMU is already running (not sure why).
> 
> I think the PCI hotplug involves interaction with QEMU, and providing
> only hotplug simplifies the code in libxl.  Anthony, do I have that
> righgt ?

I think interaction with QEMU is needed to find out the new address of
the PCI device in cases none were asked for. And have a single
implementation in libxl is certainly better.
But even if QEMU is running, I think we can still call it cold-plugged,
when it's done before emulation is supposed to have started.

Cheers,

-- 
Anthony PERARD

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

[Scott Davis]

Thanks for the review, everyone.

On 7/26/21, 9:08 AM, Jason Andryuk wrote:
> >      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?

Without the move, these inits would have stomped on the b_info.exec_ssid* 
values set below. However...

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

I believe you are correct. I also thought at first that setting these 
fields would be sufficient for the relabel, only to realize later that 
domcreate_complete does not get called for the stubdomain itself (and 
wouldn't be the correct place to relabel it even if it were). I will 
retest to confirm that these changes to libxl_dm.c are unnecessary and 
drop them in v2.

On 7/27/21, 9:30 AM, Ian Jackson wrote:
> Andrew Cooper writes:
> > >          ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
> > >                                           &d_config->b_info.device_model_ssidref);
> > > +        if (ret) {
> > > +            if (errno == ENOSYS) {
> > > +                LOGD(WARN, domid,
> > > +                     "XSM Disabled: device_model_stubdomain_init_seclabel not supported");
> > > +                ret = 0;
> >
> > Surely this wants to be a hard error?
> >
> > Not specifying a label is one thing, but specifying a label and having
> > it not take effect because code was compiled out of the hypervisor
> > sounds like a security hole.
> >
> > I see this is a pattern copied from elsewhere, but it seems very short
> > signed.
> 
> I wonder if this is to try to make it possible to boot a system whose
> config specifies XSM labels but with XSM disabled.
> 
> Marek, or someone, can you advise ?
> 
> My initial thoughts are to agree with Andrew that ignoring this error
> seems to me to be a bad plan, but maybe there is a good reason.
> 
> If we do want to improve this, maybe we need to update all the
> corresponding call sites.

My guess is that this pattern exists for cases where flask has been 
disabled at runtime via command line option or hypercall. However, I 
agree that lookup failure should be a hard error in these cases.
flask=permissive exists for temporarily disabling enforcement in 
development situations. As long as permissive mode is not broken by the 
change, I will plan to convert each of these warnings to hard errors in 
v2 unless someone feels otherwise.

On 7/28/21, 8:19 AM, Anthony PERARD wrote:
> On Tue, Jul 27, 2021 at 02:32:22PM +0100, Ian Jackson wrote:
> > Marek Marczykowski-Górecki writes:
> > > On Mon, Jul 26, 2021 at 09:07:03AM -0400, Jason Andryuk wrote:
> > > > 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.
> > >
> > > Some parts of the stubdomain setup are done after it's unpaused (but
> > > before the guest is unpaused). Especially, PCI devices are hot-plugged
> > > only when QEMU is already running (not sure why).
> >
> > I think the PCI hotplug involves interaction with QEMU, and providing
> > only hotplug simplifies the code in libxl.  Anthony, do I have that
> > righgt ?
> 
> I think interaction with QEMU is needed to find out the new address of
> the PCI device in cases none were asked for. And have a single
> implementation in libxl is certainly better.
> But even if QEMU is running, I think we can still call it cold-plugged,
> when it's done before emulation is supposed to have started.

Yes, I chose to relabel the stubdomain at the same time as the guest (and 
after unpause of the stubdomain itself) specifically so that PCI device 
passthrough to the guest can be completed prior to relabeling. This does 
require dm_dom_t's build label to be the source and target of more 
privileges than if the relabeling occurred pre-launch, but none of those 
are privileges that the combined build/run dm_dom_t label doesn't (or 
wouldn't) have already. The end goal in my mind is to be able to drop as 
many of those privileges as possible before the guest itself runs, and 
there are certainly some required to setup PCI device passthrough that 
may not be needed after, depending on the use case.

I will add a discussion of this decision to the v2 commit message.

On 7/27/21, 9:50 AM, Jason Andryuk wrote:
> On Tue, Jul 27, 2021 at 9:32 AM Ian Jackson <iwj@xenproject.org> wrote:
> > Naively, it seems to me that the security risks are limited because
> > until the guest is unpaused it doesn't have the ability to do
> > anything, so cannot yet mount an attack on qemu.  So I'm expecting
> > that qemu is still trustworthy until the guest is unpaused.
> 
> I was looking at it from the other direction - protecting and guest
> and stubdom from dom0.  The nice thing you can do is prevent dom0 from
> mapping the guest's memory after the relabeling.
> 
> The relabeling placement in this patch may be okay.  The stubdom
> itself is a dom0-supplied kernel & ramdisk.  So a window of time where
> it's running before being relabeled isn't that big of a deal.  i.e.
> instead of dom0 modifying the stubdom in that window, it could just
> supply modified kernel and ramdisk initially.
> 
> Relabeling guest & stubdom prior to unpausing the guest ensures they
> both have their desired labels before the guest is unpaused.  Like you
> said, that seems to be the important part - both domains have their
> desired label before the guest starts running.  It's when the guest
> starts running that it may have sensitive contents in its memory.

My thinking, as well.

Good day,

-Scott Davis