[v7,06/21] multi-process: define MPQemuMsg format and transmission functions

Commit Message

Elena Ufimtseva June 27, 2020, 5:09 p.m. UTC

From: Elena Ufimtseva <[e-mail]>

Defines MPQemuMsg, which is the message that is sent to the remote
process. This message is sent over QIOChannel and is used to
command the remote process to perform various tasks.

Also defined the helper functions to send and receive messages over the
QIOChannel

Signed-off-by: Jagannathan Raman <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
Signed-off-by: Elena Ufimtseva <[e-mail]>
---
 MAINTAINERS              |   2 +
 include/io/mpqemu-link.h |  75 +++++++++++++++++++
 io/Makefile.objs         |   2 +
 io/mpqemu-link.c         | 151 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 include/io/mpqemu-link.h
 create mode 100644 io/mpqemu-link.c

Comments

Stefan Hajnoczi June 30, 2020, 3:53 p.m. UTC | #1

On Sat, Jun 27, 2020 at 10:09:28AM -0700, [e-mail] wrote:
> +void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc)
> +{
> +    Error *local_err = NULL;
> +    struct iovec send[2];
> +    int *fds = NULL;
> +    size_t nfds = 0;
> +
> +    send[0].iov_base = msg;
> +    send[0].iov_len = MPQEMU_MSG_HDR_SIZE;
> +
> +    send[1].iov_base = msg->bytestream ? msg->data2 : (void *)&msg->data1;
> +    send[1].iov_len = msg->size;
> +
> +    if (msg->num_fds) {
> +        nfds = msg->num_fds;
> +        fds = msg->fds;
> +    }
> +
> +    (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds,
> +                                      &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }

Error propagation is missing. Is this by design, i.e. errors only need
to be handled in the read path, or is the patch incomplete?

> +}
> +
> +static int mpqemu_readv(QIOChannel *ioc, struct iovec *iov, int **fds,
> +                        size_t *nfds, Error **errp)
> +{
> +    size_t size, len;
> +
> +    size = iov->iov_len;
> +
> +    while (size > 0) {
> +        len = qio_channel_readv_full(ioc, iov, 1, fds, nfds, errp);

iovec buffers are overwritten when this loop iterates because iov is
not updated after reading some data.

fds is leaked when this loop iterates multiple times and there are fds
each time. fds/nfds should probably be set to 0 after the first
iteration.

> +
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            if (qemu_in_coroutine()) {
> +                qio_channel_yield(ioc, G_IO_IN);
> +            } else {
> +                qio_channel_wait(ioc, G_IO_IN);
> +            }
> +            continue;
> +        }
> +
> +        if (len <= 0) {
> +            return -EIO;
> +        }
> +
> +        size -= len;
> +    }
> +
> +    return iov->iov_len;
> +}
> +
> +int mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc)
> +{
> +    Error *local_err = NULL;
> +    int *fds = NULL;
> +    struct iovec hdr, data;
> +    size_t nfds = 0;
> +
> +    hdr.iov_base = g_malloc0(MPQEMU_MSG_HDR_SIZE);
> +    hdr.iov_len = MPQEMU_MSG_HDR_SIZE;
> +
> +    if (mpqemu_readv(ioc, &hdr, &fds, &nfds, &local_err) < 0) {
> +        return -EIO;

hdr.iov_base is leaked.

Why is hdr.iov_base allocated on the heap? Can you read directly into
msg instead of using an extra variable?

> +    }
> +
> +    memcpy(msg, hdr.iov_base, hdr.iov_len);
> +
> +    free(hdr.iov_base);
> +    if (msg->size > MPQEMU_MSG_DATA_MAX) {
> +        error_report("The message size is more than MPQEMU_MSG_DATA_MAX %d",
> +                     MPQEMU_MSG_DATA_MAX);
> +        return -EINVAL;
> +    }
> +
> +    data.iov_base = g_malloc0(msg->size);
> +    data.iov_len = msg->size;
> +
> +    if (mpqemu_readv(ioc, &data, NULL, NULL, &local_err) < 0) {
> +        return -EIO;

data.iov_base is leaked.

> +    }
> +
> +    if (msg->bytestream) {
> +        msg->data2 = calloc(1, msg->size);

QEMU uses glib memory allocation functions when possible. Please use
g_malloc0().

The calloc() and memcpy() can be avoided like this:

  msg->data2 = data.iov_base;
  data.iov_base = NULL; /* the pointer has been moved, don't free it */

> +        memcpy(msg->data2, data.iov_base, msg->size);
> +    } else {
> +        memcpy((void *)&msg->data1, data.iov_base, msg->size);

Buffer overflow when msg->size > sizeof(msg->data1). There should be a
check:

  if (msg->bytestream) {
      ...
  } else {
      if (msg->size > sizeof(msg->data1)) {
          ...handle invalid size...
	  return -EIO;
      }
      memcpy(&msg->data1, data.iov_base, msg->size);
  }

> +    }
> +
> +    free(data.iov_base);

Should be g_free() since the allocation was made with g_malloc0().

> +
> +    if (nfds) {
> +        msg->num_fds = nfds;
> +        memcpy(msg->fds, fds, nfds * sizeof(int));
> +    }

It's safer to move the msg->num_fds = nfds outside the if statement so
that it always happens. That way num_fds is set to 0 and we don't depend
on the caller initializing it to 0.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 83aae5b441..50a5fc53d6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2947,6 +2947,8 @@  F: hw/pci-host/remote.c
 F: include/hw/pci-host/remote.h
 F: hw/i386/remote.c
 F: include/hw/i386/remote.h
+F: io/mpqemu-link.c
+F: include/io/mpqemu-link.h
 
 Build and test automation
 -------------------------
diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
new file mode 100644
index 0000000000..1542e8ed07
--- /dev/null
+++ b/include/io/mpqemu-link.h
@@ -0,0 +1,75 @@ 
+/*
+ * Communication channel between QEMU and remote device process
+ *
+ * Copyright © 2018, 2020 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef MPQEMU_LINK_H
+#define MPQEMU_LINK_H
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+
+#include "qom/object.h"
+#include "qemu/thread.h"
+#include "io/channel.h"
+
+#define REMOTE_MAX_FDS 8
+
+#define MPQEMU_MSG_HDR_SIZE offsetof(MPQemuMsg, data1.u64)
+
+/**
+ * MPQemuCmd:
+ *
+ * MPQemuCmd enum type to specify the command to be executed on the remote
+ * device.
+ */
+typedef enum {
+    INIT = 0,
+    MAX = INT_MAX,
+} MPQemuCmd;
+
+/**
+ * Maximum size of data2 field in the message to be transmitted.
+ */
+#define MPQEMU_MSG_DATA_MAX 256
+
+/**
+ * MPQemuMsg:
+ * @cmd: The remote command
+ * @bytestream: Indicates if the data to be shared is structured (data1)
+ *              or unstructured (data2)
+ * @size: Size of the data to be shared
+ * @data1: Structured data
+ * @fds: File descriptors to be shared with remote device
+ * @data2: Unstructured data
+ *
+ * MPQemuMsg Format of the message sent to the remote device from QEMU.
+ *
+ */
+typedef struct {
+    int cmd;
+    int bytestream;
+    size_t size;
+
+    union {
+        uint64_t u64;
+    } data1;
+
+    int fds[REMOTE_MAX_FDS];
+    int num_fds;
+
+    /* Max size of data2 is MPQEMU_MSG_DATA_MAX */
+    uint8_t *data2;
+} MPQemuMsg;
+
+void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc);
+int mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc);
+
+bool mpqemu_msg_valid(MPQemuMsg *msg);
+
+#endif
diff --git a/io/Makefile.objs b/io/Makefile.objs
index 9a20fce4ed..5875ab0697 100644
--- a/io/Makefile.objs
+++ b/io/Makefile.objs
@@ -10,3 +10,5 @@  io-obj-y += channel-util.o
 io-obj-y += dns-resolver.o
 io-obj-y += net-listener.o
 io-obj-y += task.o
+
+io-obj-$(CONFIG_MPQEMU) += mpqemu-link.o
diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
new file mode 100644
index 0000000000..bfc542b5fd
--- /dev/null
+++ b/io/mpqemu-link.c
@@ -0,0 +1,151 @@ 
+/*
+ * Communication channel between QEMU and remote device process
+ *
+ * Copyright © 2018, 2020 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+
+#include "qemu/module.h"
+#include "io/mpqemu-link.h"
+#include "qapi/error.h"
+#include "qemu/iov.h"
+#include "qemu/error-report.h"
+
+void mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc)
+{
+    Error *local_err = NULL;
+    struct iovec send[2];
+    int *fds = NULL;
+    size_t nfds = 0;
+
+    send[0].iov_base = msg;
+    send[0].iov_len = MPQEMU_MSG_HDR_SIZE;
+
+    send[1].iov_base = msg->bytestream ? msg->data2 : (void *)&msg->data1;
+    send[1].iov_len = msg->size;
+
+    if (msg->num_fds) {
+        nfds = msg->num_fds;
+        fds = msg->fds;
+    }
+
+    (void)qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send), fds, nfds,
+                                      &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
+static int mpqemu_readv(QIOChannel *ioc, struct iovec *iov, int **fds,
+                        size_t *nfds, Error **errp)
+{
+    size_t size, len;
+
+    size = iov->iov_len;
+
+    while (size > 0) {
+        len = qio_channel_readv_full(ioc, iov, 1, fds, nfds, errp);
+
+        if (len == QIO_CHANNEL_ERR_BLOCK) {
+            if (qemu_in_coroutine()) {
+                qio_channel_yield(ioc, G_IO_IN);
+            } else {
+                qio_channel_wait(ioc, G_IO_IN);
+            }
+            continue;
+        }
+
+        if (len <= 0) {
+            return -EIO;
+        }
+
+        size -= len;
+    }
+
+    return iov->iov_len;
+}
+
+int mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc)
+{
+    Error *local_err = NULL;
+    int *fds = NULL;
+    struct iovec hdr, data;
+    size_t nfds = 0;
+
+    hdr.iov_base = g_malloc0(MPQEMU_MSG_HDR_SIZE);
+    hdr.iov_len = MPQEMU_MSG_HDR_SIZE;
+
+    if (mpqemu_readv(ioc, &hdr, &fds, &nfds, &local_err) < 0) {
+        return -EIO;
+    }
+
+    memcpy(msg, hdr.iov_base, hdr.iov_len);
+
+    free(hdr.iov_base);
+    if (msg->size > MPQEMU_MSG_DATA_MAX) {
+        error_report("The message size is more than MPQEMU_MSG_DATA_MAX %d",
+                     MPQEMU_MSG_DATA_MAX);
+        return -EINVAL;
+    }
+
+    data.iov_base = g_malloc0(msg->size);
+    data.iov_len = msg->size;
+
+    if (mpqemu_readv(ioc, &data, NULL, NULL, &local_err) < 0) {
+        return -EIO;
+    }
+
+    if (msg->bytestream) {
+        msg->data2 = calloc(1, msg->size);
+        memcpy(msg->data2, data.iov_base, msg->size);
+    } else {
+        memcpy((void *)&msg->data1, data.iov_base, msg->size);
+    }
+
+    free(data.iov_base);
+
+    if (nfds) {
+        msg->num_fds = nfds;
+        memcpy(msg->fds, fds, nfds * sizeof(int));
+    }
+
+    return 0;
+}
+
+bool mpqemu_msg_valid(MPQemuMsg *msg)
+{
+    if (msg->cmd >= MAX && msg->cmd < 0) {
+        return false;
+    }
+
+    if (msg->bytestream) {
+        if (!msg->data2) {
+            return false;
+        }
+    } else {
+        if (msg->data2) {
+            return false;
+        }
+    }
+
+    /* Verify FDs. */
+    if (msg->num_fds >= REMOTE_MAX_FDS) {
+        return false;
+    }
+
+    if (msg->num_fds > 0) {
+        for (int i = 0; i < msg->num_fds; i++) {
+            if (fcntl(msg->fds[i], F_GETFL) == -1) {
+                return false;
+            }
+        }
+    }
+
+    return true;
+}