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

[Martin Kletzander]

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.

 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!).

 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.


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150225/1b632b6f/attachment-0001.sig>