[Qemu-devel] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[Qemu-devel] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[David Hildenbrand]

Usually, when baselining two CPU models, whereby one of them has base
CPU features disabled (e.g. z14-base,msa=off), we fallback to an older
model that did not have these features in the base model. We always try to
create a "sane" CPU model (as far as possible), and one part of it is that
removing base features is no good and to be avoided.

Now, if we disable base features that were part of a z900, we're out of
luck. We won't find a CPU model and QEMU will segfault. This is a
scenario that should never happen in real life, but it can be used to
crash QEMU.

So let's make something like this:

{ "execute": "query-cpu-model-baseline",
  "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}},
                  "modelb": { "name": "z14"}} }

Produce:

{"return": {"model": {"name": "z900-base", "props": {"esan3": false}}}}

Instead of segfaulting.

This could of course be improved (e.g. to z14-base,esan3=false), however
as this ususally won't happen, let's just avoid crashes.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index cfdbccf46d..13a5d4f095 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -716,6 +716,12 @@ CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
 
     model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
                                   model.features);
+
+    /* models without early base features (esan3) are bad - fallback to z900 */
+    if (!model.def) {
+        model.def = s390_find_cpu_def(0x2064, 7, 1, NULL);
+    }
+
     /* strip off features not part of the max model */
     bitmap_and(model.features, model.features, model.def->full_feat,
                S390_FEAT_MAX);
-- 
2.17.1

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[Christian Borntraeger]

On 07/18/2018 10:24 AM, David Hildenbrand wrote:
> Usually, when baselining two CPU models, whereby one of them has base
> CPU features disabled (e.g. z14-base,msa=off), we fallback to an older
> model that did not have these features in the base model. We always try to
> create a "sane" CPU model (as far as possible), and one part of it is that
> removing base features is no good and to be avoided.
> 
> Now, if we disable base features that were part of a z900, we're out of
> luck. We won't find a CPU model and QEMU will segfault. This is a
> scenario that should never happen in real life, but it can be used to
> crash QEMU.
> 
> So let's make something like this:
> 
> { "execute": "query-cpu-model-baseline",
>   "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}},
>                   "modelb": { "name": "z14"}} }
> 
> Produce:
> 
> {"return": {"model": {"name": "z900-base", "props": {"esan3": false}}}}
> 
> Instead of segfaulting.
> 
> This could of course be improved (e.g. to z14-base,esan3=false), however
> as this ususally won't happen, let's just avoid crashes.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu_models.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index cfdbccf46d..13a5d4f095 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -716,6 +716,12 @@ CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
>  
>      model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
>                                    model.features);
> +
> +    /* models without early base features (esan3) are bad - fallback to z900 */
> +    if (!model.def) {
> +        model.def = s390_find_cpu_def(0x2064, 7, 1, NULL);
> +    }
> +

Is there a way to not even return z900 but retuning an empty model (e.g. no model that 
matches) ?


>      /* strip off features not part of the max model */
>      bitmap_and(model.features, model.features, model.def->full_feat,
>                 S390_FEAT_MAX);
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[David Hildenbrand]

On 18.07.2018 10:39, Christian Borntraeger wrote:
> 
> 
> On 07/18/2018 10:24 AM, David Hildenbrand wrote:
>> Usually, when baselining two CPU models, whereby one of them has base
>> CPU features disabled (e.g. z14-base,msa=off), we fallback to an older
>> model that did not have these features in the base model. We always try to
>> create a "sane" CPU model (as far as possible), and one part of it is that
>> removing base features is no good and to be avoided.
>>
>> Now, if we disable base features that were part of a z900, we're out of
>> luck. We won't find a CPU model and QEMU will segfault. This is a
>> scenario that should never happen in real life, but it can be used to
>> crash QEMU.
>>
>> So let's make something like this:
>>
>> { "execute": "query-cpu-model-baseline",
>>   "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}},
>>                   "modelb": { "name": "z14"}} }
>>
>> Produce:
>>
>> {"return": {"model": {"name": "z900-base", "props": {"esan3": false}}}}
>>
>> Instead of segfaulting.
>>
>> This could of course be improved (e.g. to z14-base,esan3=false), however
>> as this ususally won't happen, let's just avoid crashes.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/cpu_models.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index cfdbccf46d..13a5d4f095 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -716,6 +716,12 @@ CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
>>  
>>      model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
>>                                    model.features);
>> +
>> +    /* models without early base features (esan3) are bad - fallback to z900 */
>> +    if (!model.def) {
>> +        model.def = s390_find_cpu_def(0x2064, 7, 1, NULL);
>> +    }
>> +
> 
> Is there a way to not even return z900 but retuning an empty model (e.g. no model that 
> matches) ?

An error would be an alternative.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[Christian Borntraeger]

On 07/18/2018 10:40 AM, David Hildenbrand wrote:
> On 18.07.2018 10:39, Christian Borntraeger wrote:
>>
>>
>> On 07/18/2018 10:24 AM, David Hildenbrand wrote:
>>> Usually, when baselining two CPU models, whereby one of them has base
>>> CPU features disabled (e.g. z14-base,msa=off), we fallback to an older
>>> model that did not have these features in the base model. We always try to
>>> create a "sane" CPU model (as far as possible), and one part of it is that
>>> removing base features is no good and to be avoided.
>>>
>>> Now, if we disable base features that were part of a z900, we're out of
>>> luck. We won't find a CPU model and QEMU will segfault. This is a
>>> scenario that should never happen in real life, but it can be used to
>>> crash QEMU.
>>>
>>> So let's make something like this:
>>>
>>> { "execute": "query-cpu-model-baseline",
>>>   "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}},
>>>                   "modelb": { "name": "z14"}} }
>>>
>>> Produce:
>>>
>>> {"return": {"model": {"name": "z900-base", "props": {"esan3": false}}}}
>>>
>>> Instead of segfaulting.
>>>
>>> This could of course be improved (e.g. to z14-base,esan3=false), however
>>> as this ususally won't happen, let's just avoid crashes.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  target/s390x/cpu_models.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index cfdbccf46d..13a5d4f095 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -716,6 +716,12 @@ CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
>>>  
>>>      model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
>>>                                    model.features);
>>> +
>>> +    /* models without early base features (esan3) are bad - fallback to z900 */
>>> +    if (!model.def) {
>>> +        model.def = s390_find_cpu_def(0x2064, 7, 1, NULL);
>>> +    }
>>> +
>>
>> Is there a way to not even return z900 but retuning an empty model (e.g. no model that 
>> matches) ?
> 
> An error would be an alternative.

As N3 means new in zarch (and backported to ESA390 mode), there is no machine without N3.
So I would prefer an error.

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[David Hildenbrand]

On 18.07.2018 10:44, Christian Borntraeger wrote:
> 
> 
> On 07/18/2018 10:40 AM, David Hildenbrand wrote:
>> On 18.07.2018 10:39, Christian Borntraeger wrote:
>>>
>>>
>>> On 07/18/2018 10:24 AM, David Hildenbrand wrote:
>>>> Usually, when baselining two CPU models, whereby one of them has base
>>>> CPU features disabled (e.g. z14-base,msa=off), we fallback to an older
>>>> model that did not have these features in the base model. We always try to
>>>> create a "sane" CPU model (as far as possible), and one part of it is that
>>>> removing base features is no good and to be avoided.
>>>>
>>>> Now, if we disable base features that were part of a z900, we're out of
>>>> luck. We won't find a CPU model and QEMU will segfault. This is a
>>>> scenario that should never happen in real life, but it can be used to
>>>> crash QEMU.
>>>>
>>>> So let's make something like this:
>>>>
>>>> { "execute": "query-cpu-model-baseline",
>>>>   "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}},
>>>>                   "modelb": { "name": "z14"}} }
>>>>
>>>> Produce:
>>>>
>>>> {"return": {"model": {"name": "z900-base", "props": {"esan3": false}}}}
>>>>
>>>> Instead of segfaulting.
>>>>
>>>> This could of course be improved (e.g. to z14-base,esan3=false), however
>>>> as this ususally won't happen, let's just avoid crashes.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  target/s390x/cpu_models.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>>> index cfdbccf46d..13a5d4f095 100644
>>>> --- a/target/s390x/cpu_models.c
>>>> +++ b/target/s390x/cpu_models.c
>>>> @@ -716,6 +716,12 @@ CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
>>>>  
>>>>      model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
>>>>                                    model.features);
>>>> +
>>>> +    /* models without early base features (esan3) are bad - fallback to z900 */
>>>> +    if (!model.def) {
>>>> +        model.def = s390_find_cpu_def(0x2064, 7, 1, NULL);
>>>> +    }
>>>> +
>>>
>>> Is there a way to not even return z900 but retuning an empty model (e.g. no model that 
>>> matches) ?
>>
>> An error would be an alternative.
> 
> As N3 means new in zarch (and backported to ESA390 mode), there is no machine without N3.
> So I would prefer an error.
> 

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index cfdbccf46d..604898a882 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -716,6 +716,14 @@ CpuModelBaselineInfo
*arch_query_cpu_model_baseline(CpuModelInfo *infoa,

     model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
                                   model.features);
+
+    /* models without early base features (esan3) are bad */
+    if (!model.def) {
+        error_setg(errp, "No compatible CPU model could be created as"
+                   " important base features are disabled");
+        return NULL;
+    }
+
     /* strip off features not part of the max model */
     bitmap_and(model.features, model.features, model.def->full_feat,
                S390_FEAT_MAX);


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[Cornelia Huck]

On Wed, 18 Jul 2018 10:50:01 +0200
David Hildenbrand <david@redhat.com> wrote:

> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index cfdbccf46d..604898a882 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -716,6 +716,14 @@ CpuModelBaselineInfo
> *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
> 
>      model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
>                                    model.features);
> +
> +    /* models without early base features (esan3) are bad */
> +    if (!model.def) {
> +        error_setg(errp, "No compatible CPU model could be created as"
> +                   " important base features are disabled");
> +        return NULL;
> +    }
> +
>      /* strip off features not part of the max model */
>      bitmap_and(model.features, model.features, model.def->full_feat,
>                 S390_FEAT_MAX);
> 
> 

+1, would queue.

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390x/cpumodel: fix segmentation fault when baselining models

[Cornelia Huck]

On Wed, 18 Jul 2018 10:40:18 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 18.07.2018 10:39, Christian Borntraeger wrote:
> > 
> > 
> > On 07/18/2018 10:24 AM, David Hildenbrand wrote:  
> >> Usually, when baselining two CPU models, whereby one of them has base
> >> CPU features disabled (e.g. z14-base,msa=off), we fallback to an older
> >> model that did not have these features in the base model. We always try to
> >> create a "sane" CPU model (as far as possible), and one part of it is that
> >> removing base features is no good and to be avoided.
> >>
> >> Now, if we disable base features that were part of a z900, we're out of
> >> luck. We won't find a CPU model and QEMU will segfault. This is a
> >> scenario that should never happen in real life, but it can be used to
> >> crash QEMU.
> >>
> >> So let's make something like this:
> >>
> >> { "execute": "query-cpu-model-baseline",
> >>   "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}},
> >>                   "modelb": { "name": "z14"}} }
> >>
> >> Produce:
> >>
> >> {"return": {"model": {"name": "z900-base", "props": {"esan3": false}}}}
> >>
> >> Instead of segfaulting.
> >>
> >> This could of course be improved (e.g. to z14-base,esan3=false), however
> >> as this ususally won't happen, let's just avoid crashes.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  target/s390x/cpu_models.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> >> index cfdbccf46d..13a5d4f095 100644
> >> --- a/target/s390x/cpu_models.c
> >> +++ b/target/s390x/cpu_models.c
> >> @@ -716,6 +716,12 @@ CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelInfo *infoa,
> >>  
> >>      model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
> >>                                    model.features);
> >> +
> >> +    /* models without early base features (esan3) are bad - fallback to z900 */
> >> +    if (!model.def) {
> >> +        model.def = s390_find_cpu_def(0x2064, 7, 1, NULL);
> >> +    }
> >> +  
> > 
> > Is there a way to not even return z900 but retuning an empty model (e.g. no model that 
> > matches) ?  
> 
> An error would be an alternative.
> 
> 

An error looks a bit saner to me. As long as we avoid unexpected
segfaults :)