[v2,1/9] KVM: x86: Remove emulator's broken checks on CR0/CR3/CR4 loads
diff mbox series

Message ID 20210422022128.3464144-2-seanjc@google.com
State New
Headers show
Series
  • KVM: x86: Fixes for (benign?) truncation bugs
Related show

Commit Message

Sean Christopherson April 22, 2021, 2:21 a.m. UTC
Remove the emulator's checks for illegal CR0, CR3, and CR4 values, as
the checks are redundant, outdated, and in the case of SEV's C-bit,
broken.  The emulator manually calculates MAXPHYADDR from CPUID and
neglects to mask off the C-bit.  For all other checks, kvm_set_cr*() are
a superset of the emulator checks, e.g. see CR4.LA57.

Fixes: a780a3ea6282 ("KVM: X86: Fix reserved bits check for MOV to CR3")
Cc: Babu Moger <babu.moger@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 68 +-----------------------------------------
 1 file changed, 1 insertion(+), 67 deletions(-)

Comments

Paolo Bonzini April 22, 2021, 6:50 a.m. UTC | #1
On 22/04/21 04:21, Sean Christopherson wrote:
> Remove the emulator's checks for illegal CR0, CR3, and CR4 values, as
> the checks are redundant, outdated, and in the case of SEV's C-bit,
> broken.  The emulator manually calculates MAXPHYADDR from CPUID and
> neglects to mask off the C-bit.  For all other checks, kvm_set_cr*() are
> a superset of the emulator checks, e.g. see CR4.LA57.
> 
> Fixes: a780a3ea6282 ("KVM: X86: Fix reserved bits check for MOV to CR3")
> Cc: Babu Moger <babu.moger@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/emulate.c | 68 +-----------------------------------------
>   1 file changed, 1 insertion(+), 67 deletions(-)

This can be (opportunistically ;)) squashed on top:

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f4273b8e31fa..abd9a4db11a8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4220,15 +4220,7 @@ static bool valid_cr(int nr)
  	}
  }
  
-static int check_cr_read(struct x86_emulate_ctxt *ctxt)
-{
-	if (!valid_cr(ctxt->modrm_reg))
-		return emulate_ud(ctxt);
-
-	return X86EMUL_CONTINUE;
-}
-
-static int check_cr_write(struct x86_emulate_ctxt *ctxt)
+static int check_cr_access(struct x86_emulate_ctxt *ctxt)
  {
  	if (!valid_cr(ctxt->modrm_reg))
  		return emulate_ud(ctxt);
@@ -4775,10 +4767,10 @@ static const struct opcode twobyte_table[256] = {
  	D(ImplicitOps | ModRM | SrcMem | NoAccess), /* 8 * reserved NOP */
  	D(ImplicitOps | ModRM | SrcMem | NoAccess), /* NOP + 7 * reserved NOP */
  	/* 0x20 - 0x2F */
-	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, cr_read, check_cr_read),
+	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, cr_read, check_cr_access),
  	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, dr_read, check_dr_read),
  	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_cr_write, cr_write,
-						check_cr_write),
+						check_cr_access),
  	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_dr_write, dr_write,
  						check_dr_write),
  	N, N, N, N,


> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f7970ba6219f..f4273b8e31fa 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4230,75 +4230,9 @@ static int check_cr_read(struct x86_emulate_ctxt *ctxt)
>   
>   static int check_cr_write(struct x86_emulate_ctxt *ctxt)
>   {
> -	u64 new_val = ctxt->src.val64;
> -	int cr = ctxt->modrm_reg;
> -	u64 efer = 0;
> -
> -	static u64 cr_reserved_bits[] = {
> -		0xffffffff00000000ULL,
> -		0, 0, 0, /* CR3 checked later */
> -		CR4_RESERVED_BITS,
> -		0, 0, 0,
> -		CR8_RESERVED_BITS,
> -	};
> -
> -	if (!valid_cr(cr))
> +	if (!valid_cr(ctxt->modrm_reg))
>   		return emulate_ud(ctxt);
>   
> -	if (new_val & cr_reserved_bits[cr])
> -		return emulate_gp(ctxt, 0);
> -
> -	switch (cr) {
> -	case 0: {
> -		u64 cr4;
> -		if (((new_val & X86_CR0_PG) && !(new_val & X86_CR0_PE)) ||
> -		    ((new_val & X86_CR0_NW) && !(new_val & X86_CR0_CD)))
> -			return emulate_gp(ctxt, 0);
> -
> -		cr4 = ctxt->ops->get_cr(ctxt, 4);
> -		ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
> -
> -		if ((new_val & X86_CR0_PG) && (efer & EFER_LME) &&
> -		    !(cr4 & X86_CR4_PAE))
> -			return emulate_gp(ctxt, 0);
> -
> -		break;
> -		}
> -	case 3: {
> -		u64 rsvd = 0;
> -
> -		ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
> -		if (efer & EFER_LMA) {
> -			u64 maxphyaddr;
> -			u32 eax, ebx, ecx, edx;
> -
> -			eax = 0x80000008;
> -			ecx = 0;
> -			if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx,
> -						 &edx, true))
> -				maxphyaddr = eax & 0xff;
> -			else
> -				maxphyaddr = 36;
> -			rsvd = rsvd_bits(maxphyaddr, 63);
> -			if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
> -				rsvd &= ~X86_CR3_PCID_NOFLUSH;
> -		}
> -
> -		if (new_val & rsvd)
> -			return emulate_gp(ctxt, 0);
> -
> -		break;
> -		}
> -	case 4: {
> -		ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
> -
> -		if ((efer & EFER_LMA) && !(new_val & X86_CR4_PAE))
> -			return emulate_gp(ctxt, 0);
> -
> -		break;
> -		}
> -	}
> -
>   	return X86EMUL_CONTINUE;
>   }
>   
>

Patch
diff mbox series

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f7970ba6219f..f4273b8e31fa 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4230,75 +4230,9 @@  static int check_cr_read(struct x86_emulate_ctxt *ctxt)
 
 static int check_cr_write(struct x86_emulate_ctxt *ctxt)
 {
-	u64 new_val = ctxt->src.val64;
-	int cr = ctxt->modrm_reg;
-	u64 efer = 0;
-
-	static u64 cr_reserved_bits[] = {
-		0xffffffff00000000ULL,
-		0, 0, 0, /* CR3 checked later */
-		CR4_RESERVED_BITS,
-		0, 0, 0,
-		CR8_RESERVED_BITS,
-	};
-
-	if (!valid_cr(cr))
+	if (!valid_cr(ctxt->modrm_reg))
 		return emulate_ud(ctxt);
 
-	if (new_val & cr_reserved_bits[cr])
-		return emulate_gp(ctxt, 0);
-
-	switch (cr) {
-	case 0: {
-		u64 cr4;
-		if (((new_val & X86_CR0_PG) && !(new_val & X86_CR0_PE)) ||
-		    ((new_val & X86_CR0_NW) && !(new_val & X86_CR0_CD)))
-			return emulate_gp(ctxt, 0);
-
-		cr4 = ctxt->ops->get_cr(ctxt, 4);
-		ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
-
-		if ((new_val & X86_CR0_PG) && (efer & EFER_LME) &&
-		    !(cr4 & X86_CR4_PAE))
-			return emulate_gp(ctxt, 0);
-
-		break;
-		}
-	case 3: {
-		u64 rsvd = 0;
-
-		ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
-		if (efer & EFER_LMA) {
-			u64 maxphyaddr;
-			u32 eax, ebx, ecx, edx;
-
-			eax = 0x80000008;
-			ecx = 0;
-			if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx,
-						 &edx, true))
-				maxphyaddr = eax & 0xff;
-			else
-				maxphyaddr = 36;
-			rsvd = rsvd_bits(maxphyaddr, 63);
-			if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_PCIDE)
-				rsvd &= ~X86_CR3_PCID_NOFLUSH;
-		}
-
-		if (new_val & rsvd)
-			return emulate_gp(ctxt, 0);
-
-		break;
-		}
-	case 4: {
-		ctxt->ops->get_msr(ctxt, MSR_EFER, &efer);
-
-		if ((efer & EFER_LMA) && !(new_val & X86_CR4_PAE))
-			return emulate_gp(ctxt, 0);
-
-		break;
-		}
-	}
-
 	return X86EMUL_CONTINUE;
 }