[PATCH 0/4] User creatable object property setting fixes

[PATCH 0/4] User creatable object property setting fixes

[Kevin Wolf]

While writing a QAPI schema for all user creatable QOM objects, I found
a few problems in the existing property setting code of objects.

This series fixes some crashes and memory leaks related to property
setting in user creatable objects.

There are many more problems of the sort that updating a property at
runtime is allowed by most objects, but they aren't actually prepared to
handle the update, so it doesn't result in the expected behaviour. I'm
not trying to fix bugs of this class in this series.

Kevin Wolf (4):
  crypto: Move USER_CREATABLE to secret_common base class
  crypto: Forbid broken unloading of secrets
  crypto: Fix memory leaks in set_loaded for tls-*
  can-host: Fix crash when 'canbus' property is not set

 crypto/secret.c         | 14 --------------
 crypto/secret_common.c  | 21 ++++++++++++++++++---
 crypto/secret_keyring.c | 14 --------------
 crypto/tlscredsanon.c   |  3 +--
 crypto/tlscredspsk.c    |  3 +--
 crypto/tlscredsx509.c   |  3 +--
 net/can/can_host.c      |  5 +++++
 7 files changed, 26 insertions(+), 37 deletions(-)

-- 
2.28.0

[PATCH 1/4] crypto: Move USER_CREATABLE to secret_common base class

[Kevin Wolf]

Instead of duplicating the code for user creatable objects in secret and
secret_keyring, move it to the common base clase secret_common. As the
base class is abstract, it won't become user creatable itself.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 crypto/secret.c         | 14 --------------
 crypto/secret_common.c  | 15 +++++++++++++++
 crypto/secret_keyring.c | 14 --------------
 3 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/crypto/secret.c b/crypto/secret.c
index 281cb81f0f..44eaff16f6 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -107,13 +107,6 @@ qcrypto_secret_prop_get_file(Object *obj,
 }
 
 
-static void
-qcrypto_secret_complete(UserCreatable *uc, Error **errp)
-{
-    object_property_set_bool(OBJECT(uc), "loaded", true, errp);
-}
-
-
 static void
 qcrypto_secret_finalize(Object *obj)
 {
@@ -129,9 +122,6 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
     QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
     sic->load_data = qcrypto_secret_load_data;
 
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
-    ucc->complete = qcrypto_secret_complete;
-
     object_class_property_add_str(oc, "data",
                                   qcrypto_secret_prop_get_data,
                                   qcrypto_secret_prop_set_data);
@@ -148,10 +138,6 @@ static const TypeInfo qcrypto_secret_info = {
     .instance_finalize = qcrypto_secret_finalize,
     .class_size = sizeof(QCryptoSecretClass),
     .class_init = qcrypto_secret_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_USER_CREATABLE },
-        { }
-    }
 };
 
 
diff --git a/crypto/secret_common.c b/crypto/secret_common.c
index b03d530867..35b82cb531 100644
--- a/crypto/secret_common.c
+++ b/crypto/secret_common.c
@@ -268,6 +268,13 @@ qcrypto_secret_prop_get_keyid(Object *obj,
 }
 
 
