[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow
From: |
liujunjie (A) |
Subject: |
Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow |
Date: |
Tue, 24 Jul 2018 13:24:44 +0000 |
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?
- [Qemu-devel] [PATCH] qstring: Fix integer overflow, liujunjie, 2018/07/20
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Markus Armbruster, 2018/07/23
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, liujunjie (A), 2018/07/23
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Markus Armbruster, 2018/07/23
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, liujunjie (A), 2018/07/23
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Markus Armbruster, 2018/07/24
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Markus Armbruster, 2018/07/24
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, liujunjie (A), 2018/07/24
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Markus Armbruster, 2018/07/24
- Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow,
liujunjie (A) <=
Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Eric Blake, 2018/07/23