[PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs

Markus Armbruster posted 21 patches 6 years, 2 months ago
Maintainers: David Gibson <david@gibson.dropbear.id.au>, Igor Mammedov <imammedo@redhat.com>, Corey Minyard <minyard@acm.org>, Richard Henderson <rth@twiddle.net>, Jason Wang <jasowang@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Markus Armbruster <armbru@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, "Michael S. Tsirkin" <mst@redhat.com>, David Hildenbrand <david@redhat.com>, Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>, Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paul Burton <pburton@wavecomp.com>, Christian Borntraeger <borntraeger@de.ibm.com>
[PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs
Posted by Markus Armbruster 6 years, 2 months ago
get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
and its @errp argument is null.

apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
its @errp argument is null.

s390_realize_cpu_model() crashes when get_max_cpu_model() or
check_compatibility() fail, and its @errp argument is null.

All three messed up in commit 80560137cf "s390x/cpumodel: check and
apply the CPU model".

The bugs can't bite as no caller actually passes null.  Fix them
anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/s390x/cpu_models.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 6a29fd3ab1..c702e34a26 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -870,6 +870,7 @@ static void check_compatibility(const S390CPUModel *max_model,
 
 static S390CPUModel *get_max_cpu_model(Error **errp)
 {
+    Error *err = NULL;
     static S390CPUModel max_model;
     static bool cached;
 
@@ -878,22 +879,24 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
     }
 
     if (kvm_enabled()) {
-        kvm_s390_get_host_cpu_model(&max_model, errp);
+        kvm_s390_get_host_cpu_model(&max_model, &err);
     } else {
         max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN,
                                           QEMU_MAX_CPU_EC_GA, NULL);
         bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX);
-   }
-    if (!*errp) {
-        cached = true;
-        return &max_model;
     }
-    return NULL;
+    if (err) {
+        error_propagate(errp, err);
+        return NULL;
+    }
+    cached = true;
+    return &max_model;
 }
 
 static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 {
 #ifndef CONFIG_USER_ONLY
+    Error *err = NULL;
     static S390CPUModel applied_model;
     static bool applied;
 
@@ -909,20 +912,23 @@ static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
     }
 
     if (kvm_enabled()) {
-        kvm_s390_apply_cpu_model(model, errp);
+        kvm_s390_apply_cpu_model(model, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
     }
 
-    if (!*errp) {
-        applied = true;
-        if (model) {
-            applied_model = *model;
-        }
+    applied = true;
+    if (model) {
+        applied_model = *model;
     }
 #endif
 }
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+    Error *err = NULL;
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
     S390CPU *cpu = S390_CPU(cs);
     const S390CPUModel *max_model;
@@ -939,7 +945,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     }
 
     max_model = get_max_cpu_model(errp);
-    if (*errp) {
+    if (!max_model) {
         error_prepend(errp, "CPU models are not available: ");
         return;
     }
@@ -951,8 +957,9 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->model->cpu_ver = max_model->cpu_ver;
 
     check_consistency(cpu->model);
-    check_compatibility(max_model, cpu->model, errp);
-    if (*errp) {
+    check_compatibility(max_model, cpu->model, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
 
-- 
2.21.0


Re: [PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs
Posted by David Hildenbrand 6 years, 2 months ago
On 30.11.19 20:42, Markus Armbruster wrote:
> get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
> and its @errp argument is null.
> 
> apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
> its @errp argument is null.
> 
> s390_realize_cpu_model() crashes when get_max_cpu_model() or
> check_compatibility() fail, and its @errp argument is null.
> 
> All three messed up in commit 80560137cf "s390x/cpumodel: check and
> apply the CPU model".
> 
> The bugs can't bite as no caller actually passes null.  Fix them
> anyway.
> 

Subject is wrong, should e.g., start with "s390x/cpumodels". (I am not
aware of CPU modules :) )

[...]

Same comment regarding "local_err" and "BUG".

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb


Re: [PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs
Posted by Markus Armbruster 6 years, 2 months ago
David Hildenbrand <david@redhat.com> writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
>> and its @errp argument is null.
>> 
>> apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
>> its @errp argument is null.
>> 
>> s390_realize_cpu_model() crashes when get_max_cpu_model() or
>> check_compatibility() fail, and its @errp argument is null.
>> 
>> All three messed up in commit 80560137cf "s390x/cpumodel: check and
>> apply the CPU model".
>> 
>> The bugs can't bite as no caller actually passes null.  Fix them
>> anyway.
>> 
>
> Subject is wrong, should e.g., start with "s390x/cpumodels". (I am not
> aware of CPU modules :) )

Of course.

> [...]
>
> Same comment regarding "local_err" and "BUG".
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!