[libvirt] [PATCH v7 11/11] qemu: Add TLS support for Veritas HyperScale (VxHS)

[John Ferlan]

On 09/19/2017 02:19 PM, ashish mittal wrote:
> Hi,
> 
> Not sure how to reply to the v8 series email thread, or how to add
> myself to the libvirt email list! Updating the v7 thread instead.
> 

There's some magic you can do with the right clients... I'll remember to
include you on the next series.

> I have already updated with the results of TLS testing by statically
> adding TLS enabled VxHS devices to domain XML, booting up the guest, and
> making sure devices are properly readable/writable.
> 
> I did the same test using hot plug. libvirtd crashes when adding TLS
> device -
> 
> [root at audi Sources] 2017-09-18 19:54:27# virsh attach-device myfc24
> hotplug_disk_2.xml
> error: Disconnected from qemu:///system due to end of file
> error: Failed to attach device from hotplug_disk_2.xml
> error: End of file while reading data: Input/output error
> 
> gdb reports -
> 
> Thread 3 "libvirtd" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7f97e6b99700 (LWP 20096)]
> 0x00007f97c9c42a3d in qemuDomainGetTLSObjects (qemuCaps=0x7f97c00fcca0,
> secinfo=secinfo at entry=0x0, tlsCertdir=0x7f97c00ff000
> "/etc/pki/libvirt-vxhs",
>     tlsListen=tlsListen at entry=false, tlsVerify=tlsVerify at entry=true,
> srcAlias=0x7f97b8001e00 "virtio-disk0", tlsProps=0x7f97e6b988f0,
> tlsAlias=0x7f97e6b98998, secProps=0x0,
>     secAlias=0x0) at qemu/qemu_hotplug.c:1736
> 1736    }
> 
> 
> Reason for crash seems to be a latent issue with
> qemuDomainGetTLSObjects(). Both qemuDomainGetTLSObjects() and
> qemuBuildTLSx509BackendProps() suggest that char **secAlias may be NULL,
> but qemuDomainGetTLSObjects() dereferences it without checking for NULL.
> 
> int
> qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps,
>                         qemuDomainSecretInfoPtr secinfo,
>                         const char *tlsCertdir,
>                         bool tlsListen,
>                         bool tlsVerify,
>                         const char *srcAlias,
>                         virJSONValuePtr *tlsProps,
>                         char **tlsAlias,
>                         virJSONValuePtr *secProps,
>                         char **secAlias)
> {
>     /* Add a secret object in order to access the TLS environment.
>      * The secinfo will only be created for serial TCP device. */
>     if (secinfo) {
>         if (qemuBuildSecretInfoProps(secinfo, secProps) < 0)
>             return -1;
> 
>         if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false)))
>             return -1;
>     }
> 
>     if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify,
>                                      *secAlias, qemuCaps, tlsProps) < 0)
>   <=== *secAlias is dereferencing char **secAlias, which can be NULL
>         return -1;
> ...
> }
> 

Good thing we test before pushing!

> 
> The following diff takes care of the above issue. 
> 
> [root at audi libvirt] 2017-09-18 23:19:31# git diff
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4c1074d..1448fe7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1715,6 +1715,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps,
>                          virJSONValuePtr *secProps,
>                          char **secAlias)
>  {
> +    char *secretAlias = NULL;
> +
>      /* Add a secret object in order to access the TLS environment.
>       * The secinfo will only be created for serial TCP device. */
>      if (secinfo) {
> @@ -1723,10 +1725,11 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps,
> 
>          if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false)))
>              return -1;
> +        secretAlias = *secAlias;
>      }
> 
>      if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify,
> -                                     *secAlias, qemuCaps, tlsProps) < 0)
> +                                     secretAlias, qemuCaps, tlsProps) < 0)
>          return -1;
> 
>      if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias)))
> 

But it's not really right... it's OK to have NULL secalias - we just
have to handle it better.  There's no need to create a local for it. I
think if you do something that turns "*secalias," into "secalias ?
*secalias : NULL," that'd probably be better.  it it works, could you
just post a separate patch to fix that issue?


> I was able to attach and detach two TLS enabled VxHS disks to a running
> guest multiple times with the above fix in place. Did a attach/detach of
> these disks back-to-back without any problems. Was able to format these
> disks and copy files to both these TLS hot plugged disks from within the
> guest without any problems. Will send detailed test logs in a separate
> email.
> 

In the v8 review, Peter would like some adjustments which I'm in the
process of making.  I'm going to push the first part of the series, but
repost the TLS code as a v9.

Tks -

John

[...]