Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 09.03.2016 um 14:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
>> >> On 03/08/2016 07:57 PM, Peter Xu wrote:
>> >> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
>> >> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>> >> >>> Same arguments as for PATCH 2, except here an argument on the maximum
>> >> >>> length of subqdict would probably be easier.
>> >> >>
>> >> >> Yes, these are constant string literals in all callers, including the
>> >> >> one non-test case in quorum.
>> >> >>
>> >> >> Let's simply assert a reasonable maximum for subqdict_length. The
>> >> >> minimum we need to allow with the existing callers is 9, and I expect
>> >> >> we'll never get keys longer than 16 characters.
>> >> > 
>> >> > Hi, Kevin, Markus,
>> >> > 
>> >> > The patch should be trying to do as mentioned above. To make it
>> >> > clearer, how about the following one:
>> >> > 
>> >> > diff --git a/qobject/qdict.c b/qobject/qdict.c
>> >> > index 9833bd0..dde99e0 100644
>> >> > --- a/qobject/qdict.c
>> >> > +++ b/qobject/qdict.c
>> >> > @@ -704,17 +704,16 @@ int qdict_array_entries(QDict *src, const char 
>> >> > *subqdict)
>> >> >      for (i = 0; i < INT_MAX; i++) {
>> >> >          QObject *subqobj;
>> >> >          int subqdict_entries;
>> >> > -        size_t slen = 32 + subqdict_len;
>> >> > -        char indexstr[slen], prefix[slen];
>> >> > +        char indexstr[128], prefix[128];
>> >> >          size_t snprintf_ret;
>> >> > 
>> >> > -        snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i);
>> >> > -        assert(snprintf_ret < slen);
>> >> > +        snprintf_ret = snprintf(indexstr, ARRAY_SIZE(indexstr), 
>> >> > "%s%u", subqdict, i);
>> >> > +        assert(snprintf_ret < ARRAY_SIZE(indexstr));
>> >> 
>> >> sizeof(indexstr) works, and is a bit nicer than ARRAY_SIZE() when
>> >> dealing with char.
>> >> 
>> >> But I'm worried that this can trigger an abort() by someone hammering on
>> >> the command line.  Just because we don't expect any QMP command to
>> >> validate with a key name longer than 128 doesn't mean that we don't have
>> >> to deal with a command line with a garbage key name that long.  What's
>> >> wrong with just using g_strdup_printf() and heap-allocating the result,
>> >> avoiding snprintf() and fixed lengths altogether?
>> >
>> > I can only repeat myself, we're not dealing with user data here, but
>> > with constant literal strings. Put an assert(subqdict_len < 32); at the
>> > beginning of the function and be done with it. Any violation of it is
>> > not unexpected user input, but a caller bug.
>> 
>> The fact that the keys are literals is a non-local, not-quite-obvious
>> argument.
>> 
>> It's non-local, because in qdict.c we don't know what its users store in
>> their qdicts.
>> 
>> It's not quite obvious for the only user so far, quorum_open().  New
>> uses outside the block layer are possible, but seem unlikely; the block
>> layer is the most demanding user of QDicts.
>> 
>> The block layer's use of qdicts and QemuOpts is "interesting" enough for
>> me to call it not quite obvious.  In particular, QemuOpts can either
>> accept a fixed set of keys, or arbitrary keys.  In the latter case,
>> unknown keys should be rejected by the consumer of the QemuOpts.
>> Whether that happens before they can reach qdict_array_entries() is not
>> obvious to me.
>
> And it doesn't matter here because the variable part that determines the
> array size isn't the length of a key in the QDict, but the key name
> prefix we're looking for, i.e. the name of the QAPI array we want to
> parse. Unless you plan to introduce computed field names in QAPI, I
> can't imagine a reason why this could ever be something else than a
> constant string.

I think I see now.

>> We can rely on non-local or subtle arguments when it's worthwhile, but
>> I'm not sure it's worthwhile here.  I'd use g_strdup_printf() and call
>> it a day.
>
> I think it's unnecessary, but fine with me. I'm just trying to say that
> making it a fixed 128 byte array on the stack certainly doesn't improve
> anything.

It trades a few bytes of stack for a fixed stack frame.  A fixed stack
frame is a bit more efficient (not that it matters here), and lets us
scratch the function permanently from the list of stack fram size false
positives.  I think that's a reasonable trade.

[...]