[Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Daniel P. Berrangé]

A user trying out SMBIOS "OEM strings" feature reported that the data
they are exposing to the guest was truncated at 1023 bytes, which breaks
the app consuming in the guest. After searching for the cause I
eventually found that the QemuOpts parsing is using fixed length 1024
byte array for option values and 128 byte array for key names.

We can certainly debate whether it is sane to have such long command
line argument values (it is not sane), but if the OS was capable of
exec'ing QEMU with such an ARGV array, there is little good reason for
imposing an artificial length restriction when parsing it. Even worse is
that we silently truncate without reporting an error when hitting limits
resulting in a semantically incorrect behaviour, possibly even leading
to security flaws depending on the data that was truncated.

Thus this patch series removes the artificial length limits by killing
the fixed length buffers.

Separately I intend to make it possible to read "OEM strings" data from
a file, to avoid need to have long command line args.

Daniel P. Berrangé (3):
  accel: use g_strsplit for parsing accelerator names
  opts: don't silently truncate long parameter keys
  opts: don't silently truncate long option values

 accel/accel.c          |  16 +++---
 hw/i386/multiboot.c    |  33 +++++++----
 include/qemu/option.h  |   3 +-
 tests/test-qemu-opts.c |  18 ------
 util/qemu-option.c     | 150 ++++++++++++++++++++++++++-----------------------
 5 files changed, 108 insertions(+), 112 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] accel: use g_strsplit for parsing accelerator names

[Daniel P. Berrangé]

Instead of re-using the get_opt_name() method from QemuOpts to split a
string on ':', just use g_strsplit().

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 accel/accel.c         | 16 +++++++---------
 include/qemu/option.h |  1 -
 util/qemu-option.c    |  3 ++-
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 93e2434c87..4981e7fff3 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -70,8 +70,8 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
 
 void configure_accelerator(MachineState *ms)
 {
-    const char *accel, *p;
-    char buf[10];
+    const char *accel;
+    char **accel_list, **tmp;
     int ret;
     bool accel_initialised = false;
     bool init_failed = false;
@@ -83,13 +83,10 @@ void configure_accelerator(MachineState *ms)
         accel = "tcg";
     }
 
-    p = accel;
-    while (!accel_initialised && *p != '\0') {
-        if (*p == ':') {
-            p++;
-        }
-        p = get_opt_name(buf, sizeof(buf), p, ':');
-        acc = accel_find(buf);
+    accel_list = g_strsplit(accel, ":", 0);
+
+    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
+        acc = accel_find(*tmp);
         if (!acc) {
             continue;
         }
@@ -107,6 +104,7 @@ void configure_accelerator(MachineState *ms)
             accel_initialised = true;
         }
     }
+    g_strfreev(accel_list);
 
     if (!accel_initialised) {
         if (!init_failed) {
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 306fdb5f7a..1cfe5cbc2d 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -28,7 +28,6 @@
 
 #include "qemu/queue.h"
 
-const char *get_opt_name(char *buf, int buf_size, const char *p, char delim);
 const char *get_opt_value(char *buf, int buf_size, const char *p);
 
 void parse_option_size(const char *name, const char *value,
diff --git a/util/qemu-option.c b/util/qemu-option.c
index d0756fda58..baca40fb94 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -49,7 +49,8 @@
  * The return value is the position of the delimiter/zero byte after the option
  * name in p.
  */
-const char *get_opt_name(char *buf, int buf_size, const char *p, char delim)
+static const char *get_opt_name(char *buf, int buf_size, const char *p,
+                                char delim)
 {
     char *q;
 
-- 
2.14.3

[Qemu-devel] [PATCH 2/3] opts: don't silently truncate long parameter keys

[Daniel P. Berrangé]

The existing QemuOpts parsing code uses a fixed size 128 byte buffer
for storing the parameter keys. If a key exceeded this size it was
silently truncate and no error reported to the user. This behaviour was
reasonable & harmless because traditionally the key names are all
statically declared, and it was known that no code was declaring a key
longer than 127 bytes. This assumption, however, ceased to be valid once
the block layer added support for dot-separate compound keys. This
syntax allows for keys that can be arbitrarily long, limited only by the
number of block drivers you can stack up. With this usage, silently
truncating the key name can never lead to correct behaviour.

Hopefully such truncation would turn into an error, when the block code
then tried to extract options later, but there's no guarantee that will
happen. It is conceivable that an option specified by the user may be
truncated and then ignored. This could have serious consequences,
possibly even leading to security problems if the ignored option set a
security relevant parameter.

If the operating system didn't limit the user's argv when spawning QEMU,
the code should honour whatever length arguments were given without
imposing its own length restrictions. This patch thus changes the code
to use a heap allocated buffer for storing the keys during parsing,
lifting the arbitrary length restriction.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/test-qemu-opts.c | 18 ------------------
 util/qemu-option.c     | 44 ++++++++++++++++++++++----------------------
 2 files changed, 22 insertions(+), 40 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 2c422abcd4..4508f95a9b 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -459,8 +459,6 @@ static void test_opts_parse(void)
 {
     Error *err = NULL;
     QemuOpts *opts;
-    char long_key[129];
-    char *params;
 
     /* Nothing */
     opts = qemu_opts_parse(&opts_list_03, "", false, &error_abort);
@@ -471,22 +469,6 @@ static void test_opts_parse(void)
     g_assert_cmpuint(opts_count(opts), ==, 1);
     g_assert_cmpstr(qemu_opt_get(opts, ""), ==, "val");
 
-    /* Long key */
-    memset(long_key, 'a', 127);
-    long_key[127] = 'z';
-    long_key[128] = 0;
-    params = g_strdup_printf("%s=v", long_key);
-    opts = qemu_opts_parse(&opts_list_03, params + 1, NULL, &error_abort);
-    g_assert_cmpuint(opts_count(opts), ==, 1);
-    g_assert_cmpstr(qemu_opt_get(opts, long_key + 1), ==, "v");
-
-    /* Overlong key gets truncated */
-    opts = qemu_opts_parse(&opts_list_03, params, NULL, &error_abort);
-    g_assert(opts_count(opts) == 1);
-    long_key[127] = 0;
-    g_assert_cmpstr(qemu_opt_get(opts, long_key), ==, "v");
-    g_free(params);
-
     /* Multiple keys, last one wins */
     opts = qemu_opts_parse(&opts_list_03, "a=1,b=2,,x,a=3",
                            false, &error_abort);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index baca40fb94..fa1a9f17fc 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -43,27 +43,23 @@
  * first byte of the option name)
  *
  * The option name is delimited by delim (usually , or =) or the string end
- * and is copied into buf. If the option name is longer than buf_size, it is
- * truncated. buf is always zero terminated.
+ * and is copied into option. The caller is responsible for free'ing option
+ * when no longer required.
  *
  * The return value is the position of the delimiter/zero byte after the option
  * name in p.
  */
-static const char *get_opt_name(char *buf, int buf_size, const char *p,
-                                char delim)
+static const char *get_opt_name(const char *p, char **option, char delim)
 {
-    char *q;
+    char *offset = strchr(p, delim);
 
-    q = buf;
-    while (*p != '\0' && *p != delim) {
-        if (q && (q - buf) < buf_size - 1)
-            *q++ = *p;
-        p++;
+    if (offset) {
+        *option = g_strndup(p, offset - p);
+        return offset;
+    } else {
+        *option = g_strdup(p);
+        return p + strlen(p);
     }
-    if (q)
-        *q = '\0';
-
-    return p;
 }
 
 /*
@@ -758,7 +754,8 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
 static void opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname, bool prepend, Error **errp)
 {
-    char option[128], value[1024];
+    char *option = NULL;
+    char value[1024];
     const char *p,*pe,*pc;
     Error *local_err = NULL;
 
@@ -769,11 +766,11 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             /* found "foo,more" */
             if (p == params && firstname) {
                 /* implicitly named first option */
-                pstrcpy(option, sizeof(option), firstname);
+                option = g_strdup(firstname);
                 p = get_opt_value(value, sizeof(value), p);
             } else {
                 /* option without value, probably a flag */
-                p = get_opt_name(option, sizeof(option), p, ',');
+                p = get_opt_name(p, &option, ',');
                 if (strncmp(option, "no", 2) == 0) {
                     memmove(option, option+2, strlen(option+2)+1);
                     pstrcpy(value, sizeof(value), "off");
@@ -783,10 +780,8 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             }
         } else {
             /* found "foo=bar,more" */
-            p = get_opt_name(option, sizeof(option), p, '=');
-            if (*p != '=') {
-                break;
-            }
+            p = get_opt_name(p, &option, '=');
+            assert(*p == '=');
             p++;
             p = get_opt_value(value, sizeof(value), p);
         }
@@ -795,13 +790,18 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             opt_set(opts, option, value, prepend, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
-                return;
+                goto cleanup;
             }
         }
         if (*p != ',') {
             break;
         }
+        g_free(option);
+        option = NULL;
     }
+
+ cleanup:
+    g_free(option);
 }
 
 /**
-- 
2.14.3

[Qemu-devel] [PATCH 3/3] opts: don't silently truncate long option values

[Daniel P. Berrangé]

The existing QemuOpts parsing code uses a fixed size 1024 byte buffer
for storing the option values. If a value exceeded this size it was
silently truncated and no error reported to the user. Long option values
is not a common scenario, but it is conceivable that they will happen.
eg if the user has a very deeply nested filesystem it would be possible
to come up with a disk path that was > 1024 bytes. Most of the time if
such data was silently truncated, the user would get an error about
opening a non-existant disk. If they're unlucky though, QEMU might use a
completely different disk image from another VM, which could be
considered a security issue. Another example program was in using the
-smbios command line arg with very large data blobs. In this case the
silent truncation will be providing semantically incorrect data to the
guest OS for SMBIOS tables.

If the operating system didn't limit the user's argv when spawning QEMU,
the code should honour whatever length arguments were given without
imposing its own length restrictions. This patch thus changes the code
to use a heap allocated buffer for storing the values during parsing,
lifting the arbitrary length restriction.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/multiboot.c   |  33 +++++++++------
 include/qemu/option.h |   2 +-
 util/qemu-option.c    | 111 +++++++++++++++++++++++++++-----------------------
 3 files changed, 81 insertions(+), 65 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 5bc0a2cddb..7a2953e26f 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -291,12 +291,16 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len = strlen(kernel_filename) + 1;
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
-        const char *r = initrd_filename;
+        const char *r = get_opt_value(initrd_filename, NULL);
         cmdline_len += strlen(r) + 1;
         mbs.mb_mods_avail = 1;
-        while (*(r = get_opt_value(NULL, 0, r))) {
-           mbs.mb_mods_avail++;
-           r++;
+        while (1) {
+            mbs.mb_mods_avail++;
+            r = get_opt_value(r, NULL);
+            if (!*r) {
+                break;
+            }
+            r++;
         }
     }
 
@@ -313,7 +317,8 @@ int load_multiboot(FWCfgState *fw_cfg,
 
     if (initrd_filename) {
         const char *next_initrd;
-        char not_last, tmpbuf[strlen(initrd_filename) + 1];
+        char not_last;
+        char *one_file = NULL;
 
         mbs.offset_mods = mbs.mb_buf_size;
 
@@ -322,24 +327,26 @@ int load_multiboot(FWCfgState *fw_cfg,
             int mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
 
-            next_initrd = get_opt_value(tmpbuf, sizeof(tmpbuf), initrd_filename);
+            next_initrd = get_opt_value(initrd_filename, &one_file);
             not_last = *next_initrd;
             /* if a space comes after the module filename, treat everything
                after that as parameters */
-            hwaddr c = mb_add_cmdline(&mbs, tmpbuf);
-            if ((next_space = strchr(tmpbuf, ' ')))
+            hwaddr c = mb_add_cmdline(&mbs, one_file);
+            next_space = strchr(one_file, ' ');
+            if (next_space) {
                 *next_space = '\0';
-            mb_debug("multiboot loading module: %s", tmpbuf);
-            mb_mod_length = get_image_size(tmpbuf);
+            }
+            mb_debug("multiboot loading module: %s", one_file);
+            mb_mod_length = get_image_size(one_file);
             if (mb_mod_length < 0) {
-                error_report("Failed to open file '%s'", tmpbuf);
+                error_report("Failed to open file '%s'", one_file);
                 exit(1);
             }
 
             mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + mbs.mb_buf_size);
             mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
 
-            load_image(tmpbuf, (unsigned char *)mbs.mb_buf + offs);
+            load_image(one_file, (unsigned char *)mbs.mb_buf + offs);
             mb_add_mod(&mbs, mbs.mb_buf_phys + offs,
                        mbs.mb_buf_phys + offs + mb_mod_length, c);
 
@@ -347,6 +354,8 @@ int load_multiboot(FWCfgState *fw_cfg,
                      (char *)mbs.mb_buf + offs,
                      (char *)mbs.mb_buf + offs + mb_mod_length, c);
             initrd_filename = next_initrd+1;
+            g_free(one_file);
+            one_file = NULL;
         } while (not_last);
     }
 
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 1cfe5cbc2d..3dfb4493cc 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -28,7 +28,7 @@
 
 #include "qemu/queue.h"
 
-const char *get_opt_value(char *buf, int buf_size, const char *p);
+const char *get_opt_value(const char *p, char **value);
 
 void parse_option_size(const char *name, const char *value,
                        uint64_t *ret, Error **errp);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index fa1a9f17fc..58d1c23893 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -70,25 +70,37 @@ static const char *get_opt_name(const char *p, char **option, char delim)
  * delimiter is fixed to be comma which starts a new option. To specify an
  * option value that contains commas, double each comma.
  */
-const char *get_opt_value(char *buf, int buf_size, const char *p)
+const char *get_opt_value(const char *p, char **value)
 {
-    char *q;
+    size_t capacity = 0, length;
+    const char *offset;
+
+    *value = NULL;
+    while (1) {
+        offset = strchr(p, ',');
+        if (!offset) {
+            offset = p + strlen(p);
+        }
 
-    q = buf;
-    while (*p != '\0') {
-        if (*p == ',') {
-            if (*(p + 1) != ',')
-                break;
-            p++;
+        length = offset - p;
+        if (*offset != '\0' && *(offset + 1) == ',') {
+            length++;
+        }
+        if (value) {
+            *value = g_renew(char, *value, capacity + length + 1);
+            strncpy(*value + capacity, p, length);
+            (*value)[capacity + length] = '\0';
+        }
+        capacity += length;
+        if (*offset == '\0' ||
+            *(offset + 1) != ',') {
+            break;
         }
-        if (q && (q - buf) < buf_size - 1)
-            *q++ = *p;
-        p++;
+
+        p += (offset - p) + 2;
     }
-    if (q)
-        *q = '\0';
 
-    return p;
+    return offset;
 }
 
 static void parse_option_bool(const char *name, const char *value, bool *ret,
@@ -162,50 +174,43 @@ void parse_option_size(const char *name, const char *value,
 
 bool has_help_option(const char *param)
 {
-    size_t buflen = strlen(param) + 1;
-    char *buf = g_malloc(buflen);
     const char *p = param;
     bool result = false;
 
-    while (*p) {
-        p = get_opt_value(buf, buflen, p);
+    while (*p && !result) {
+        char *value;
+
+        p = get_opt_value(p, &value);
         if (*p) {
             p++;
         }
 
-        if (is_help_option(buf)) {
-            result = true;
-            goto out;
-        }
+        result = is_help_option(value);
+        g_free(value);
     }
 
-out:
-    g_free(buf);
     return result;
 }
 
-bool is_valid_option_list(const char *param)
+bool is_valid_option_list(const char *p)
 {
-    size_t buflen = strlen(param) + 1;
-    char *buf = g_malloc(buflen);
-    const char *p = param;
-    bool result = true;
+    char *value = NULL;
+    bool result = false;
 
     while (*p) {
-        p = get_opt_value(buf, buflen, p);
-        if (*p && !*++p) {
-            result = false;
+        p = get_opt_value(p, &value);
+        if ((*p && !*++p) ||
+            (!*value || *value == ',')) {
             goto out;
         }
 
-        if (!*buf || *buf == ',') {
-            result = false;
-            goto out;
-        }
+        g_free(value);
+        value = NULL;
     }
 
+    result = true;
 out:
-    g_free(buf);
+    g_free(value);
     return result;
 }
 
@@ -487,7 +492,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
     }
 }
 
-static void opt_set(QemuOpts *opts, const char *name, const char *value,
+static void opt_set(QemuOpts *opts, const char *name, char *value,
                     bool prepend, Error **errp)
 {
     QemuOpt *opt;
@@ -496,6 +501,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
 
     desc = find_desc_by_name(opts->list->desc, name);
     if (!desc && !opts_accepts_any(opts)) {
+        g_free(value);
         error_setg(errp, QERR_INVALID_PARAMETER, name);
         return;
     }
@@ -509,8 +515,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
         QTAILQ_INSERT_TAIL(&opts->head, opt, next);
     }
     opt->desc = desc;
-    opt->str = g_strdup(value);
-    assert(opt->str);
+    opt->str = value;
     qemu_opt_parse(opt, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -521,7 +526,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
 void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
                   Error **errp)
 {
-    opt_set(opts, name, value, false, errp);
+    opt_set(opts, name, g_strdup(value), false, errp);
 }
 
 void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
@@ -755,7 +760,7 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname, bool prepend, Error **errp)
 {
     char *option = NULL;
-    char value[1024];
+    char *value = NULL;
     const char *p,*pe,*pc;
     Error *local_err = NULL;
 
@@ -767,15 +772,15 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             if (p == params && firstname) {
                 /* implicitly named first option */
                 option = g_strdup(firstname);
-                p = get_opt_value(value, sizeof(value), p);
+                p = get_opt_value(p, &value);
             } else {
                 /* option without value, probably a flag */
                 p = get_opt_name(p, &option, ',');
                 if (strncmp(option, "no", 2) == 0) {
                     memmove(option, option+2, strlen(option+2)+1);
-                    pstrcpy(value, sizeof(value), "off");
+                    value = g_strdup("off");
                 } else {
-                    pstrcpy(value, sizeof(value), "on");
+                    value = g_strdup("on");
                 }
             }
         } else {
@@ -783,11 +788,12 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             p = get_opt_name(p, &option, '=');
             assert(*p == '=');
             p++;
-            p = get_opt_value(value, sizeof(value), p);
+            p = get_opt_value(p, &value);
         }
         if (strcmp(option, "id") != 0) {
             /* store and parse */
             opt_set(opts, option, value, prepend, &local_err);
+            value = NULL;
             if (local_err) {
                 error_propagate(errp, local_err);
                 goto cleanup;
@@ -797,11 +803,13 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             break;
         }
         g_free(option);
-        option = NULL;
+        g_free(value);
+        option = value = NULL;
     }
 
  cleanup:
     g_free(option);
+    g_free(value);
 }
 
 /**
@@ -820,7 +828,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
                             bool permit_abbrev, bool defaults, Error **errp)
 {
     const char *firstname;
-    char value[1024], *id = NULL;
+    char *id = NULL;
     const char *p;
     QemuOpts *opts;
     Error *local_err = NULL;
@@ -829,11 +837,9 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
     firstname = permit_abbrev ? list->implied_opt_name : NULL;
 
     if (strncmp(params, "id=", 3) == 0) {
-        get_opt_value(value, sizeof(value), params+3);
-        id = value;
+        get_opt_value(params + 3, &id);
     } else if ((p = strstr(params, ",id=")) != NULL) {
-        get_opt_value(value, sizeof(value), p+4);
-        id = value;
+        get_opt_value(p + 4, &id);
     }
 
     /*
@@ -845,6 +851,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
      */
     assert(!defaults || list->merge_lists);
     opts = qemu_opts_create(list, id, !defaults, &local_err);
+    g_free(id);
     if (opts == NULL) {
         error_propagate(errp, local_err);
         return NULL;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/3] accel: use g_strsplit for parsing accelerator names

[Philippe Mathieu-Daudé]

On 04/16/2018 08:17 AM, Daniel P. Berrangé wrote:
> Instead of re-using the get_opt_name() method from QemuOpts to split a
> string on ':', just use g_strsplit().
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  accel/accel.c         | 16 +++++++---------
>  include/qemu/option.h |  1 -
>  util/qemu-option.c    |  3 ++-
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 93e2434c87..4981e7fff3 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -70,8 +70,8 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>  
>  void configure_accelerator(MachineState *ms)
>  {
> -    const char *accel, *p;
> -    char buf[10];
> +    const char *accel;
> +    char **accel_list, **tmp;
>      int ret;
>      bool accel_initialised = false;
>      bool init_failed = false;
> @@ -83,13 +83,10 @@ void configure_accelerator(MachineState *ms)
>          accel = "tcg";
>      }
>  
> -    p = accel;
> -    while (!accel_initialised && *p != '\0') {
> -        if (*p == ':') {
> -            p++;
> -        }
> -        p = get_opt_name(buf, sizeof(buf), p, ':');
> -        acc = accel_find(buf);
> +    accel_list = g_strsplit(accel, ":", 0);
> +
> +    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        acc = accel_find(*tmp);
>          if (!acc) {
>              continue;
>          }
> @@ -107,6 +104,7 @@ void configure_accelerator(MachineState *ms)
>              accel_initialised = true;
>          }
>      }
> +    g_strfreev(accel_list);
>  
>      if (!accel_initialised) {
>          if (!init_failed) {
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 306fdb5f7a..1cfe5cbc2d 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -28,7 +28,6 @@
>  
>  #include "qemu/queue.h"
>  
> -const char *get_opt_name(char *buf, int buf_size, const char *p, char delim);
>  const char *get_opt_value(char *buf, int buf_size, const char *p);
>  
>  void parse_option_size(const char *name, const char *value,
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index d0756fda58..baca40fb94 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -49,7 +49,8 @@
>   * The return value is the position of the delimiter/zero byte after the option
>   * name in p.
>   */
> -const char *get_opt_name(char *buf, int buf_size, const char *p, char delim)
> +static const char *get_opt_name(char *buf, int buf_size, const char *p,
> +                                char delim)
>  {
>      char *q;
>  
> 

Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Paolo Bonzini]

On 16/04/2018 13:17, Daniel P. Berrangé wrote:
> A user trying out SMBIOS "OEM strings" feature reported that the data
> they are exposing to the guest was truncated at 1023 bytes, which breaks
> the app consuming in the guest. After searching for the cause I
> eventually found that the QemuOpts parsing is using fixed length 1024
> byte array for option values and 128 byte array for key names.
> 
> We can certainly debate whether it is sane to have such long command
> line argument values (it is not sane), but if the OS was capable of
> exec'ing QEMU with such an ARGV array, there is little good reason for
> imposing an artificial length restriction when parsing it. Even worse is
> that we silently truncate without reporting an error when hitting limits
> resulting in a semantically incorrect behaviour, possibly even leading
> to security flaws depending on the data that was truncated.
> 
> Thus this patch series removes the artificial length limits by killing
> the fixed length buffers.
> 
> Separately I intend to make it possible to read "OEM strings" data from
> a file, to avoid need to have long command line args.
> 
> Daniel P. Berrangé (3):
>   accel: use g_strsplit for parsing accelerator names
>   opts: don't silently truncate long parameter keys
>   opts: don't silently truncate long option values
> 
>  accel/accel.c          |  16 +++---
>  hw/i386/multiboot.c    |  33 +++++++----
>  include/qemu/option.h  |   3 +-
>  tests/test-qemu-opts.c |  18 ------
>  util/qemu-option.c     | 150 ++++++++++++++++++++++++++-----------------------
>  5 files changed, 108 insertions(+), 112 deletions(-)
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> A user trying out SMBIOS "OEM strings" feature reported that the data
> they are exposing to the guest was truncated at 1023 bytes, which breaks
> the app consuming in the guest. After searching for the cause I
> eventually found that the QemuOpts parsing is using fixed length 1024
> byte array for option values and 128 byte array for key names.
>
> We can certainly debate whether it is sane to have such long command
> line argument values (it is not sane), but if the OS was capable of
> exec'ing QEMU with such an ARGV array, there is little good reason for
> imposing an artificial length restriction when parsing it. Even worse is
> that we silently truncate without reporting an error when hitting limits
> resulting in a semantically incorrect behaviour, possibly even leading
> to security flaws depending on the data that was truncated.
>
> Thus this patch series removes the artificial length limits by killing
> the fixed length buffers.
>
> Separately I intend to make it possible to read "OEM strings" data from
> a file, to avoid need to have long command line args.

Too bad I haven't been able to complete my quest to kill QemuOpts.

As far as I know, keyval.c's only arbitrary limit is the length of a key
fragment (the things separated by '.').

Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Daniel P. Berrangé]

On Mon, Apr 16, 2018 at 06:30:45PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > A user trying out SMBIOS "OEM strings" feature reported that the data
> > they are exposing to the guest was truncated at 1023 bytes, which breaks
> > the app consuming in the guest. After searching for the cause I
> > eventually found that the QemuOpts parsing is using fixed length 1024
> > byte array for option values and 128 byte array for key names.
> >
> > We can certainly debate whether it is sane to have such long command
> > line argument values (it is not sane), but if the OS was capable of
> > exec'ing QEMU with such an ARGV array, there is little good reason for
> > imposing an artificial length restriction when parsing it. Even worse is
> > that we silently truncate without reporting an error when hitting limits
> > resulting in a semantically incorrect behaviour, possibly even leading
> > to security flaws depending on the data that was truncated.
> >
> > Thus this patch series removes the artificial length limits by killing
> > the fixed length buffers.
> >
> > Separately I intend to make it possible to read "OEM strings" data from
> > a file, to avoid need to have long command line args.
> 
> Too bad I haven't been able to complete my quest to kill QemuOpts.
> 
> As far as I know, keyval.c's only arbitrary limit is the length of a key
> fragment (the things separated by '.').

Looks like that's the same scenario I tried to address in patch 2. The
'key' part in QemuOpts has the same 128 byte limit as in the keyval.c
code. I fear that could be hit with -blockdev when setting params on
very deeply nested block backends.  On the plus side  keyval.c actually
reports an error when it hits its 128 byte limit, instead of silently
carrying on as if all was well like QemuOpts did :-)

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 :|

Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Apr 16, 2018 at 06:30:45PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > A user trying out SMBIOS "OEM strings" feature reported that the data
>> > they are exposing to the guest was truncated at 1023 bytes, which breaks
>> > the app consuming in the guest. After searching for the cause I
>> > eventually found that the QemuOpts parsing is using fixed length 1024
>> > byte array for option values and 128 byte array for key names.
>> >
>> > We can certainly debate whether it is sane to have such long command
>> > line argument values (it is not sane), but if the OS was capable of
>> > exec'ing QEMU with such an ARGV array, there is little good reason for
>> > imposing an artificial length restriction when parsing it. Even worse is
>> > that we silently truncate without reporting an error when hitting limits
>> > resulting in a semantically incorrect behaviour, possibly even leading
>> > to security flaws depending on the data that was truncated.
>> >
>> > Thus this patch series removes the artificial length limits by killing
>> > the fixed length buffers.
>> >
>> > Separately I intend to make it possible to read "OEM strings" data from
>> > a file, to avoid need to have long command line args.
>> 
>> Too bad I haven't been able to complete my quest to kill QemuOpts.
>> 
>> As far as I know, keyval.c's only arbitrary limit is the length of a key
>> fragment (the things separated by '.').
>
> Looks like that's the same scenario I tried to address in patch 2. The
> 'key' part in QemuOpts has the same 128 byte limit as in the keyval.c
> code. I fear that could be hit with -blockdev when setting params on
> very deeply nested block backends.  On the plus side  keyval.c actually
> reports an error when it hits its 128 byte limit, instead of silently
> carrying on as if all was well like QemuOpts did :-)

In keyval.c, the key (things like "a.b.c") can be arbitrarily long
(well, until g_malloc() throws in the towel), but each key fragment
("a", "b" and "c") is limited to 128 bytes.

If key length was limited there, I would've asked you to fix it there,
too.

Re: [Qemu-devel] [PATCH 0/3] Remove artificial length limits when parsing options

[Daniel P. Berrangé]

On Mon, Apr 16, 2018 at 08:13:42PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Apr 16, 2018 at 06:30:45PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > A user trying out SMBIOS "OEM strings" feature reported that the data
> >> > they are exposing to the guest was truncated at 1023 bytes, which breaks
> >> > the app consuming in the guest. After searching for the cause I
> >> > eventually found that the QemuOpts parsing is using fixed length 1024
> >> > byte array for option values and 128 byte array for key names.
> >> >
> >> > We can certainly debate whether it is sane to have such long command
> >> > line argument values (it is not sane), but if the OS was capable of
> >> > exec'ing QEMU with such an ARGV array, there is little good reason for
> >> > imposing an artificial length restriction when parsing it. Even worse is
> >> > that we silently truncate without reporting an error when hitting limits
> >> > resulting in a semantically incorrect behaviour, possibly even leading
> >> > to security flaws depending on the data that was truncated.
> >> >
> >> > Thus this patch series removes the artificial length limits by killing
> >> > the fixed length buffers.
> >> >
> >> > Separately I intend to make it possible to read "OEM strings" data from
> >> > a file, to avoid need to have long command line args.
> >> 
> >> Too bad I haven't been able to complete my quest to kill QemuOpts.
> >> 
> >> As far as I know, keyval.c's only arbitrary limit is the length of a key
> >> fragment (the things separated by '.').
> >
> > Looks like that's the same scenario I tried to address in patch 2. The
> > 'key' part in QemuOpts has the same 128 byte limit as in the keyval.c
> > code. I fear that could be hit with -blockdev when setting params on
> > very deeply nested block backends.  On the plus side  keyval.c actually
> > reports an error when it hits its 128 byte limit, instead of silently
> > carrying on as if all was well like QemuOpts did :-)
> 
> In keyval.c, the key (things like "a.b.c") can be arbitrarily long
> (well, until g_malloc() throws in the towel), but each key fragment
> ("a", "b" and "c") is limited to 128 bytes.
> 
> If key length was limited there, I would've asked you to fix it there,
> too.

Agreed, if only fragments are limited, that's fine because we know that
no code ever declares a key long enough to exceed the individual fragment
size.


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 :|