[libvirt] [PATCH] conf: fix crash when parse a invalid vcpus in virDomainThreadSchedParse()
When we set a invalid vcpus in /domain/cputune/vcpusched, like this: <vcpusched vcpus='0,^0' scheduler='fifo' priority='1'/> libvirtd will get segfault, because we will free bitmap (here is sp->ids) we passed in virBitmapParse() then return -1, then call virBitmapIsAllClear() with a null pointer bitmap will crash libvirtd. Split this check in two part to report parse error when cannot parse vcpus, and report invalid argument error when the cpu number is not correct. virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649 649 for (i = 0; i < bitmap->map_len; i++) (gdb) bt #0 virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649 #1 0x00007f28322f1245 in virDomainThreadSchedParse at conf/domain_conf.c:13464 #2 0x00007f283238aec8 in virDomainDefParseXML at conf/domain_conf.c:14059 #3 0x00007f2832390ddb in virDomainDefParseNode at conf/domain_conf.c:15588 #4 0x00007f2832390f02 in virDomainDefParse at conf/domain_conf.c:15530 #5 0x00007f2832390f53 in virDomainDefParseString at conf/domain_conf.c:15546 #6 0x00007f2819d8e8de in qemuDomainDefineXMLFlags at qemu/qemu_driver.c:7385 Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1763305..46dfea1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13473,12 +13473,11 @@ virDomainThreadSchedParse(xmlNodePtr node, goto error; } - if (!virBitmapParse(tmp, 0, &sp->ids, - VIR_DOMAIN_CPUMASK_LEN) || - virBitmapIsAllClear(sp->ids) || - virBitmapNextSetBit(sp->ids, -1) < minid || - virBitmapLastSetBit(sp->ids) > maxid) { + if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error; + if (virBitmapNextSetBit(sp->ids, -1) < minid || + virBitmapLastSetBit(sp->ids) > maxid) { virReportError(VIR_ERR_XML_ERROR, _("Invalid value of '%s': %s"), name, tmp); -- 1.8.3.1
On 04/09/2015 09:39 AM, Luyao Huang wrote:
When we set a invalid vcpus in /domain/cputune/vcpusched, like this:
<vcpusched vcpus='0,^0' scheduler='fifo' priority='1'/>
libvirtd will get segfault, because we will free bitmap (here is sp->ids) we passed in virBitmapParse() then return -1, then call virBitmapIsAllClear() with a null pointer bitmap will crash libvirtd.
Split this check in two part to report parse error when cannot parse vcpus, and report invalid argument error when the cpu number is not correct.
virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649 649 for (i = 0; i < bitmap->map_len; i++) (gdb) bt #0 virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649 #1 0x00007f28322f1245 in virDomainThreadSchedParse at conf/domain_conf.c:13464 #2 0x00007f283238aec8 in virDomainDefParseXML at conf/domain_conf.c:14059 #3 0x00007f2832390ddb in virDomainDefParseNode at conf/domain_conf.c:15588 #4 0x00007f2832390f02 in virDomainDefParse at conf/domain_conf.c:15530 #5 0x00007f2832390f53 in virDomainDefParseString at conf/domain_conf.c:15546 #6 0x00007f2819d8e8de in qemuDomainDefineXMLFlags at qemu/qemu_driver.c:7385
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1763305..46dfea1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13473,12 +13473,11 @@ virDomainThreadSchedParse(xmlNodePtr node, goto error; }
- if (!virBitmapParse(tmp, 0, &sp->ids, - VIR_DOMAIN_CPUMASK_LEN) || - virBitmapIsAllClear(sp->ids) ||
This line ^ should be left as is, see below.
- virBitmapNextSetBit(sp->ids, -1) < minid || - virBitmapLastSetBit(sp->ids) > maxid) { + if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error;
Although this line is correct, the problem resides in the virBitmapParse function itself. The virBitmapIsAllClear check should be removed from there and placed to every relevant place after the virBitmapParse call.
+ if (virBitmapNextSetBit(sp->ids, -1) < minid || + virBitmapLastSetBit(sp->ids) > maxid) { virReportError(VIR_ERR_XML_ERROR, _("Invalid value of '%s': %s"), name, tmp);
I sent 2 patches to the list, first adding the checks to relevant spots and the second partially reverting commit 983f5a which broke the virBitmapParse logic. Erik
On 04/10/2015 06:43 PM, Erik Skultety wrote:
When we set a invalid vcpus in /domain/cputune/vcpusched, like this:
<vcpusched vcpus='0,^0' scheduler='fifo' priority='1'/>
libvirtd will get segfault, because we will free bitmap (here is sp->ids) we passed in virBitmapParse() then return -1, then call virBitmapIsAllClear() with a null pointer bitmap will crash libvirtd.
Split this check in two part to report parse error when cannot parse vcpus, and report invalid argument error when the cpu number is not correct.
virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649 649 for (i = 0; i < bitmap->map_len; i++) (gdb) bt #0 virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649 #1 0x00007f28322f1245 in virDomainThreadSchedParse at conf/domain_conf.c:13464 #2 0x00007f283238aec8 in virDomainDefParseXML at conf/domain_conf.c:14059 #3 0x00007f2832390ddb in virDomainDefParseNode at conf/domain_conf.c:15588 #4 0x00007f2832390f02 in virDomainDefParse at conf/domain_conf.c:15530 #5 0x00007f2832390f53 in virDomainDefParseString at conf/domain_conf.c:15546 #6 0x00007f2819d8e8de in qemuDomainDefineXMLFlags at qemu/qemu_driver.c:7385
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1763305..46dfea1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13473,12 +13473,11 @@ virDomainThreadSchedParse(xmlNodePtr node, goto error; }
- if (!virBitmapParse(tmp, 0, &sp->ids, - VIR_DOMAIN_CPUMASK_LEN) || - virBitmapIsAllClear(sp->ids) || This line ^ should be left as is, see below. - virBitmapNextSetBit(sp->ids, -1) < minid || - virBitmapLastSetBit(sp->ids) > maxid) { + if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error; Although this line is correct, the problem resides in the virBitmapParse function itself. The virBitmapIsAllClear check should be removed from
On 04/09/2015 09:39 AM, Luyao Huang wrote: there and placed to every relevant place after the virBitmapParse call.
Oh, i see, reasonable to me, thanks for pointing out this :)
+ if (virBitmapNextSetBit(sp->ids, -1) < minid || + virBitmapLastSetBit(sp->ids) > maxid) { virReportError(VIR_ERR_XML_ERROR, _("Invalid value of '%s': %s"), name, tmp);
I sent 2 patches to the list, first adding the checks to relevant spots and the second partially reverting commit 983f5a which broke the virBitmapParse logic.
Okay, so no need I write a new version :) Thanks for your review
Erik
Luyao
participants (2)
-
Erik Skultety -
Luyao Huang