[PATCH 0/3] i386: Ensure feature names are always defined

[PATCH 0/3] i386: Ensure feature names are always defined

[Eduardo Habkost]

Forgetting to adding feature names to the feature array
seems to be a very common mistake.

Examples:

- Missing name for MSR_VMX_EPT_WB
  commit 0723cc8a5558 ("target/i386: add VMX features to named CPU models")
- Missing name for "ibrs" at
  https://lore.kernel.org/qemu-devel/0ad4017d-e755-94a3-859e-800661bcd2d1@amd.com

This series fixes the MSR_VMX_EPT_WB problem and adds a runtime
check that should detect similar mistakes even before CPU model
classes are registered.

Eduardo Habkost (3):
  i386: Add missing "vmx-ept-wb" feature name
  i386: Move asserts to separate x86_cpudef_validate() function
  i386: Sanity check CPU model feature sets

 target/i386/cpu.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

-- 
2.28.0

[PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Eduardo Habkost]

Not having a feature name in feature_word_info breaks error
reporting and query-cpu-model-expansion.  Add the missing feature
name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].

Fixes: 0723cc8a5558 ("target/i386: add VMX features to named CPU models")
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ae89024d366..2bf3ab78056 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1262,7 +1262,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "vmx-ept-execonly", NULL, NULL, NULL,
             NULL, NULL, "vmx-page-walk-4", "vmx-page-walk-5",
             NULL, NULL, NULL, NULL,
-            NULL, NULL, NULL, NULL,
+            NULL, NULL, "vmx-ept-wb", NULL,
             "vmx-ept-2mb", "vmx-ept-1gb", NULL, NULL,
             "vmx-invept", "vmx-eptad", "vmx-ept-advanced-exitinfo", NULL,
             NULL, "vmx-invept-single-context", "vmx-invept-all-context", NULL,
-- 
2.28.0

[PATCH 2/3] i386: Move asserts to separate x86_cpudef_validate() function

[Eduardo Habkost]

Additional sanity checks will be added to the code, so move the
existing asserts to a separate function.

Wrap the whole function in `#ifndef NDEBUG` because the checks
will become more complex than trivial assert() calls.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2bf3ab78056..6285fb00eb8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5431,17 +5431,25 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model)
     type_register(&ti);
 }
 
-static void x86_register_cpudef_types(X86CPUDefinition *def)
+/* Sanity check CPU model definition before registering it */
+static void x86_cpudef_validate(X86CPUDefinition *def)
 {
-    X86CPUModel *m;
-    const X86CPUVersionDefinition *vdef;
-
+#ifndef NDEBUG
     /* AMD aliases are handled at runtime based on CPUID vendor, so
      * they shouldn't be set on the CPU model table.
      */
     assert(!(def->features[FEAT_8000_0001_EDX] & CPUID_EXT2_AMD_ALIASES));
     /* catch mistakes instead of silently truncating model_id when too long */
     assert(def->model_id && strlen(def->model_id) <= 48);
+#endif
+}
+
+static void x86_register_cpudef_types(X86CPUDefinition *def)
+{
+    X86CPUModel *m;
+    const X86CPUVersionDefinition *vdef;
+
+    x86_cpudef_validate(def);
 
     /* Unversioned model: */
     m = g_new0(X86CPUModel, 1);
-- 
2.28.0

[PATCH 3/3] i386: Sanity check CPU model feature sets

[Eduardo Habkost]

All CPU models must refer only to features that have their names
defined in feature_word_info[].feat_names, otherwise error
reporting and query-cpu-model-expansion will break.

Validate CPU feature flags in x86_cpudef_validate(), we can catch
mistakes more easily.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6285fb00eb8..3c066738e82 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5435,12 +5435,27 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model)
 static void x86_cpudef_validate(X86CPUDefinition *def)
 {
 #ifndef NDEBUG
+    FeatureWord w;
+    int bitnr;
+
     /* AMD aliases are handled at runtime based on CPUID vendor, so
      * they shouldn't be set on the CPU model table.
      */
     assert(!(def->features[FEAT_8000_0001_EDX] & CPUID_EXT2_AMD_ALIASES));
     /* catch mistakes instead of silently truncating model_id when too long */
     assert(def->model_id && strlen(def->model_id) <= 48);
+
+    /*
+     * CPU models must enable only features with valid names, otherwise
+     * error reporting and query-cpu-model-expansion can't work correctly.
+     */
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        for (bitnr = 0; bitnr < 64; bitnr++) {
+            uint64_t mask = (1ULL << bitnr);
+            assert(!(def->features[w] & mask) ||
+                   feature_word_info[w].feat_names[bitnr]);
+        }
+    }
 #endif
 }
 
