[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: |
Mon, 23 Jul 2018 14:36:39 +0000 |
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:address@hidden
> Sent: Monday, July 23, 2018 8:48 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 <address@hidden> writes:
>
> > From: l00425170 <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> > 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.
- [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) <=
- 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), 2018/07/24
Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow, Eric Blake, 2018/07/23