1. Patchew
  2. QEMU
  3. [PULL 00/15] Misc bugfix patches for 2021-07-22
  4. View patch

[PULL 08/15] target/i386: Added consistency checks for CR4

Paolo Bonzini posted 15 patches 4 years, 6 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <ehabkost@redhat.com>
[PULL 08/15] target/i386: Added consistency checks for CR4
Posted by Paolo Bonzini 4 years, 6 months ago 
From: Lara Lazier <laramglazier@gmail.com>

All MBZ bits in CR4 must be zero. (APM2 15.5)
Added reserved bitmask and added checks in both
helper_vmrun and helper_write_crN.

Signed-off-by: Lara Lazier <laramglazier@gmail.com>
Message-Id: <20210721152651.14683-2-laramglazier@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h                    | 39 ++++++++++++++++++++++++++++
 target/i386/tcg/sysemu/misc_helper.c |  3 +++
 target/i386/tcg/sysemu/svm_helper.c  |  9 ++++---
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5d98a4e7c0..1f7e8d7f0a 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -240,6 +240,7 @@ typedef enum X86Seg {
 #define CR4_OSFXSR_SHIFT 9
 #define CR4_OSFXSR_MASK (1U << CR4_OSFXSR_SHIFT)
 #define CR4_OSXMMEXCPT_MASK  (1U << 10)
+#define CR4_UMIP_MASK   (1U << 11)
 #define CR4_LA57_MASK   (1U << 12)
 #define CR4_VMXE_MASK   (1U << 13)
 #define CR4_SMXE_MASK   (1U << 14)
@@ -251,6 +252,14 @@ typedef enum X86Seg {
 #define CR4_PKE_MASK   (1U << 22)
 #define CR4_PKS_MASK   (1U << 24)
 
+#define CR4_RESERVED_MASK \
+(~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
+                | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
+                | CR4_MCE_MASK | CR4_PGE_MASK | CR4_PCE_MASK \
+                | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK |CR4_UMIP_MASK \
+                | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
+                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK))
+
 #define DR6_BD          (1 << 13)
 #define DR6_BS          (1 << 14)
 #define DR6_BT          (1 << 15)
@@ -2196,6 +2205,36 @@ static inline bool hyperv_feat_enabled(X86CPU *cpu, int feat)
     return !!(cpu->hyperv_features & BIT(feat));
 }
 
+static inline uint64_t cr4_reserved_bits(CPUX86State *env)
+{
+    uint64_t reserved_bits = CR4_RESERVED_MASK;
+    if (!env->features[FEAT_XSAVE]) {
+        reserved_bits |= CR4_OSXSAVE_MASK;
+    }
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SMEP)) {
+        reserved_bits |= CR4_SMEP_MASK;
+    }
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SMAP)) {
+        reserved_bits |= CR4_SMAP_MASK;
+    }
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FSGSBASE)) {
+        reserved_bits |= CR4_FSGSBASE_MASK;
+    }
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKU)) {
+        reserved_bits |= CR4_PKE_MASK;
+    }
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_LA57)) {
+        reserved_bits |= CR4_LA57_MASK;
+    }
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_UMIP)) {
+        reserved_bits |= CR4_UMIP_MASK;
+    }
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS)) {
+        reserved_bits |= CR4_PKS_MASK;
+    }
+    return reserved_bits;
+}
+
 #if defined(TARGET_X86_64) && \
     defined(CONFIG_USER_ONLY) && \
     defined(CONFIG_LINUX)
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
index db0d8a9d79..a2af2c9bba 100644
--- a/target/i386/tcg/sysemu/misc_helper.c
+++ b/target/i386/tcg/sysemu/misc_helper.c
@@ -99,6 +99,9 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
         cpu_x86_update_cr3(env, t0);
         break;
     case 4:
+        if (t0 & cr4_reserved_bits(env)) {
+            cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+        }
         if (((t0 ^ env->cr[4]) & CR4_LA57_MASK) &&
             (env->hflags & HF_CS64_MASK)) {
             raise_exception_ra(env, EXCP0D_GPF, GETPC());
diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
index 72b03a345d..d7d7a86aa9 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -85,6 +85,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     uint32_t int_ctl;
     uint32_t asid;
     uint64_t new_cr0;
+    uint64_t new_cr4;
 
     cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
 
@@ -225,14 +226,16 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
     if ((new_cr0 & CR0_NW_MASK) && !(new_cr0 & CR0_CD_MASK)) {
         cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
     }
+    new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4));
+    if (new_cr4 & cr4_reserved_bits(env)) {
+        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+    }
     /* clear exit_info_2 so we behave like the real hardware */
     x86_stq_phys(cs,
              env->vm_vmcb + offsetof(struct vmcb, control.exit_info_2), 0);
 
     cpu_x86_update_cr0(env, new_cr0);
-    cpu_x86_update_cr4(env, x86_ldq_phys(cs,
-                                     env->vm_vmcb + offsetof(struct vmcb,
-                                                             save.cr4)));
+    cpu_x86_update_cr4(env, new_cr4);
     cpu_x86_update_cr3(env, x86_ldq_phys(cs,
                                      env->vm_vmcb + offsetof(struct vmcb,
                                                              save.cr3)));
-- 
2.31.1
Re: [PULL 08/15] target/i386: Added consistency checks for CR4
Posted by Richard W.M. Jones 4 years, 5 months ago 
On Thu, Jul 22, 2021 at 05:36:05PM +0200, Paolo Bonzini wrote:
> From: Lara Lazier <laramglazier@gmail.com>
> 
> All MBZ bits in CR4 must be zero. (APM2 15.5)
> Added reserved bitmask and added checks in both
> helper_vmrun and helper_write_crN.
> 
> Signed-off-by: Lara Lazier <laramglazier@gmail.com>
> Message-Id: <20210721152651.14683-2-laramglazier@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

FYI this commit causes a regression with -cpu max (regression analyzed
by Daniel Berrange).  See:

https://bugzilla.redhat.com/show_bug.cgi?id=1999700

Rich.

>  target/i386/cpu.h                    | 39 ++++++++++++++++++++++++++++
>  target/i386/tcg/sysemu/misc_helper.c |  3 +++
>  target/i386/tcg/sysemu/svm_helper.c  |  9 ++++---
>  3 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 5d98a4e7c0..1f7e8d7f0a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -240,6 +240,7 @@ typedef enum X86Seg {
>  #define CR4_OSFXSR_SHIFT 9
>  #define CR4_OSFXSR_MASK (1U << CR4_OSFXSR_SHIFT)
>  #define CR4_OSXMMEXCPT_MASK  (1U << 10)
> +#define CR4_UMIP_MASK   (1U << 11)
>  #define CR4_LA57_MASK   (1U << 12)
>  #define CR4_VMXE_MASK   (1U << 13)
>  #define CR4_SMXE_MASK   (1U << 14)
> @@ -251,6 +252,14 @@ typedef enum X86Seg {
>  #define CR4_PKE_MASK   (1U << 22)
>  #define CR4_PKS_MASK   (1U << 24)
>  
> +#define CR4_RESERVED_MASK \
> +(~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
> +                | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
> +                | CR4_MCE_MASK | CR4_PGE_MASK | CR4_PCE_MASK \
> +                | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK |CR4_UMIP_MASK \
> +                | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
> +                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK))
> +
>  #define DR6_BD          (1 << 13)
>  #define DR6_BS          (1 << 14)
>  #define DR6_BT          (1 << 15)
> @@ -2196,6 +2205,36 @@ static inline bool hyperv_feat_enabled(X86CPU *cpu, int feat)
>      return !!(cpu->hyperv_features & BIT(feat));
>  }
>  
> +static inline uint64_t cr4_reserved_bits(CPUX86State *env)
> +{
> +    uint64_t reserved_bits = CR4_RESERVED_MASK;
> +    if (!env->features[FEAT_XSAVE]) {
> +        reserved_bits |= CR4_OSXSAVE_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SMEP)) {
> +        reserved_bits |= CR4_SMEP_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SMAP)) {
> +        reserved_bits |= CR4_SMAP_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FSGSBASE)) {
> +        reserved_bits |= CR4_FSGSBASE_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKU)) {
> +        reserved_bits |= CR4_PKE_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_LA57)) {
> +        reserved_bits |= CR4_LA57_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_UMIP)) {
> +        reserved_bits |= CR4_UMIP_MASK;
> +    }
> +    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS)) {
> +        reserved_bits |= CR4_PKS_MASK;
> +    }
> +    return reserved_bits;
> +}
> +
>  #if defined(TARGET_X86_64) && \
>      defined(CONFIG_USER_ONLY) && \
>      defined(CONFIG_LINUX)
> diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c
> index db0d8a9d79..a2af2c9bba 100644
> --- a/target/i386/tcg/sysemu/misc_helper.c
> +++ b/target/i386/tcg/sysemu/misc_helper.c
> @@ -99,6 +99,9 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
>          cpu_x86_update_cr3(env, t0);
>          break;
>      case 4:
> +        if (t0 & cr4_reserved_bits(env)) {
> +            cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +        }
>          if (((t0 ^ env->cr[4]) & CR4_LA57_MASK) &&
>              (env->hflags & HF_CS64_MASK)) {
>              raise_exception_ra(env, EXCP0D_GPF, GETPC());
> diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c
> index 72b03a345d..d7d7a86aa9 100644
> --- a/target/i386/tcg/sysemu/svm_helper.c
> +++ b/target/i386/tcg/sysemu/svm_helper.c
> @@ -85,6 +85,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
>      uint32_t int_ctl;
>      uint32_t asid;
>      uint64_t new_cr0;
> +    uint64_t new_cr4;
>  
>      cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC());
>  
> @@ -225,14 +226,16 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
>      if ((new_cr0 & CR0_NW_MASK) && !(new_cr0 & CR0_CD_MASK)) {
>          cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
>      }
> +    new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4));
> +    if (new_cr4 & cr4_reserved_bits(env)) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }
>      /* clear exit_info_2 so we behave like the real hardware */
>      x86_stq_phys(cs,
>               env->vm_vmcb + offsetof(struct vmcb, control.exit_info_2), 0);
>  
>      cpu_x86_update_cr0(env, new_cr0);
> -    cpu_x86_update_cr4(env, x86_ldq_phys(cs,
> -                                     env->vm_vmcb + offsetof(struct vmcb,
> -                                                             save.cr4)));
> +    cpu_x86_update_cr4(env, new_cr4);
>      cpu_x86_update_cr3(env, x86_ldq_phys(cs,
>                                       env->vm_vmcb + offsetof(struct vmcb,
>                                                               save.cr3)));
> -- 
> 2.31.1
> 
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org
Re: [PULL 08/15] target/i386: Added consistency checks for CR4
Posted by Daniel P. Berrangé 4 years, 5 months ago 
On Tue, Aug 31, 2021 at 06:03:04PM +0100, Richard W.M. Jones wrote:
> On Thu, Jul 22, 2021 at 05:36:05PM +0200, Paolo Bonzini wrote:
> > From: Lara Lazier <laramglazier@gmail.com>
> > 
> > All MBZ bits in CR4 must be zero. (APM2 15.5)
> > Added reserved bitmask and added checks in both
> > helper_vmrun and helper_write_crN.
> > 
> > Signed-off-by: Lara Lazier <laramglazier@gmail.com>
> > Message-Id: <20210721152651.14683-2-laramglazier@gmail.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> FYI this commit causes a regression with -cpu max (regression analyzed
> by Daniel Berrange).  See:

Specifically this commit breaks the ability to boot current fedora
kernels with --cpu max.   Disabling 'la57' feature makes it work
again. Similarly enabling 'la57' on any named CPU model makes that
then break.


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 :|
Re: [PULL 08/15] target/i386: Added consistency checks for CR4
Posted by Daniel P. Berrangé 4 years, 5 months ago 
On Tue, Aug 31, 2021 at 06:03:04PM +0100, Richard W.M. Jones wrote:
> On Thu, Jul 22, 2021 at 05:36:05PM +0200, Paolo Bonzini wrote:
> > From: Lara Lazier <laramglazier@gmail.com>
> > 
> > All MBZ bits in CR4 must be zero. (APM2 15.5)
> > Added reserved bitmask and added checks in both
> > helper_vmrun and helper_write_crN.
> > 
> > Signed-off-by: Lara Lazier <laramglazier@gmail.com>
> > Message-Id: <20210721152651.14683-2-laramglazier@gmail.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> FYI this commit causes a regression with -cpu max (regression analyzed
> by Daniel Berrange).  See:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1999700
> 
> Rich.
> 
> >  target/i386/cpu.h                    | 39 ++++++++++++++++++++++++++++
> >  target/i386/tcg/sysemu/misc_helper.c |  3 +++
> >  target/i386/tcg/sysemu/svm_helper.c  |  9 ++++---
> >  3 files changed, 48 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 5d98a4e7c0..1f7e8d7f0a 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -240,6 +240,7 @@ typedef enum X86Seg {
> >  #define CR4_OSFXSR_SHIFT 9
> >  #define CR4_OSFXSR_MASK (1U << CR4_OSFXSR_SHIFT)
> >  #define CR4_OSXMMEXCPT_MASK  (1U << 10)
> > +#define CR4_UMIP_MASK   (1U << 11)
> >  #define CR4_LA57_MASK   (1U << 12)
> >  #define CR4_VMXE_MASK   (1U << 13)
> >  #define CR4_SMXE_MASK   (1U << 14)
> > @@ -251,6 +252,14 @@ typedef enum X86Seg {
> >  #define CR4_PKE_MASK   (1U << 22)
> >  #define CR4_PKS_MASK   (1U << 24)
> >  
> > +#define CR4_RESERVED_MASK \
> > +(~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
> > +                | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
> > +                | CR4_MCE_MASK | CR4_PGE_MASK | CR4_PCE_MASK \
> > +                | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK |CR4_UMIP_MASK \
> > +                | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
> > +                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK))
> > +

This is missing  CR4_LA57_MASK, and adding that makes Fedora kernels
boot again.

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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.