[Qemu-devel] [PATCH] qstring: Fix integer overflow

[Qemu-devel] [PATCH] qstring: Fix integer overflow

[liujunjie]

From: l00425170 <liujunjie23@huawei.com>

The incoming parameters "start" and "end" is int type in
qstring_from_substr(), but this function can be called by
qstring_from_str, which is size_t type in strlen(str).
It may result in coredump when called g_malloc later.
One scene to triger is to call hmp "into tlb", which may have
too long length of string.

Signed-off-by: l00425170 <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 b3b3d44..3e83e3a 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 afca54b..18b8eb8 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;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Markus Armbruster]

liujunjie <liujunjie23@huawei.com> writes:

> From: l00425170 <liujunjie23@huawei.com>
>
> The incoming parameters "start" and "end" is int type in
> qstring_from_substr(), but this function can be called by
> qstring_from_str, which is size_t type in strlen(str).

Yes, there's a conversion from size_t to int in

    return qstring_from_substr(str, 0, strlen(str) - 1);

The conversion truncates when strlen(str) - 1 exceeds INT_MAX.

strlen(str) - 1 wraps around to SIZE_MAX when @str is empty.

> It may result in coredump when called g_malloc later.
> One scene to triger is to call hmp "into tlb", which may have
> too long length of string.

Really?  How exactly can this happen?  Please explain step by step.

Aside: @end is only used as @end + 1, and all callers pass some X - 1.
Both the addition and the subtraction can overflow.  Changing the
function to take the index behind the last character instead of the
index of the last character would almost certainly simplify things.  Not
this patch's problem.

>
> Signed-off-by: l00425170 <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 b3b3d44..3e83e3a 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 afca54b..18b8eb8 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;

The patch makes sense, but the commit message makes claims that need to
be substantiated.

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[liujunjie (A)]

Thanks for your reply.
> Really?  How exactly can this happen?  Please explain step by step.
There exist a qemu core related to this. You have mention that "The conversion truncates when strlen(str) - 1 exceeds INT_MAX".
Later in function qstring_from_substr, this truncated "end" will be assigned to "qstring->length" again, which is size_t. This is the key point why qemu coredumped.
Because when "end" is truncated, it can be negative number. If we assign a negative number to a size_t variable, this size_t variable can become very large.
At last, we call g_malloc to try to alloc a large number of member which cannot success. So qemu coredump.
In my example, use gdb to debug function qstring_from_substr, I can get the following message.
(gdb) p	qstring->length
$4 = 18446744072383980732  (too large to allocate)
(gdb) p	(int) (qstring->length)
$5 = -1325570884
(gdb) p/x (int)	qstring->length
$6 = 0xb0fd64bc
(gdb) p/x qstring->length
$7 = 0xffffffffb0fd64bc
(gdb) p	end
$8 = <optimized out>

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Monday, July 23, 2018 8:48 PM
> To: liujunjie (A) <liujunjie23@huawei.com>
> Cc: wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; Huangweidong (C)
> <weidong.huang@huawei.com>; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
> 
> liujunjie <liujunjie23@huawei.com> writes:
> 
> > From: l00425170 <liujunjie23@huawei.com>
> >
> > The incoming parameters "start" and "end" is int type in
> > qstring_from_substr(), but this function can be called by
> > qstring_from_str, which is size_t type in strlen(str).
> 
> Yes, there's a conversion from size_t to int in
> 
>     return qstring_from_substr(str, 0, strlen(str) - 1);
> 
> The conversion truncates when strlen(str) - 1 exceeds INT_MAX.
> 
> strlen(str) - 1 wraps around to SIZE_MAX when @str is empty.
> 
> > It may result in coredump when called g_malloc later.
> > One scene to triger is to call hmp "into tlb", which may have too long
> > length of string.
> 
> Really?  How exactly can this happen?  Please explain step by step.
> 
> Aside: @end is only used as @end + 1, and all callers pass some X - 1.
> Both the addition and the subtraction can overflow.  Changing the function to
> take the index behind the last character instead of the index of the last
> character would almost certainly simplify things.  Not this patch's problem.
> 
> >
> > Signed-off-by: l00425170 <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 b3b3d44..3e83e3a 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 afca54b..18b8eb8 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;
> 
> The patch makes sense, but the commit message makes claims that need to be
> substantiated.

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Markus Armbruster]

"liujunjie (A)" <liujunjie23@huawei.com> writes:

> Thanks for your reply.
>> Really?  How exactly can this happen?  Please explain step by step.
> There exist a qemu core related to this. You have mention that "The conversion truncates when strlen(str) - 1 exceeds INT_MAX".
> Later in function qstring_from_substr, this truncated "end" will be assigned to "qstring->length" again, which is size_t. This is the key point why qemu coredumped.
> Because when "end" is truncated, it can be negative number. If we assign a negative number to a size_t variable, this size_t variable can become very large.
> At last, we call g_malloc to try to alloc a large number of member which cannot success. So qemu coredump.
> In my example, use gdb to debug function qstring_from_substr, I can get the following message.
> (gdb) p	qstring->length
> $4 = 18446744072383980732  (too large to allocate)
> (gdb) p	(int) (qstring->length)
> $5 = -1325570884
> (gdb) p/x (int)	qstring->length
> $6 = 0xb0fd64bc
> (gdb) p/x qstring->length
> $7 = 0xffffffffb0fd64bc
> (gdb) p	end
> $8 = <optimized out>

Can you provide a stack backtrace, too?

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[liujunjie (A)]

The stack backtrace is as follows:
(gdb) bt
#0  0x00007f1dc3c7b091 in _g_log_abort () from /usr/lib64/libglib-2.0.so.0
#1  0x00007f1dc3c7c0bd in g_log_default_handler () from /usr/lib64/libglib-2.0.so.0
#2  0x00007f1dc3c7c341 in g_logv () from /usr/lib64/libglib-2.0.so.0
#3  0x00007f1dc3c7c5cf in g_log () from /usr/lib64/libglib-2.0.so.0
#4  0x00007f1dc3c7ac4c in g_malloc () from /usr/lib64/libglib-2.0.so.0
#5  0x00000000008300b7 in qstring_from_substr (
    str=0x7f13a2e67010 "00000000777b8000: 0000000003083000 ----A--U-\r\n00000000777b9000: 0000000005984000 ----A--U-\r\n00000000777ba000: 0000000005985000 ----A--U-\r\n00000000777bb000: 0000000003086000 ----A--U-\r\n00000000777bc000"..., start=start@entry=0, end=<optimized out>) at qobject/qstring.c:51
#6  0x0000000000830113 in qstring_from_str (str=<optimized out>) at qobject/qstring.c:66
#7  0x000000000082be98 in qobject_output_type_str (v=<optimized out>, name=0x89703b "unused", obj=0x7ffff0135f98, errp=<optimized out>)
    at qapi/qobject_output_visitor.c:172
#8  0x0000000000829d2c in visit_type_str (v=v@entry=0x4d9d940, name=name@entry=0x89703b "unused", obj=obj@entry=0x7ffff0135f98, errp=errp@entry=0x7ffff0135fa8)
    at qapi/qapi_visit_core.c:291
#9  0x0000000000576135 in qmp_marshal_output_str (
    ret_in=0x7f13a2e67010 "00000000777b8000: 0000000003083000 ----A--U-\r\n00000000777b9000: 0000000005984000 ----A--U-\r\n00000000777ba000: 0000000005985000 ----A--U-\r\n00000000777bb000: 0000000003086000 ----A--U-\r\n00000000777bc000"..., ret_out=ret_out@entry=0x7ffff0136068, errp=errp@entry=0x7ffff0135fe8) at qmp-marshal.c:2022
#10 0x00000000005762cb in qmp_marshal_human_monitor_command (args=<optimized out>, ret=0x7ffff0136068, errp=0x7ffff0136060) at qmp-marshal.c:2059
#11 0x000000000082c897 in do_qmp_dispatch (request=request@entry=0x1fcda50, errp=errp@entry=0x7ffff01360b8) at qapi/qmp_dispatch.c:114
#12 0x000000000082caeb in qmp_dispatch (request=request@entry=0x1fcda50) at qapi/qmp_dispatch.c:141
#13 0x000000000045e0d2 in handle_qmp_command (parser=<optimized out>, tokens=<optimized out>) at /usr/src/debug/qemu-kvm-2.8.1/monitor.c:3907
#14 0x000000000083355e in json_message_process_token (lexer=0x1a608e8, input=0x19f26e0, type=JSON_RCURLY, x=94, y=263) at qobject/json_streamer.c:105
#15 0x0000000000861650 in json_lexer_feed_char (lexer=lexer@entry=0x1a608e8, ch=125 '}', flush=flush@entry=false) at qobject/json_lexer.c:319
#16 0x0000000000861766 in json_lexer_feed (lexer=0x1a608e8, buffer=<optimized out>, size=<optimized out>) at qobject/json_lexer.c:369
#17 0x0000000000833661 in json_message_parser_feed (parser=<optimized out>, buffer=<optimized out>, size=<optimized out>) at qobject/json_streamer.c:124
#18 0x000000000045cc32 in monitor_qmp_read (opaque=<optimized out>, buf=<optimized out>, size=<optimized out>) at /usr/src/debug/qemu-kvm-2.8.1/monitor.c:3937
#19 0x00000000005652f3 in tcp_chr_read (chan=<optimized out>, cond=<optimized out>, opaque=0x1a5c020) at qemu_char.c:3253
#20 0x00007f1dc3c75609 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
#21 0x0000000000779a06 in glib_pollfds_poll () at main_loop.c:228
#22 0x0000000000779aab in os_host_main_loop_wait (timeout=4830848) at main_loop.c:273
#23 0x0000000000779bd5 in main_loop_wait (nonblocking=nonblocking@entry=0) at main_loop.c:521
#24 0x0000000000570851 in main_loop () at vl.c:2100
#25 0x0000000000420d1e in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4980

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Monday, July 23, 2018 11:46 PM
> To: liujunjie (A) <liujunjie23@huawei.com>
> Cc: wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; Huangweidong (C)
> <weidong.huang@huawei.com>; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
> 
> "liujunjie (A)" <liujunjie23@huawei.com> writes:
> 
> > Thanks for your reply.
> >> Really?  How exactly can this happen?  Please explain step by step.
> > There exist a qemu core related to this. You have mention that "The
> conversion truncates when strlen(str) - 1 exceeds INT_MAX".
> > Later in function qstring_from_substr, this truncated "end" will be assigned
> to "qstring->length" again, which is size_t. This is the key point why qemu
> coredumped.
> > Because when "end" is truncated, it can be negative number. If we assign a
> negative number to a size_t variable, this size_t variable can become very
> large.
> > At last, we call g_malloc to try to alloc a large number of member which
> cannot success. So qemu coredump.
> > In my example, use gdb to debug function qstring_from_substr, I can get the
> following message.
> > (gdb) p	qstring->length
> > $4 = 18446744072383980732  (too large to allocate)
> > (gdb) p	(int) (qstring->length)
> > $5 = -1325570884
> > (gdb) p/x (int)	qstring->length
> > $6 = 0xb0fd64bc
> > (gdb) p/x qstring->length
> > $7 = 0xffffffffb0fd64bc
> > (gdb) p	end
> > $8 = <optimized out>
> 
> Can you provide a stack backtrace, too?

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Markus Armbruster]

"liujunjie (A)" <liujunjie23@huawei.com> writes:

> The stack backtrace is as follows:
> (gdb) bt
> #0  0x00007f1dc3c7b091 in _g_log_abort () from /usr/lib64/libglib-2.0.so.0
> #1  0x00007f1dc3c7c0bd in g_log_default_handler () from /usr/lib64/libglib-2.0.so.0
> #2  0x00007f1dc3c7c341 in g_logv () from /usr/lib64/libglib-2.0.so.0
> #3  0x00007f1dc3c7c5cf in g_log () from /usr/lib64/libglib-2.0.so.0
> #4  0x00007f1dc3c7ac4c in g_malloc () from /usr/lib64/libglib-2.0.so.0
> #5  0x00000000008300b7 in qstring_from_substr (
>     str=0x7f13a2e67010 "00000000777b8000: 0000000003083000 ----A--U-\r\n00000000777b9000: 0000000005984000 ----A--U-\r\n00000000777ba000: 0000000005985000 ----A--U-\r\n00000000777bb000: 0000000003086000 ----A--U-\r\n00000000777bc000"..., start=start@entry=0, end=<optimized out>) at qobject/qstring.c:51
> #6  0x0000000000830113 in qstring_from_str (str=<optimized out>) at qobject/qstring.c:66
> #7  0x000000000082be98 in qobject_output_type_str (v=<optimized out>, name=0x89703b "unused", obj=0x7ffff0135f98, errp=<optimized out>)
>     at qapi/qobject_output_visitor.c:172
> #8  0x0000000000829d2c in visit_type_str (v=v@entry=0x4d9d940, name=name@entry=0x89703b "unused", obj=obj@entry=0x7ffff0135f98, errp=errp@entry=0x7ffff0135fa8)
>     at qapi/qapi_visit_core.c:291
> #9  0x0000000000576135 in qmp_marshal_output_str (
>     ret_in=0x7f13a2e67010 "00000000777b8000: 0000000003083000 ----A--U-\r\n00000000777b9000: 0000000005984000 ----A--U-\r\n00000000777ba000: 0000000005985000 ----A--U-\r\n00000000777bb000: 0000000003086000 ----A--U-\r\n00000000777bc000"..., ret_out=ret_out@entry=0x7ffff0136068, errp=errp@entry=0x7ffff0135fe8) at qmp-marshal.c:2022
> #10 0x00000000005762cb in qmp_marshal_human_monitor_command (args=<optimized out>, ret=0x7ffff0136068, errp=0x7ffff0136060) at qmp-marshal.c:2059
> #11 0x000000000082c897 in do_qmp_dispatch (request=request@entry=0x1fcda50, errp=errp@entry=0x7ffff01360b8) at qapi/qmp_dispatch.c:114
> #12 0x000000000082caeb in qmp_dispatch (request=request@entry=0x1fcda50) at qapi/qmp_dispatch.c:141
> #13 0x000000000045e0d2 in handle_qmp_command (parser=<optimized out>, tokens=<optimized out>) at /usr/src/debug/qemu-kvm-2.8.1/monitor.c:3907
[...]

The code is trying to marshall the return value of
qmp_human_monitor_command().  It's @ret_in in qmp_marshal_output_str()
(frame#9, abbreviated by GDB), and @str in qstring_from_substr()
(frame#5).  Also @str in qstring_from_str() (frame#6), but GDB can't see
it there.  Sadly, GDB can't see shows qstring_from_substr()'s @end,
either.  However, you previously examined qstring->length there:

>> > (gdb) p/x qstring->length
>> > $7 = 0xffffffffb0fd64bc
>> > (gdb) p	end
>> > $8 = <optimized out>

We know

    qstring->length = end - start + 1;

If GDB shows the true value (always in doubt for optimized code), then
@end must be qstring->length - 1, because @start is zero.  But that's
not plausible at all!  It's almost 16 exabyte.

I suspect GDB is lying to you.  Please show us the complete string, like
this:

    (gdb) set print elements unlimited
    (gdb) print str

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> "liujunjie (A)" <liujunjie23@huawei.com> writes:
>
>> The stack backtrace is as follows:
>> (gdb) bt
>> #0  0x00007f1dc3c7b091 in _g_log_abort () from /usr/lib64/libglib-2.0.so.0
>> #1  0x00007f1dc3c7c0bd in g_log_default_handler () from /usr/lib64/libglib-2.0.so.0
>> #2  0x00007f1dc3c7c341 in g_logv () from /usr/lib64/libglib-2.0.so.0
>> #3  0x00007f1dc3c7c5cf in g_log () from /usr/lib64/libglib-2.0.so.0
>> #4  0x00007f1dc3c7ac4c in g_malloc () from /usr/lib64/libglib-2.0.so.0
>> #5  0x00000000008300b7 in qstring_from_substr (
>>     str=0x7f13a2e67010 "00000000777b8000: 0000000003083000 ----A--U-\r\n00000000777b9000: 0000000005984000 ----A--U-\r\n00000000777ba000: 0000000005985000 ----A--U-\r\n00000000777bb000: 0000000003086000 ----A--U-\r\n00000000777bc000"..., start=start@entry=0, end=<optimized out>) at qobject/qstring.c:51
>> #6  0x0000000000830113 in qstring_from_str (str=<optimized out>) at qobject/qstring.c:66
>> #7  0x000000000082be98 in qobject_output_type_str (v=<optimized out>, name=0x89703b "unused", obj=0x7ffff0135f98, errp=<optimized out>)
>>     at qapi/qobject_output_visitor.c:172
>> #8  0x0000000000829d2c in visit_type_str (v=v@entry=0x4d9d940, name=name@entry=0x89703b "unused", obj=obj@entry=0x7ffff0135f98, errp=errp@entry=0x7ffff0135fa8)
>>     at qapi/qapi_visit_core.c:291
>> #9  0x0000000000576135 in qmp_marshal_output_str (
>>     ret_in=0x7f13a2e67010 "00000000777b8000: 0000000003083000 ----A--U-\r\n00000000777b9000: 0000000005984000 ----A--U-\r\n00000000777ba000: 0000000005985000 ----A--U-\r\n00000000777bb000: 0000000003086000 ----A--U-\r\n00000000777bc000"..., ret_out=ret_out@entry=0x7ffff0136068, errp=errp@entry=0x7ffff0135fe8) at qmp-marshal.c:2022
>> #10 0x00000000005762cb in qmp_marshal_human_monitor_command (args=<optimized out>, ret=0x7ffff0136068, errp=0x7ffff0136060) at qmp-marshal.c:2059
>> #11 0x000000000082c897 in do_qmp_dispatch (request=request@entry=0x1fcda50, errp=errp@entry=0x7ffff01360b8) at qapi/qmp_dispatch.c:114
>> #12 0x000000000082caeb in qmp_dispatch (request=request@entry=0x1fcda50) at qapi/qmp_dispatch.c:141
>> #13 0x000000000045e0d2 in handle_qmp_command (parser=<optimized out>, tokens=<optimized out>) at /usr/src/debug/qemu-kvm-2.8.1/monitor.c:3907
> [...]
>
> The code is trying to marshall the return value of
> qmp_human_monitor_command().  It's @ret_in in qmp_marshal_output_str()
> (frame#9, abbreviated by GDB), and @str in qstring_from_substr()
> (frame#5).  Also @str in qstring_from_str() (frame#6), but GDB can't see
> it there.  Sadly, GDB can't see shows qstring_from_substr()'s @end,
> either.  However, you previously examined qstring->length there:
>
>>> > (gdb) p/x qstring->length
>>> > $7 = 0xffffffffb0fd64bc
>>> > (gdb) p	end
>>> > $8 = <optimized out>
>
> We know
>
>     qstring->length = end - start + 1;
>
> If GDB shows the true value (always in doubt for optimized code), then
> @end must be qstring->length - 1, because @start is zero.  But that's
> not plausible at all!  It's almost 16 exabyte.
>
> I suspect GDB is lying to you.  Please show us the complete string, like
> this:
>
>     (gdb) set print elements unlimited
>     (gdb) print str

Actually, I'm interested only in the true length of @str, and the print
is just a simple way to find it.  No need to post the output of print if
it's inconveniently long.

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[liujunjie (A)]

Even using gdb command: set print elements 0, is still too large to print the whole string.
So I try to obtain the string in another way.
After several attempts(not easy in fact), I finally obtain the real length. The way is as follows:
(gdb) p (int *)str
$1 = (int *) 0x7f13a2e67010
(gdb) p *(char*) (0x7f13a2e67010 + 0x8B0FD63FF)@192
$2 = "--W\r\nffffffffffd17000: 00000000fec00000 XG-DACT-W\r\nffffffffffd18000: ", '0' <repeats 11 times>, "77000 XG-DA---W\r\nffffffffffd19000: ", '0 "78000 XG-DA---W\r\nffffffffffd1a000: ", '0' <repeats 11 times>, "79000 XG-DA---W\r\n\000\000"
With \000 at the end, we can find out the length is 0x8B0FD63FF + 192 - 2, that is 37329134781.
With this length, we can write a simple c code demo to reproduce the result we obtain before. Such as:
-----------------------------
#include<stdio.h>
int main()
{
    char * tmp = "";
    size_t a =  37329134781;
    int end = a;
    size_t b = end;
    printf("%zu", b)
    return 0;
}
-----------------------------


> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, July 24, 2018 4:47 PM
> To: Markus Armbruster <armbru@redhat.com>
> Cc: liujunjie (A) <liujunjie23@huawei.com>; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> Huangweidong (C) <weidong.huang@huawei.com>; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > "liujunjie (A)" <liujunjie23@huawei.com> writes:
> >
> >> The stack backtrace is as follows:
> >> (gdb) bt
> >> #0  0x00007f1dc3c7b091 in _g_log_abort () from
> >> /usr/lib64/libglib-2.0.so.0
> >> #1  0x00007f1dc3c7c0bd in g_log_default_handler () from
> >> /usr/lib64/libglib-2.0.so.0
> >> #2  0x00007f1dc3c7c341 in g_logv () from /usr/lib64/libglib-2.0.so.0
> >> #3  0x00007f1dc3c7c5cf in g_log () from /usr/lib64/libglib-2.0.so.0
> >> #4  0x00007f1dc3c7ac4c in g_malloc () from
> >> /usr/lib64/libglib-2.0.so.0
> >> #5  0x00000000008300b7 in qstring_from_substr (
> >>     str=0x7f13a2e67010 "00000000777b8000: 0000000003083000
> >> ----A--U-\r\n00000000777b9000: 0000000005984000
> >> ----A--U-\r\n00000000777ba000: 0000000005985000
> >> ----A--U-\r\n00000000777bb000: 0000000003086000
> >> ----A--U-\r\n00000000777bc000"..., start=start@entry=0,
> >> end=<optimized out>) at qobject/qstring.c:51
> >> #6  0x0000000000830113 in qstring_from_str (str=<optimized out>) at
> >> qobject/qstring.c:66
> >> #7  0x000000000082be98 in qobject_output_type_str (v=<optimized out>,
> name=0x89703b "unused", obj=0x7ffff0135f98, errp=<optimized out>)
> >>     at qapi/qobject_output_visitor.c:172
> >> #8  0x0000000000829d2c in visit_type_str (v=v@entry=0x4d9d940,
> name=name@entry=0x89703b "unused", obj=obj@entry=0x7ffff0135f98,
> errp=errp@entry=0x7ffff0135fa8)
> >>     at qapi/qapi_visit_core.c:291
> >> #9  0x0000000000576135 in qmp_marshal_output_str (
> >>     ret_in=0x7f13a2e67010 "00000000777b8000: 0000000003083000
> >> ----A--U-\r\n00000000777b9000: 0000000005984000
> >> ----A--U-\r\n00000000777ba000: 0000000005985000
> >> ----A--U-\r\n00000000777bb000: 0000000003086000
> >> ----A--U-\r\n00000000777bc000"...,
> >> ret_out=ret_out@entry=0x7ffff0136068, errp=errp@entry=0x7ffff0135fe8)
> >> at qmp-marshal.c:2022
> >> #10 0x00000000005762cb in qmp_marshal_human_monitor_command
> >> (args=<optimized out>, ret=0x7ffff0136068, errp=0x7ffff0136060) at
> >> qmp-marshal.c:2059
> >> #11 0x000000000082c897 in do_qmp_dispatch
> >> (request=request@entry=0x1fcda50, errp=errp@entry=0x7ffff01360b8) at
> >> qapi/qmp_dispatch.c:114
> >> #12 0x000000000082caeb in qmp_dispatch
> >> (request=request@entry=0x1fcda50) at qapi/qmp_dispatch.c:141
> >> #13 0x000000000045e0d2 in handle_qmp_command (parser=<optimized
> out>,
> >> tokens=<optimized out>) at
> >> /usr/src/debug/qemu-kvm-2.8.1/monitor.c:3907
> > [...]
> >
> > The code is trying to marshall the return value of
> > qmp_human_monitor_command().  It's @ret_in in
> qmp_marshal_output_str()
> > (frame#9, abbreviated by GDB), and @str in qstring_from_substr()
> > (frame#5).  Also @str in qstring_from_str() (frame#6), but GDB can't
> > see it there.  Sadly, GDB can't see shows qstring_from_substr()'s
> > @end, either.  However, you previously examined qstring->length there:
> >
> >>> > (gdb) p/x qstring->length
> >>> > $7 = 0xffffffffb0fd64bc
> >>> > (gdb) p	end
> >>> > $8 = <optimized out>
> >
> > We know
> >
> >     qstring->length = end - start + 1;
> >
> > If GDB shows the true value (always in doubt for optimized code), then
> > @end must be qstring->length - 1, because @start is zero.  But that's
> > not plausible at all!  It's almost 16 exabyte.
> >
> > I suspect GDB is lying to you.  Please show us the complete string,
> > like
> > this:
> >
> >     (gdb) set print elements unlimited
> >     (gdb) print str
> 
> Actually, I'm interested only in the true length of @str, and the print is just a
> simple way to find it.  No need to post the output of print if it's inconveniently
> long.

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Markus Armbruster]

"liujunjie (A)" <liujunjie23@huawei.com> writes:

> Even using gdb command: set print elements 0, is still too large to print the whole string.
> So I try to obtain the string in another way.
> After several attempts(not easy in fact), I finally obtain the real length. The way is as follows:
> (gdb) p (int *)str
> $1 = (int *) 0x7f13a2e67010
> (gdb) p *(char*) (0x7f13a2e67010 + 0x8B0FD63FF)@192
> $2 = "--W\r\nffffffffffd17000: 00000000fec00000 XG-DACT-W\r\nffffffffffd18000: ", '0' <repeats 11 times>, "77000 XG-DA---W\r\nffffffffffd19000: ", '0 "78000 XG-DA---W\r\nffffffffffd1a000: ", '0' <repeats 11 times>, "79000 XG-DA---W\r\n\000\000"
> With \000 at the end, we can find out the length is 0x8B0FD63FF + 192 - 2, that is 37329134781.

Awesome, thanks!

37329134781 is 0x8B0FD64BD.  That's almost 35 GiB.  How on earth can
"info tlb" print several Gigabytes?  That's a not a sane use of QMP!  No
excuse for crashing, of course.

> With this length, we can write a simple c code demo to reproduce the result we obtain before. Such as:
> -----------------------------
> #include<stdio.h>
> int main()
> {
>     char * tmp = "";
>     size_t a =  37329134781;
>     int end = a;
>     size_t b = end;
>     printf("%zu", b)
>     return 0;
> }
> -----------------------------

Yes.

When strlen(str) exceeds INT_MAX - 1 in qstring_from_str(), the @end
argument for qstring_from_substr() gets truncated.  This is what
appeared to happen in your case: it got truncated to -1325570883.

qstring_from_substr() has an (unstated) precondition: start >= 0 && end
>= 0 && end - start >= -1.  This precondition is violated.  end - start
+ 1 gets sign-extended to a huge value, and g_malloc() duly fails.

All callers of qstring_from_substr() outside tests pass arguments of
type size_t or ptrdiff_t.  They'd fail just like qstring_from_str() for
sub-strings exceeding 2 GiB.  Your patch fixes them all.  Good, I'll
take your patch, but the commit message needs a bit of polish.  Here's
my attempt:

    qstring: Fix qstring_from_substr() not to provoke int overflow

    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.

If this works for you, please reply with your corrected Signed-off-by.

However, please note that allocating the correct 35 GiB may not behave
much better for you than allocating 16 EiB.  I doubt it'll succeed, and
if it does, then memcpy() will dirty 35 GiB of virtual memory, which is
unlikely to end well.

Are you sure "info tlb" is supposed to print that much?  Or is there
another bug still in hiding that somehow enlarged this string?

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[liujunjie (A)]

Thanks for your polish. I will update the patch as soon as possible.

I have tested the patch in my environment. What I use before is : virsh qemu-monitor-command xxxx --hmp "info tlb"
With this patch, when I query tlb, after some time, the console only print: "error: No complete monitor response found in 10485760 bytes: Numerical result out of range".
It is because libvirt have noticed this scene and try to avoid memory denial-of-service.

Besides, I login in the corresponding guestos which is win10 with 29G memory and find that a test tool called "BurnLnTest" is running busily. 
Maybe this test tool enlarge the size of tlb.

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, July 24, 2018 8:08 PM
> To: liujunjie (A) <liujunjie23@huawei.com>
> Cc: wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; Huangweidong (C)
> <weidong.huang@huawei.com>; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
> 
> "liujunjie (A)" <liujunjie23@huawei.com> writes:
> 
> > Even using gdb command: set print elements 0, is still too large to print the
> whole string.
> > So I try to obtain the string in another way.
> > After several attempts(not easy in fact), I finally obtain the real length. The
> way is as follows:
> > (gdb) p (int *)str
> > $1 = (int *) 0x7f13a2e67010
> > (gdb) p *(char*) (0x7f13a2e67010 + 0x8B0FD63FF)@192
> > $2 = "--W\r\nffffffffffd17000: 00000000fec00000
> XG-DACT-W\r\nffffffffffd18000: ", '0' <repeats 11 times>, "77000
> XG-DA---W\r\nffffffffffd19000: ", '0 "78000 XG-DA---W\r\nffffffffffd1a000: ", '0'
> <repeats 11 times>, "79000 XG-DA---W\r\n\000\000"
> > With \000 at the end, we can find out the length is 0x8B0FD63FF + 192 - 2,
> that is 37329134781.
> 
> Awesome, thanks!
> 
> 37329134781 is 0x8B0FD64BD.  That's almost 35 GiB.  How on earth can
> "info tlb" print several Gigabytes?  That's a not a sane use of QMP!  No
> excuse for crashing, of course.
> 
> > With this length, we can write a simple c code demo to reproduce the result
> we obtain before. Such as:
> > -----------------------------
> > #include<stdio.h>
> > int main()
> > {
> >     char * tmp = "";
> >     size_t a =  37329134781;
> >     int end = a;
> >     size_t b = end;
> >     printf("%zu", b)
> >     return 0;
> > }
> > -----------------------------
> 
> Yes.
> 
> When strlen(str) exceeds INT_MAX - 1 in qstring_from_str(), the @end
> argument for qstring_from_substr() gets truncated.  This is what appeared to
> happen in your case: it got truncated to -1325570883.
> 
> qstring_from_substr() has an (unstated) precondition: start >= 0 && end
> >= 0 && end - start >= -1.  This precondition is violated.  end - start
> + 1 gets sign-extended to a huge value, and g_malloc() duly fails.
> 
> All callers of qstring_from_substr() outside tests pass arguments of type size_t
> or ptrdiff_t.  They'd fail just like qstring_from_str() for sub-strings exceeding 2
> GiB.  Your patch fixes them all.  Good, I'll take your patch, but the commit
> message needs a bit of polish.  Here's my attempt:
> 
>     qstring: Fix qstring_from_substr() not to provoke int overflow
> 
>     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.
> 
> If this works for you, please reply with your corrected Signed-off-by.
> 
> However, please note that allocating the correct 35 GiB may not behave much
> better for you than allocating 16 EiB.  I doubt it'll succeed, and if it does, then
> memcpy() will dirty 35 GiB of virtual memory, which is unlikely to end well.
> 
> Are you sure "info tlb" is supposed to print that much?  Or is there another
> bug still in hiding that somehow enlarged this string?

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Eric Blake]

