[RFC 0/4] libxl: Enable save/restore/migration of a restricted QEMU

[RFC 0/4] libxl: Enable save/restore/migration of a restricted QEMU

[Anthony PERARD]

The first two patchs fix save of a VM with a restricted QEMU, and the later two
patchs try to fix the restore path, but it is still WIP.

Checkout the last patch comments for more information.

Anthony PERARD (4):
  libxl: Learned to send FD through QMP to QEMU
  libxl: Have QEMU save its state to a file descriptor
  libxl_qmp: Implement query-status command
  HACK libxl_exec: Check QEMU status via QMP instead of xenstore

 tools/libxl/libxl_dm.c       |  3 ++
 tools/libxl/libxl_exec.c     | 95 ++++++++++++++++++++++++++++++++++++++++----
 tools/libxl/libxl_internal.h | 17 ++++++++
 tools/libxl/libxl_qmp.c      | 95 ++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 199 insertions(+), 11 deletions(-)

-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC 1/4] libxl: Learned to send FD through QMP to QEMU

[Anthony PERARD]

Adding the ability to send a file descriptor from libxl to QEMU via the
QMP interface. This will be use with the "add-fd" QMP command.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index d03cb51668..5bf5236240 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -80,6 +80,9 @@ struct libxl__qmp_handler {
         int minor;
         int micro;
     } version;
+
+    /* File descriptor to send to QEMU on the next command */
+    int fd_to_send;
 };
 
 static int qmp_send(libxl__qmp_handler *qmp,
@@ -378,6 +381,8 @@ static libxl__qmp_handler *qmp_init_handler(libxl__gc *gc, uint32_t domid)
 
     LIBXL_STAILQ_INIT(&qmp->callback_list);
 
+    qmp->fd_to_send = -1;
+
     return qmp;
 }
 
@@ -602,9 +607,16 @@ static int qmp_send(libxl__qmp_handler *qmp,
         goto out;
     }
 
-    if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, buf, strlen(buf),
-                            "QMP command", "QMP socket"))
-        goto out;
+    if (qmp->fd_to_send >= 0) {
+        if (libxl__sendmsg_fds(gc, qmp->qmp_fd, buf, strlen(buf),
+                               1, &qmp->fd_to_send, "QMP socket"))
+            goto out;
+        qmp->fd_to_send = -1;
+    } else {
+        if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, buf, strlen(buf),
+                                "QMP command", "QMP socket"))
+            goto out;
+    }
     if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
                             "CRLF", "QMP socket"))
         goto out;
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC 2/4] libxl: Have QEMU save its state to a file descriptor

[Anthony PERARD]

In case QEMU have restricted access to the system, open the file for it,
and QEMU will save its state to this file descritor.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 5bf5236240..7b69d6a4de 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -952,25 +952,61 @@ int libxl__qmp_system_wakeup(libxl__gc *gc, int domid)
     return qmp_run_command(gc, domid, "system_wakeup", NULL, NULL, NULL);
 }
 
+/* Find out which fdset have been allocated */
+static int qmp_fdset_add_fd_callback(libxl__qmp_handler *qmp,
+                                     const libxl__json_object *ret,
+                                     void *opaque)
+{
+    const libxl__json_object *o;
+    int fdset;
+
+    o = libxl__json_map_get("fdset-id", ret, JSON_INTEGER);
+    if (!o)
+        return 1;
+
+    fdset = libxl__json_object_get_integer(o);
+    *(int*)opaque = fdset;
+    return 0;
+}
+
 int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
 {
     libxl__json_object *args = NULL;
     libxl__qmp_handler *qmp = NULL;
     int rc;
+    int state_fd;
+    int new_fdset;
 
     qmp = libxl__qmp_initialize(gc, domid);
     if (!qmp)
         return ERROR_FAIL;
 
-    qmp_parameters_add_string(gc, &args, "filename", (char *)filename);
+    state_fd = open(filename, O_WRONLY | O_CREAT, 0600);
+    if (state_fd < 0) {
+        LOGED(ERROR, domid,
+              "Failed to open file %s for QEMU", filename);
+        goto out;
+    }
+
+    qmp->fd_to_send = state_fd;
+
+    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
+                              qmp_fdset_add_fd_callback, &new_fdset,
+                              qmp->timeout);
+    if (rc)
+        goto out;
 
     /* live parameter was added to QEMU 2.11. It signal QEMU that the save
      * operation is for a live migration rather that for taking a snapshot. */
     if (qmp_qemu_check_version(qmp, 2, 11, 0))
         qmp_parameters_add_bool(gc, &args, "live", live);
 
+    QMP_PARAMETERS_SPRINTF(&args, "filename", "/dev/fdset/%d", new_fdset);
     rc = qmp_synchronous_send(qmp, "xen-save-devices-state", args,
                               NULL, NULL, qmp->timeout);
+out:
+    if (state_fd >= 0)
+        close(state_fd);
     libxl__qmp_close(qmp);
     return rc;
 }
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC 3/4] libxl_qmp: Implement query-status command

[Anthony PERARD]

It check via QMP if QEMU as reach the intended status.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_qmp.c      | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 8dd63319fc..d5e98114d6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1856,6 +1856,9 @@ _hidden int libxl__qmp_nbd_server_stop(libxl__gc *gc, int domid);
 _hidden int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid,
                                          const char *parant,
                                          const char *child, const char *node);
+_hidden int libxl__qmp_query_status(libxl__gc *gc,
+                                    int domid,
+                                    const char *intended_status);
 /* run a hmp command in qmp mode */
 _hidden int libxl__qmp_hmp(libxl__gc *gc, int domid, const char *command_line,
                            char **out);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 7b69d6a4de..9f4ba12ff0 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1209,6 +1209,45 @@ int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid, const char *parent,
     return qmp_run_command(gc, domid, "x-blockdev-change", args, NULL, NULL);
 }
 
+static int qmp_check_status(libxl__qmp_handler *qmp,
+                            const libxl__json_object *response,
+                            void *opaque)
+{
+    char **status = opaque;
+    GC_INIT(qmp->ctx);
+    const libxl__json_object *o;
+
+    o = libxl__json_map_get("status", response, JSON_STRING);
+    if (!o)
+        return 1;
+    *status = libxl__strdup(gc, libxl__json_object_get_string(o));
+    return 0;
+}
+
+int libxl__qmp_query_status(libxl__gc *gc,
+                            int domid,
+                            const char *intended_status)
+{
+    char *status = NULL;
+    int rc;
+
+    rc = qmp_run_command(gc, domid, "query-status", NULL,
+                         qmp_check_status, &status);
+    if (rc < 0)
+        return rc;
+    if (rc == 1)
+        /* QMP command returned unexpected result */
+        return ERROR_FAIL;
+
+    LOGD(DEBUG, domid, "query-status result: %s", status);
+    if (!strcmp(intended_status, status))
+        /* success and ready */
+        return 0;
+
+    /* command success, status != intended_status */
+    return ERROR_NOT_READY;
+}
+
 static int hmp_callback(libxl__qmp_handler *qmp,
                         const libxl__json_object *response,
                         void *opaque)
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Anthony PERARD]

This path is more of a prof of concept reather than a patch as this
would break qemu-trad.

When qemu is restricted, the qemu on the receiving side cann't write
anything to xenstore once the migration is started. So it cann't tell
libxl that it is ready to continue running the guest.

For libxl, the only way to find out if qemu is ready on migrate/restore,
it is to connect to the QMP socket and run "query-status".

This patch succeed in implementing that, but QMP doesn't fit well with
the libxl__ev_* infrastructure. One main issue would be qmp_open(), it
tries to connect to the QMP socket during 5 seconds without ever giving
back the hand to libxl.

Also right now, xswait is disabled, but libxl could check both
xenstore and QMP at the same time.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c       |  3 ++
 tools/libxl/libxl_exec.c     | 95 ++++++++++++++++++++++++++++++++++++++++----
 tools/libxl/libxl_internal.h | 14 +++++++
 3 files changed, 105 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a3cddce8b7..43314e3309 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2350,6 +2350,9 @@ retry_transaction:
     spawn->failure_cb = device_model_startup_failed;
     spawn->detached_cb = device_model_detached;
 
+    // HACK, disable xenstore watch, will instead use QMP
+    spawn->xspath = NULL;
+
     rc = libxl__spawn_spawn(egc, spawn);
     if (rc < 0)
         goto out_close;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 02e6c917f0..2b5db5197a 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -274,6 +274,58 @@ void libxl__spawn_init(libxl__spawn_state *ss)
     libxl__xswait_init(&ss->xswait);
 }
 
+static void qmpwait_callback(libxl__egc *egc,
+                          libxl__ev_time *ev,
+                          const struct timeval *requested_abs,
+                          int rc)
+{
+    libxl__spawn_state *ss = CONTAINER_OF(ev, *ss, qmpwait.qmp_ev);
+    libxl__dm_spawn_state *dmss = CONTAINER_OF(ss, *dmss, spawn);
+    STATE_AO_GC(ss->ao);
+
+    if (rc == ERROR_TIMEDOUT) /* As intended */
+        rc = 0;
+    else
+        goto out_err;
+
+    rc = libxl__qmp_query_status(gc, dmss->guest_domid, "running");
+
+    if (rc) {
+        /* retry QMP connection later */
+        libxl__ev_time_register_rel(ss->ao,
+                                    ev,
+                                    qmpwait_callback,
+                                    100);
+        return;
+    }
+
+    libxl__spawn_initiate_detach(gc, ss);
+    return;
+out_err:
+    LOG(DEBUG, "qmpwait failure: %d", rc);
+    ss->failure_cb(egc, ss, rc);
+}
+
+static void qmpwait_report_error(libxl__egc *egc, libxl__qmpwait_state *qmpwa,
+                                int rc)
+{
+    libxl__spawn_state *ss = CONTAINER_OF(qmpwa, *ss, qmpwait);
+    EGC_GC;
+    libxl__ev_time_deregister(gc, &qmpwa->time_ev);
+    libxl__ev_time_deregister(gc, &qmpwa->qmp_ev);
+    qmpwa->callback(egc, &ss->xswait, rc, 0);
+}
+
+static void qmpwait_timeout_callback(libxl__egc *egc, libxl__ev_time *ev,
+                             const struct timeval *requested_abs,
+                             int rc)
+{
+    EGC_GC;
+    libxl__qmpwait_state *qmpwa = CONTAINER_OF(ev, *qmpwa, time_ev);
+    LOG(DEBUG, "%s: qmpwait timeout", qmpwa->what);
+    qmpwait_report_error(egc, qmpwa, rc);
+}
+
 int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
 {
     STATE_AO_GC(ss->ao);
@@ -284,13 +336,35 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     libxl__spawn_init(ss);
     ss->rc = ss->detaching = 0;
 
-    ss->xswait.ao = ao;
-    ss->xswait.what = GCSPRINTF("%s startup", ss->what);
-    ss->xswait.path = ss->xspath;
-    ss->xswait.timeout_ms = ss->timeout_ms;
-    ss->xswait.callback = spawn_watch_event;
-    rc = libxl__xswait_start(gc, &ss->xswait);
-    if (rc) goto out_err;
+    if (ss->xspath) {
+        ss->xswait.ao = ao;
+        ss->xswait.what = GCSPRINTF("%s startup", ss->what);
+        ss->xswait.path = ss->xspath;
+        ss->xswait.timeout_ms = ss->timeout_ms;
+        ss->xswait.callback = spawn_watch_event;
+        rc = libxl__xswait_start(gc, &ss->xswait);
+        if (rc) goto out_err;
+    } else {
+        libxl__qmpwait_state *qmpwa = &ss->qmpwait;
+
+        ss->qmpwait.ao = ao;
+        ss->qmpwait.what = GCSPRINTF("%s startup (QMP)", ss->what);
+        /*ss->qmpwait.guest_domid = ;*/
+        ss->qmpwait.timeout_ms = ss->timeout_ms;
+        ss->qmpwait.callback = spawn_watch_event;
+
+        libxl__ev_time_init(&qmpwa->time_ev);
+        libxl__ev_time_init(&qmpwa->qmp_ev);
+
+        rc = libxl__ev_time_register_rel(qmpwa->ao, &qmpwa->time_ev,
+                                         qmpwait_timeout_callback,
+                                         qmpwa->timeout_ms);
+        if (rc) goto out_err;
+
+        rc = libxl__ev_time_register_rel(ss->ao, &qmpwa->qmp_ev,
+                                         qmpwait_callback, 100);
+        if (rc) goto out_err;
+    }
 
     pid_t middle = libxl__ev_child_fork(gc, &ss->mid, spawn_middle_death);
     if (middle ==-1) { rc = ERROR_FAIL; goto out_err; }
@@ -343,10 +417,16 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     return rc;
 }
 
