[Qemu-devel] [PATCH 0/2] restrict the privilege of the xenstore connection

[Qemu-devel] [PATCH 0/2] restrict the privilege of the xenstore connection

[Stefano Stabellini]

Hi all,

this patch series introduces a new command line option to restrict the
privilege of the xenstore connection. Used together with -runas, can
help secure the execution of QEMU in Dom0.


Stefano Stabellini (2):
      xen: separate the xenstore_record_dm_state calls for pv and hvm machines
      xen: introduce xsrestrict

 hw/xenpv/xen_machine_pv.c |   11 +++++++++++
 include/hw/xen/xen.h      |    4 ++++
 qemu-options.hx           |   15 +++++++++++++++
 vl.c                      |    8 ++++++++
 xen-common-stub.c         |    6 ++++++
 xen-common.c              |   15 +--------------
 xen-hvm.c                 |   38 ++++++++++++++++++++++++++++++--------
 7 files changed, 75 insertions(+), 22 deletions(-)


Cheers,

Stefano

[Qemu-devel] [PATCH 1/2] xen: separate the xenstore_record_dm_state calls for pv and hvm machines

[Stefano Stabellini]

The following patch will introduce a new option to restrict the
privilege of the xenstore connection. In that case, we do not want to
use multiple xenstore connections, but just the one, with lower
privileges.

For this reason, split the xenstore_record_dm_state calls for pv and hvm
machines, so that in the hvm case QEMU will reuse the same xenstore
connection. (At the moment it opens two and uses the second one for the
xenstore_record_dm_state call.)

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xenpv/xen_machine_pv.c |   11 +++++++++++
 include/hw/xen/xen.h      |    2 ++
 xen-common-stub.c         |    4 ++++
 xen-common.c              |   15 +--------------
 xen-hvm.c                 |    1 +
 5 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 2e545d2..5ad22e3 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -28,6 +28,15 @@
 #include "xen_domainbuild.h"
 #include "sysemu/block-backend.h"
 
+static void xen_change_state_handler(void *opaque, int running,
+                                     RunState state)
+{
+    if (running) {
+        /* record state running */
+        xenstore_record_dm_state(xenstore, "running");
+    }
+}
+
 static void xen_init_pv(MachineState *machine)
 {
     const char *kernel_filename = machine->kernel_filename;
@@ -91,6 +100,8 @@ static void xen_init_pv(MachineState *machine)
 
     /* setup framebuffer */
     xen_init_display(xen_domid);
+
+    qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
 }
 
 static QEMUMachine xenpv_machine = {
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index b0ed04c..d118b56 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -37,6 +37,8 @@ void xen_cmos_set_s3_resume(void *opaque, int irq, int level);
 qemu_irq *xen_interrupt_controller_init(void);
 
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
+extern struct xs_handle *xs;
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
 int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
diff --git a/xen-common-stub.c b/xen-common-stub.c
index 906f991..6fcfc96 100644
--- a/xen-common-stub.c
+++ b/xen-common-stub.c
@@ -11,3 +11,7 @@
 void xenstore_store_pv_console_info(int i, CharDriverState *chr)
 {
 }
+
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
+{
+}
diff --git a/xen-common.c b/xen-common.c
index 56359ca..97fc312 100644
--- a/xen-common.c
+++ b/xen-common.c
@@ -83,8 +83,7 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
     }
 }
 
-
-static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
+void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
 {
     char path[50];
 
@@ -100,16 +99,6 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
     }
 }
 
-
-static void xen_change_state_handler(void *opaque, int running,
-                                     RunState state)
-{
-    if (running) {
-        /* record state running */
-        xenstore_record_dm_state(xenstore, "running");
-    }
-}
-
 static int xen_init(MachineState *ms)
 {
     xen_xc = xen_xc_interface_open(0, 0, 0);
@@ -117,8 +106,6 @@ static int xen_init(MachineState *ms)
         xen_be_printf(NULL, 0, "can't open xen interface\n");
         return -1;
     }
-    qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
-
     return 0;
 }
 
