Create a vhost-user-backend object that holds a connection to a
vhost-user backend and can be referenced from virtio devices that
support it. See later patches for input & gpu usage.

A chardev can be specified to communicate with the vhost-user backend,
ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
vhost-user-backend,id=vuid,chardev=char0.

Alternatively, an executable with its arguments may be given as 'cmd'
property, ex: -object
vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
executable is then spawn and, by convention, the vhost-user socket is
passed as fd=3. It may be considered a security breach to allow
creating processes that may execute arbitrary executables, so this may
be restricted to some known executables (via signature etc) or
directory.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/vhost-user-backend.h |  58 +++++
 backends/vhost-user.c               | 330 ++++++++++++++++++++++++++++
 vl.c                                |   3 +-
 backends/Makefile.objs              |   4 +
 4 files changed, 394 insertions(+), 1 deletion(-)
 create mode 100644 include/sysemu/vhost-user-backend.h
 create mode 100644 backends/vhost-user.c

diff --git a/include/sysemu/vhost-user-backend.h b/include/sysemu/vhost-user-backend.h
new file mode 100644
index 0000000000..890db176fd
--- /dev/null
+++ b/include/sysemu/vhost-user-backend.h
@@ -0,0 +1,58 @@
+/*
+ * QEMU vhost-user backend
+ *
+ * Copyright (C) 2018 Red Hat Inc
+ *
+ * Authors:
+ *  Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * 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 QEMU_VHOST_USER_BACKEND_H
+#define QEMU_VHOST_USER_BACKEND_H
+
+#include "qom/object.h"
+#include "exec/memory.h"
+#include "qemu/option.h"
+#include "qemu/bitmap.h"
+#include "hw/virtio/vhost.h"
+#include "chardev/char-fe.h"
+#include "io/channel.h"
+
+#define TYPE_VHOST_USER_BACKEND "vhost-user-backend"
+#define VHOST_USER_BACKEND(obj) \
+    OBJECT_CHECK(VhostUserBackend, (obj), TYPE_VHOST_USER_BACKEND)
+#define VHOST_USER_BACKEND_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(VhostUserBackendClass, (obj), TYPE_VHOST_USER_BACKEND)
+#define VHOST_USER_BACKEND_CLASS(klass) \
+    OBJECT_CLASS_CHECK(VhostUserBackendClass, (klass), TYPE_VHOST_USER_BACKEND)
+
+typedef struct VhostUserBackend VhostUserBackend;
+typedef struct VhostUserBackendClass VhostUserBackendClass;
+
+struct VhostUserBackendClass {
+    ObjectClass parent_class;
+};
+
+struct VhostUserBackend {
+    /* private */
+    Object parent;
+
+    char *cmd;
+    char *chr_name;
+
+    CharBackend chr;
+    struct vhost_dev dev;
+    QIOChannel *child;
+    VirtIODevice *vdev;
+    bool started;
+    bool completed;
+};
+
+int vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
+                                unsigned nvqs, Error **errp);
+void vhost_user_backend_start(VhostUserBackend *b);
+void vhost_user_backend_stop(VhostUserBackend *b);
+
+#endif
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
new file mode 100644
index 0000000000..ae40dee373
--- /dev/null
+++ b/backends/vhost-user.c
@@ -0,0 +1,330 @@
+/*
+ * QEMU vhost-user backend
+ *
+ * Copyright (C) 2018 Red Hat Inc
+ *
+ * Authors:
+ *  Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * 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 "hw/qdev.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/error-report.h"
+#include "qom/object_interfaces.h"
+#include "sysemu/vhost-user-backend.h"
+#include "sysemu/kvm.h"
+#include "io/channel-command.h"
+#include "hw/virtio/virtio-bus.h"
+
+static bool
+ioeventfd_enabled(void)
+{
+    return kvm_enabled() && kvm_eventfds_enabled();
+}
+
+int
+vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
+                            unsigned nvqs, Error **errp)
+{
+    int ret;
+
+    assert(!b->vdev && vdev);
+
+    if (!ioeventfd_enabled()) {
+        error_setg(errp, "vhost initialization failed: requires kvm");
+        return -1;
+    }
+
+    b->vdev = vdev;
+    b->dev.nvqs = nvqs;
+    b->dev.vqs = g_new(struct vhost_virtqueue, nvqs);
+
+    ret = vhost_dev_init(&b->dev, &b->chr, VHOST_BACKEND_TYPE_USER, 0);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "vhost initialization failed");
+        return -1;
+    }
+
+    return 0;
+}
+
+void
+vhost_user_backend_start(VhostUserBackend *b)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    int ret, i ;
+
+    if (b->started) {
+        return;
+    }
+
+    if (!k->set_guest_notifiers) {
+        error_report("binding does not support guest notifiers");
+        return;
+    }
+
+    ret = vhost_dev_enable_notifiers(&b->dev, b->vdev);
+    if (ret < 0) {
+        return;
+    }
+
+    ret = k->set_guest_notifiers(qbus->parent, b->dev.nvqs, true);
+    if (ret < 0) {
+        error_report("Error binding guest notifier");
+        goto err_host_notifiers;
+    }
+
+    b->dev.acked_features = b->vdev->guest_features;
+    ret = vhost_dev_start(&b->dev, b->vdev);
+    if (ret < 0) {
+        error_report("Error start vhost dev");
+        goto err_guest_notifiers;
+    }
+
+    /* guest_notifier_mask/pending not used yet, so just unmask
+     * everything here.  virtio-pci will do the right thing by
+     * enabling/disabling irqfd.
+     */
+    for (i = 0; i < b->dev.nvqs; i++) {
+        vhost_virtqueue_mask(&b->dev, b->vdev,
+                             b->dev.vq_index + i, false);
+    }
+
+    b->started = true;
+    return;
+
+err_guest_notifiers:
+    k->set_guest_notifiers(qbus->parent, b->dev.nvqs, false);
+err_host_notifiers:
+    vhost_dev_disable_notifiers(&b->dev, b->vdev);
+}
+
+void
+vhost_user_backend_stop(VhostUserBackend *b)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    int ret = 0;
+
+    if (!b->started) {
+        return;
+    }
+
+    vhost_dev_stop(&b->dev, b->vdev);
+
+    if (k->set_guest_notifiers) {
+        ret = k->set_guest_notifiers(qbus->parent,
+                                     b->dev.nvqs, false);
+        if (ret < 0) {
+            error_report("vhost guest notifier cleanup failed: %d", ret);
+        }
+    }
+    assert(ret >= 0);
+
+    vhost_dev_disable_notifiers(&b->dev, b->vdev);
+    b->started = false;
+}
+
+static int
+vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error **errp)
+{
+    int devnull = open("/dev/null", O_RDWR);
+    pid_t pid;
+
+    assert(!b->child);
+
+    if (!b->cmd) {
+        error_setg_errno(errp, errno, "Missing cmd property");
+        return -1;
+    }
+    if (devnull < 0) {
+        error_setg_errno(errp, errno, "Unable to open /dev/null");
+        return -1;
+    }
+
+    pid = qemu_fork(errp);
+    if (pid < 0) {
+        close(devnull);
+        return -1;
+    }
+
+    if (pid == 0) { /* child */
+        int fd, maxfd = sysconf(_SC_OPEN_MAX);
+
+        dup2(devnull, STDIN_FILENO);
+        dup2(devnull, STDOUT_FILENO);
+        dup2(vhostfd, 3);
+
+        signal(SIGINT, SIG_IGN);
+
+        for (fd = 4; fd < maxfd; fd++) {
+            close(fd);
+        }
+
+        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
+        _exit(1);
+    }
+
+    b->child = QIO_CHANNEL(qio_channel_command_new_pid(devnull, devnull, pid));
+
+    return 0;
+}
+
+static void
+vhost_user_backend_complete(UserCreatable *uc, Error **errp)
+{
+    VhostUserBackend *b = VHOST_USER_BACKEND(uc);
+    Chardev *chr;
+
+    if (!!b->chr_name + !!b->cmd != 1) {
+        error_setg(errp, "You may specificy only one of 'chardev' or 'cmd'.");
+        return;
+    }
+
+    if (b->chr_name) {
+        chr = qemu_chr_find(b->chr_name);
+        if (chr == NULL) {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Chardev '%s' not found", b->chr_name);
+            return;
+        }
+
+        if (!qemu_chr_fe_init(&b->chr, chr, errp)) {
+            return;
+        }
+    } else {
+        int sv[2];
+
+        if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
+            error_setg_errno(errp, errno, "socketpair() failed");
+            return;
+        }
+
+        chr = CHARDEV(object_new(TYPE_CHARDEV_SOCKET));
+        if (!chr || qemu_chr_add_client(chr, sv[0]) == -1) {
+            error_setg(errp, "Failed to make socket chardev");
+            object_unref(OBJECT(chr));
+            return;
+        }
+
+        if (!qemu_chr_fe_init(&b->chr, chr, errp)) {
+            return;
+        }
+
+        vhost_user_backend_spawn_cmd(b, sv[1], errp);
+
+        close(sv[1]);
+    }
+
+    b->completed = true;
+    /* could vhost_dev_init() happen here, so early vhost-user message
+     * can be exchanged */
+}
+
+static char *get_cmd(Object *obj, Error **errp)
+{
+    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
+
+    return g_strdup(b->cmd);
+}
+
+static void set_cmd(Object *obj, const char *str, Error **errp)
+{
+    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
+
+    if (b->child) {
+        error_setg(errp, "cannot change property value");
+        return;
+    }
+
+    g_free(b->cmd);
+    b->cmd = g_strdup(str);
+}
+
+static void set_chardev(Object *obj, const char *value, Error **errp)
+{
+    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
+
+    if (b->completed) {
+        error_setg(errp, QERR_PERMISSION_DENIED);
+    } else {
+        g_free(b->chr_name);
+        b->chr_name = g_strdup(value);
+    }
+}
+
+static char *get_chardev(Object *obj, Error **errp)
+{
+    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
+    Chardev *chr = qemu_chr_fe_get_driver(&b->chr);
+
+    if (chr && chr->label) {
+        return g_strdup(chr->label);
+    }
+
+    return NULL;
+}
+
+static void vhost_user_backend_init(Object *obj)
+{
+    object_property_add_str(obj, "cmd", get_cmd, set_cmd, NULL);
+    object_property_add_str(obj, "chardev", get_chardev, set_chardev, NULL);
+}
+
+static void vhost_user_backend_finalize(Object *obj)
+{
+    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
+
+    g_free(b->dev.vqs);
+    g_free(b->cmd);
+    g_free(b->chr_name);
+
+    qemu_chr_fe_deinit(&b->chr, true);
+
+    if (b->child) {
+        object_unref(OBJECT(b->child));
+    }
+}
+
+static bool
+vhost_user_backend_can_be_deleted(UserCreatable *uc)
+{
+    return true;
+}
+
+static void
+vhost_user_backend_class_init(ObjectClass *oc, void *data)
+{
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+    ucc->complete = vhost_user_backend_complete;
+    ucc->can_be_deleted = vhost_user_backend_can_be_deleted;
+}
+
+static const TypeInfo vhost_user_backend_info = {
+    .name = TYPE_VHOST_USER_BACKEND,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(VhostUserBackend),
+    .instance_init = vhost_user_backend_init,
+    .instance_finalize = vhost_user_backend_finalize,
+    .class_size = sizeof(VhostUserBackendClass),
+    .class_init = vhost_user_backend_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void register_types(void)
+{
+    type_register_static(&vhost_user_backend_info);
+}
+
+type_init(register_types);
diff --git a/vl.c b/vl.c
index c4fe25560c..4ed094dd71 100644
--- a/vl.c
+++ b/vl.c
@@ -2833,7 +2833,8 @@ static bool object_create_initial(const char *type)
     }
 
 #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
