* [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3)
@ 2018-07-27 15:55 Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 1/3] qstring: Fix qstring_from_substr() not to provoke int overflow Markus Armbruster
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Markus Armbruster @ 2018-07-27 15:55 UTC (permalink / raw)
To: qemu-devel
The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
are available in the Git repository at:
git://repo.or.cz/qemu/armbru.git tags/pull-qobject-2018-07-27
for you to fetch changes up to 307fb894ce0608aede990ec40ce84eaeb358c8ec:
qstring: Move qstring_from_substr()'s @end one to the right (2018-07-27 17:16:03 +0200)
----------------------------------------------------------------
QObject patches for 2018-07-27 (3.0.0-rc3)
This pull request fixes an integer overflow bug, and hardens the code
in question a bit. Abuse of QMP can make the bug crash QEMU, so it
seems worth fixing at this late stage.
----------------------------------------------------------------
Markus Armbruster (2):
qstring: Assert size calculations don't overflow
qstring: Move qstring_from_substr()'s @end one to the right
liujunjie (1):
qstring: Fix qstring_from_substr() not to provoke int overflow
block/blkdebug.c | 2 +-
block/blkverify.c | 2 +-
block/nbd.c | 2 +-
include/qapi/qmp/qstring.h | 2 +-
qobject/qstring.c | 12 ++++++++----
tests/check-qobject.c | 2 +-
tests/check-qstring.c | 2 +-
7 files changed, 14 insertions(+), 10 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PULL 1/3] qstring: Fix qstring_from_substr() not to provoke int overflow
2018-07-27 15:55 [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Markus Armbruster
@ 2018-07-27 15:56 ` Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 2/3] qstring: Assert size calculations don't overflow Markus Armbruster
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2018-07-27 15:56 UTC (permalink / raw)
To: qemu-devel; +Cc: liujunjie
From: liujunjie <liujunjie23@huawei.com>
qstring_from_substr() parameters @start and @end are of type int.
blkdebug_parse_filename(), blkverify_parse_filename(), nbd_parse_uri(),
and qstring_from_str() pass @end values of type size_t or ptrdiff_t.
Values exceeding INT_MAX get truncated, with possibly disastrous
results.
Such huge substrings seem unlikely, but we found one in a core dump,
where "info tlb" executed via QMP's human-monitor-command apparently
produced 35 GiB of output.
Fix by changing the parameters size_t.
Signed-off-by: liujunjie <liujunjie23@huawei.com>
Message-Id: <20180724134339.17832-1-liujunjie23@huawei.com>
---
include/qapi/qmp/qstring.h | 2 +-
qobject/qstring.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index b3b3d444d2..3e83e3a95d 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -24,7 +24,7 @@ struct QString {
QString *qstring_new(void);
QString *qstring_from_str(const char *str);
-QString *qstring_from_substr(const char *str, int start, int end);
+QString *qstring_from_substr(const char *str, size_t start, size_t end);
size_t qstring_get_length(const QString *qstring);
const char *qstring_get_str(const QString *qstring);
const char *qstring_get_try_str(const QString *qstring);
diff --git a/qobject/qstring.c b/qobject/qstring.c
index afca54b47a..18b8eb82f8 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -37,7 +37,7 @@ size_t qstring_get_length(const QString *qstring)
*
* Return string reference
*/
-QString *qstring_from_substr(const char *str, int start, int end)
+QString *qstring_from_substr(const char *str, size_t start, size_t end)
{
QString *qstring;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PULL 2/3] qstring: Assert size calculations don't overflow
2018-07-27 15:55 [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 1/3] qstring: Fix qstring_from_substr() not to provoke int overflow Markus Armbruster
@ 2018-07-27 15:56 ` Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 3/3] qstring: Move qstring_from_substr()'s @end one to the right Markus Armbruster
2018-07-27 17:19 ` [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Peter Maydell
3 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2018-07-27 15:56 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20180727062204.10401-2-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qobject/qstring.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 18b8eb82f8..1bb7784a88 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -41,17 +41,19 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
{
QString *qstring;
+ assert(start <= end + 1);
+
qstring = g_malloc(sizeof(*qstring));
qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
qstring->length = end - start + 1;
qstring->capacity = qstring->length;
+ assert(qstring->capacity < SIZE_MAX);
qstring->string = g_malloc(qstring->capacity + 1);
memcpy(qstring->string, str + start, qstring->length);
qstring->string[qstring->length] = 0;
-
return qstring;
}
@@ -68,7 +70,9 @@ QString *qstring_from_str(const char *str)
static void capacity_increase(QString *qstring, size_t len)
{
if (qstring->capacity < (qstring->length + len)) {
+ assert(len <= SIZE_MAX - qstring->capacity);
qstring->capacity += len;
+ assert(qstring->capacity <= SIZE_MAX / 2);
qstring->capacity *= 2; /* use exponential growth */
qstring->string = g_realloc(qstring->string, qstring->capacity + 1);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PULL 3/3] qstring: Move qstring_from_substr()'s @end one to the right
2018-07-27 15:55 [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 1/3] qstring: Fix qstring_from_substr() not to provoke int overflow Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 2/3] qstring: Assert size calculations don't overflow Markus Armbruster
@ 2018-07-27 15:56 ` Markus Armbruster
2018-07-27 17:19 ` [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Peter Maydell
3 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2018-07-27 15:56 UTC (permalink / raw)
To: qemu-devel
qstring_from_substr() takes the index of the substring's first and
last character. qstring_from_substr(s, 0, SIZE_MAX) denotes an empty
substring. Awkward.
Shift the end index one to the right. This simplifies both
qstring_from_substr() and its callers.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180727062204.10401-3-armbru@redhat.com>
---
block/blkdebug.c | 2 +-
block/blkverify.c | 2 +-
block/nbd.c | 2 +-
qobject/qstring.c | 6 +++---
tests/check-qobject.c | 2 +-
tests/check-qstring.c | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 0457bf5b66..0759452925 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -305,7 +305,7 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
if (c != filename) {
QString *config_path;
- config_path = qstring_from_substr(filename, 0, c - filename - 1);
+ config_path = qstring_from_substr(filename, 0, c - filename);
qdict_put(options, "config", config_path);
}
diff --git a/block/blkverify.c b/block/blkverify.c
index da97ee5927..89bf4386e3 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -80,7 +80,7 @@ static void blkverify_parse_filename(const char *filename, QDict *options,
}
/* TODO Implement option pass-through and set raw.filename here */
- raw_path = qstring_from_substr(filename, 0, c - filename - 1);
+ raw_path = qstring_from_substr(filename, 0, c - filename);
qdict_put(options, "x-raw", raw_path);
/* TODO Allow multi-level nesting and set file.filename here */
diff --git a/block/nbd.c b/block/nbd.c
index b198ad775f..e87699fb73 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -109,7 +109,7 @@ static int nbd_parse_uri(const char *filename, QDict *options)
/* strip braces from literal IPv6 address */
if (uri->server[0] == '[') {
host = qstring_from_substr(uri->server, 1,
- strlen(uri->server) - 2);
+ strlen(uri->server) - 1);
} else {
host = qstring_from_str(uri->server);
}
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 1bb7784a88..0f1510e792 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -41,12 +41,12 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
{
QString *qstring;
- assert(start <= end + 1);
+ assert(start <= end);
qstring = g_malloc(sizeof(*qstring));
qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
- qstring->length = end - start + 1;
+ qstring->length = end - start;
qstring->capacity = qstring->length;
assert(qstring->capacity < SIZE_MAX);
@@ -64,7 +64,7 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
*/
QString *qstring_from_str(const char *str)
{
- return qstring_from_substr(str, 0, strlen(str) - 1);
+ return qstring_from_substr(str, 0, strlen(str));
}
static void capacity_increase(QString *qstring, size_t len)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 16ccbde82c..593c3a0618 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -154,7 +154,7 @@ static void qobject_is_equal_string_test(void)
str_case = qstring_from_str("Foo");
/* Should yield "foo" */
- str_built = qstring_from_substr("form", 0, 1);
+ str_built = qstring_from_substr("form", 0, 2);
qstring_append_chr(str_built, 'o');
check_unequal(str_base, str_whitespace_0, str_whitespace_1,
diff --git a/tests/check-qstring.c b/tests/check-qstring.c
index f11a7a8605..2d079921e3 100644
--- a/tests/check-qstring.c
+++ b/tests/check-qstring.c
@@ -66,7 +66,7 @@ static void qstring_from_substr_test(void)
{
QString *qs;
- qs = qstring_from_substr("virtualization", 3, 9);
+ qs = qstring_from_substr("virtualization", 3, 10);
g_assert(qs != NULL);
g_assert(strcmp(qstring_get_str(qs), "tualiza") == 0);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3)
2018-07-27 15:55 [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Markus Armbruster
` (2 preceding siblings ...)
2018-07-27 15:56 ` [Qemu-devel] [PULL 3/3] qstring: Move qstring_from_substr()'s @end one to the right Markus Armbruster
@ 2018-07-27 17:19 ` Peter Maydell
2018-07-28 7:16 ` Markus Armbruster
3 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2018-07-27 17:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers
On 27 July 2018 at 16:55, Markus Armbruster <armbru@redhat.com> wrote:
> The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
>
> Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
>
> are available in the Git repository at:
>
> git://repo.or.cz/qemu/armbru.git tags/pull-qobject-2018-07-27
>
> for you to fetch changes up to 307fb894ce0608aede990ec40ce84eaeb358c8ec:
>
> qstring: Move qstring_from_substr()'s @end one to the right (2018-07-27 17:16:03 +0200)
>
> ----------------------------------------------------------------
> QObject patches for 2018-07-27 (3.0.0-rc3)
>
> This pull request fixes an integer overflow bug, and hardens the code
> in question a bit. Abuse of QMP can make the bug crash QEMU, so it
> seems worth fixing at this late stage.
>
> ----------------------------------------------------------------
> Markus Armbruster (2):
> qstring: Assert size calculations don't overflow
> qstring: Move qstring_from_substr()'s @end one to the right
>
> liujunjie (1):
> qstring: Fix qstring_from_substr() not to provoke int overflow
Hi -- this passes my buildtests, but the commit from liujunjie
seems to be missing your maintainer signed-off-by (and possibly
a reviewed-by tag ?) Can I ask you to fix that up and resend,
please?
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3)
2018-07-27 17:19 ` [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Peter Maydell
@ 2018-07-28 7:16 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2018-07-28 7:16 UTC (permalink / raw)
To: Peter Maydell; +Cc: Markus Armbruster, QEMU Developers
Peter Maydell <peter.maydell@linaro.org> writes:
> On 27 July 2018 at 16:55, Markus Armbruster <armbru@redhat.com> wrote:
>> The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
>>
>> Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
>>
>> are available in the Git repository at:
>>
>> git://repo.or.cz/qemu/armbru.git tags/pull-qobject-2018-07-27
>>
>> for you to fetch changes up to 307fb894ce0608aede990ec40ce84eaeb358c8ec:
>>
>> qstring: Move qstring_from_substr()'s @end one to the right (2018-07-27 17:16:03 +0200)
>>
>> ----------------------------------------------------------------
>> QObject patches for 2018-07-27 (3.0.0-rc3)
>>
>> This pull request fixes an integer overflow bug, and hardens the code
>> in question a bit. Abuse of QMP can make the bug crash QEMU, so it
>> seems worth fixing at this late stage.
>>
>> ----------------------------------------------------------------
>> Markus Armbruster (2):
>> qstring: Assert size calculations don't overflow
>> qstring: Move qstring_from_substr()'s @end one to the right
>>
>> liujunjie (1):
>> qstring: Fix qstring_from_substr() not to provoke int overflow
>
>
> Hi -- this passes my buildtests, but the commit from liujunjie
> seems to be missing your maintainer signed-off-by (and possibly
> a reviewed-by tag ?) Can I ask you to fix that up and resend,
> please?
My apologies. v2 sent.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-28 7:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-27 15:55 [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 1/3] qstring: Fix qstring_from_substr() not to provoke int overflow Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 2/3] qstring: Assert size calculations don't overflow Markus Armbruster
2018-07-27 15:56 ` [Qemu-devel] [PULL 3/3] qstring: Move qstring_from_substr()'s @end one to the right Markus Armbruster
2018-07-27 17:19 ` [Qemu-devel] [PULL 0/3] QObject patches for 2018-07-27 (3.0.0-rc3) Peter Maydell
2018-07-28 7:16 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.