10:25 a.m.
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>
---
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,
--
2.7.4