[libvirt] [PATCH] qemu: forbid large value wraparound in balloon period collection
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. 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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/25/2015 08: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.
Well stats collection period is only used by QEMU, but the issue here is it's quite impossible to do it unified for all drivers, possibly at one place, because you can't do it right after the domain gets parsed (otherwise domain disappears after daemon's restart) and we already do check this at client's side (both virsh cmd routine and our public API virDomainSetMemoryStatsPeriod). However, this patch doesn't fix the issue anyway, as the check is at a wrong place.
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!).
Will include fix for this one in the next versions :).
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.
We can't do much about this, can we? Thank you for your review :), Erik
On Thu, Feb 26, 2015 at 05:42:10PM +0100, Erik Skultety wrote:
On 02/25/2015 08: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.
Well stats collection period is only used by QEMU, but the issue here is it's quite impossible to do it unified for all drivers, possibly at one place, because you can't do it right after the domain gets parsed (otherwise domain disappears after daemon's restart) and we already do check this at client's side (both virsh cmd routine and our public API virDomainSetMemoryStatsPeriod). However, this patch doesn't fix the issue anyway, as the check is at a wrong place.
You're right, I didn't know we have checks in APIs on the way already.
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!).
Will include fix for this one in the next versions :).
Great! That'd be nice (although it's not an error per se).
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.
We can't do much about this, can we?
Sure we can, the question is whether that makes sense. I'd either shout at the time when qemu is starting the domain and fail when int period <= 0 or ignore such values quietly (with VIR_DEBUG() at most). When it comes to where to check this, it'd be best to check this inside qemuMonitorSetMemoryStatsPeriod(), but beware! We are screwing up the error messages all over the place and leaving them, duplicating them, etc. So even though I'd accept a simple fix in that function, I'd like to see (at least later on) some cleanup to this.
Thank you for your review :), Erik
You're welcome ;)
participants (3)
-
Erik Skultety -
John Ferlan -
Martin Kletzander