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

[Richard W.M. Jones]

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