-- 
2.28.0

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

This is intentional, because there's no way that any hypervisor can run if
this feature is disabled.

Paolo

Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha scritto:

> Not having a feature name in feature_word_info breaks error
> reporting and query-cpu-model-expansion.  Add the missing feature
> name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
>
> Fixes: 0723cc8a5558 ("target/i386: add VMX features to named CPU models")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ae89024d366..2bf3ab78056 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1262,7 +1262,7 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
>              "vmx-ept-execonly", NULL, NULL, NULL,
>              NULL, NULL, "vmx-page-walk-4", "vmx-page-walk-5",
>              NULL, NULL, NULL, NULL,
> -            NULL, NULL, NULL, NULL,
> +            NULL, NULL, "vmx-ept-wb", NULL,
>              "vmx-ept-2mb", "vmx-ept-1gb", NULL, NULL,
>              "vmx-invept", "vmx-eptad", "vmx-ept-advanced-exitinfo", NULL,
>              NULL, "vmx-invept-single-context", "vmx-invept-all-context",
> NULL,
> --
> 2.28.0
>
>

[-- Attachment #2: Type: text/html, Size: 1955 bytes --]

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Eduardo Habkost]

On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> 
> > Not having a feature name in feature_word_info breaks error
> > reporting and query-cpu-model-expansion.  Add the missing feature
> > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> >
> This is intentional, because there's no way that any hypervisor can run if
> this feature is disabled.

If leaving the feature without name enables some desirable
behavior, that's by accident and not by design.  Which part of
the existing behavior is intentional?

-- 
Eduardo

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:

> On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> > Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
> scritto:
> >
> > > Not having a feature name in feature_word_info breaks error
> > > reporting and query-cpu-model-expansion.  Add the missing feature
> > > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> > >
> > This is intentional, because there's no way that any hypervisor can run
> if
> > this feature is disabled.
>
> If leaving the feature without name enables some desirable
> behavior, that's by accident and not by design.  Which part of
> the existing behavior is intentional?
>

Not being able to disable it.

Paolo


> --
> Eduardo
>
>

[-- Attachment #2: Type: text/html, Size: 1520 bytes --]

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Eduardo Habkost]

On Tue, Feb 02, 2021 at 12:28:38AM +0100, Paolo Bonzini wrote:
> Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> 
> > On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> > > Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
> > scritto:
> > >
> > > > Not having a feature name in feature_word_info breaks error
> > > > reporting and query-cpu-model-expansion.  Add the missing feature
> > > > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> > > >
> > > This is intentional, because there's no way that any hypervisor can run
> > if
> > > this feature is disabled.
> >
> > If leaving the feature without name enables some desirable
> > behavior, that's by accident and not by design.  Which part of
> > the existing behavior is intentional?
> >
> 
> Not being able to disable it.

We can make it a hard dependency of vmx, then.  We shouldn't
leave it without a name, though.

-- 
Eduardo

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Paolo Bonzini]

On 02/02/21 01:18, Eduardo Habkost wrote:
> On Tue, Feb 02, 2021 at 12:28:38AM +0100, Paolo Bonzini wrote:
>> Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
>>
>>> On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
>>>> Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
>>> scritto:
>>>>
>>>>> Not having a feature name in feature_word_info breaks error
>>>>> reporting and query-cpu-model-expansion.  Add the missing feature
>>>>> name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
>>>>>
>>>> This is intentional, because there's no way that any hypervisor can run
>>> if
>>>> this feature is disabled.
>>>
>>> If leaving the feature without name enables some desirable
>>> behavior, that's by accident and not by design.  Which part of
>>> the existing behavior is intentional?
>>>
>>
>> Not being able to disable it.
> 
> We can make it a hard dependency of vmx, then.  We shouldn't
> leave it without a name, though.

The feature is already added to the MSRs unconditionally in 
kvm_msr_entry_add_vmx.  I think we can just remove it from the models 
instead.

Paolo

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

[Eduardo Habkost]

On Tue, Feb 02, 2021 at 08:54:30AM +0100, Paolo Bonzini wrote:
> On 02/02/21 01:18, Eduardo Habkost wrote:
> > On Tue, Feb 02, 2021 at 12:28:38AM +0100, Paolo Bonzini wrote:
> > > Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> > > 
> > > > On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> > > > > Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
> > > > scritto:
> > > > > 
> > > > > > Not having a feature name in feature_word_info breaks error
> > > > > > reporting and query-cpu-model-expansion.  Add the missing feature
> > > > > > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> > > > > > 
> > > > > This is intentional, because there's no way that any hypervisor can run
> > > > if
> > > > > this feature is disabled.
> > > > 
> > > > If leaving the feature without name enables some desirable
> > > > behavior, that's by accident and not by design.  Which part of
> > > > the existing behavior is intentional?
> > > > 
> > > 
> > > Not being able to disable it.
> > 
> > We can make it a hard dependency of vmx, then.  We shouldn't
> > leave it without a name, though.
> 
> The feature is already added to the MSRs unconditionally in
> kvm_msr_entry_add_vmx.  I think we can just remove it from the models
> instead.

Sounds even simpler, and better.  I'll do it in v2.

-- 
Eduardo

Re: [PATCH 2/3] i386: Move asserts to separate x86_cpudef_validate() function

[Philippe Mathieu-Daudé]

On 2/1/21 11:54 PM, Eduardo Habkost wrote:
> Additional sanity checks will be added to the code, so move the
> existing asserts to a separate function.
> 
> Wrap the whole function in `#ifndef NDEBUG` because the checks
> will become more complex than trivial assert() calls.

How can you build with NDEBUG? See commit 262a69f4282:

    osdep.h: Prohibit disabling assert() in supported builds

    We already have several files that knowingly require assert()
    to work, sometimes because refactoring the code for proper
    error handling has not been tackled yet; there are probably
    other files that have a similar situation but with no comments
    documenting the same.  In fact, we have places in migration
    that handle untrusted input with assertions, where disabling
    the assertions risks a worse security hole than the current
    behavior of losing the guest to SIGABRT when migration fails
    because of the assertion.  Promote our current per-file
    safety-valve to instead be project-wide, and expand it to also
    cover glib's g_assert().

and "qemu/osdep.h":

 /*
  * We have a lot of unaudited code that may fail in strange ways, or
  * even be a security risk during migration, if you disable assertions
  * at compile-time.  You may comment out these safety checks if you
  * absolutely want to disable assertion overhead, but it is not
  * supported upstream so the risk is all yours.  Meanwhile, please
  * submit patches to remove any side-effects inside an assertion, or
  * fixing error handling that should use Error instead of assert.
  */
 #ifdef NDEBUG
 #error building with NDEBUG is not supported
 #endif

IOW no need for the #idef.

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 2bf3ab78056..6285fb00eb8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5431,17 +5431,25 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model)
>      type_register(&ti);
>  }
>  
> -static void x86_register_cpudef_types(X86CPUDefinition *def)
> +/* Sanity check CPU model definition before registering it */
> +static void x86_cpudef_validate(X86CPUDefinition *def)
>  {
> -    X86CPUModel *m;
> -    const X86CPUVersionDefinition *vdef;
> -
> +#ifndef NDEBUG
>      /* AMD aliases are handled at runtime based on CPUID vendor, so
>       * they shouldn't be set on the CPU model table.
>       */
>      assert(!(def->features[FEAT_8000_0001_EDX] & CPUID_EXT2_AMD_ALIASES));
>      /* catch mistakes instead of silently truncating model_id when too long */
>      assert(def->model_id && strlen(def->model_id) <= 48);
> +#endif
> +}
> +
> +static void x86_register_cpudef_types(X86CPUDefinition *def)
> +{
> +    X86CPUModel *m;
> +    const X86CPUVersionDefinition *vdef;
> +
> +    x86_cpudef_validate(def);
>  
>      /* Unversioned model: */
>      m = g_new0(X86CPUModel, 1);
>