-    if (g_str_equal(type, "cryptodev-vhost-user")) {
+    if (g_str_equal(type, "cryptodev-vhost-user") ||
+        g_str_equal(type, "vhost-user-backend")) {
         return false;
     }
 #endif
diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index ad7c0325ed..bac27de11b 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -16,3 +16,7 @@ common-obj-$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX)) += \
 endif
 
 common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
+
+ifdef CONFIG_LINUX
+common-obj-$(CONFIG_VIRTIO) += vhost-user.o
+endif
-- 
2.17.1.906.g10fd178552

On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
> Create a vhost-user-backend object that holds a connection to a
> vhost-user backend and can be referenced from virtio devices that
> support it. See later patches for input & gpu usage.
> 
> A chardev can be specified to communicate with the vhost-user backend,
> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
> vhost-user-backend,id=vuid,chardev=char0.
> 
> Alternatively, an executable with its arguments may be given as 'cmd'
> property, ex: -object
> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
> executable is then spawn and, by convention, the vhost-user socket is
> passed as fd=3. It may be considered a security breach to allow
> creating processes that may execute arbitrary executables, so this may
> be restricted to some known executables (via signature etc) or
> directory.

Passing a binary and args as a string blob.....

> +static int
> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error **errp)
> +{
> +    int devnull = open("/dev/null", O_RDWR);
> +    pid_t pid;
> +
> +    assert(!b->child);
> +
> +    if (!b->cmd) {
> +        error_setg_errno(errp, errno, "Missing cmd property");
> +        return -1;
> +    }
> +    if (devnull < 0) {
> +        error_setg_errno(errp, errno, "Unable to open /dev/null");
> +        return -1;
> +    }
> +
> +    pid = qemu_fork(errp);
> +    if (pid < 0) {
> +        close(devnull);
> +        return -1;
> +    }
> +
> +    if (pid == 0) { /* child */
> +        int fd, maxfd = sysconf(_SC_OPEN_MAX);
> +
> +        dup2(devnull, STDIN_FILENO);
> +        dup2(devnull, STDOUT_FILENO);
> +        dup2(vhostfd, 3);
> +
> +        signal(SIGINT, SIG_IGN);

Why ignore SIGINT ?  Surely we want this extra process to be killed
someone ctrl-c's the parent QEMU.

> +
> +        for (fd = 4; fd < maxfd; fd++) {
> +            close(fd);
> +        }
> +
> +        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);

...which is then interpreted by the shell is a recipe for security
flaws. There needs to be a way to pass the command + arguments
to QEMU as an argv[] we can directly exec without involving the
shell.

> +        _exit(1);
> +    }
> +
> +    b->child = QIO_CHANNEL(qio_channel_command_new_pid(devnull, devnull, pid));

