[Qemu-devel] [PATCH for-2.7? 0/5] block: Use QemuOpts for runtime options

[Qemu-devel] [PATCH for-2.7? 0/5] block: Use QemuOpts for runtime options

[Max Reitz]

The SSH and NBD block drivers currently directly extract their runtime
options from the options QDict they receive. This is bad practice and
can lead to segmentation faults (which, however, will always be a NULL
pointer dereference, so it should not be exploitable beyond a DoS).

This series fixes that by using QemuOpts instead (like all the other
block drivers do).

With this series applied, there are only two instances of "qdict_get"
left in block/, both of which appear to be safe.


Max Reitz (5):
  block/ssh: Use QemuOpts for runtime options
  block/nbd: Use QemuOpts for runtime options
  block/blkdebug: Store config filename
  block/nbd: Store runtime option values
  iotests: Test case for wrong runtime option types

 block/blkdebug.c           |  17 +++--
 block/nbd.c                | 159 ++++++++++++++++++++++++++++++---------------
 block/ssh.c                |  77 +++++++++++++++-------
 tests/qemu-iotests/162     |  96 +++++++++++++++++++++++++++
 tests/qemu-iotests/162.out |  17 +++++
 tests/qemu-iotests/group   |   1 +
 6 files changed, 284 insertions(+), 83 deletions(-)
 create mode 100755 tests/qemu-iotests/162
 create mode 100644 tests/qemu-iotests/162.out

-- 
2.9.2

[Qemu-devel] [PATCH for-2.7? 1/5] block/ssh: Use QemuOpts for runtime options

[Max Reitz]

Using QemuOpts will prevent qemu from crashing if the input options have
not been validated (which is the case when they are specified on the
command line or in a json: filename) and some have the wrong type.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/ssh.c | 77 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 24 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index bcbb0e4..4b90f34 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -508,36 +508,73 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
     return ret;
 }
 
+static QemuOptsList runtime_opts = {
+    .name = "ssh",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+            .help = "Host to connect to",
+        },
+        {
+            .name = "port",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Port to connect to",
+        },
+        {
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+            .help = "Path of the image on the host",
+        },
+        {
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+            .help = "User as which to connect",
+        },
+        {
+            .name = "host_key_check",
+            .type = QEMU_OPT_STRING,
+            .help = "Defines how and what to check the host key against",
+        },
+    },
+};
+
 static int connect_to_ssh(BDRVSSHState *s, QDict *options,
                           int ssh_flags, int creat_mode, Error **errp)
 {
     int r, ret;
+    QemuOpts *opts = NULL;
+    Error *local_err = NULL;
     const char *host, *user, *path, *host_key_check;
     int port;
 
-    if (!qdict_haskey(options, "host")) {
+    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
         ret = -EINVAL;
-        error_setg(errp, "No hostname was specified");
+        error_propagate(errp, local_err);
         goto err;
     }
-    host = qdict_get_str(options, "host");
 
-    if (qdict_haskey(options, "port")) {
-        port = qdict_get_int(options, "port");
-    } else {
-        port = 22;
+    host = qemu_opt_get(opts, "host");
+    if (!host) {
+        ret = -EINVAL;
+        error_setg(errp, "No hostname was specified");
+        goto err;
     }
 
-    if (!qdict_haskey(options, "path")) {
+    port = qemu_opt_get_number(opts, "port", 22);
+
+    path = qemu_opt_get(opts, "path");
+    if (!path) {
         ret = -EINVAL;
         error_setg(errp, "No path was specified");
         goto err;
     }
-    path = qdict_get_str(options, "path");
 
-    if (qdict_haskey(options, "user")) {
-        user = qdict_get_str(options, "user");
-    } else {
+    user = qemu_opt_get(opts, "user");
+    if (!user) {
         user = g_get_user_name();
         if (!user) {
             error_setg_errno(errp, errno, "Can't get user name");
@@ -546,9 +583,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
         }
     }
 
-    if (qdict_haskey(options, "host_key_check")) {
-        host_key_check = qdict_get_str(options, "host_key_check");
-    } else {
+    host_key_check = qemu_opt_get(opts, "host_key_check");
+    if (!host_key_check) {
         host_key_check = "yes";
     }
 
@@ -618,15 +654,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
         return -EINVAL;
     }
 
-    /* Delete the options we've used; any not deleted will cause the
-     * block layer to give an error about unused options.
-     */
-    qdict_del(options, "host");
-    qdict_del(options, "port");
-    qdict_del(options, "user");
-    qdict_del(options, "path");
-    qdict_del(options, "host_key_check");
-
     return 0;
 
  err:
@@ -646,6 +673,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
     s->session = NULL;
 
+    qemu_opts_del(opts);
+
     return ret;
 }
 
-- 
2.9.2

Re: [Qemu-devel] [PATCH for-2.7? 1/5] block/ssh: Use QemuOpts for runtime options

[Kevin Wolf]

Am 12.08.2016 um 19:15 hat Max Reitz geschrieben:
> Using QemuOpts will prevent qemu from crashing if the input options have
> not been validated (which is the case when they are specified on the
> command line or in a json: filename) and some have the wrong type.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/ssh.c | 77 ++++++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 24 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index bcbb0e4..4b90f34 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -508,36 +508,73 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
>      return ret;
>  }
>  
> +static QemuOptsList runtime_opts = {

I would generally prefer some prefix to make the name unique instead of
having something called "runtime_opts" in every driver. Currently we're
inconsistent, but most driver do use a prefix.

> @@ -618,15 +654,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>          return -EINVAL;
>      }
>  
> -    /* Delete the options we've used; any not deleted will cause the
> -     * block layer to give an error about unused options.
> -     */
> -    qdict_del(options, "host");
> -    qdict_del(options, "port");
> -    qdict_del(options, "user");
> -    qdict_del(options, "path");
> -    qdict_del(options, "host_key_check");
> -
>      return 0;
>  
>   err:
> @@ -646,6 +673,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>      }
>      s->session = NULL;
>  
> +    qemu_opts_del(opts);
> +
>      return ret;
>  }