+static void libxl__qmpwait_stop(libxl__gc *gc, libxl__qmpwait_state *qmpwa)
+{
+    libxl__ev_time_deregister(gc, &qmpwa->time_ev);
+    libxl__ev_time_deregister(gc, &qmpwa->qmp_ev);
+}
 static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss)
 {
     assert(!libxl__ev_child_inuse(&ss->mid));
     libxl__xswait_stop(gc, &ss->xswait);
+    libxl__qmpwait_stop(gc, &ss->qmpwait);
 }
 
 static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
@@ -359,6 +439,7 @@ static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
     assert(libxl__ev_child_inuse(&ss->mid));
     assert(ss->detaching || ss->rc);
     libxl__xswait_stop(gc, &ss->xswait);
+    libxl__qmpwait_stop(gc, &ss->qmpwait);
 
     pid_t child = ss->mid.pid;
     r = kill(child, SIGKILL);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d5e98114d6..fdeeeb5f45 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1550,6 +1550,19 @@ typedef void libxl__spawn_confirm_cb(libxl__egc*, libxl__spawn_state*,
  */
 typedef void libxl__spawn_detached_cb(libxl__egc*, libxl__spawn_state*);
 
+// struct use for calling the QMP command "query-status" of a starting QEMU.
+typedef struct libxl__qmpwait_state {
+    /* caller must fill these in, and they must all remain valid */
+    libxl__ao *ao;
+    const char *what; /* for error msgs: noun phrase, what we're waiting for */
+    int guest_domid;
+    int timeout_ms; /* as for poll(2) */
+    /* remaining fields are private to qmpwait */
+    libxl__ev_time time_ev;
+    libxl__ev_time qmp_ev;
+    libxl__xswait_callback *callback;
+} libxl__qmpwait_state;
+
 struct libxl__spawn_state {
     /* must be filled in by user and remain valid */
     libxl__ao *ao;
@@ -1567,6 +1580,7 @@ struct libxl__spawn_state {
     int rc; /* might be non-0 whenever we are not Idle */
     libxl__ev_child mid; /* always in use whenever we are not Idle */
     libxl__xswait_state xswait;
+    libxl__qmpwait_state qmpwait;
 };
 
 static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 1/4] libxl: Learned to send FD through QMP to QEMU

[Ian Jackson]

(George, CC'ing you wrt your depriv doc patch - see below.)

Anthony PERARD writes ("[RFC 1/4] libxl: Learned to send FD through QMP to QEMU"):
> Adding the ability to send a file descriptor from libxl to QEMU via the
> QMP interface. This will be use with the "add-fd" QMP command.

The code looks plausible.

> +    /* File descriptor to send to QEMU on the next command */
> +    int fd_to_send;

I did wonder if this was a layering violation, or a poor API in some
other sense.  AFAICT it isn't, and libxl__qmp_handler is completely
transparent to everything in libxl_qmp.c.

I think this whole file would benefit from some doc comments about the
internal interfaces.  Particularly, something describing the boundary
between operation-specific code and the generic qmp_send machinery
would help review of both (i) new operations and (ii) extensions of
the generic machinery.

Looking at this and the next patch, I think (almost?) every user of
this new feature will need to tell qmp_send to call
qmp_fdset_add_fd_callback.  Is that right ?  Maybe this means we want
to provide a more cooked version.

Anthony PERARD writes ("[RFC 2/4] libxl: Have QEMU save its state to a file descriptor"):
> In case QEMU have restricted access to the system, open the file for it,
> and QEMU will save its state to this file descritor.

This 2nd patch looks reasonable, but it prompted to notice two new
kinds of hazard introduced by the deprivileging design goal:

>  int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
>  {
...
> +    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
> +                              qmp_fdset_add_fd_callback, &new_fdset,
> +                              qmp->timeout);
> +    if (rc)
> +        goto out;

By this point, a depriv'd qemu must be assumed to be compromised by
its guest - ie we must treat it as hostile.

This is not consistent with use of qmp_synchronous_send, because
qmp_synchronous_send will block with both the domain and ctx locks
held.  That is, a malicious qemu can deny service; it even has the
ability to prevent its serviced domain from being destroyed.

Secondly, the point about qemu now being malicious means that we need
to audit the code which handles communications with qemu for safety.

I think this means that:

 * George's todo list patch for the depriv doc should mention
   the need to replace qmp_synchronous_send with qemp_send.

 * Likewise it should mention the need for this audit.

 * We should write a comment somewhere (near the top of libxl_qmp.c
   perhaps) warning developers not to treat qemu as trusted.  That
   would usefully fit into your own series.

I volunteer to do the audit.  Some internal commentary about the
internal interfaces (as I discuss above) would be helpful for that.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Ian Jackson]

Anthony PERARD writes ("[RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> This path is more of a prof of concept reather than a patch as this
> would break qemu-trad.
...
> For libxl, the only way to find out if qemu is ready on migrate/restore,
> it is to connect to the QMP socket and run "query-status".
> 
> This patch succeed in implementing that, but QMP doesn't fit well with
> the libxl__ev_* infrastructure. One main issue would be qmp_open(), it
> tries to connect to the QMP socket during 5 seconds without ever giving
> back the hand to libxl.

There are two problems here, I think.  The first one is an internal
libxl api issue: ie, that the libxl qmp code does not have the proper
callback-style API.  That can be fixed inside libxl, although it's
probably annoying.

The second is that AFAICT there is no way other than xenstore to get a
notification of any kind when qemu is ready.  So the only possible
approach is polling.  That's pretty nasty.  I haven't looked at the
qemu code in detail to check if this is really true.  Perhaps looking
at libvirt would give us a clue...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 1/4] libxl: Learned to send FD through QMP to QEMU

[George Dunlap]

On 03/27/2018 11:29 AM, Ian Jackson wrote:
> (George, CC'ing you wrt your depriv doc patch - see below.)
> 
> Anthony PERARD writes ("[RFC 1/4] libxl: Learned to send FD through QMP to QEMU"):
>> Adding the ability to send a file descriptor from libxl to QEMU via the
>> QMP interface. This will be use with the "add-fd" QMP command.
> 
> The code looks plausible.
> 
>> +    /* File descriptor to send to QEMU on the next command */
>> +    int fd_to_send;
> 
> I did wonder if this was a layering violation, or a poor API in some
> other sense.  AFAICT it isn't, and libxl__qmp_handler is completely
> transparent to everything in libxl_qmp.c.
> 
> I think this whole file would benefit from some doc comments about the
> internal interfaces.  Particularly, something describing the boundary
> between operation-specific code and the generic qmp_send machinery
> would help review of both (i) new operations and (ii) extensions of
> the generic machinery.
> 
> Looking at this and the next patch, I think (almost?) every user of
> this new feature will need to tell qmp_send to call
> qmp_fdset_add_fd_callback.  Is that right ?  Maybe this means we want
> to provide a more cooked version.
> 
> Anthony PERARD writes ("[RFC 2/4] libxl: Have QEMU save its state to a file descriptor"):
>> In case QEMU have restricted access to the system, open the file for it,
>> and QEMU will save its state to this file descritor.
> 
> This 2nd patch looks reasonable, but it prompted to notice two new
> kinds of hazard introduced by the deprivileging design goal:
> 
>>  int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
>>  {
> ...
>> +    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
>> +                              qmp_fdset_add_fd_callback, &new_fdset,
>> +                              qmp->timeout);
>> +    if (rc)
>> +        goto out;
> 
> By this point, a depriv'd qemu must be assumed to be compromised by
> its guest - ie we must treat it as hostile.
> 
> This is not consistent with use of qmp_synchronous_send, because
> qmp_synchronous_send will block with both the domain and ctx locks
> held.  That is, a malicious qemu can deny service; it even has the
> ability to prevent its serviced domain from being destroyed.

Will qmp_synchronous_send() wait forever, or is there a timeout?

In any case, we certainly do need to remember to treat QEMU as hostile
and audit the interactions with it.  This will help the stubdom case as
well.

I'll add it to the list.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Anthony PERARD]

On Tue, Mar 27, 2018 at 11:36:00AM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> > This path is more of a prof of concept reather than a patch as this
> > would break qemu-trad.
> ...
> > For libxl, the only way to find out if qemu is ready on migrate/restore,
> > it is to connect to the QMP socket and run "query-status".
> > 
> > This patch succeed in implementing that, but QMP doesn't fit well with
> > the libxl__ev_* infrastructure. One main issue would be qmp_open(), it
> > tries to connect to the QMP socket during 5 seconds without ever giving
> > back the hand to libxl.
> 
> There are two problems here, I think.  The first one is an internal
> libxl api issue: ie, that the libxl qmp code does not have the proper
> callback-style API.  That can be fixed inside libxl, although it's
> probably annoying.
> 
> The second is that AFAICT there is no way other than xenstore to get a
> notification of any kind when qemu is ready.  So the only possible
> approach is polling.  That's pretty nasty.  I haven't looked at the
> qemu code in detail to check if this is really true.  Perhaps looking
> at libvirt would give us a clue...

Actually, I think that once connected to QMP, we can get "events" from
QEMU. And there is an event for when qemu changes it's status.

So I guess one workflow we could adopt is:
- try connect to QMP socket
- "query-status", to check current status
- keep listening for events.

I'll try to find out if libvirt is doing anything for that.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 1/4] libxl: Learned to send FD through QMP to QEMU

[Anthony PERARD]

On Tue, Mar 27, 2018 at 11:58:45AM +0100, George Dunlap wrote:
> On 03/27/2018 11:29 AM, Ian Jackson wrote:
> > This 2nd patch looks reasonable, but it prompted to notice two new
> > kinds of hazard introduced by the deprivileging design goal:
> > 
> >>  int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
> >>  {
> > ...
> >> +    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
> >> +                              qmp_fdset_add_fd_callback, &new_fdset,
> >> +                              qmp->timeout);
> >> +    if (rc)
> >> +        goto out;
> > 
> > By this point, a depriv'd qemu must be assumed to be compromised by
> > its guest - ie we must treat it as hostile.
> > 
> > This is not consistent with use of qmp_synchronous_send, because
> > qmp_synchronous_send will block with both the domain and ctx locks
> > held.  That is, a malicious qemu can deny service; it even has the
> > ability to prevent its serviced domain from being destroyed.
> 
> Will qmp_synchronous_send() wait forever, or is there a timeout?

There is some kind of timeout, but I'm not sure it is true at all time.

This is a few functions that does handle connection/send/receive:
- qmp_open()
  this one as a 5s timeout on connecting to the socket.
- qmp_send()
  This use write/sendmsg with no timeout, but the fd is set to
  O_NONBLOCK.
- qmp_next()
  This function use select with a 5s timeout, so read should not block.
  But I think the timout is reset every time something have been read
  from the socket.

So I guess a malicious qemu could have the qmp_next() function wait
forever.

Also I think every time a "response" or an "event" is processed,
qmp_next() will return, and qmp_synchronous_send() will call qmp_next
again until it got the response it is waiting for.

So a few opportunity to wait forever.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Ian Jackson]

Anthony PERARD writes ("Re: [Xen-devel] [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> Actually, I think that once connected to QMP, we can get "events" from
> QEMU. And there is an event for when qemu changes it's status.

Right, but doesn't one still have to poll on connect() ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Anthony PERARD]

On Tue, Mar 27, 2018 at 02:43:22PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [Xen-devel] [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> > Actually, I think that once connected to QMP, we can get "events" from
> > QEMU. And there is an event for when qemu changes it's status.
> 
> Right, but doesn't one still have to poll on connect() ?

Yes, we need to poll on connect().

libvirt those it also to open the socket:
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor.c;h=e169553b7e1781da307ddb0be23fed5540baf36c;hb=HEAD#l375

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Ian Jackson]

Anthony PERARD writes ("Re: [Xen-devel] [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> On Tue, Mar 27, 2018 at 02:43:22PM +0100, Ian Jackson wrote:
> > Right, but doesn't one still have to poll on connect() ?
> 
> Yes, we need to poll on connect().
> 
> libvirt those it also to open the socket:
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor.c;h=e169553b7e1781da307ddb0be23fed5540baf36c;hb=HEAD#l375

Urgh.  Maybe we could fix this in qemu, or do some kind of hack.

For example, maybe we could instruct qemu to open some file or other,
which is actually a pipe.  If it would happen late enought (after the
qmp socket is set up).

Or we could add something to the Xen machine, eg which listens to an
env var and closes an fd.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore

[Anthony PERARD]

On Tue, Mar 27, 2018 at 03:48:29PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [Xen-devel] [RFC 4/4] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> > On Tue, Mar 27, 2018 at 02:43:22PM +0100, Ian Jackson wrote:
> > > Right, but doesn't one still have to poll on connect() ?
> > 
> > Yes, we need to poll on connect().
> > 
> > libvirt those it also to open the socket:
> > https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor.c;h=e169553b7e1781da307ddb0be23fed5540baf36c;hb=HEAD#l375
> 
> Urgh.  Maybe we could fix this in qemu, or do some kind of hack.
> 
> For example, maybe we could instruct qemu to open some file or other,
> which is actually a pipe.  If it would happen late enought (after the
> qmp socket is set up).

I think that can be done.

If we add something like:
'-chardev','pipe,id=inotify,path=/tmp/pipe',
'-mon','chardev=inotify,mode=control',

And we just needs to wait until qemu write the QMP greating into the
pipe. At this point, the socket we cared for exist.

That /tmp/pipe can actually be a fd, I did not test it, only read the
code:
'-add-fd','fd=fd,set=1',
'-chardev','pipe,id=inotify,path=/dev/fdset/1',

I just need to write some code in libxl now.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC 1/4] libxl: Learned to send FD through QMP to QEMU

[Anthony PERARD]

On Tue, Mar 27, 2018 at 11:29:35AM +0100, Ian Jackson wrote:
> (George, CC'ing you wrt your depriv doc patch - see below.)
> 
> Anthony PERARD writes ("[RFC 1/4] libxl: Learned to send FD through QMP to QEMU"):
> > +    /* File descriptor to send to QEMU on the next command */
> > +    int fd_to_send;
> 
> I did wonder if this was a layering violation, or a poor API in some
> other sense.  AFAICT it isn't, and libxl__qmp_handler is completely
> transparent to everything in libxl_qmp.c.
> 
> I think this whole file would benefit from some doc comments about the
> internal interfaces.  Particularly, something describing the boundary
> between operation-specific code and the generic qmp_send machinery
> would help review of both (i) new operations and (ii) extensions of
> the generic machinery.

I'll try to write some documentation.

> Looking at this and the next patch, I think (almost?) every user of
> this new feature will need to tell qmp_send to call
> qmp_fdset_add_fd_callback.  Is that right ?  Maybe this means we want
> to provide a more cooked version.

Not exactly. We can add a parameter to "add-fd" to use a specific
"fdset-id".

> Anthony PERARD writes ("[RFC 2/4] libxl: Have QEMU save its state to a file descriptor"):
> > In case QEMU have restricted access to the system, open the file for it,
> > and QEMU will save its state to this file descritor.
> 
> This 2nd patch looks reasonable, but it prompted to notice two new
> kinds of hazard introduced by the deprivileging design goal:
> 
> >  int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
> >  {
> ...
> > +    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
> > +                              qmp_fdset_add_fd_callback, &new_fdset,
> > +                              qmp->timeout);
> > +    if (rc)
> > +        goto out;
> 
> By this point, a depriv'd qemu must be assumed to be compromised by
> its guest - ie we must treat it as hostile.
> 
> This is not consistent with use of qmp_synchronous_send, because
> qmp_synchronous_send will block with both the domain and ctx locks
> held.  That is, a malicious qemu can deny service; it even has the
> ability to prevent its serviced domain from being destroyed.
> 
> Secondly, the point about qemu now being malicious means that we need
> to audit the code which handles communications with qemu for safety.
> 
> I think this means that:
> 
>  * George's todo list patch for the depriv doc should mention
>    the need to replace qmp_synchronous_send with qemp_send.
> 
>  * Likewise it should mention the need for this audit.
> 
>  * We should write a comment somewhere (near the top of libxl_qmp.c
>    perhaps) warning developers not to treat qemu as trusted.  That
>    would usefully fit into your own series.
> 
> I volunteer to do the audit.  Some internal commentary about the
> internal interfaces (as I discuss above) would be helpful for that.

I think we would need to rewrite part of libxl_qmp.c to use the
libxl__ev_*, so QEMU would not be able to block libxl.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel