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,

On 08/17/2016 05:40 AM, Daniel P. Berrange wrote: > 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. > It does... >> 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. Hmm. OK, so something like: int expt_fmt = fileTypeInfo[meta->format].cryptInfo[i].format; if (!meta->encryption) { if (VIR_ALLOC(meta->encryption) < 0) goto cleanup; meta->encryption->format = expt_fmt; } else { if (meta->encryption->format != expt_fmt) { virReportError(VIR_ERR_XML_ERROR, _("encryption format %d doesn't match " "expected format %d"), meta->encryption->format, expt_fmt); goto cleanup; } } I can post a v2, but wanted to make sure I wasn't misunderstanding your request.