opts is leaked in the normal path and only freed on error.

Kevin

[Qemu-devel] [PATCH for-2.7? 2/5] block/nbd: Use QemuOpts for runtime options

[Max Reitz]

Using QemuOpts will prevent qemu from crashing if the input options have
not been validated (which is the case when they are specified on the
command line or in a json: filename) and some have the wrong type.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 20 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 8d57220..3dec860 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -188,13 +188,13 @@ out:
     g_free(file);
 }
 
-static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
+static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, char **export,
                                  Error **errp)
 {
     SocketAddress *saddr;
 
-    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
-        if (qdict_haskey(options, "path")) {
+    if (!qemu_opt_get(opts, "path") == !qemu_opt_get(opts, "host")) {
+        if (qemu_opt_get(opts, "path")) {
             error_setg(errp, "path and host may not be used at the same time.");
         } else {
             error_setg(errp, "one of path and host must be specified.");
@@ -204,32 +204,25 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
 
     saddr = g_new0(SocketAddress, 1);
 
-    if (qdict_haskey(options, "path")) {
+    if (qemu_opt_get(opts, "path")) {
         UnixSocketAddress *q_unix;
         saddr->type = SOCKET_ADDRESS_KIND_UNIX;
         q_unix = saddr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-        q_unix->path = g_strdup(qdict_get_str(options, "path"));
-        qdict_del(options, "path");
+        q_unix->path = g_strdup(qemu_opt_get(opts, "path"));
     } else {
         InetSocketAddress *inet;
         saddr->type = SOCKET_ADDRESS_KIND_INET;
         inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
-        inet->host = g_strdup(qdict_get_str(options, "host"));
-        if (!qdict_get_try_str(options, "port")) {
+        inet->host = g_strdup(qemu_opt_get(opts, "host"));
+        inet->port = g_strdup(qemu_opt_get(opts, "port"));
+        if (!inet->port) {
             inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
-        } else {
-            inet->port = g_strdup(qdict_get_str(options, "port"));
         }
-        qdict_del(options, "host");
-        qdict_del(options, "port");
     }
 
     s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
 
-    *export = g_strdup(qdict_get_try_str(options, "export"));
-    if (*export) {
-        qdict_del(options, "export");
-    }
+    *export = g_strdup(qemu_opt_get(opts, "export"));
 
     return saddr;
 }
@@ -292,27 +285,67 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
 }
 
 
+static QemuOptsList runtime_opts = {
+    .name = "nbd",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+            .help = "TCP host to connect to",
+        },
+        {
+            .name = "port",
+            .type = QEMU_OPT_STRING,
+            .help = "TCP port to connect to",
+        },
+        {
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+            .help = "Unix socket path to connect to",
+        },
+        {
+            .name = "export",
+            .type = QEMU_OPT_STRING,
+            .help = "Name of the NBD export to open",
+        },
+        {
+            .name = "tls-creds",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the TLS credentials to use",
+        },
+    },
+};
+
 static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     BDRVNBDState *s = bs->opaque;
+    QemuOpts *opts = NULL;
+    Error *local_err = NULL;
     char *export = NULL;
     QIOChannelSocket *sioc = NULL;
-    SocketAddress *saddr;
+    SocketAddress *saddr = NULL;
     const char *tlscredsid;
     QCryptoTLSCreds *tlscreds = NULL;
     const char *hostname = NULL;
     int ret = -EINVAL;
 
+    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto error;
+    }
+
     /* Pop the config into our state object. Exit if invalid. */
-    saddr = nbd_config(s, options, &export, errp);
+    saddr = nbd_config(s, opts, &export, errp);
     if (!saddr) {
         goto error;
     }
 
-    tlscredsid = g_strdup(qdict_get_try_str(options, "tls-creds"));
+    tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
     if (tlscredsid) {
-        qdict_del(options, "tls-creds");
         tlscreds = nbd_get_tls_creds(tlscredsid, errp);
         if (!tlscreds) {
             goto error;
@@ -346,6 +379,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
     qapi_free_SocketAddress(saddr);
     g_free(export);
+    qemu_opts_del(opts);
     return ret;
 }
 
-- 
2.9.2

[Qemu-devel] [PATCH for-2.7? 3/5] block/blkdebug: Store config filename

[Max Reitz]

Store the configuration file's filename so it can later be used in
bdrv_refresh_filename() without having to directly access the options
QDict which may contain a value of a non-string type.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkdebug.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index fb29283..d5db166 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -39,6 +39,9 @@ typedef struct BDRVBlkdebugState {
     int new_state;
     int align;
 
+    /* For blkdebug_refresh_filename() */
+    char *config_file;
+
     QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
     QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
     QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
@@ -351,7 +354,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *config;
     uint64_t align;
     int ret;
 
@@ -364,8 +366,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Read rules from config file or command line options */
-    config = qemu_opt_get(opts, "config");
-    ret = read_config(s, config, options, errp);
+    s->config_file = g_strdup(qemu_opt_get(opts, "config"));
+    ret = read_config(s, s->config_file, options, errp);
     if (ret) {
         goto out;
     }
@@ -398,6 +400,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
 fail_unref:
     bdrv_unref_child(bs, bs->file);
 out:
+    if (ret < 0) {
+        g_free(s->config_file);
+    }
     qemu_opts_del(opts);
     return ret;
 }
@@ -515,6 +520,8 @@ static void blkdebug_close(BlockDriverState *bs)
             remove_rule(rule);
         }
     }
+
+    g_free(s->config_file);
 }
 
 static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
@@ -679,6 +686,7 @@ static int blkdebug_truncate(BlockDriverState *bs, int64_t offset)
 
 static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
 {
+    BDRVBlkdebugState *s = bs->opaque;
     QDict *opts;
     const QDictEntry *e;
     bool force_json = false;
@@ -700,8 +708,7 @@ static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
 
     if (!force_json && bs->file->bs->exact_filename[0]) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "blkdebug:%s:%s",
-                 qdict_get_try_str(options, "config") ?: "",
+                 "blkdebug:%s:%s", s->config_file ?: "",
                  bs->file->bs->exact_filename);
     }
 
-- 
2.9.2

[Qemu-devel] [PATCH for-2.7? 4/5] block/nbd: Store runtime option values

[Max Reitz]

Store the runtime option values in the BDRVNBDState so they can later be
used in nbd_refresh_filename() without having to directly access the
options QDict which may contain values of non-string types.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 105 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 44 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 3dec860..7403195 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -42,6 +42,9 @@
 
 typedef struct BDRVNBDState {
     NbdClientSession client;
+
+    /* For nbd_refresh_filename() */
+    char *path, *host, *port, *export, *tlscredsid;
 } BDRVNBDState;
 
 static int nbd_parse_uri(const char *filename, QDict *options)