On 07/20/2018 08:09 AM, liujunjie wrote:
> From: l00425170 <liujunjie23@huawei.com>
> 
> The incoming parameters "start" and "end" is int type in
> qstring_from_substr(), but this function can be called by
> qstring_from_str, which is size_t type in strlen(str).
> It may result in coredump when called g_malloc later.
> One scene to triger is to call hmp "into tlb", which may have
> too long length of string.
> 
> Signed-off-by: l00425170 <liujunjie23@huawei.com>

Using what looks like a username for your Author and S-o-b designation 
rather than a legal name is fishy.  If 'l00425170' is really an alias 
that you have frequently used in other open source projects, it might be 
okay (and if so, you could back it up by pointing us to a URL of such 
contributions to other projects).  But in general, it's better to own 
your patches with your real name (git supports UTF-8, if you would like 
your name to appear in native characters instead of or in addition to a 
Latin-ized form).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[liujunjie (A)]

I am sorry for that. I have changed the git setting to avoid using this symbol next time.

> -----Original Message-----
> From: Eric Blake [mailto:eblake@redhat.com]
> Sent: Monday, July 23, 2018 10:52 PM
> To: liujunjie (A) <liujunjie23@huawei.com>; armbru@redhat.com
> Cc: wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; Huangweidong (C)
> <weidong.huang@huawei.com>; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
> 
> On 07/20/2018 08:09 AM, liujunjie wrote:
> > From: l00425170 <liujunjie23@huawei.com>
> >
> > The incoming parameters "start" and "end" is int type in
> > qstring_from_substr(), but this function can be called by
> > qstring_from_str, which is size_t type in strlen(str).
> > It may result in coredump when called g_malloc later.
> > One scene to triger is to call hmp "into tlb", which may have too long
> > length of string.
> >
> > Signed-off-by: l00425170 <liujunjie23@huawei.com>
> 
> Using what looks like a username for your Author and S-o-b designation rather
> than a legal name is fishy.  If 'l00425170' is really an alias that you have
> frequently used in other open source projects, it might be okay (and if so, you
> could back it up by pointing us to a URL of such contributions to other projects).
> But in general, it's better to own your patches with your real name (git
> supports UTF-8, if you would like your name to appear in native characters
> instead of or in addition to a Latin-ized form).
> 
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org