diff --git a/xen-hvm.c b/xen-hvm.c
index 315864c..1ea567d 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -1108,6 +1108,7 @@ static void xen_hvm_change_state_handler(void *opaque, int running,
 
     if (running) {
         xen_main_loop_prepare(state);
+        xenstore_record_dm_state(state->xenstore, "running");
     }
 
     xen_set_ioreq_server_state(xen_xc, xen_domid,
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/2] xen: introduce xsrestrict

[Stefano Stabellini]

Introduce a new command line option "xenopts", with one boolean
suboption "xsrestrict".  When xsrestrict=on is passed, QEMU will
restrict the xenstore connection calling xs_restrict. Also it won't
initialize the pv backends as they require higher privileges.

It requires a toolstack change to allow it to read/write to
/local/domain/0/device-model/$DOMID.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 include/hw/xen/xen.h |    2 ++
 qemu-options.hx      |   15 +++++++++++++++
 vl.c                 |    8 ++++++++
 xen-common-stub.c    |    2 ++
 xen-hvm.c            |   37 +++++++++++++++++++++++++++++--------
 5 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index d118b56..e91bea9 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -54,4 +54,6 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
 #  define HVM_MAX_VCPUS 32
 #endif
 
+extern QemuOptsList qemu_xen_opts;
+
 #endif /* QEMU_HW_XEN_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index 64af16d..104f138 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3057,6 +3057,21 @@ the guest clock runs ahead of the host clock. Typically this happens
 when the shift value is high (how high depends on the host machine).
 ETEXI
 
+DEF("xenopts", HAS_ARG, QEMU_OPTION_xenopts, \
+    "-xenopts [xsrestrict=on|off]\n" \
+    "                Xen Specific Options\n", QEMU_ARCH_ALL)
+STEXI
+@item -xenopts [xsrestrict=on|off]
+@findex -xenopts
+Options for the Xen hypervisor:
+
+@option{xsrestrict=on} will cause QEMU to restrict its xenstore
+connection to the privilege level of the guest it is serving. This will
+cause QEMU not to initialize the Xen PV backends, as they require an higher
+privilege level.
+ETEXI
+
+
 DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
     "-watchdog i6300esb|ib700\n" \
     "                enable virtual hardware watchdog [default=none]\n",
diff --git a/vl.c b/vl.c
index 81d80ae..acd4eea 100644
--- a/vl.c
+++ b/vl.c
@@ -2815,6 +2815,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_name_opts);
     qemu_add_opts(&qemu_numa_opts);
     qemu_add_opts(&qemu_icount_opts);
+    qemu_add_opts(&qemu_xen_opts);
 
     runstate_init();
 
@@ -3666,6 +3667,13 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_xenopts:
+                opts = qemu_opts_parse(qemu_find_opts("xenopts"),
+                                              optarg, 0);
+                if (!opts) {
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
                 runstate_set(RUN_STATE_INMIGRATE);
diff --git a/xen-common-stub.c b/xen-common-stub.c
index 6fcfc96..579b4ce 100644
--- a/xen-common-stub.c
+++ b/xen-common-stub.c
@@ -8,6 +8,8 @@
 #include "qemu-common.h"
 #include "hw/xen/xen.h"
 
+QemuOptsList qemu_xen_opts = { };
+
 void xenstore_store_pv_console_info(int i, CharDriverState *chr)
 {
 }
diff --git a/xen-hvm.c b/xen-hvm.c
index 1ea567d..ac985b6 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -36,6 +36,19 @@
     do { } while (0)
 #endif
 
+QemuOptsList qemu_xen_opts = {
+    .name = "xenopts",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_xen_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "xsrestrict",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end of list */ }
+    },
+};
+
 static MemoryRegion ram_memory, ram_640k, ram_lo, ram_hi;
 static MemoryRegion *framebuffer;
 static bool xen_in_migration;
@@ -1186,6 +1199,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
     xen_pfn_t bufioreq_pfn;
     evtchn_port_t bufioreq_evtchn;
     XenIOState *state;
+    QemuOpts *opts;
 
     state = g_malloc0(sizeof (XenIOState));
 
@@ -1304,16 +1318,23 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
     state->device_listener = xen_device_listener;
     device_listener_register(&state->device_listener);
 
-    /* Initialize backend core & drivers */
-    if (xen_be_init() != 0) {
-        fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
-        return -1;
-    }
-    xen_be_register("console", &xen_console_ops);
-    xen_be_register("vkbd", &xen_kbdmouse_ops);
-    xen_be_register("qdisk", &xen_blkdev_ops);
     xen_read_physmap(state);
 
+    opts = QTAILQ_FIRST(&qemu_xen_opts.head);
+    if (qemu_opt_get_bool(opts, "xsrestrict", false)) {
+        xs_restrict(state->xenstore, xen_domid);
+    } else {
+        /* Initialize backend core & drivers */
+        if (xen_be_init() != 0) {
+            fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
+            return -1;
+        }
+
+        xen_be_register("console", &xen_console_ops);
+        xen_be_register("vkbd", &xen_kbdmouse_ops);
+        xen_be_register("qdisk", &xen_blkdev_ops);
+    }
+
     return 0;
 }
 
-- 
1.7.10.4