Overall this method overall duplicates much of the
qio_channel_command_new_argv(), though you do have a few differences.

I'd prefer if we could make qio_channel_command_new_argv more flexible to
handle these extra needs though.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi

On Mon, Jun 4, 2018 at 11:36 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
>> Create a vhost-user-backend object that holds a connection to a
>> vhost-user backend and can be referenced from virtio devices that
>> support it. See later patches for input & gpu usage.
>>
>> A chardev can be specified to communicate with the vhost-user backend,
>> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
>> vhost-user-backend,id=vuid,chardev=char0.
>>
>> Alternatively, an executable with its arguments may be given as 'cmd'
>> property, ex: -object
>> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
>> executable is then spawn and, by convention, the vhost-user socket is
>> passed as fd=3. It may be considered a security breach to allow
>> creating processes that may execute arbitrary executables, so this may
>> be restricted to some known executables (via signature etc) or
>> directory.
>
> Passing a binary and args as a string blob.....
>
>> +static int
>> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error **errp)
>> +{
>> +    int devnull = open("/dev/null", O_RDWR);
>> +    pid_t pid;
>> +
>> +    assert(!b->child);
>> +
>> +    if (!b->cmd) {
>> +        error_setg_errno(errp, errno, "Missing cmd property");
>> +        return -1;
>> +    }
>> +    if (devnull < 0) {
>> +        error_setg_errno(errp, errno, "Unable to open /dev/null");
>> +        return -1;
>> +    }
>> +
>> +    pid = qemu_fork(errp);
>> +    if (pid < 0) {
>> +        close(devnull);
>> +        return -1;
>> +    }
>> +
>> +    if (pid == 0) { /* child */
>> +        int fd, maxfd = sysconf(_SC_OPEN_MAX);
>> +
>> +        dup2(devnull, STDIN_FILENO);
>> +        dup2(devnull, STDOUT_FILENO);
>> +        dup2(vhostfd, 3);
>> +
>> +        signal(SIGINT, SIG_IGN);
>
> Why ignore SIGINT ?  Surely we want this extra process to be killed
> someone ctrl-c's the parent QEMU.

leftover, removed

>
>> +
>> +        for (fd = 4; fd < maxfd; fd++) {
>> +            close(fd);
>> +        }
>> +
>> +        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
>
> ...which is then interpreted by the shell is a recipe for security
> flaws. There needs to be a way to pass the command + arguments
> to QEMU as an argv[] we can directly exec without involving the
> shell.
>

For now, I use g_shell_parse_argv(). Do you have a better idea?

>> +        _exit(1);
>> +    }
>> +
>> +    b->child = QIO_CHANNEL(qio_channel_command_new_pid(devnull, devnull, pid));
>
> Overall this method overall duplicates much of the
> qio_channel_command_new_argv(), though you do have a few differences.
>
> I'd prefer if we could make qio_channel_command_new_argv more flexible to
> handle these extra needs though.
>

Ok I added a pre-exec callback for the extra dup2() & close().

Thanks,




-- 
Marc-André Lureau

On Fri, Jun 08, 2018 at 12:34:15AM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jun 4, 2018 at 11:36 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
> >> Create a vhost-user-backend object that holds a connection to a
> >> vhost-user backend and can be referenced from virtio devices that
> >> support it. See later patches for input & gpu usage.
> >>
> >> A chardev can be specified to communicate with the vhost-user backend,
> >> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
> >> vhost-user-backend,id=vuid,chardev=char0.
> >>
> >> Alternatively, an executable with its arguments may be given as 'cmd'
> >> property, ex: -object
> >> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
> >> executable is then spawn and, by convention, the vhost-user socket is
> >> passed as fd=3. It may be considered a security breach to allow
> >> creating processes that may execute arbitrary executables, so this may
> >> be restricted to some known executables (via signature etc) or
> >> directory.
> >
> > Passing a binary and args as a string blob.....
> >
> >> +static int
> >> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error **errp)
> >> +{
> >> +    int devnull = open("/dev/null", O_RDWR);
> >> +    pid_t pid;
> >> +
> >> +    assert(!b->child);
> >> +
> >> +    if (!b->cmd) {
> >> +        error_setg_errno(errp, errno, "Missing cmd property");
> >> +        return -1;
> >> +    }
> >> +    if (devnull < 0) {
> >> +        error_setg_errno(errp, errno, "Unable to open /dev/null");
> >> +        return -1;
> >> +    }
> >> +
> >> +    pid = qemu_fork(errp);
> >> +    if (pid < 0) {
> >> +        close(devnull);
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (pid == 0) { /* child */
> >> +        int fd, maxfd = sysconf(_SC_OPEN_MAX);
> >> +
> >> +        dup2(devnull, STDIN_FILENO);
> >> +        dup2(devnull, STDOUT_FILENO);
> >> +        dup2(vhostfd, 3);
> >> +
> >> +        signal(SIGINT, SIG_IGN);
> >
> > Why ignore SIGINT ?  Surely we want this extra process to be killed
> > someone ctrl-c's the parent QEMU.
> 
> leftover, removed
> 
> >
> >> +
> >> +        for (fd = 4; fd < maxfd; fd++) {
> >> +            close(fd);
> >> +        }
> >> +
> >> +        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
> >
> > ...which is then interpreted by the shell is a recipe for security
> > flaws. There needs to be a way to pass the command + arguments
> > to QEMU as an argv[] we can directly exec without involving the
> > shell.
> >
> 
> For now, I use g_shell_parse_argv(). Do you have a better idea?

Accept individual args at the cli level is far preferrable - we don't
want anything to be parsing shell strings:

 vhost-user-backend,id=vui,binary=/sbin/vhost-user-input,arg=/dev/input,arg=foo,arg=bar



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi

On Fri, Jun 8, 2018 at 10:43 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Fri, Jun 08, 2018 at 12:34:15AM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Mon, Jun 4, 2018 at 11:36 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
>> >> Create a vhost-user-backend object that holds a connection to a
>> >> vhost-user backend and can be referenced from virtio devices that
>> >> support it. See later patches for input & gpu usage.
>> >>
>> >> A chardev can be specified to communicate with the vhost-user backend,
>> >> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
>> >> vhost-user-backend,id=vuid,chardev=char0.
>> >>
>> >> Alternatively, an executable with its arguments may be given as 'cmd'
>> >> property, ex: -object
>> >> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
>> >> executable is then spawn and, by convention, the vhost-user socket is
>> >> passed as fd=3. It may be considered a security breach to allow
>> >> creating processes that may execute arbitrary executables, so this may
>> >> be restricted to some known executables (via signature etc) or
>> >> directory.
>> >
>> > Passing a binary and args as a string blob.....
>> >
>> >> +static int
>> >> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error **errp)
>> >> +{
>> >> +    int devnull = open("/dev/null", O_RDWR);
>> >> +    pid_t pid;
>> >> +
>> >> +    assert(!b->child);
>> >> +
>> >> +    if (!b->cmd) {
>> >> +        error_setg_errno(errp, errno, "Missing cmd property");
>> >> +        return -1;
>> >> +    }
>> >> +    if (devnull < 0) {
>> >> +        error_setg_errno(errp, errno, "Unable to open /dev/null");
>> >> +        return -1;
>> >> +    }
>> >> +
>> >> +    pid = qemu_fork(errp);
>> >> +    if (pid < 0) {
>> >> +        close(devnull);
>> >> +        return -1;
>> >> +    }
>> >> +
>> >> +    if (pid == 0) { /* child */
>> >> +        int fd, maxfd = sysconf(_SC_OPEN_MAX);
>> >> +
>> >> +        dup2(devnull, STDIN_FILENO);
>> >> +        dup2(devnull, STDOUT_FILENO);
>> >> +        dup2(vhostfd, 3);
>> >> +
>> >> +        signal(SIGINT, SIG_IGN);
>> >
>> > Why ignore SIGINT ?  Surely we want this extra process to be killed
>> > someone ctrl-c's the parent QEMU.
>>
>> leftover, removed
>>
>> >
>> >> +
>> >> +        for (fd = 4; fd < maxfd; fd++) {
>> >> +            close(fd);
>> >> +        }
>> >> +
>> >> +        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
>> >
>> > ...which is then interpreted by the shell is a recipe for security
>> > flaws. There needs to be a way to pass the command + arguments
>> > to QEMU as an argv[] we can directly exec without involving the
>> > shell.
>> >
>>
>> For now, I use g_shell_parse_argv(). Do you have a better idea?
>
> Accept individual args at the cli level is far preferrable - we don't
> want anything to be parsing shell strings:
>
>  vhost-user-backend,id=vui,binary=/sbin/vhost-user-input,arg=/dev/input,arg=foo,arg=bar

Object arguments are populated in a dictionary. Only the last value
specified is used.

g_shell_parse_argv() isn't that scary imho. But if there is a
blacklist of functions, it would be worth to have them listed
somewhere.

On Tue, Jun 12, 2018 at 04:53:54PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jun 8, 2018 at 10:43 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Fri, Jun 08, 2018 at 12:34:15AM +0200, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Mon, Jun 4, 2018 at 11:36 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >> > On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
> >> >> Create a vhost-user-backend object that holds a connection to a
> >> >> vhost-user backend and can be referenced from virtio devices that
> >> >> support it. See later patches for input & gpu usage.
> >> >>
> >> >> A chardev can be specified to communicate with the vhost-user backend,
> >> >> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
> >> >> vhost-user-backend,id=vuid,chardev=char0.
> >> >>
> >> >> Alternatively, an executable with its arguments may be given as 'cmd'
> >> >> property, ex: -object
> >> >> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
> >> >> executable is then spawn and, by convention, the vhost-user socket is
> >> >> passed as fd=3. It may be considered a security breach to allow
> >> >> creating processes that may execute arbitrary executables, so this may
> >> >> be restricted to some known executables (via signature etc) or
> >> >> directory.
> >> >
> >> > Passing a binary and args as a string blob.....
> >> >
> >> >> +static int
> >> >> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error **errp)
> >> >> +{
> >> >> +    int devnull = open("/dev/null", O_RDWR);
> >> >> +    pid_t pid;
> >> >> +
> >> >> +    assert(!b->child);
> >> >> +
> >> >> +    if (!b->cmd) {
> >> >> +        error_setg_errno(errp, errno, "Missing cmd property");
> >> >> +        return -1;
> >> >> +    }
> >> >> +    if (devnull < 0) {
> >> >> +        error_setg_errno(errp, errno, "Unable to open /dev/null");
> >> >> +        return -1;
> >> >> +    }
> >> >> +
> >> >> +    pid = qemu_fork(errp);
> >> >> +    if (pid < 0) {
> >> >> +        close(devnull);
> >> >> +        return -1;
> >> >> +    }
> >> >> +
> >> >> +    if (pid == 0) { /* child */
> >> >> +        int fd, maxfd = sysconf(_SC_OPEN_MAX);
> >> >> +
> >> >> +        dup2(devnull, STDIN_FILENO);
> >> >> +        dup2(devnull, STDOUT_FILENO);
> >> >> +        dup2(vhostfd, 3);
> >> >> +
> >> >> +        signal(SIGINT, SIG_IGN);
> >> >
> >> > Why ignore SIGINT ?  Surely we want this extra process to be killed
> >> > someone ctrl-c's the parent QEMU.
> >>
> >> leftover, removed
> >>
> >> >
> >> >> +
> >> >> +        for (fd = 4; fd < maxfd; fd++) {
> >> >> +            close(fd);
> >> >> +        }
> >> >> +
> >> >> +        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
> >> >
> >> > ...which is then interpreted by the shell is a recipe for security
> >> > flaws. There needs to be a way to pass the command + arguments
> >> > to QEMU as an argv[] we can directly exec without involving the
> >> > shell.
> >> >
> >>
> >> For now, I use g_shell_parse_argv(). Do you have a better idea?
> >
> > Accept individual args at the cli level is far preferrable - we don't
> > want anything to be parsing shell strings:
> >
> >  vhost-user-backend,id=vui,binary=/sbin/vhost-user-input,arg=/dev/input,arg=foo,arg=bar
> 
> Object arguments are populated in a dictionary. Only the last value
> specified is used.

Sigh, yes, this goes back to -object not having a full syntax for
non-scalar properties.

The quickest short term solution to this is to get the support for
"-object json:..."  working probably, then it will be just as
expressive as QMP's object_add.

> g_shell_parse_argv() isn't that scary imho. But if there is a
> blacklist of functions, it would be worth to have them listed
> somewhere.

We don't want to be using shell at all - we are going to be directly
passing the binary to execve(), so shell won't be involved. The
g_shell_parse_argv() is not simply splitting a string based on
whitespace, it is intepreting shell quoting rules, comments and
more. IOW, it is mangling args we'll be passing to execve() based
on shell rules that we're not intending to be using. It is simply
wrong to use g_shell_parse_argv() for this.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|