[libvirt] [PATCH] qemu: forbid large value wraparound in balloon period collection

[John Ferlan]

On 02/25/2015 02:10 AM, Martin Kletzander wrote:
> On Tue, Feb 24, 2015 at 04:28:18PM +0100, Erik Skultety wrote:
>> We do parse and represent period collection as unsigned int in our
>> internal structures, however commit
>> d5c67e7f4523450023b89b69c16472582c85eeaf converts this to int, thus
>> wrapping
>> around inputs greater than INT_MAX which results in an error from QEMU.
>> This patch adds a check into QEMU driver, because period attribute is
>> only supported by QEMU.
>>
> 
> Well, there are couple more things broken.
> 
> 1) Change to this patch:
>    It is written in the description that period can me 0 or more, so
>    it would make sense to guard all the drivers (ideally at one
>    place) together.  If that changes, the check can be moved to
>    individual drivers, but for now anything outside <0,INT_MAX>
>    doesn't make sense.

correct... for that matter a really long period doesn't make much sense,
but if someone wants one, then they can have it...

> 
> 2) Curiosity:
>    <membaloon model='virtio'>
>      <stats period='5'/>
>      <address .../>
>    </membaloon>
> 
>    This ^^ fails validation because <stats/> must come *after*
>    <address/> (WAT) even though all other device elements *require*
>    the address to be last (why would we even do that!).

Probably because I was still getting used to the XML/RNG
formatting/syntax "rules".... This would be easily dealt with by an
<interleave .../>, right?  Then again, review missed it too ;-)

> 
> 3) Invalid number gets still parsed in the XML and it only wraps to
>    negative when sending to the monitor at the guest's start.
> 

The input parsing for that value was created (or copied from some other
attribute) before the virStrToLong_ui[p] API's were created...  I think
at the time those API's were created there was a comment about maybe
spending some cycles looking at all the various parsed 'int' or 'uint'
attributes to convert them to use the newer API, but that never got
anywhere.

John
> 
> Since these are all small things, I expect them to be fixed as well
> (be sure the BZ will come back anyway if you don't do that :) ),
> although not necessarily in the same commit.  One note: we must be
> able to parse old XMLs, so you can either reject the number at start
> (more pain then security) or you can just make period < 0 behave like
> period == 0 or many other variants.  You might even wrap everything to
> unsigned as it is unsigned in qemu anyways.  The other thing you can't
> do is to fix our API without creating a new one.  I wonder if wraping
> negative values could be written in the docs so we have more options.
> But anyway, who's going to request stats gathering period greater than
> 68 years except QE, right? ;)
> 
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140958
>> ---
>> src/qemu/qemu_driver.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index bec05d4..46bd880 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2414,6 +2414,12 @@ static int
>> qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
>>     /* Set the balloon driver collection interval */
>>     priv = vm->privateData;
>>
>> +    if (period < 0) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                       _("invalid value for collection period"));
>> +        goto endjob;
>> +    }
>> +
>>     if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>>
>>         qemuDomainObjEnterMonitor(driver, vm);
>> -- 
>> 1.9.3
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>