[PATCH] qemu: Audit VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag usage
There is plenty of places where a domain XML is parsed using VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag, but not all are warranted. The flag usage is okay when parsing an XML produced by us (e.g. when copying virDomainDef). In the rest of places (especially when the XML might come from user) we need to validate the XML, otherwise we may start QEMU assuming it has certain capabilities while in fact it doesn't. For instance, in this specific case when migrating a guest with virtio-mem to a QEMU that has virtio-mem disabled, loading migration fails with: qemu-kvm: ... 'virtio-mem-pci' is not a valid device model name This bug is more visible the more we transfer validation from qemu_command.c into qemu_validate.c. There is a possibility that we might prevent migration because of a bug in our validator, but that's better than starting a QEMU with features it doesn't support. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2048435 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++--- src/qemu/qemu_migration.c | 9 +++------ src/qemu/qemu_saveimage.c | 3 +-- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 370d223198..7d12b187ba 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2681,8 +2681,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, if (!(def = virDomainDefParseString(xmlin, driver->xmlopt, priv->qemuCaps, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) { + VIR_DOMAIN_DEF_PARSE_INACTIVE))) { goto endjob; } if (!qemuDomainCheckABIStability(driver, vm, def)) @@ -7944,7 +7943,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(virDomainDeviceDef) dev = NULL; virDomainDeviceDef *dev_copy = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + unsigned int parse_flags = 0; g_autoptr(virDomainDef) vmdef = NULL; int ret = -1; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2635ef1162..c83eb41693 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2469,8 +2469,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, if (xmlin) { if (!(def = virDomainDefParseString(xmlin, driver->xmlopt, priv->qemuCaps, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + VIR_DOMAIN_DEF_PARSE_INACTIVE))) return NULL; if (!qemuDomainCheckABIStability(driver, vm, def)) @@ -2858,8 +2857,7 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout); newdef = virDomainDefParseString(xmlout, driver->xmlopt, NULL, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); + VIR_DOMAIN_DEF_PARSE_INACTIVE); if (!newdef) goto cleanup; @@ -3355,8 +3353,7 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, if (!(def = virDomainDefParseString(dom_xml, driver->xmlopt, qemuCaps, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup; if (dname) { diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 557ee2cd21..b106e5b299 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -556,8 +556,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, /* Create a domain from this XML */ if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + VIR_DOMAIN_DEF_PARSE_INACTIVE))) return -1; *ret_def = g_steal_pointer(&def); -- 2.34.1
On Mon, Jan 31, 2022 at 10:42:28 +0100, Michal Privoznik wrote:
There is plenty of places where a domain XML is parsed using VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag, but not all are warranted. The flag usage is okay when parsing an XML produced by us (e.g. when copying virDomainDef). In the rest of places (especially when the XML might come from user) we need to validate the XML, otherwise we may start QEMU assuming it has certain capabilities while in fact it doesn't. For instance, in this specific case when migrating a guest with virtio-mem to a QEMU that has virtio-mem disabled, loading migration fails with:
qemu-kvm: ... 'virtio-mem-pci' is not a valid device model name
This bug is more visible the more we transfer validation from qemu_command.c into qemu_validate.c.
This is weird, because the validation should be done on the destination when starting the qemu process with the already parsed definition. In case we skipt that for migration for any reason that would be the place to fix it rather than messing with the parsing step. The handling should be equivalent to what happens when you have an un-validated XML already defined and attempt to start the VM from such definition.
On 1/31/22 11:01, Peter Krempa wrote:
On Mon, Jan 31, 2022 at 10:42:28 +0100, Michal Privoznik wrote:
There is plenty of places where a domain XML is parsed using VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag, but not all are warranted. The flag usage is okay when parsing an XML produced by us (e.g. when copying virDomainDef). In the rest of places (especially when the XML might come from user) we need to validate the XML, otherwise we may start QEMU assuming it has certain capabilities while in fact it doesn't. For instance, in this specific case when migrating a guest with virtio-mem to a QEMU that has virtio-mem disabled, loading migration fails with:
qemu-kvm: ... 'virtio-mem-pci' is not a valid device model name
This bug is more visible the more we transfer validation from qemu_command.c into qemu_validate.c.
This is weird, because the validation should be done on the destination when starting the qemu process with the already parsed definition.
I think my patch does exactly that. I mean, the places I'm fixing are run on the destination. And running the validation on the source - well, we are running ABI stability check anyway so why not run validator?
In case we skipt that for migration for any reason that would be the place to fix it rather than messing with the parsing step.
The handling should be equivalent to what happens when you have an un-validated XML already defined and attempt to start the VM from such definition.
Fair enough, so in that case we need to remove the check from qemuProcessStartValidate(). Michal
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa