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:address@hidden
> Sent: Tuesday, July 24, 2018 8:08 PM
> To: liujunjie (A) <address@hidden>
> Cc: wangxin (U) <address@hidden>; Gonglei (Arei)
> <address@hidden>; Huangweidong (C)
> <address@hidden>; address@hidden
> Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
> 
> "liujunjie (A)" <address@hidden> 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?