+static void
+qcrypto_secret_complete(UserCreatable *uc, Error **errp)
+{
+    object_property_set_bool(OBJECT(uc), "loaded", true, errp);
+}
+
+
 static void
 qcrypto_secret_finalize(Object *obj)
 {
@@ -281,6 +288,10 @@ qcrypto_secret_finalize(Object *obj)
 static void
 qcrypto_secret_class_init(ObjectClass *oc, void *data)
 {
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+    ucc->complete = qcrypto_secret_complete;
+
     object_class_property_add_bool(oc, "loaded",
                                    qcrypto_secret_prop_get_loaded,
                                    qcrypto_secret_prop_set_loaded);
@@ -390,6 +401,10 @@ static const TypeInfo qcrypto_secret_info = {
     .class_size = sizeof(QCryptoSecretCommonClass),
     .class_init = qcrypto_secret_class_init,
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
 };
 
 
diff --git a/crypto/secret_keyring.c b/crypto/secret_keyring.c
index 10d8bc48a0..1b7edec84a 100644
--- a/crypto/secret_keyring.c
+++ b/crypto/secret_keyring.c
@@ -102,22 +102,12 @@ qcrypto_secret_prop_get_key(Object *obj, Visitor *v,
 }
 
 
-static void
-qcrypto_secret_keyring_complete(UserCreatable *uc, Error **errp)
-{
-    object_property_set_bool(OBJECT(uc), "loaded", true, errp);
-}
-
-
 static void
 qcrypto_secret_keyring_class_init(ObjectClass *oc, void *data)
 {
     QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
     sic->load_data = qcrypto_secret_keyring_load_data;
 
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
-    ucc->complete = qcrypto_secret_keyring_complete;
-
     object_class_property_add(oc, "serial", "int32_t",
                                   qcrypto_secret_prop_get_key,
                                   qcrypto_secret_prop_set_key,
@@ -130,10 +120,6 @@ static const TypeInfo qcrypto_secret_info = {
     .name = TYPE_QCRYPTO_SECRET_KEYRING,
     .instance_size = sizeof(QCryptoSecretKeyring),
     .class_init = qcrypto_secret_keyring_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_USER_CREATABLE },
-        { }
-    }
 };
 
 
-- 
2.28.0

[PATCH 2/4] crypto: Forbid broken unloading of secrets

[Kevin Wolf]

qcrypto_secret_prop_set_loaded() forgets to reset secret->rawdata after
unloading a secret, which will lead to a double free at some point.

Because there is no use case for unloading an already loaded secret
(apart from deleting the whole secret object) and we know that nobody
could use this because it would lead to crashes, let's just forbid the
operation instead of fixing the unloading.

Eventually, we'll want to get rid of 'loaded' in the external interface,
but for the meantime this is more consistent with rng, which has a
similar property 'opened' that also can't be reset to false after it
became true.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 crypto/secret_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/secret_common.c b/crypto/secret_common.c
index 35b82cb531..714a15d5e5 100644
--- a/crypto/secret_common.c
+++ b/crypto/secret_common.c
@@ -191,9 +191,9 @@ qcrypto_secret_prop_set_loaded(Object *obj,
 
         secret->rawdata = input;
         secret->rawlen = inputlen;
-    } else {
-        g_free(secret->rawdata);
-        secret->rawlen = 0;
+    } else if (secret->rawdata) {
+        error_setg(errp, "Cannot unload secret");
+        return;
     }
 }
 
-- 
2.28.0

[PATCH 3/4] crypto: Fix memory leaks in set_loaded for tls-*

[Kevin Wolf]

If you set the loaded property to true when it was already true, the
state is overwritten without freeing the old state first. Change the
set_loaded callback so that it always frees the old state (which is a
no-op if nothing was loaded) and only then load if requestsd.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 crypto/tlscredsanon.c | 3 +--
 crypto/tlscredspsk.c  | 3 +--
 crypto/tlscredsx509.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c
index 30275b6847..bea5f76c55 100644
--- a/crypto/tlscredsanon.c
+++ b/crypto/tlscredsanon.c
@@ -123,10 +123,9 @@ qcrypto_tls_creds_anon_prop_set_loaded(Object *obj,
 {
     QCryptoTLSCredsAnon *creds = QCRYPTO_TLS_CREDS_ANON(obj);
 
+    qcrypto_tls_creds_anon_unload(creds);
     if (value) {
         qcrypto_tls_creds_anon_load(creds, errp);
-    } else {
-        qcrypto_tls_creds_anon_unload(creds);
     }
 }
 
diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
index e26807b899..f5a31108d1 100644
--- a/crypto/tlscredspsk.c
+++ b/crypto/tlscredspsk.c
@@ -192,10 +192,9 @@ qcrypto_tls_creds_psk_prop_set_loaded(Object *obj,
 {
     QCryptoTLSCredsPSK *creds = QCRYPTO_TLS_CREDS_PSK(obj);
 
+    qcrypto_tls_creds_psk_unload(creds);
     if (value) {
         qcrypto_tls_creds_psk_load(creds, errp);
-    } else {
-        qcrypto_tls_creds_psk_unload(creds);
     }
 }
 
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index dd7267ccdb..ed6e1dc202 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -694,10 +694,9 @@ qcrypto_tls_creds_x509_prop_set_loaded(Object *obj,
 {
     QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
 
+    qcrypto_tls_creds_x509_unload(creds);
     if (value) {
         qcrypto_tls_creds_x509_load(creds, errp);
-    } else {
-        qcrypto_tls_creds_x509_unload(creds);
     }
 }
 
-- 
2.28.0

[PATCH 4/4] can-host: Fix crash when 'canbus' property is not set

[Kevin Wolf]

Providing the 'if' property, but not 'canbus' segfaults like this:

 #0  0x0000555555b0f14d in can_bus_insert_client (bus=0x0, client=0x555556aa9af0) at ../net/can/can_core.c:88
 #1  0x00005555559c3803 in can_host_connect (ch=0x555556aa9ac0, errp=0x7fffffffd568) at ../net/can/can_host.c:62
 #2  0x00005555559c386a in can_host_complete (uc=0x555556aa9ac0, errp=0x7fffffffd568) at ../net/can/can_host.c:72
 #3  0x0000555555d52de9 in user_creatable_complete (uc=0x555556aa9ac0, errp=0x7fffffffd5c8) at ../qom/object_interfaces.c:23

Add the missing NULL check.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 net/can/can_host.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/can/can_host.c b/net/can/can_host.c
index be4547d913..ba7f163d0a 100644
--- a/net/can/can_host.c
+++ b/net/can/can_host.c
@@ -53,6 +53,11 @@ static void can_host_connect(CanHostState *ch, Error **errp)
     CanHostClass *chc = CAN_HOST_GET_CLASS(ch);
     Error *local_err = NULL;
 
+    if (ch->bus == NULL) {
+        error_setg(errp, "'canbus' property not set");
+        return;
+    }
+
     chc->connect(ch, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-- 
2.28.0

Re: [PATCH 0/4] User creatable object property setting fixes

[Daniel P. Berrangé]

On Mon, Nov 30, 2020 at 11:56:11AM +0100, Kevin Wolf wrote:
> While writing a QAPI schema for all user creatable QOM objects, I found
> a few problems in the existing property setting code of objects.
> 
> This series fixes some crashes and memory leaks related to property
> setting in user creatable objects.
> 
> There are many more problems of the sort that updating a property at
> runtime is allowed by most objects, but they aren't actually prepared to
> handle the update, so it doesn't result in the expected behaviour. I'm
> not trying to fix bugs of this class in this series.
> 
> Kevin Wolf (4):
>   crypto: Move USER_CREATABLE to secret_common base class
>   crypto: Forbid broken unloading of secrets
>   crypto: Fix memory leaks in set_loaded for tls-*

I've queued these three patches

>   can-host: Fix crash when 'canbus' property is not set
> 
>  crypto/secret.c         | 14 --------------
>  crypto/secret_common.c  | 21 ++++++++++++++++++---
>  crypto/secret_keyring.c | 14 --------------
>  crypto/tlscredsanon.c   |  3 +--
>  crypto/tlscredspsk.c    |  3 +--
>  crypto/tlscredsx509.c   |  3 +--
>  net/can/can_host.c      |  5 +++++
>  7 files changed, 26 insertions(+), 37 deletions(-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|