diff mbox

[v4,10/29] HACK: vhost-user-backend: allow to specify binary to execute

Message ID 20180713130916.4153-11-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc-André Lureau July 13, 2018, 1:08 p.m. UTC
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.

To make the patch more acceptable, the command argument would have to
be passed via an array (probably via -object json: syntax), instead of
using g_shell_parse_argv().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 backends/vhost-user.c | 96 ++++++++++++++++++++++++++++++++++++++-----
 qemu-options.hx       | 12 ++++--
 2 files changed, 94 insertions(+), 14 deletions(-)

Comments

Daniel P. Berrangé Aug. 28, 2018, 3:44 p.m. UTC | #1
On Fri, Jul 13, 2018 at 03:08:57PM +0200, Marc-André Lureau wrote:
> 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.

NB, Libvirt runs all QEMU instances with a seccomp policy that forbids
any use of execve(), so libvirt won't use 'cmd' at all.

> To make the patch more acceptable, the command argument would have to
> be passed via an array (probably via -object json: syntax), instead of
> using g_shell_parse_argv().

I still think we should not allow args to be specified at all. Declare
a stable API contract for this script where QEMU defines what argv and
env will be provided, in the same way we do for TAP ifup scripts. Then
on the CLI we'd only need a binary path.

> diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> index bf39c0751d..32d3ec0e8b 100644
> --- a/backends/vhost-user.c
> +++ b/backends/vhost-user.c
> @@ -136,31 +136,105 @@ vhost_user_backend_stop(VhostUserBackend *b)
>      b->started = false;
>  }
>  
> +static void
> +pre_exec_cb(void *data)
> +{
> +    int *sv = data;
> +    int maxfd = sysconf(_SC_OPEN_MAX);
> +    int fd;
> +
> +    dup2(sv[1], 3);

error checking...

> +    for (fd = 4; fd < maxfd; fd++) {
> +        close(fd);

This shouldn't fail, but to be robust we should check
errors anyway.

> +    }
> +}
> +

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 413b97d5e9..9243a5f8ab 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4292,16 +4292,20 @@ secondary:
>  If you want to know the detail of above command line, you can read
>  the colo-compare git log.
>  
> -@item -object vhost-user-backend,id=id=@var{id},chardev=@var{chardevid}
> +@item -object vhost-user-backend,id=id=@var{id}[,chardev=@var{chardevid},cmd=@var{cmd}]
>  
>  Create a vhost-user-backend object that holds a connection to a
>  vhost-user backend and can be referenced from virtio/vhost-user
>  devices that support it.
>  
>  The @var{id} parameter is a unique ID that will be used to reference
> -this vhost-user backend from the @option{vhost-user} device. The
> -@var{chardev} parameter is the unique ID of a character device backend
> -that provides the connection to the vhost-user slave process. (Since 3.0)
> +this vhost-user backend from the @option{vhost-user} device.
> +
> +You must specify either @var{chardev} or @var{cmd}. The @var{chardev}
> +parameter is the unique ID of a character device backend that provides
> +the connection to the vhost-user slave process.  The @var{cmd}
> +parameter will simplify handling of the backend, by running the given
> +command and establishing the connection. (Since 3.1)

Should note that 'cmd' is not usable with seccomp policy that blocks
execve.

Regards,
Daniel
diff mbox

Patch

diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index bf39c0751d..32d3ec0e8b 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -136,31 +136,105 @@  vhost_user_backend_stop(VhostUserBackend *b)
     b->started = false;
 }
 
+static void
+pre_exec_cb(void *data)
+{
+    int *sv = data;
+    int maxfd = sysconf(_SC_OPEN_MAX);
+    int fd;
+
+    dup2(sv[1], 3);
+    for (fd = 4; fd < maxfd; fd++) {
+        close(fd);
+    }
+}
+
 static void
 vhost_user_backend_complete(UserCreatable *uc, Error **errp)
 {
     VhostUserBackend *b = VHOST_USER_BACKEND(uc);
     Chardev *chr;
+    char **argv = NULL;
 
-    if (!b->chr_name) {
-        error_setg(errp, "You must specificy 'chardev'.");
+    if (!!b->chr_name + !!b->cmd != 1) {
+        error_setg(errp, "You may specificy only one of 'chardev' or 'cmd'.");
         return;
     }
 
-    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 (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;
+        if (!qemu_chr_fe_init(&b->chr, chr, errp)) {
+            return;
+        }
+    } else {
+        QIOChannelCommand *child;
+        GError *err;
+        int sv[2];
+
+        if (!g_shell_parse_argv(b->cmd, NULL, &argv, &err)) {
+            error_setg(errp, "Fail to parse command: %s", err->message);
+            g_error_free(err);
+            return;
+        }
+
+        if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
+            error_setg_errno(errp, errno, "socketpair() failed");
+            goto end;
+        }
+
+        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));
+            goto end;
+        }
+
+        if (!qemu_chr_fe_init(&b->chr, chr, errp)) {
+            goto end;
+        }
+
+        child = qio_channel_command_new_spawn_with_pre_exec(
+            (const char * const *)argv, O_RDONLY | O_WRONLY,
+            pre_exec_cb, sv, errp);
+        if (!child) {
+            goto end;
+        }
+        b->child = QIO_CHANNEL(child);
+
+        close(sv[1]);
     }
 
     b->completed = true;
     /* could vhost_dev_init() happen here, so early vhost-user message
      * can be exchanged */
+end:
+    g_strfreev(argv);
+}
+
+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)
@@ -189,6 +263,7 @@  static char *get_chardev(Object *obj, Error **errp)
 
 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);
 }
 
@@ -197,6 +272,7 @@  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);
 
     vhost_user_cleanup(&b->vhost_user);
diff --git a/qemu-options.hx b/qemu-options.hx
index 413b97d5e9..9243a5f8ab 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4292,16 +4292,20 @@  secondary:
 If you want to know the detail of above command line, you can read
 the colo-compare git log.
 
-@item -object vhost-user-backend,id=id=@var{id},chardev=@var{chardevid}
+@item -object vhost-user-backend,id=id=@var{id}[,chardev=@var{chardevid},cmd=@var{cmd}]
 
 Create a vhost-user-backend object that holds a connection to a
 vhost-user backend and can be referenced from virtio/vhost-user
 devices that support it.
 
 The @var{id} parameter is a unique ID that will be used to reference
-this vhost-user backend from the @option{vhost-user} device. The
-@var{chardev} parameter is the unique ID of a character device backend
-that provides the connection to the vhost-user slave process. (Since 3.0)
+this vhost-user backend from the @option{vhost-user} device.
+
+You must specify either @var{chardev} or @var{cmd}. The @var{chardev}
+parameter is the unique ID of a character device backend that provides
+the connection to the vhost-user slave process.  The @var{cmd}
+parameter will simplify handling of the backend, by running the given
+command and establishing the connection. (Since 3.1)
 
 @example