[PATCH 0/9] Bug fixes
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
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@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
'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@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
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@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
Similarly to other cases users may specify the feature flag multiple times. Signed-off-by: Peter Krempa <pkrempa@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
Few of the handlers didn't take that possibility into account. Warn others. Signed-off-by: Peter Krempa <pkrempa@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
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@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
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@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
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@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
Add a test case that the numeric overflow when parsing disk target is detected. Signed-off-by: Peter Krempa <pkrempa@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
participants (1)
-
Peter Krempa