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

Message ID 20191130194240.10517-17-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
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(-)

Comments

David Hildenbrand Dec. 1, 2019, 2:25 p.m. UTC | #1
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>
Markus Armbruster Dec. 2, 2019, 5:02 a.m. UTC | #2
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!

Patch
diff mbox series

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;
     }