[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PULL 14/30] opts: don't silently truncate long parameter k
From: |
Paolo Bonzini |
Subject: |
[Qemu-devel] [PULL 14/30] opts: don't silently truncate long parameter keys |
Date: |
Wed, 9 May 2018 00:14:31 +0200 |
From: Daniel P. Berrangé <address@hidden>
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é <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>
Signed-off-by: Daniel P. Berrangé <address@hidden>
---
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 77dd72b..7092e21 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 baca40f..fa1a9f1 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);
}
/**
--
1.8.3.1
- [Qemu-devel] [PULL 04/30] checkpatch.pl: add common glib defines to typelist, (continued)
- [Qemu-devel] [PULL 04/30] checkpatch.pl: add common glib defines to typelist, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 06/30] memdev: remove "id" property, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 05/30] qom: allow object_get_canonical_path_component without parent, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 09/30] exec: extract address_space_translate_iommu, fix page_mask corner case, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 08/30] exec: small changes to flatview_do_translate, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 10/30] exec: reintroduce MemoryRegion caching, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 11/30] qemu-thread: always keep the posix wrapper layer, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 12/30] update-linux-headers: drop hyperv.h, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 07/30] exec: move memory access declarations to a common header, inline *_phys functions, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 13/30] accel: use g_strsplit for parsing accelerator names, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 14/30] opts: don't silently truncate long parameter keys,
Paolo Bonzini <=
- [Qemu-devel] [PULL 17/30] qemu-options: Mark -virtioconsole as deprecated, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 18/30] qemu-options: Remove remainders of the -tdf option, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 19/30] qemu-options: Bail out on unsupported options instead of silently ignoring them, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 20/30] qemu-options: Remove deprecated -no-kvm-pit-reinjection, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 16/30] target/i386: sev: fix memory leaks, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 15/30] opts: don't silently truncate long option values, Paolo Bonzini, 2018/05/08
- [Qemu-devel] [PULL 21/30] qemu-options: Remove deprecated -no-kvm-irqchip, Paolo Bonzini, 2018/05/08