Commit id 02b031a4 added a secondary path from which the
incoming @secinfo would not be free'd until the private
data was freed in qemuDomainStorageSourcePrivateDispose.

However, by doing this the original intention to free
@*secinfo afterwards is lost and thus the pass by value
of the secinfo->s.aes (or secinfo->s.plain for its method)
results in not keeping the NULL setting in the various
secret.{username|iv|ciphertext} fields upon return to
qemuDomainSecretInfoClear and eventually will result in
a double free at domain destroy:

    raise ()
    abort ()
    __libc_message ()
    malloc_printerr ()
    _int_free ()
    virFree
    qemuDomainSecretAESClear
    qemuDomainSecretInfoClear
    qemuDomainSecretInfoFree
    qemuDomainStorageSourcePrivateDispose
    virObjectUnref
    virStorageSourceClear
    virStorageSourceFree
    virDomainDiskDefFree
    virDomainDefFree
    virDomainObjRemoveTransientDef
    qemuProcessStop
    qemuDomainDestroyFlags
    virDomainDestroy

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/qemu/qemu_domain.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

 Domains w/ secrets weren't very happy when I went to destroy them
 today during testing...

 Fortunately issue is not in 4.4.0...

 I modified both Plain and AES just because it's probably best to
 avoid something like this in the future.

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f135117a95..1fb1ef1deb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -945,23 +945,23 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
 
 
 static void
-qemuDomainSecretPlainClear(qemuDomainSecretPlain secret)
+qemuDomainSecretPlainClear(qemuDomainSecretPlainPtr secret)
 {
-    VIR_FREE(secret.username);
-    VIR_DISPOSE_N(secret.secret, secret.secretlen);
+    VIR_FREE(secret->username);
+    VIR_DISPOSE_N(secret->secret, secret->secretlen);
 }
 
 
 static void
-qemuDomainSecretAESClear(qemuDomainSecretAES secret,
+qemuDomainSecretAESClear(qemuDomainSecretAESPtr secret,
                          bool keepAlias)
 {
     if (!keepAlias)
-        VIR_FREE(secret.alias);
+        VIR_FREE(secret->alias);
 
-    VIR_FREE(secret.username);
-    VIR_FREE(secret.iv);
-    VIR_FREE(secret.ciphertext);
+    VIR_FREE(secret->username);
+    VIR_FREE(secret->iv);
+    VIR_FREE(secret->ciphertext);
 }
 
 
@@ -974,11 +974,11 @@ qemuDomainSecretInfoClear(qemuDomainSecretInfoPtr secinfo,
 
     switch ((qemuDomainSecretInfoType) secinfo->type) {
     case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN:
-        qemuDomainSecretPlainClear(secinfo->s.plain);
+        qemuDomainSecretPlainClear(&secinfo->s.plain);
         break;
 
     case VIR_DOMAIN_SECRET_INFO_TYPE_AES:
-        qemuDomainSecretAESClear(secinfo->s.aes, keepAlias);
+        qemuDomainSecretAESClear(&secinfo->s.aes, keepAlias);
         break;
 
     case VIR_DOMAIN_SECRET_INFO_TYPE_LAST:
-- 
2.14.4

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

On Tue, Jun 05, 2018 at 16:34:47 -0400, John Ferlan wrote:
> Commit id 02b031a4 added a secondary path from which the
> incoming @secinfo would not be free'd until the private
> data was freed in qemuDomainStorageSourcePrivateDispose.
> 
> However, by doing this the original intention to free
> @*secinfo afterwards is lost and thus the pass by value
> of the secinfo->s.aes (or secinfo->s.plain for its method)
> results in not keeping the NULL setting in the various
> secret.{username|iv|ciphertext} fields upon return to
> qemuDomainSecretInfoClear and eventually will result in
> a double free at domain destroy:
> 
>     raise ()
>     abort ()
>     __libc_message ()
>     malloc_printerr ()
>     _int_free ()
>     virFree
>     qemuDomainSecretAESClear
>     qemuDomainSecretInfoClear
>     qemuDomainSecretInfoFree
>     qemuDomainStorageSourcePrivateDispose
>     virObjectUnref
>     virStorageSourceClear
>     virStorageSourceFree
>     virDomainDiskDefFree
>     virDomainDefFree
>     virDomainObjRemoveTransientDef
>     qemuProcessStop
>     qemuDomainDestroyFlags
>     virDomainDestroy
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
>  Domains w/ secrets weren't very happy when I went to destroy them
>  today during testing...
> 
>  Fortunately issue is not in 4.4.0...
> 
>  I modified both Plain and AES just because it's probably best to
>  avoid something like this in the future.

Yep, I did not notice that in the first place.

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