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: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.