* [PATCH v2] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg
@ 2021-07-30 19:34 Scott Davis
2021-08-18 1:15 ` Jason Andryuk
0 siblings, 1 reply; 3+ messages in thread
From: Scott Davis @ 2021-07-30 19:34 UTC (permalink / raw)
To: xen-devel
Cc: Scott Davis, Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
Anthony PERARD, Juergen Gross, Daniel De Graaf, Daniel P . Smith,
Jason Andryuk, Marek Marczykowski-Górecki, Andrew Cooper
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 device-model stubdomain is relabeled at the same time as its guest,
just before the guest is unpaused. This requires the stubdomain itself
to be unpaused and run for a short time prior to being relabeled, but
allows PCI device assignments specified in xl.cfg to be completed prior
to relabeling. This choice allows the privileges required to perform
assignments to be dropped in the relabeling.
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>
---
Changes in v2:
- removed superfluous chanegs to libxl_dm.c
- changed all security label lookup failures due to FLASK being disabled
from warnings to hard errors based on mailing list discussion
- added discussion of relabel point to commit message
---
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 | 42 ++++++++++++++++++++--------
tools/libs/light/libxl_types.idl | 2 ++
tools/xl/xl_parse.c | 12 +++++++-
7 files changed, 71 insertions(+), 12 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 4b1e3028d2..7c8a696d61 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2766,6 +2766,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 bfc1e7f312..a70eb5eb58 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1023,6 +1023,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)
@@ -1354,6 +1356,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 09a3bb67e2..a76570cae7 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -488,6 +488,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 b9ba16d698..ca3ec3e53d 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1081,6 +1081,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..892527c4b4 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1032,12 +1032,11 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
&d_config->c_info.ssidref);
if (ret) {
if (errno == ENOSYS) {
- LOGD(WARN, domid, "XSM Disabled: init_seclabel not supported");
- ret = 0;
+ LOGD(ERROR, domid, "XSM Disabled: init_seclabel not supported");
} else {
LOGD(ERROR, domid, "Invalid init_seclabel: %s", s);
- goto error_out;
}
+ goto error_out;
}
}
@@ -1047,12 +1046,11 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
&d_config->b_info.exec_ssidref);
if (ret) {
if (errno == ENOSYS) {
- LOGD(WARN, domid, "XSM Disabled: seclabel not supported");
- ret = 0;
+ LOGD(ERROR, domid, "XSM Disabled: seclabel not supported");
} else {
LOGD(ERROR, domid, "Invalid seclabel: %s", s);
- goto error_out;
}
+ goto error_out;
}
}
@@ -1062,14 +1060,30 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
&d_config->b_info.device_model_ssidref);
if (ret) {
if (errno == ENOSYS) {
- LOGD(WARN, domid,
- "XSM Disabled: device_model_stubdomain_seclabel not supported");
- ret = 0;
+ LOGD(ERROR, domid,
+ "XSM Disabled: device_model_stubdomain_init_seclabel not supported");
} else {
- LOGD(ERROR, domid, "Invalid device_model_stubdomain_seclabel: %s", s);
+ 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(ERROR, domid,
+ "XSM Disabled: device_model_stubdomain_seclabel not supported");
+ } else {
+ LOGD(ERROR, domid,
+ "Invalid device_model_stubdomain_seclabel: %s", s);
+ }
+ goto error_out;
+ }
}
if (d_config->c_info.pool_name) {
@@ -1935,7 +1949,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_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff653a..490d0fa059 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -529,6 +529,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 17dddb4cd5..211fcdc0c8 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg
2021-07-30 19:34 [PATCH v2] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg Scott Davis
@ 2021-08-18 1:15 ` Jason Andryuk
2021-08-23 17:52 ` Scott Davis
0 siblings, 1 reply; 3+ messages in thread
From: Jason Andryuk @ 2021-08-18 1:15 UTC (permalink / raw)
To: Scott Davis
Cc: xen-devel, Scott Davis, Ian Jackson, Wei Liu, George Dunlap,
Nick Rosbrook, Anthony PERARD, Juergen Gross, Daniel De Graaf,
Daniel P . Smith, Marek Marczykowski-Górecki, Andrew Cooper
Hi, Scott
On Fri, Jul 30, 2021 at 3:35 PM 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 device-model stubdomain is relabeled at the same time as its guest,
> just before the guest is unpaused. This requires the stubdomain itself
> to be unpaused and run for a short time prior to being relabeled, but
> allows PCI device assignments specified in xl.cfg to be completed prior
> to relabeling. This choice allows the privileges required to perform
> assignments to be dropped in the relabeling.
>
> The implementation mirrors that of the 'seclabel' and 'init_seclabel'
> options for user domains. When all used in concert, this enables the
Drop all -> "When used in concert"?
> 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>
> ---
> Changes in v2:
> - removed superfluous chanegs to libxl_dm.c
> - changed all security label lookup failures due to FLASK being disabled
> from warnings to hard errors based on mailing list discussion
I think this should be a separate patch before this one, since it is
distinct from adding device_model_stubdomain_init_seclabel.
> - added discussion of relabel point to commit message
I was hoping for more people's thoughts on this. I'm just going to
write down my thoughts on this to get them out there.
The non-stubdom case is very straightforward:
<-init-><-relabel-><-unpause-><-exec->
Relabeling before unpause is a nice delineation point. Since the
guest hasn't run, it can't be mid-operation when the relabel happens.
The stubdom case as implemented by the patch is this:
Guest:
<---------init------------->|<-relabel-><-unpause-><-exec->
Stubdom: |
<-init--><-unpause-><-exec->|<-relabel-><-exec->
Here the stubdom runs for some time and then gets relabeled.
The alternative would be to relabel the stubdom before unpause and
then relabel the guest:
Guest:
<----------init---------------------->|<-relabel-><-unpause-><-exec->
Stubdom: |
<-init-><-relabel-><-unpause-><-exec--|--exec->
Here relabel then unpause is maintained for both guest and stubdom.
Relabeling the *running* stubdom gives me pause. We don't have any
synchronization with the stubdom's & QEMU's state. QEMU wrote
"running" to indicate it was up, but that doesn't say anything about
the rest of the kernel. From the other side, the stubdom and QEMU
running and having the guest label change could be surprising.
Scott, you are using this, so I may be imagining concerns that don't
exist. And you did mention your approach gives the PCI assignment
only to the stubdom init label, which could be a nice feature. It's
just the lack of clear delineation for the stubdom that makes me
wonder about it.
Having said that, for your approach, I think the code looks good.
Regards,
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg
2021-08-18 1:15 ` Jason Andryuk
@ 2021-08-23 17:52 ` Scott Davis
0 siblings, 0 replies; 3+ messages in thread
From: Scott Davis @ 2021-08-23 17:52 UTC (permalink / raw)
To: Jason Andryuk
Cc: xen-devel, Scott Davis, Ian Jackson, Wei Liu, George Dunlap,
Nick Rosbrook, Anthony PERARD, Juergen Gross, Daniel De Graaf,
Daniel P . Smith, Marek Marczykowski-Górecki, Andrew Cooper
Thanks for the review, Jason!
On Tue, Aug 17, 2021 at 9:15 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>> The implementation mirrors that of the 'seclabel' and 'init_seclabel'
>> options for user domains. When all used in concert, this enables the
>
> Drop all -> "When used in concert"?
Ack for v3.
>> - changed all security label lookup failures due to FLASK being disabled
>> from warnings to hard errors based on mailing list discussion
>
> I think this should be a separate patch before this one, since it is
> distinct from adding device_model_stubdomain_init_seclabel.
Ack for v3.
>> - added discussion of relabel point to commit message
>
> I was hoping for more people's thoughts on this. I'm just going to
> write down my thoughts on this to get them out there.
>
> The non-stubdom case is very straightforward:
> <-init-><-relabel-><-unpause-><-exec->
>
> Relabeling before unpause is a nice delineation point. Since the
> guest hasn't run, it can't be mid-operation when the relabel happens.
>
> The stubdom case as implemented by the patch is this:
> Guest:
> <---------init------------->|<-relabel-><-unpause-><-exec->
> Stubdom: |
> <-init--><-unpause-><-exec->|<-relabel-><-exec->
>
> Here the stubdom runs for some time and then gets relabeled.
>
> The alternative would be to relabel the stubdom before unpause and
> then relabel the guest:
> Guest:
> <----------init---------------------->|<-relabel-><-unpause-><-exec->
> Stubdom: |
> <-init-><-relabel-><-unpause-><-exec--|--exec->
>
> Here relabel then unpause is maintained for both guest and stubdom.
>
> Relabeling the *running* stubdom gives me pause. We don't have any
> synchronization with the stubdom's & QEMU's state. QEMU wrote
> "running" to indicate it was up, but that doesn't say anything about
> the rest of the kernel. From the other side, the stubdom and QEMU
> running and having the guest label change could be surprising.
>
> Scott, you are using this, so I may be imagining concerns that don't
> exist. And you did mention your approach gives the PCI assignment
> only to the stubdom init label, which could be a nice feature. It's
> just the lack of clear delineation for the stubdom that makes me
> wonder about it.
>
> Having said that, for your approach, I think the code looks good.
As you said, there are advantages to both approaches. Relabeling the
stubdom before its launch would eliminate potential race conditions
between stubdom init and the run-time relabeling, which could cause
sporadic failures in any init steps that can fail to complete before
the relabel. On the other hand, waiting to relabel allows any
(dom0 -> stubdom) privileges that are required only for stubdom init,
particularly device passthrough setup, to be dropped from the run-time
label. (stubdom/dom0 -> domU) privileges are of less concern, since
they can be dropped by relabeling of the guest regardless.
At this point I can neither rule out synchronization issues with the late
relabeling approach nor quantify the benefits, as the policy I've been
using to verify this is fairly maximal in terms of run-time privileges.
I do intend to spend some time experimenting and shrinking it down in
the near future, though. Before that experimentation, I'll add a second
stubdom relabel point just before the stubdom is unpaused, along with a
config boolean to select which relabel point is used. That should allow
me to determine how much security benefit we'd be getting from the
later relabeling, and may be how things should go in upstream so that
users have a choice of approach.
In the meantime, input from any other devs is certainly welcome.
Good day,
Scott
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-23 17:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-30 19:34 [PATCH v2] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg Scott Davis
2021-08-18 1:15 ` Jason Andryuk
2021-08-23 17:52 ` Scott Davis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.