diff mbox

[1/2] libxl: dm_restrict: Move to domain_build_info

Message ID 1507807267-13709-1-git-send-email-ian.jackson@eu.citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ian Jackson Oct. 12, 2017, 11:21 a.m. UTC
Right now, this is broken because libxl__build_device_model_args_new
is used also for the qemu run for pv guests for qdisk devices, pvfb,
etc.

We can either make this option properly HVM-specific, or make it
generic.

In principle it is a reasonable request, to make the PV qemu
deprivileged (even though it is not likely to be implemented any time
soon).  So make this option generic.

We retain the name "device model" even though it is arguably
inaccurate, because the xl docs already say, for example
  For a PV guest a device-model is sometimes used to provide backends
  for certain PV devices

The documentation patch here is pure code motion.  For ease of review
we will fix up the docs, so the wording to be right for the new
context, in the next patch.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reported-by: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/man/xl.cfg.pod.5.in    | 198 ++++++++++++++++++++++----------------------
 tools/libxl/libxl_create.c  |   2 +-
 tools/libxl/libxl_dm.c      |   6 +-
 tools/libxl/libxl_types.idl |   2 +-
 4 files changed, 104 insertions(+), 104 deletions(-)

Comments

Wei Liu Oct. 12, 2017, 11:23 a.m. UTC | #1
On Thu, Oct 12, 2017 at 12:21:06PM +0100, Ian Jackson wrote:
> Right now, this is broken because libxl__build_device_model_args_new
> is used also for the qemu run for pv guests for qdisk devices, pvfb,
> etc.
> 
> We can either make this option properly HVM-specific, or make it
> generic.
> 
> In principle it is a reasonable request, to make the PV qemu
> deprivileged (even though it is not likely to be implemented any time
> soon).  So make this option generic.
> 
> We retain the name "device model" even though it is arguably
> inaccurate, because the xl docs already say, for example
>   For a PV guest a device-model is sometimes used to provide backends
>   for certain PV devices
> 
> The documentation patch here is pure code motion.  For ease of review
> we will fix up the docs, so the wording to be right for the new
> context, in the next patch.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Roger Pau Monné Oct. 12, 2017, 11:44 a.m. UTC | #2
On Thu, Oct 12, 2017 at 11:21:06AM +0000, Ian Jackson wrote:
> Right now, this is broken because libxl__build_device_model_args_new
> is used also for the qemu run for pv guests for qdisk devices, pvfb,
> etc.
> 
> We can either make this option properly HVM-specific, or make it
> generic.
> 
> In principle it is a reasonable request, to make the PV qemu
> deprivileged (even though it is not likely to be implemented any time
> soon).  So make this option generic.
> 
> We retain the name "device model" even though it is arguably
> inaccurate, because the xl docs already say, for example
>   For a PV guest a device-model is sometimes used to provide backends
>   for certain PV devices
> 
> The documentation patch here is pure code motion.  For ease of review
> we will fix up the docs, so the wording to be right for the new
> context, in the next patch.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  docs/man/xl.cfg.pod.5.in    | 198 ++++++++++++++++++++++----------------------
>  tools/libxl/libxl_create.c  |   2 +-
>  tools/libxl/libxl_dm.c      |   6 +-
>  tools/libxl/libxl_types.idl |   2 +-

This is at least missing a change to xl_parse.c AFAICT.

The rest LGTM.

Thanks, Roger.
diff mbox

Patch

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index cf3fa0e..8125dfb 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1270,6 +1270,105 @@  connectors=id0:1920x1080;id1:800x600;id2:640x480
 
 =back
 
+=item B<dm_restrict=BOOLEAN>
+
+Restrict the HVM device model after startup,
+to limit the consequencese of security vulnerabilities in qemu.
+
+With this feature enabled,
+a compromise of the device model,
+via such a vulnerability,
+will not provide a privilege escalation attack on the whole system.
+
+This feature is a B<technology preview>.
+There are some significant limitations:
+
+=over 4
+
+=item
+
+You must have a new enough qemu.
+In particular,
+if your qemu does not have the commit
+B<xen: restrict: use xentoolcore_restrict_all>
+the restriction request will be silently ineffective!
+
+=item
+
+The mechanisms used are not effective against
+denial of service problems.
+A compromised qemu can probably still impair
+or perhaps even prevent
+the proper functioning of the whole system,
+(at the very least, but not limited to,
+through resource exhaustion).
+
+=item
+
+It is not known whether the protection is
+effective when a domain is migrated.
+
+=item
+
+Some domain management functions do not work.
+For example, cdrom insert will fail.
+
+=item
+
+You should say C<vga="none">.
+Domains with stdvga graphics cards to not work.
+Domains with cirrus vga may seem to work.
+
+=item
+
+You must create user(s) for qemu to run as.
+
+Ideally, set aside a range of 32752 uids
+(from N to N+32751)
+and create a user
+whose name is B<xen-qemuuser-range-base>
+and whose uid is N
+and whose gid is a plain unprivileged gid.
+libxl will use one such user for each domid.
+
+Alternatively, either create
+B<xen-qemuuser-domid$domid>
+for every $domid from 1 to 32751 inclusive,
+or
+B<xen-qemuuser-shared>
+(in which case different guests will not
+be protected against each other).
+
+=item
+
+There are no countermeasures taken against reuse
+of the same unix user (uid)
+for subsequent domains,
+even if the B<xen-qemuuser-domid$domid> users are created.
+So a past domain with the same domid may be able to
+interferer with future domains.
+Possibly, even after a reboot.
+
+=item
+
+A compromised qemu will be able to read world-readable
+files in the dom0 operating system.
+
+=item
+
+Because of these limitations, this functionality,
+while it may enhance your security,
+should not be relied on.
+Any further limitations discovered in the current version
+will B<not> be handled via the Xen Project Security Process.
+
+=item
+
+In the future as we enhance this feature to improve the security,
+we may break backward compatibility.
+
+=back
+
 =head2 Paravirtualised (PV) Guest Specific Options
 
 The following options apply only to Paravirtual (PV) guests.
