[libvirt] [PATCH v1.2.[1-5]-maint] qemu: blockcopy: Don't remove existing disk mirror info
From: Peter Krempa <pkrempa@redhat.com> When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists. (cherry picked from commit 02b364e186d487f54ed410c01af042f23e812d42) This fixes a regression introduced in commit ff5f30b. Signed-off-by: Eric Blake <eblake@redhat.com> Conflicts: src/qemu/qemu_driver.c - no refactoring of commit 7b7bf001 --- As Peter's patch resolves a regression, I'd like to backport it to the maint branches; however, that means redoing the patch. src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59185c6..591864f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15203,6 +15203,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, struct stat st; bool need_unlink = false; char *mirror = NULL; + int mirrorFormat; virQEMUDriverConfigPtr cfg = NULL; /* Preliminaries: find the disk we are editing, sanity checks */ @@ -15290,10 +15291,10 @@ qemuDomainBlockCopy(virDomainObjPtr vm, goto endjob; VIR_FORCE_CLOSE(fd); if (!format) - disk->mirrorFormat = disk->src.format; + mirrorFormat = disk->src.format; } else if (format) { - disk->mirrorFormat = virStorageFileFormatTypeFromString(format); - if (disk->mirrorFormat <= 0) { + mirrorFormat = virStorageFileFormatTypeFromString(format); + if (mirrorFormat <= 0) { virReportError(VIR_ERR_INVALID_ARG, _("unrecognized format '%s'"), format); goto endjob; @@ -15303,11 +15304,11 @@ qemuDomainBlockCopy(virDomainObjPtr vm, * also passed the RAW flag (and format is non-NULL), or it is * safe for us to probe the format from the file that we will * be using. */ - disk->mirrorFormat = virStorageFileProbeFormat(dest, cfg->user, - cfg->group); + mirrorFormat = virStorageFileProbeFormat(dest, cfg->user, + cfg->group); } - if (!format && disk->mirrorFormat > 0) - format = virStorageFileFormatTypeToString(disk->mirrorFormat); + if (!format && mirrorFormat > 0) + format = virStorageFileFormatTypeToString(mirrorFormat); if (VIR_STRDUP(mirror, dest) < 0) goto endjob; @@ -15333,13 +15334,12 @@ qemuDomainBlockCopy(virDomainObjPtr vm, /* Update vm in place to match changes. */ need_unlink = false; disk->mirror = mirror; + disk->mirrorFormat = mirrorFormat; mirror = NULL; endjob: if (need_unlink && unlink(dest)) VIR_WARN("unable to unlink just-created %s", dest); - if (ret < 0 && disk) - disk->mirrorFormat = VIR_STORAGE_FILE_NONE; VIR_FREE(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; -- 1.9.3
On 06/26/14 15:51, Eric Blake wrote:
From: Peter Krempa <pkrempa@redhat.com>
When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists.
(cherry picked from commit 02b364e186d487f54ed410c01af042f23e812d42)
This fixes a regression introduced in commit ff5f30b.
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts: src/qemu/qemu_driver.c - no refactoring of commit 7b7bf001 ---
As Peter's patch resolves a regression, I'd like to backport it to the maint branches; however, that means redoing the patch.
src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
ACK, Peter
On 06/26/2014 07:56 AM, Peter Krempa wrote:
On 06/26/14 15:51, Eric Blake wrote:
From: Peter Krempa <pkrempa@redhat.com>
When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists.
(cherry picked from commit 02b364e186d487f54ed410c01af042f23e812d42)
This fixes a regression introduced in commit ff5f30b.
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts: src/qemu/qemu_driver.c - no refactoring of commit 7b7bf001 ---
As Peter's patch resolves a regression, I'd like to backport it to the maint branches; however, that means redoing the patch.
src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
ACK,
I'm awaiting word on whether this regression represents a CVE. Obviously, the fix is already public, so I'm not making the situation any worse by mentioning that this patch is under evaluation; but at the same time, I'm not going into the details of the scenario I found while while playing with this patch. Worse, the regression was introduced when plugging an earlier CVE last year - it's never fun when solving one CVE causes another, so all the more reason that I hope the libvirt-security list doesn't deem this as a vulnerability. At any rate, whether or not this gets a CVE designation, it was more than just v1.2.1-maint affected - everything back to v0.9.12-maint had the bug by virtue of CVE-2013-6458; I'm in the process of backporting this patch to ALL branches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/26/2014 07:51 AM, Eric Blake wrote:
From: Peter Krempa <pkrempa@redhat.com>
When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists.
(cherry picked from commit 02b364e186d487f54ed410c01af042f23e812d42)
This fixes a regression introduced in commit ff5f30b.
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts: src/qemu/qemu_driver.c - no refactoring of commit 7b7bf001 ---
As Peter's patch resolves a regression, I'd like to backport it to the maint branches; however, that means redoing the patch.
src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59185c6..591864f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15203,6 +15203,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, struct stat st; bool need_unlink = false; char *mirror = NULL; + int mirrorFormat;
Uninitialized...
virQEMUDriverConfigPtr cfg = NULL;
/* Preliminaries: find the disk we are editing, sanity checks */ @@ -15290,10 +15291,10 @@ qemuDomainBlockCopy(virDomainObjPtr vm, goto endjob; VIR_FORCE_CLOSE(fd); if (!format) - disk->mirrorFormat = disk->src.format; + mirrorFormat = disk->src.format;
but here, if the user did not request reusing a file but DID request raw, mirrorFormat is still uninitialized...
} else if (format) { - disk->mirrorFormat = virStorageFileFormatTypeFromString(format); - if (disk->mirrorFormat <= 0) { + mirrorFormat = virStorageFileFormatTypeFromString(format); + if (mirrorFormat <= 0) { virReportError(VIR_ERR_INVALID_ARG, _("unrecognized format '%s'"), format); goto endjob; @@ -15303,11 +15304,11 @@ qemuDomainBlockCopy(virDomainObjPtr vm, * also passed the RAW flag (and format is non-NULL), or it is * safe for us to probe the format from the file that we will * be using. */ - disk->mirrorFormat = virStorageFileProbeFormat(dest, cfg->user, - cfg->group); + mirrorFormat = virStorageFileProbeFormat(dest, cfg->user, + cfg->group); } - if (!format && disk->mirrorFormat > 0) - format = virStorageFileFormatTypeToString(disk->mirrorFormat); + if (!format && mirrorFormat > 0) + format = virStorageFileFormatTypeToString(mirrorFormat);
and here we are using the uninitialized value :( It looks like this has been broken ever since v1.0.0 (when blockcopy was first introduced), so it is not a regression that we have been mishandling 'virsh blockcopy $dom vda /path/to/file --raw', but disk->mirrorFormat was at least initialized to 0 compared to my patch using an uninitialized value. Peter will be proposing a patch soon, which must make it into 1.2.6. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa