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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
>> ---
>>
>>  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.

Tks -

John
> 
> 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
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]