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

[Qemu-devel] [PATCH v2 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.


Changes in v2:
- remove xenstore_record_dm_state and open code the xenstore write
instead
- change the xenpv machine xenstore path for startup notification to
device-model/$DOMID/pv/state


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 |   10 ++++++++++
 include/hw/xen/xen.h      |    2 ++
 qemu-options.hx           |   15 +++++++++++++++
 vl.c                      |    8 ++++++++
 xen-common-stub.c         |    2 ++
 xen-common.c              |   29 -----------------------------
 xen-hvm.c                 |   44 ++++++++++++++++++++++++++++++++++++--------
 7 files changed, 73 insertions(+), 37 deletions(-)

Cheers,

Stefano

[Qemu-devel] [PATCH v2 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.)

As the xenstore_record_dm_state is not very useful anymore, just remove
it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
Changes in v2:

- remove xenstore_record_dm_state and open code the xenstore write
instead
---
 hw/xenpv/xen_machine_pv.c |   10 ++++++++++
 xen-common.c              |   29 -----------------------------
 xen-hvm.c                 |    7 +++++++
 3 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 2e545d2..68758a0 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -45,6 +45,16 @@ static void xen_init_pv(MachineState *machine)
     switch (xen_mode) {
     case XEN_ATTACH:
         /* nothing to do, xend handles everything */
+        {
+            char path[50];
+            /* record state running */
+            snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
+            if (!xs_write(xenstore, XBT_NULL, path, "running", strlen("running"))) {
+                fprintf(stderr, "error recording state\n");
+                exit(1);
+            }
+        }
+
         break;
     case XEN_CREATE:
         if (xen_domain_build_pv(kernel_filename, initrd_filename,
diff --git a/xen-common.c b/xen-common.c
index 56359ca..5573c9e 100644
--- a/xen-common.c
+++ b/xen-common.c
@@ -83,33 +83,6 @@ void xenstore_store_pv_console_info(int i, CharDriverState *chr)
     }
 }
 
-
-static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
-{
-    char path[50];
-
-    if (xs == NULL) {
-        fprintf(stderr, "xenstore connection not initialized\n");
-        exit(1);
-    }
-
-    snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
-    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
-        fprintf(stderr, "error recording dm state\n");
-        exit(1);
-    }
-}
-
-
-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 +90,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..8079b8e 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -1107,7 +1107,14 @@ static void xen_hvm_change_state_handler(void *opaque, int running,
     XenIOState *state = opaque;
 
     if (running) {
+        char path[50];
         xen_main_loop_prepare(state);
+
+        snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
+        if (!xs_write(state->xenstore, XBT_NULL, path, "running", strlen("running"))) {
+            fprintf(stderr, "error recording state\n");
+            exit(1);
+        }
     }
 
     xen_set_ioreq_server_state(xen_xc, xen_domid,
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 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.

Change the xenpv machine xenstore path for startup notification to
/local/domain/0/device-model/$DOMID/pv/state, so that it doesn't get
confused with the device model path.

It requires a toolstack change to allow it to read/write to
/local/domain/0/device-model/$DOMID, and listen to
/local/domain/0/device-model/$DOMID/pv/state for xenpv machines.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
Changes in v2:

- change the xenpv machine xenstore path for startup notification to
device-model/$DOMID/pv/state.
---
 hw/xenpv/xen_machine_pv.c |    2 +-
 include/hw/xen/xen.h      |    2 ++
 qemu-options.hx           |   15 +++++++++++++++
 vl.c                      |    8 ++++++++
 xen-common-stub.c         |    2 ++
 xen-hvm.c                 |   37 +++++++++++++++++++++++++++++--------
 6 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 68758a0..262a8ae 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -48,7 +48,7 @@ static void xen_init_pv(MachineState *machine)
         {
             char path[50];
             /* record state running */
-            snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
+            snprintf(path, sizeof (path), "device-model/%u/pv/state", xen_domid);
             if (!xs_write(xenstore, XBT_NULL, path, "running", strlen("running"))) {
                 fprintf(stderr, "error recording state\n");
                 exit(1);
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index b0ed04c..6e864e0 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -52,4 +52,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 906f991..6792c2c 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 8079b8e..30fac46 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;
@@ -1192,6 +1205,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));
 
@@ -1310,16 +1324,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