@@ -2197,105 +2296,6 @@  specified, enabling the use of XenServer PV drivers in the guest.
 This parameter only takes effect when device_model_version=qemu-xen.
 See B<xen-pci-device-reservations(7)> for more information.
 
-=item B<dm_restrict=BOOLEAN>
-
-Restrict the HVM device model after startup,
-to limit the consequencese of security vulnerabilities in qemu.
-
-With this feature enabled,
-a compromise of the device model,
-via such a vulnerability,
-will not provide a privilege escalation attack on the whole system.
-
-This feature is a B<technology preview>.
-There are some significant limitations:
-
-=over 4
-
-=item
-
-You must have a new enough qemu.
-In particular,
-if your qemu does not have the commit
-B<xen: restrict: use xentoolcore_restrict_all>
-the restriction request will be silently ineffective!
-
-=item
-
-The mechanisms used are not effective against
-denial of service problems.
-A compromised qemu can probably still impair
-or perhaps even prevent
-the proper functioning of the whole system,
-(at the very least, but not limited to,
-through resource exhaustion).
-
-=item
-
-It is not known whether the protection is
-effective when a domain is migrated.
-
-=item
-
-Some domain management functions do not work.
-For example, cdrom insert will fail.
-
-=item
-
-You should say C<vga="none">.
-Domains with stdvga graphics cards to not work.
-Domains with cirrus vga may seem to work.
-
-=item
-
-You must create user(s) for qemu to run as.
-
-Ideally, set aside a range of 32752 uids
-(from N to N+32751)
-and create a user
-whose name is B<xen-qemuuser-range-base>
-and whose uid is N
-and whose gid is a plain unprivileged gid.
-libxl will use one such user for each domid.
-
-Alternatively, either create
-B<xen-qemuuser-domid$domid>
-for every $domid from 1 to 32751 inclusive,
-or
-B<xen-qemuuser-shared>
-(in which case different guests will not
-be protected against each other).
-
-=item
-
-There are no countermeasures taken against reuse
-of the same unix user (uid)
-for subsequent domains,
-even if the B<xen-qemuuser-domid$domid> users are created.
-So a past domain with the same domid may be able to
-interferer with future domains.
-Possibly, even after a reboot.
-
-=item
-
-A compromised qemu will be able to read world-readable
-files in the dom0 operating system.
-
-=item
-
-Because of these limitations, this functionality,
-while it may enhance your security,
-should not be relied on.
-Any further limitations discovered in the current version
-will B<not> be handled via the Xen Project Security Process.
-
-=item
-
-In the future as we enhance this feature to improve the security,
-we may break backward compatibility.
-
-=back
-
 =back
 
 =head2 PVH Guest Specific Options
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0db9c0e..f15fb21 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -216,6 +216,7 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
         b_info->event_channels = 1023;
 
     libxl__arch_domain_build_info_acpi_setdefault(b_info);
+    libxl_defbool_setdefault(&b_info->dm_restrict, false);
 
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
@@ -308,7 +309,6 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.altp2m,             false);
         libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
-        libxl_defbool_setdefault(&b_info->u.hvm.dm_restrict,        false);
 
         libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
         if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0a5b0f8..7caf471 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -642,7 +642,7 @@  static int libxl__build_device_model_args_old(libxl__gc *gc,
             flexarray_append(dm_args, "-nographic");
     }
 
-    if (libxl_defbool_val(b_info->u.hvm.dm_restrict)) {
+    if (libxl_defbool_val(b_info->dm_restrict)) {
         LOGD(ERROR, domid,
              "dm_restrict not supported by qemu-xen-traditional");
         return ERROR_INVAL;
@@ -1421,7 +1421,7 @@  static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
     }
 
-    if (libxl_defbool_val(b_info->u.hvm.dm_restrict))
+    if (libxl_defbool_val(b_info->dm_restrict))
         flexarray_append(dm_args, "-xen-domid-restrict");
 
     if (state->saved_state) {
@@ -1653,7 +1653,7 @@  static int libxl__build_device_model_args_new(libxl__gc *gc,
             goto end_search;
         }
 
-        if (!libxl_defbool_val(b_info->u.hvm.dm_restrict)) {
+        if (!libxl_defbool_val(b_info->dm_restrict)) {
             LOGD(DEBUG, guest_domid,
                  "dm_restrict disabled, starting QEMU as root");
             goto end_search;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 65d32cb..a239324 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -521,6 +521,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
     ("timer_mode",       libxl_timer_mode),
     ("nested_hvm",       libxl_defbool),
     ("apic",             libxl_defbool),
+    ("dm_restrict",      libxl_defbool),
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
@@ -580,7 +581,6 @@  libxl_domain_build_info = Struct("domain_build_info",[
                                        ("rdm", libxl_rdm_reserve),
                                        ("rdm_mem_boundary_memkb", MemKB),
                                        ("mca_caps",         uint64),
-                                       ("dm_restrict",      libxl_defbool),
                                        ])),
                  ("pv", Struct(None, [("kernel", string),
                                       ("slack_memkb", MemKB),