Re: [libvirt] [PATCH] qemu: Fix crash hot plugging luks volume

https://bugzilla.redhat.com/show_bug.cgi?id=1367259 Crash occurs because 'secrets' is being dereferenced in call: if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, VIR_SECRET_USAGE_TYPE_VOLUME, NULL, &src->encryption->secrets[0]->seclookupdef, true) < 0) (gdb) p *src->encryption $1 = {format = 2, nsecrets = 0, secrets = 0x0, encinfo = {cipher_size = 0, cipher_name = 0x0, cipher_mode = 0x0, cipher_hash = 0x0, ivgen_name = 0x0, ivgen_hash = 0x0}} (gdb) bt priv=priv@entry=0x7fffc03be160, disk=disk@entry=0x7fffb4002ae0) at qemu/qemu_domain.c:1087 disk=0x7fffb4002ae0, vm=0x7fffc03a2580, driver=0x7fffc02ca390, conn=0x7fffb00009a0) at qemu/qemu_hotplug.c:355 Upon entry to qemuDomainAttachVirtioDiskDevice, src->encryption points at a valid 'secret' buffer w/ nsecrets == 1; however, the call to qemuDomainDetermineDiskChain will call virStorageFileGetMetadata and eventually virStorageFileGetMetadataInternal where the src->encryption was overwritten when probing the volume. Commit id 'a48c7141' added code to virStorageFileGetMetadataInternal to determine if the disk/volume would use/need encryption and allocated a meta->encryption. This overwrote an existing encryption buffer already provided by the XML This patch adds an argument to virStorageFileGetMetadataInternal in order to avoid the encryption probe of the volume. This will only be be set to true for the storage volume buffer/fd parsing. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- NB: This I believe the bare bones that could be done. If desired, I could also modify virStorageFileGetMetadata[Recurse] and all the callers in order to specifically add a 'probe_encryption' variable there as well. For this path, the "root" callers would add the 'false', hence the reason I chose to just modify the *Recurse function to pass 'false'. I have tested creating a luks volume and of course hot plugging with this patch...

src/storage/storage_driver.c | 2 +- src/util/virstoragefile.c | 8 +++++--- src/util/virstoragefile.h | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6f1e372..fe0e164 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3239,7 +3239,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, &buf)) < 0) goto cleanup; - if (virStorageFileGetMetadataInternal(src, buf, headerLen, + if (virStorageFileGetMetadataInternal(src, buf, headerLen, false, &backingFormat) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 471aa1f..4b6bde3 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -928,6 +928,7 @@ int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, size_t len, + bool probe_encryption, int *backingFormat) { int ret = -1; @@ -946,7 +947,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, goto cleanup; } - if (fileTypeInfo[meta->format].cryptInfo != NULL) { + if (probe_encryption && fileTypeInfo[meta->format].cryptInfo != NULL) { for (i = 0; fileTypeInfo[meta->format].cryptInfo[i].format != 0; i++) { if (virStorageFileHasEncryptionFormat(&fileTypeInfo[meta->format].cryptInfo[i], buf, len)) { @@ -1129,7 +1130,7 @@ virStorageFileGetMetadataFromBuf(const char *path, if (!(ret = virStorageFileMetadataNew(path, format))) return NULL; - if (virStorageFileGetMetadataInternal(ret, buf, len, + if (virStorageFileGetMetadataInternal(ret, buf, len, true, backingFormat) < 0) { virStorageSourceFree(ret); return NULL; @@ -1200,7 +1201,8 @@ virStorageFileGetMetadataFromFD(const char *path, goto cleanup; } - if (virStorageFileGetMetadataInternal(meta, buf, len, backingFormat) < 0) + if (virStorageFileGetMetadataInternal(meta, buf, len, true, + backingFormat) < 0) goto cleanup; if (S_ISREG(sb.st_mode)) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3d09468..bb71372 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -289,8 +289,9 @@ int virStorageFileProbeFormatFromBuf(const char *path, int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, size_t len, + bool probe_encryption, int *backingFormat) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd,