9:46 a.m.
Set of patches fixing the recently reported issues.
Peter Krempa (9):
virDomainDefParseBootInitOptions: Don't leak 'name' on failure
virBitmapShrink: Do not attempt to clear bits beyond end of buffer
virDomainFeaturesHyperVDefParse: Don't overwrite hypervisor vendor_id
virDomainFeaturesTCGDefParse: Don't leak 'tcg_features' when
'<tcg>'
feature is repeated
virDomainFeaturesDefParse: Add comment warning about features being
specified repeatedly
internal: Add helper macro for checking multiply and add overflows
virconf: Properly fix numeric overflow when parsing numbers in conf
files
virDiskNameParse: Fix integer overflow in disk name parsing
qemuxmlconfttest: Add test case for invalid disk target
src/conf/domain_conf.c | 23 +++++++----
src/internal.h | 11 ++++++
src/util/virbitmap.c | 6 +++
src/util/virconf.c | 6 ++-
src/util/virutil.c | 10 ++++-
.../disk-target-overflow.x86_64-latest.err | 1 +
.../qemuxmlconfdata/disk-target-overflow.xml | 29 ++++++++++++++
tests/qemuxmlconftest.c | 1 +
tests/virbitmaptest.c | 39 ++++++++++++++++---
9 files changed, 109 insertions(+), 17 deletions(-)
create mode 100644 tests/qemuxmlconfdata/disk-target-overflow.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/disk-target-overflow.xml
--
2.46.0
9:46 a.m.
New subject: [PATCH 1/9] virDomainDefParseBootInitOptions: Don't leak 'name' on failure
One of the failure paths skips code which would assign the string from
the temporary variable to the parsed struct, thus leaking it on failure.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/672
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a263612ef7..d72870d87d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17518,7 +17518,6 @@ static int
virDomainDefParseBootInitOptions(virDomainDef *def,
xmlXPathContextPtr ctxt)
{
- char *name = NULL;
size_t i;
int n;
g_autofree xmlNodePtr *nodes = NULL;
@@ -17550,6 +17549,8 @@ virDomainDefParseBootInitOptions(virDomainDef *def,
def->os.initenv = g_new0(virDomainOSEnv *, n + 1);
for (i = 0; i < n; i++) {
+ g_autofree char *name = NULL;
+
if (!(name = virXMLPropString(nodes[i], "name"))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("No name supplied for <initenv> element"));
@@ -17565,7 +17566,7 @@ virDomainDefParseBootInitOptions(virDomainDef *def,
}
def->os.initenv[i] = g_new0(virDomainOSEnv, 1);
- def->os.initenv[i]->name = name;
+ def->os.initenv[i]->name = g_steal_pointer(&name);
def->os.initenv[i]->value = g_strdup((const char
*)nodes[i]->children->content);
}
def->os.initenv[n] = NULL;
--
2.46.0
9:46 a.m.
New subject: [PATCH 2/9] virBitmapShrink: Do not attempt to clear bits beyond end of buffer
'virBitmapShrink' clears the bits beyond the end of the bitmap when
shrinking and then reallocates to match the new size. As it uses the
address of the first bit beyond the bitmap do do the clearing it can
overrun the allocated buffer if we're no actually going to shrink it and
the last bit's address is on the chunk boundary.
Fix it by returning in that corner case and add few more tests to be
sure.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/673
Fixes: d6e582da80d
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virbitmap.c | 6 ++++++
tests/virbitmaptest.c | 39 +++++++++++++++++++++++++++++++++------
2 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 775bbf1532..b8d0352bb1 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -1183,6 +1183,12 @@ virBitmapShrink(virBitmap *map,
nl = map->nbits / VIR_BITMAP_BITS_PER_UNIT;
nb = map->nbits % VIR_BITMAP_BITS_PER_UNIT;
+
+ /* If we're at the end of the allocation the attempt to clear
'map->nbit'
+ * and further would be beyond the end of the bitmap */
+ if (nl >= map->map_alloc)
+ return;
+
map->map[nl] &= ((1UL << nb) - 1);
toremove = map->map_alloc - (nl + 1);
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index adc956ca3d..27b6c13114 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -577,18 +577,45 @@ test12b(const void *opaque G_GNUC_UNUSED)
{
g_autoptr(virBitmap) map = NULL;
- if (!(map = virBitmapParseUnlimited("34,1023")))
+ if (!(map = virBitmapParseUnlimited("31,32,63,64,1023")))
return -1;
- if (checkBitmap(map, "34,1023", 1024) < 0)
+ if (checkBitmap(map, "31-32,63-64,1023", 1024) < 0)
return -1;
- virBitmapShrink(map, 35);
- if (checkBitmap(map, "34", 35) < 0)
+ /* no shrink at full alloc */
+ virBitmapShrink(map, 1025);
+ if (checkBitmap(map, "31-32,63-64,1023", 1024) < 0)
return -1;
- virBitmapShrink(map, 34);
- if (checkBitmap(map, "", 34) < 0)
+ /* shrink at the end */
+ virBitmapShrink(map, 1023);
+ if (checkBitmap(map, "31-32,63-64", 1023) < 0)
+ return -1;
+
+ /* extend back to see whether tail was cleared */
+ virBitmapSetBitExpand(map, 1022);
+ if (checkBitmap(map, "31-32,63-64,1022", 1023) < 0)
+ return -1;
+
+ virBitmapShrink(map, 64);
+ if (checkBitmap(map, "31-32,63", 64) < 0)
+ return -1;
+
+ virBitmapShrink(map, 65);
+ if (checkBitmap(map, "31-32,63", 64) < 0)
+ return -1;
+
+ virBitmapShrink(map, 63);
+ if (checkBitmap(map, "31-32", 63) < 0)
+ return -1;
+
+ virBitmapShrink(map, 32);
+ if (checkBitmap(map, "31", 32) < 0)
+ return -1;
+
+ virBitmapShrink(map, 31);
+ if (checkBitmap(map, "", 31) < 0)
return -1;
return 0;
--
2.46.0
9:46 a.m.
New subject: [PATCH 3/9] virDomainFeaturesHyperVDefParse: Don't overwrite hypervisor vendor_id
In case when the user specifies the '<hyperv/>' feature multiple times
we could overwrite already parsed data. Clear it beforehand.
As before this isn't trying to address the case of features being
specified multiple times not making much sense.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/675
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d72870d87d..e31b674bc2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16621,6 +16621,8 @@ virDomainFeaturesHyperVDefParse(virDomainDef *def,
if (value != VIR_TRISTATE_SWITCH_ON)
break;
+ g_clear_pointer(&def->hyperv_vendor_id, g_free);
+
if (!(def->hyperv_vendor_id = virXMLPropString(node, "value")))
{
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing 'value' attribute for HyperV
feature 'vendor_id'"));
--
2.46.0
9:46 a.m.
New subject: [PATCH 4/9] virDomainFeaturesTCGDefParse: Don't leak 'tcg_features' when '<tcg>' feature is repeated
Similarly to other cases users may specify the feature flag multiple
times.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e31b674bc2..0f0488f3a2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16807,21 +16807,25 @@ virDomainFeaturesTCGDefParse(virDomainDef *def,
xmlXPathContextPtr ctxt,
xmlNodePtr node)
{
- g_autofree virDomainFeatureTCG *tcg = NULL;
+ unsigned long long tb_cache;
VIR_XPATH_NODE_AUTORESTORE(ctxt);
- tcg = g_new0(virDomainFeatureTCG, 1);
ctxt->node = node;
if (virDomainParseMemory("./tb-cache", "./tb-cache/@unit",
- ctxt, &tcg->tb_cache, false, false) < 0)
+ ctxt, &tb_cache, false, false) < 0)
return -1;
- if (tcg->tb_cache == 0)
+ if (tb_cache == 0)
return 0;
+ if (!def->tcg_features)
+ def->tcg_features = g_new0(virDomainFeatureTCG, 1);
+
+ def->tcg_features->tb_cache = tb_cache;
+
+
def->features[VIR_DOMAIN_FEATURE_TCG] = VIR_TRISTATE_SWITCH_ON;
- def->tcg_features = g_steal_pointer(&tcg);
return 0;
}
--
2.46.0
9:46 a.m.
New subject: [PATCH 5/9] virDomainFeaturesDefParse: Add comment warning about features being specified repeatedly
Few of the handlers didn't take that possibility into account. Warn
others.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0f0488f3a2..cf4b1b2aef 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16849,6 +16849,8 @@ virDomainFeaturesDefParse(virDomainDef *def,
return -1;
}
+ /* Beware that users can specify the given feature multiple times, so
+ * the parser must be able to handle that */
switch ((virDomainFeature) val) {
case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_PAE:
--
2.46.0
9:46 a.m.
New subject: [PATCH 6/9] internal: Add helper macro for checking multiply and add overflows
The macro does the two checks together so that it's obvious what we're
checking as doing it in place is really unpleasant.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/internal.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index 01860efad9..20aa9b1d41 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -43,6 +43,17 @@
#define VIR_INT_MULTIPLY_OVERFLOW(a,b) (G_UNLIKELY ((b) > 0 && (a) >
G_MAXINT / (b)))
+/**
+ * VIR_MULTIPLY_ADD_IS_OVERFLOW:
+ * @limit: maximum value of data type
+ * @value: current value
+ * @multiply: number @value is going to be multiplied by
+ * @add: number that will be added to @value after multiplication
+ */
+#define VIR_MULTIPLY_ADD_IS_OVERFLOW(limit, value, multiply, add) \
+ (G_UNLIKELY(((multiply) > 0 && (value) > (limit) / (multiply)) || \
+ ((limit) - ((value) * (multiply)) < (add))))
+
/* The library itself is allowed to use deprecated functions /
* variables, so effectively undefine the deprecated attribute
* which would otherwise be defined in libvirt.h.
--
2.46.0
9:46 a.m.
New subject: [PATCH 7/9] virconf: Properly fix numeric overflow when parsing numbers in conf files
The previous fix didn't check the overflow in addition. Use the new
macro to check both multiplication and addition overflows.
Fixes: 8666523b7d0891c38a7c9c138c4cc318eddfefeb
Closes: https://gitlab.com/libvirt/libvirt/-/issues/671
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virconf.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index da07af178d..66b3e0482e 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -347,13 +347,15 @@ virConfParseLong(virConfParserCtxt *ctxt, long long *val)
return -1;
}
while ((ctxt->cur < ctxt->end) && (g_ascii_isdigit(CUR))) {
- if (l > LLONG_MAX / 10) {
+ long long c = (CUR - '0');
+
+ if (VIR_MULTIPLY_ADD_IS_OVERFLOW(LLONG_MAX, l, 10, c)) {
virConfError(ctxt, VIR_ERR_OVERFLOW,
_("numeric overflow in conf value"));
return -1;
}
- l = l * 10 + (CUR - '0');
+ l = l * 10 + c;
NEXT;
}
if (neg)
--
2.46.0
9:46 a.m.
New subject: [PATCH 8/9] virDiskNameParse: Fix integer overflow in disk name parsing
The conversion to index entails multiplication and accumulation by user
provided data which can easily overflow use VIR_MULTIPLY_ADD_IS_OVERFLOW
to check if the string is valid.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/674
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virutil.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index dc5009f11d..6c89a48e51 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -338,11 +338,17 @@ int virDiskNameParse(const char *name, int *disk, int *partition)
return -1;
for (i = 0; *ptr; i++) {
+ int c = *ptr - 'a';
+
if (!g_ascii_islower(*ptr))
break;
- idx = (idx + (i < 1 ? 0 : 1)) * 26;
- idx += *ptr - 'a';
+ idx = (idx + (i < 1 ? 0 : 1));
+
+ if (VIR_MULTIPLY_ADD_IS_OVERFLOW(INT_MAX, idx, 26, c))
+ return -1;
+
+ idx = idx * 26 + c;
ptr++;
}
--
2.46.0
9:46 a.m.
New subject: [PATCH 9/9] qemuxmlconfttest: Add test case for invalid disk target
Add a test case that the numeric overflow when parsing disk target is
detected.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
.../disk-target-overflow.x86_64-latest.err | 1 +
.../qemuxmlconfdata/disk-target-overflow.xml | 29 +++++++++++++++++++
tests/qemuxmlconftest.c | 1 +
3 files changed, 31 insertions(+)
create mode 100644 tests/qemuxmlconfdata/disk-target-overflow.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/disk-target-overflow.xml
diff --git a/tests/qemuxmlconfdata/disk-target-overflow.x86_64-latest.err
b/tests/qemuxmlconfdata/disk-target-overflow.x86_64-latest.err
new file mode 100644
index 0000000000..cae259fad7
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-target-overflow.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: Unknown disk name 'hdaxxxxxxxxxx' and no address specified
diff --git a/tests/qemuxmlconfdata/disk-target-overflow.xml
b/tests/qemuxmlconfdata/disk-target-overflow.xml
new file mode 100644
index 0000000000..8c8a2ab843
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-target-overflow.xml
@@ -0,0 +1,29 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='qcow2'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hdaxxxxxxxxxx' bus='virtio'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 5497fb2ba1..323fd9d721 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1622,6 +1622,7 @@ mymain(void)
DO_TEST_CAPS_LATEST("controller-virtio-scsi");
DO_TEST_CAPS_LATEST("controller-scsi-auto");
DO_TEST_CAPS_LATEST("disk-sata-device");
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-target-overflow");
DO_TEST_CAPS_LATEST("disk-aio");
DO_TEST_CAPS_LATEST("disk-aio-io_uring");
DO_TEST_CAPS_LATEST("disk-source-pool");
--
2.46.0
67
days inactive
67
days old
9 comments
1 participants
participants (1)
-
Peter Krempa