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

On Tue, Aug 16, 2016 at 11:25:35AM -0400, John Ferlan wrote: > 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... I'll assume you tested that 'virsh vol-dumpxml /path/to/storage/vol' still shows the encryption details too.

> 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, So a thought occurrs to me that perhaps rather than adding the flag, we could take a slightly different route. In virStorageFileGetMetadataInternal() we could still probe the encryption method unconditionally. If no meta->encryption is present, then we could just set that, but if one is present, then we can validate that the probed data matches the existing data and raise an error on mis-match.

This would give us more useful diagnostics in the case where someone specifies qcow2 encryption in the guest XML, for a volume that in fact uses luks, or vica-verca. Regards, Daniel