@@ -188,13 +191,15 @@ out:
     g_free(file);
 }
 
-static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, char **export,
-                                 Error **errp)
+static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
 {
     SocketAddress *saddr;
 
-    if (!qemu_opt_get(opts, "path") == !qemu_opt_get(opts, "host")) {
-        if (qemu_opt_get(opts, "path")) {
+    s->path = g_strdup(qemu_opt_get(opts, "path"));
+    s->host = g_strdup(qemu_opt_get(opts, "host"));
+
+    if (!s->path == !s->host) {
+        if (s->path) {
             error_setg(errp, "path and host may not be used at the same time.");
         } else {
             error_setg(errp, "one of path and host must be specified.");
@@ -204,17 +209,20 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, char **export,
 
     saddr = g_new0(SocketAddress, 1);
 
-    if (qemu_opt_get(opts, "path")) {
+    if (s->path) {
         UnixSocketAddress *q_unix;
         saddr->type = SOCKET_ADDRESS_KIND_UNIX;
         q_unix = saddr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-        q_unix->path = g_strdup(qemu_opt_get(opts, "path"));
+        q_unix->path = g_strdup(s->path);
     } else {
         InetSocketAddress *inet;
+
+        s->port = g_strdup(qemu_opt_get(opts, "port"));
+
         saddr->type = SOCKET_ADDRESS_KIND_INET;
         inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
-        inet->host = g_strdup(qemu_opt_get(opts, "host"));
-        inet->port = g_strdup(qemu_opt_get(opts, "port"));
+        inet->host = g_strdup(s->host);
+        inet->port = g_strdup(s->port);
         if (!inet->port) {
             inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
         }
@@ -222,7 +230,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, char **export,
 
     s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
 
-    *export = g_strdup(qemu_opt_get(opts, "export"));
+    s->export = g_strdup(qemu_opt_get(opts, "export"));
 
     return saddr;
 }
@@ -323,10 +331,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVNBDState *s = bs->opaque;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
-    char *export = NULL;
     QIOChannelSocket *sioc = NULL;
     SocketAddress *saddr = NULL;
-    const char *tlscredsid;
     QCryptoTLSCreds *tlscreds = NULL;
     const char *hostname = NULL;
     int ret = -EINVAL;
@@ -339,14 +345,14 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Pop the config into our state object. Exit if invalid. */
-    saddr = nbd_config(s, opts, &export, errp);
+    saddr = nbd_config(s, opts, errp);
     if (!saddr) {
         goto error;
     }
 
-    tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
-    if (tlscredsid) {
-        tlscreds = nbd_get_tls_creds(tlscredsid, errp);
+    s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
+    if (s->tlscredsid) {
+        tlscreds = nbd_get_tls_creds(s->tlscredsid, errp);
         if (!tlscreds) {
             goto error;
         }
@@ -368,7 +374,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* NBD handshake */
-    ret = nbd_client_init(bs, sioc, export,
+    ret = nbd_client_init(bs, sioc, s->export,
                           tlscreds, hostname, errp);
  error:
     if (sioc) {
@@ -377,8 +383,14 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     if (tlscreds) {
         object_unref(OBJECT(tlscreds));
     }
+    if (ret < 0) {
+        g_free(s->path);
+        g_free(s->host);
+        g_free(s->port);
+        g_free(s->export);
+        g_free(s->tlscredsid);
+    }
     qapi_free_SocketAddress(saddr);
-    g_free(export);
     qemu_opts_del(opts);
     return ret;
 }
@@ -396,7 +408,15 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
 
 static void nbd_close(BlockDriverState *bs)
 {
+    BDRVNBDState *s = bs->opaque;
+
     nbd_client_close(bs);
+
+    g_free(s->path);
+    g_free(s->host);
+    g_free(s->port);
+    g_free(s->export);
+    g_free(s->tlscredsid);
 }
 
 static int64_t nbd_getlength(BlockDriverState *bs)
@@ -419,48 +439,45 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
 
 static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
 {
+    BDRVNBDState *s = bs->opaque;
     QDict *opts = qdict_new();
-    const char *path   = qdict_get_try_str(options, "path");
-    const char *host   = qdict_get_try_str(options, "host");
-    const char *port   = qdict_get_try_str(options, "port");
-    const char *export = qdict_get_try_str(options, "export");
-    const char *tlscreds = qdict_get_try_str(options, "tls-creds");
 
     qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
 
-    if (path && export) {
+    if (s->path && s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix:///%s?socket=%s", export, path);
-    } else if (path && !export) {
+                 "nbd+unix:///%s?socket=%s", s->export, s->path);
+    } else if (s->path && !s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix://?socket=%s", path);
-    } else if (!path && export && port) {
+                 "nbd+unix://?socket=%s", s->path);
+    } else if (!s->path && s->export && s->port) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s/%s", host, port, export);
-    } else if (!path && export && !port) {
+                 "nbd://%s:%s/%s", s->host, s->port, s->export);
+    } else if (!s->path && s->export && !s->port) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s/%s", host, export);
-    } else if (!path && !export && port) {
+                 "nbd://%s/%s", s->host, s->export);
+    } else if (!s->path && !s->export && s->port) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s", host, port);
-    } else if (!path && !export && !port) {
+                 "nbd://%s:%s", s->host, s->port);
+    } else if (!s->path && !s->export && !s->port) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s", host);
+                 "nbd://%s", s->host);
     }
 
-    if (path) {
-        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
-    } else if (port) {
-        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
-        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
+    if (s->path) {
+        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(s->path)));
+    } else if (s->port) {
+        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(s->host)));
+        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(s->port)));
     } else {
-        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
+        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(s->host)));
     }
-    if (export) {
-        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
+    if (s->export) {
+        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(s->export)));
     }
-    if (tlscreds) {
-        qdict_put_obj(opts, "tls-creds", QOBJECT(qstring_from_str(tlscreds)));
+    if (s->tlscredsid) {
+        qdict_put_obj(opts, "tls-creds",
+                      QOBJECT(qstring_from_str(s->tlscredsid)));
     }
 
     bs->full_open_options = opts;
-- 
2.9.2

[Qemu-devel] [PATCH for-2.7? 5/5] iotests: Test case for wrong runtime option types

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/162     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/162.out | 17 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 114 insertions(+)
 create mode 100755 tests/qemu-iotests/162
 create mode 100644 tests/qemu-iotests/162.out

diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
new file mode 100755
index 0000000..0b43ea3
--- /dev/null
+++ b/tests/qemu-iotests/162
@@ -0,0 +1,96 @@
+#!/bin/bash
+#
+# Test case for specifying runtime options of the wrong type to some
+# block drivers
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_os Linux
+
+echo
+echo '=== NBD ==='
+# NBD expects all of its arguments to be strings
+
+# So this should not crash
+$QEMU_IMG info 'json:{"driver": "nbd", "host": 42}'
+
+# And this should not treat @port as if it had not been specified
+# (We cannot use localhost with an invalid port here, but we need to use a
+#  non-existing domain, because otherwise the error message will not contain
+#  the port)
+$QEMU_IMG info 'json:{"driver": "nbd", "host": "does.not.exist.example.com", "port": 42}'
+
+# This is a test for NBD's bdrv_refresh_filename() implementation: It expects
+# either host or path to be set, but it must not assume that they are set to
+# strings in the options QDict
+$QEMU_NBD -k "$PWD/42" -f raw null-co:// &
+sleep 0.5
+$QEMU_IMG info 'json:{"driver": "nbd", "path": 42}' | grep '^image'
+rm -f 42
+
+
+echo
+echo '=== SSH ==='
+# SSH expects all of its arguments to be strings, except for @port, which is
+# expected to be an integer
+
+# So "0" should be converted to an integer here (instead of crashing)
+$QEMU_IMG info 'json:{"driver": "ssh", "host": "localhost", "port": "0", "path": "/foo"}'
+# The same, basically (all values for --image-opts are seen as strings in qemu)
+$QEMU_IMG info --image-opts \
+    driver=ssh,host=localhost,port=0,path=/foo
+
+# This, however, should fail because of the wrong type
+$QEMU_IMG info 'json:{"driver": "ssh", "host": "localhost", "port": 0.42, "path": "/foo"}'
+# Not really the same: Here, "0.42" will be passed instead of 0.42, but still,
+# qemu should not try to convert "0.42" to an integer
+$QEMU_IMG info --image-opts \
+    driver=ssh,host=localhost,port=0.42,path=/foo
+
+
+echo
+echo '=== blkdebug ==='
+# blkdebug expects all of its arguments to be strings, but its
+# bdrv_refresh_filename() implementation should not assume that they have been
+# passed as strings in the original options QDict.
+# So this should emit blkdebug:42:null-co:// as the filename:
+touch 42
+$QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
+                      "image.driver": "null-co"}' \
+    | grep '^image'
+rm -f 42
+
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/162.out b/tests/qemu-iotests/162.out
new file mode 100644
index 0000000..9bba723
--- /dev/null
+++ b/tests/qemu-iotests/162.out
@@ -0,0 +1,17 @@
+QA output created by 162
+
+=== NBD ===
+qemu-img: Could not open 'json:{"driver": "nbd", "host": 42}': Failed to connect socket: Invalid argument
+qemu-img: Could not open 'json:{"driver": "nbd", "host": "does.not.exist.example.com", "port": 42}': address resolution failed for does.not.exist.example.com:42: Name or service not known
+image: nbd+unix://?socket=42
+
+=== SSH ===
+qemu-img: Could not open 'json:{"driver": "ssh", "host": "localhost", "port": "0", "path": "/foo"}': Failed to connect socket: Connection refused
+qemu-img: Could not open 'driver=ssh,host=localhost,port=0,path=/foo': Failed to connect socket: Connection refused
+qemu-img: Could not open 'json:{"driver": "ssh", "host": "localhost", "port": 0.42, "path": "/foo"}': Parameter 'port' expects a number
+qemu-img: Could not open 'driver=ssh,host=localhost,port=0.42,path=/foo': Parameter 'port' expects a number
+
+=== blkdebug ===
+image: blkdebug:42:null-co://
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3a3973e..50ddeed 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -157,3 +157,4 @@
 155 rw auto
 156 rw auto quick
 157 auto
+162 auto quick
-- 
2.9.2

Re: [Qemu-devel] [PATCH for-2.7? 0/5] block: Use QemuOpts for runtime options

[Kevin Wolf]

Am 12.08.2016 um 19:15 hat Max Reitz geschrieben:
> The SSH and NBD block drivers currently directly extract their runtime
> options from the options QDict they receive. This is bad practice and
> can lead to segmentation faults (which, however, will always be a NULL
> pointer dereference, so it should not be exploitable beyond a DoS).
> 
> This series fixes that by using QemuOpts instead (like all the other
> block drivers do).
> 
> With this series applied, there are only two instances of "qdict_get"
> left in block/, both of which appear to be safe.

Patches 2-5:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>