[PATCH v2 0/9] KVM: x86: Fixes for (benign?) truncation bugs

[PATCH v2 0/9] KVM: x86: Fixes for (benign?) truncation bugs

[Sean Christopherson]

Patches 01 and 02 fix theoretical bugs related to loading CRs through
the emulator.  The rest of the patches are a bunch of small fixes for
cases where KVM reads/writes a 64-bit register outside of 64-bit mode.

I stumbled on this when puzzling over commit 0107973a80ad ("KVM: x86:
Introduce cr3_lm_rsvd_bits in kvm_vcpu_arch"), which stated that SEV
guests failed to boot on PCID-enabled hosts.  Why only PCID hosts?

After much staring, I realized that the initial CR3 load in
rsm_enter_protected_mode() would skip the MAXPHYADDR check due to the
vCPU not being in long mode.  But due to the ordering problems with
PCID, when PCID is enabled in the guest, the second load of CR3 would
be done with long mode enabled and thus hit the SEV C-bit bug.

Changing kvm_set_cr3() made me look at the callers, and seeing that
SVM didn't properly truncate the value made me look at everything else,
and here we are.

Note, I strongly suspect the emulator still has bugs.  But, unless the
guest is deliberately trying to hit these types of bugs, even the ones
fixed here, they're likely benign.  I figured I was more likely to break
something than I was to fix something by diving into the emulator, so I
left it alone.  For now. :-)

v2: Rebase to kvm/queue, commit 89a22e37c8c2 ("KVM: avoid "deadlock"
    between install_new_memslots and MMU notifier")

v1: https://lkml.kernel.org/r/20210213010518.1682691-1-seanjc@google.com

Sean Christopherson (9):
  KVM: x86: Remove emulator's broken checks on CR0/CR3/CR4 loads
  KVM: x86: Check CR3 GPA for validity regardless of vCPU mode
  KVM: SVM: Truncate GPR value for DR and CR accesses in !64-bit mode
  KVM: VMX: Truncate GPR value for DR and CR reads in !64-bit mode
  KVM: nVMX: Truncate bits 63:32 of VMCS field on nested check in
    !64-bit
  KVM: nVMX: Truncate base/index GPR value on address calc in !64-bit
  KVM: x86/xen: Drop RAX[63:32] when processing hypercall
  KVM: SVM: Use default rAX size for INVLPGA emulation
  KVM: x86: Rename GPR accessors to make mode-aware variants the
    defaults

 arch/x86/kvm/emulate.c        | 68 +----------------------------------
 arch/x86/kvm/kvm_cache_regs.h | 19 ++++++----
 arch/x86/kvm/svm/svm.c        | 12 +++++--
 arch/x86/kvm/vmx/nested.c     | 14 ++++----
 arch/x86/kvm/vmx/vmx.c        |  6 ++--
 arch/x86/kvm/x86.c            | 19 ++++++----
 arch/x86/kvm/x86.h            |  8 ++---
 7 files changed, 48 insertions(+), 98 deletions(-)

-- 
2.31.1.498.g6c1eba8ee3d-goog

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

[Sean Christopherson]

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(-)

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;
 }
 
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 2/9] KVM: x86: Check CR3 GPA for validity regardless of vCPU mode

[Sean Christopherson]

Check CR3 for an invalid GPA even if the vCPU isn't in long mode.  For
bigger emulation flows, notably RSM, the vCPU mode may not be accurate
if CR0/CR4 are loaded after CR3.  For MOV CR3 and similar flows, the
caller is responsible for truncating the value.

Note, SMRAM.CR3 is read-only, so this is mostly a theoretical bug since
KVM will not have stored an illegal CR3 into SMRAM during SMI emulation.

Fixes: 660a5d517aaa ("KVM: x86: save/load state on SMM switch")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c9ba6f2d9bcd..63af93211871 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1078,10 +1078,15 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 		return 0;
 	}
 
-	if (is_long_mode(vcpu) && kvm_vcpu_is_illegal_gpa(vcpu, cr3))
+	/*
+	 * Do not condition the GPA check on long mode, this helper is used to
+	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee that
+	 * the current vCPU mode is accurate.
+	 */
+	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
 		return 1;
-	else if (is_pae_paging(vcpu) &&
-		 !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
+
+	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
 		return 1;
 
 	kvm_mmu_new_pgd(vcpu, cr3, skip_tlb_flush, skip_tlb_flush);
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 3/9] KVM: SVM: Truncate GPR value for DR and CR accesses in !64-bit mode

[Sean Christopherson]

Drop bits 63:32 on loads/stores to/from DRs and CRs when the vCPU is not
in 64-bit mode.  The APM states bits 63:32 are dropped for both DRs and
CRs:

  In 64-bit mode, the operand size is fixed at 64 bits without the need
  for a REX prefix. In non-64-bit mode, the operand size is fixed at 32
  bits and the upper 32 bits of the destination are forced to 0.

Fixes: 7ff76d58a9dc ("KVM: SVM: enhance MOV CR intercept handler")
Fixes: cae3797a4639 ("KVM: SVM: enhance mov DR intercept handler")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cd8c333ed2dc..6df12d7967db 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2462,7 +2462,7 @@ static int cr_interception(struct kvm_vcpu *vcpu)
 	err = 0;
 	if (cr >= 16) { /* mov to cr */
 		cr -= 16;
-		val = kvm_register_read(vcpu, reg);
+		val = kvm_register_readl(vcpu, reg);
 		trace_kvm_cr_write(cr, val);
 		switch (cr) {
 		case 0:
@@ -2508,7 +2508,7 @@ static int cr_interception(struct kvm_vcpu *vcpu)
 			kvm_queue_exception(vcpu, UD_VECTOR);
 			return 1;
 		}
-		kvm_register_write(vcpu, reg, val);
+		kvm_register_writel(vcpu, reg, val);
 		trace_kvm_cr_read(cr, val);
 	}
 	return kvm_complete_insn_gp(vcpu, err);
@@ -2574,11 +2574,11 @@ static int dr_interception(struct kvm_vcpu *vcpu)
 	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
 	if (dr >= 16) { /* mov to DRn  */
 		dr -= 16;
-		val = kvm_register_read(vcpu, reg);
+		val = kvm_register_readl(vcpu, reg);
 		err = kvm_set_dr(vcpu, dr, val);
 	} else {
 		kvm_get_dr(vcpu, dr, &val);
-		kvm_register_write(vcpu, reg, val);
+		kvm_register_writel(vcpu, reg, val);
 	}
 
 	return kvm_complete_insn_gp(vcpu, err);
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 4/9] KVM: VMX: Truncate GPR value for DR and CR reads in !64-bit mode

[Sean Christopherson]

Drop bits 63:32 when storing a DR/CR to a GPR when the vCPU is not in
64-bit mode.  Per the SDM:

  The operand size for these instructions is always 32 bits in non-64-bit
  modes, regardless of the operand-size attribute.

CR8 technically isn't affected as CR8 isn't accessible outside of 64-bit
mode, but fix it up for consistency and to allow for future cleanup.

Fixes: 6aa8b732ca01 ("[PATCH] kvm: userspace interface")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6501d66167b8..7ef4c11d655b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5132,12 +5132,12 @@ static int handle_cr(struct kvm_vcpu *vcpu)
 		case 3:
 			WARN_ON_ONCE(enable_unrestricted_guest);
 			val = kvm_read_cr3(vcpu);
-			kvm_register_write(vcpu, reg, val);
+			kvm_register_writel(vcpu, reg, val);
 			trace_kvm_cr_read(cr, val);
 			return kvm_skip_emulated_instruction(vcpu);
 		case 8:
 			val = kvm_get_cr8(vcpu);
-			kvm_register_write(vcpu, reg, val);
+			kvm_register_writel(vcpu, reg, val);
 			trace_kvm_cr_read(cr, val);
 			return kvm_skip_emulated_instruction(vcpu);
 		}
@@ -5210,7 +5210,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		unsigned long val;
 
 		kvm_get_dr(vcpu, dr, &val);
-		kvm_register_write(vcpu, reg, val);
+		kvm_register_writel(vcpu, reg, val);
 		err = 0;
 	} else {
 		err = kvm_set_dr(vcpu, dr, kvm_register_readl(vcpu, reg));
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 5/9] KVM: nVMX: Truncate bits 63:32 of VMCS field on nested check in !64-bit

[Sean Christopherson]

Drop bits 63:32 of the VMCS field encoding when checking for a nested
VM-Exit on VMREAD/VMWRITE in !64-bit mode.  VMREAD and VMWRITE always
use 32-bit operands outside of 64-bit mode.

The actual emulation of VMREAD/VMWRITE does the right thing, this bug is
purely limited to incorrectly causing a nested VM-Exit if a GPR happens
to have bits 63:32 set outside of 64-bit mode.

Fixes: a7cde481b6e8 ("KVM: nVMX: Do not forward VMREAD/VMWRITE VMExits to L1 if required so by vmcs12 vmread/vmwrite bitmaps")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8b111682fe5c..0e580305a1ee 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5743,7 +5743,7 @@ static bool nested_vmx_exit_handled_vmcs_access(struct kvm_vcpu *vcpu,
 
 	/* Decode instruction info and find the field to access */
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	field = kvm_register_read(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+	field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
 
 	/* Out-of-range fields always cause a VM exit from L2 to L1 */
 	if (field >> 15)
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 6/9] KVM: nVMX: Truncate base/index GPR value on address calc in !64-bit

[Sean Christopherson]

Drop bits 63:32 of the base and/or index GPRs when calculating the
effective address of a VMX instruction memory operand.  Outside of 64-bit
mode, memory encodings are strictly limited to E*X and below.

Fixes: 064aea774768 ("KVM: nVMX: Decoding memory operands of VMX instructions")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0e580305a1ee..4daf1ff45221 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4617,9 +4617,9 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
 	else if (addr_size == 0)
 		off = (gva_t)sign_extend64(off, 15);
 	if (base_is_valid)
-		off += kvm_register_read(vcpu, base_reg);
+		off += kvm_register_readl(vcpu, base_reg);
 	if (index_is_valid)
-		off += kvm_register_read(vcpu, index_reg) << scaling;
+		off += kvm_register_readl(vcpu, index_reg) << scaling;
 	vmx_get_segment(vcpu, &s, seg_reg);
 
 	/*
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 7/9] KVM: x86/xen: Drop RAX[63:32] when processing hypercall

[Sean Christopherson]

Truncate RAX to 32 bits, i.e. consume EAX, when retrieving the hypecall
index for a Xen hypercall.  Per Xen documentation[*], the index is EAX
when the vCPU is not in 64-bit mode.

[*] http://xenbits.xenproject.org/docs/sphinx-unstable/guest-guide/x86/hypercall-abi.html

Fixes: 23200b7a30de ("KVM: x86/xen: intercept xen hypercalls if enabled")
Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/xen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index ae17250e1efe..7f27bb65a572 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -673,7 +673,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 	bool longmode;
 	u64 input, params[6];
 
-	input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
+	input = (u64)kvm_register_readl(vcpu, VCPU_REGS_RAX);
 
 	/* Hyper-V hypercalls get bit 31 set in EAX */
 	if ((input & 0x80000000) &&
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 8/9] KVM: SVM: Use default rAX size for INVLPGA emulation

[Sean Christopherson]

Drop bits 63:32 of RAX when grabbing the address for INVLPGA emulation
outside of 64-bit mode to make KVM's emulation slightly less wrong.  The
address for INVLPGA is determined by the effective address size, i.e.
it's not hardcoded to 64/32 bits for a given mode.  Add a FIXME to call
out that the emulation is wrong.

Opportunistically tweak the ASID handling to make it clear that it's
defined by ECX, not rCX.

Per the APM:
   The portion of rAX used to form the address is determined by the
   effective address size (current execution mode and optional address
   size prefix). The ASID is taken from ECX.

Fixes: ff092385e828 ("KVM: SVM: Implement INVLPGA")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 6df12d7967db..ccf9499f2683 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2301,11 +2301,17 @@ static int clgi_interception(struct kvm_vcpu *vcpu)
 
 static int invlpga_interception(struct kvm_vcpu *vcpu)
 {
-	trace_kvm_invlpga(to_svm(vcpu)->vmcb->save.rip, kvm_rcx_read(vcpu),
-			  kvm_rax_read(vcpu));
+	gva_t gva = kvm_rax_read(vcpu);
+	u32 asid = kvm_rcx_read(vcpu);
+
+	/* FIXME: Handle an address size prefix. */
+	if (!is_long_mode(vcpu))
+		gva = (u32)gva;
+
+	trace_kvm_invlpga(to_svm(vcpu)->vmcb->save.rip, asid, gva);
 
 	/* Let's treat INVLPGA the same as INVLPG (can be optimized!) */
-	kvm_mmu_invlpg(vcpu, kvm_rax_read(vcpu));
+	kvm_mmu_invlpg(vcpu, gva);
 
 	return kvm_skip_emulated_instruction(vcpu);
 }
-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH v2 9/9] KVM: x86: Rename GPR accessors to make mode-aware variants the defaults

[Sean Christopherson]

Append raw to the direct variants of kvm_register_read/write(), and
drop the "l" from the mode-aware variants.  I.e. make the mode-aware
variants the default, and make the direct variants scary sounding so as
to discourage use.  Accessing the full 64-bit values irrespective of
mode is rarely the desired behavior.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/kvm_cache_regs.h | 19 ++++++++++++-------
 arch/x86/kvm/svm/svm.c        |  8 ++++----
 arch/x86/kvm/vmx/nested.c     | 20 ++++++++++----------
 arch/x86/kvm/vmx/vmx.c        | 12 ++++++------
 arch/x86/kvm/x86.c            |  8 ++++----
 arch/x86/kvm/x86.h            |  8 ++++----
 arch/x86/kvm/xen.c            |  2 +-
 7 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 2e11da2f5621..3db5c42c9ecd 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -62,7 +62,12 @@ static inline void kvm_register_mark_dirty(struct kvm_vcpu *vcpu,
 	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
 }
 
-static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg)
+/*
+ * The "raw" register helpers are only for cases where the full 64 bits of a
+ * register are read/written irrespective of current vCPU mode.  In other words,
+ * odds are good you shouldn't be using the raw variants.
+ */
+static inline unsigned long kvm_register_read_raw(struct kvm_vcpu *vcpu, int reg)
 {
 	if (WARN_ON_ONCE((unsigned int)reg >= NR_VCPU_REGS))
 		return 0;
@@ -73,8 +78,8 @@ static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg)
 	return vcpu->arch.regs[reg];
 }
 
-static inline void kvm_register_write(struct kvm_vcpu *vcpu, int reg,
-				      unsigned long val)
+static inline void kvm_register_write_raw(struct kvm_vcpu *vcpu, int reg,
+					  unsigned long val)
 {
 	if (WARN_ON_ONCE((unsigned int)reg >= NR_VCPU_REGS))
 		return;
@@ -85,22 +90,22 @@ static inline void kvm_register_write(struct kvm_vcpu *vcpu, int reg,
 
 static inline unsigned long kvm_rip_read(struct kvm_vcpu *vcpu)
 {
-	return kvm_register_read(vcpu, VCPU_REGS_RIP);
+	return kvm_register_read_raw(vcpu, VCPU_REGS_RIP);
 }
 
 static inline void kvm_rip_write(struct kvm_vcpu *vcpu, unsigned long val)
 {
-	kvm_register_write(vcpu, VCPU_REGS_RIP, val);
+	kvm_register_write_raw(vcpu, VCPU_REGS_RIP, val);
 }
 
 static inline unsigned long kvm_rsp_read(struct kvm_vcpu *vcpu)
 {
-	return kvm_register_read(vcpu, VCPU_REGS_RSP);
+	return kvm_register_read_raw(vcpu, VCPU_REGS_RSP);
 }
 
 static inline void kvm_rsp_write(struct kvm_vcpu *vcpu, unsigned long val)
 {
-	kvm_register_write(vcpu, VCPU_REGS_RSP, val);
+	kvm_register_write_raw(vcpu, VCPU_REGS_RSP, val);
 }
 
 static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu, int index)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ccf9499f2683..96df12157a8a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2468,7 +2468,7 @@ static int cr_interception(struct kvm_vcpu *vcpu)
 	err = 0;
 	if (cr >= 16) { /* mov to cr */
 		cr -= 16;
-		val = kvm_register_readl(vcpu, reg);
+		val = kvm_register_read(vcpu, reg);
 		trace_kvm_cr_write(cr, val);
 		switch (cr) {
 		case 0:
@@ -2514,7 +2514,7 @@ static int cr_interception(struct kvm_vcpu *vcpu)
 			kvm_queue_exception(vcpu, UD_VECTOR);
 			return 1;
 		}
-		kvm_register_writel(vcpu, reg, val);
+		kvm_register_write(vcpu, reg, val);
 		trace_kvm_cr_read(cr, val);
 	}
 	return kvm_complete_insn_gp(vcpu, err);
@@ -2580,11 +2580,11 @@ static int dr_interception(struct kvm_vcpu *vcpu)
 	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
 	if (dr >= 16) { /* mov to DRn  */
 		dr -= 16;
-		val = kvm_register_readl(vcpu, reg);
+		val = kvm_register_read(vcpu, reg);
 		err = kvm_set_dr(vcpu, dr, val);
 	} else {
 		kvm_get_dr(vcpu, dr, &val);
-		kvm_register_writel(vcpu, reg, val);
+		kvm_register_write(vcpu, reg, val);
 	}
 
 	return kvm_complete_insn_gp(vcpu, err);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 4daf1ff45221..89976c95091e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4617,9 +4617,9 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
 	else if (addr_size == 0)
 		off = (gva_t)sign_extend64(off, 15);
 	if (base_is_valid)
-		off += kvm_register_readl(vcpu, base_reg);
+		off += kvm_register_read(vcpu, base_reg);
 	if (index_is_valid)
-		off += kvm_register_readl(vcpu, index_reg) << scaling;
+		off += kvm_register_read(vcpu, index_reg) << scaling;
 	vmx_get_segment(vcpu, &s, seg_reg);
 
 	/*
@@ -5021,7 +5021,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
 		return nested_vmx_failInvalid(vcpu);
 
 	/* Decode instruction info and find the field to read */
-	field = kvm_register_readl(vcpu, (((instr_info) >> 28) & 0xf));
+	field = kvm_register_read(vcpu, (((instr_info) >> 28) & 0xf));
 
 	offset = vmcs_field_to_offset(field);
 	if (offset < 0)
@@ -5039,7 +5039,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
 	 * on the guest's mode (32 or 64 bit), not on the given field's length.
 	 */
 	if (instr_info & BIT(10)) {
-		kvm_register_writel(vcpu, (((instr_info) >> 3) & 0xf), value);
+		kvm_register_write(vcpu, (((instr_info) >> 3) & 0xf), value);
 	} else {
 		len = is_64_bit_mode(vcpu) ? 8 : 4;
 		if (get_vmx_mem_address(vcpu, exit_qualification,
@@ -5113,7 +5113,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
 		return nested_vmx_failInvalid(vcpu);
 
 	if (instr_info & BIT(10))
-		value = kvm_register_readl(vcpu, (((instr_info) >> 3) & 0xf));
+		value = kvm_register_read(vcpu, (((instr_info) >> 3) & 0xf));
 	else {
 		len = is_64_bit_mode(vcpu) ? 8 : 4;
 		if (get_vmx_mem_address(vcpu, exit_qualification,
@@ -5124,7 +5124,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
 			return kvm_handle_memory_failure(vcpu, r, &e);
 	}
 
-	field = kvm_register_readl(vcpu, (((instr_info) >> 28) & 0xf));
+	field = kvm_register_read(vcpu, (((instr_info) >> 28) & 0xf));
 
 	offset = vmcs_field_to_offset(field);
 	if (offset < 0)
@@ -5321,7 +5321,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 		return 1;
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
 
 	types = (vmx->nested.msrs.ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6;
 
@@ -5401,7 +5401,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 		return 1;
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
 
 	types = (vmx->nested.msrs.vpid_caps &
 			VMX_VPID_EXTENT_SUPPORTED_MASK) >> 8;
@@ -5657,7 +5657,7 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu,
 	switch ((exit_qualification >> 4) & 3) {
 	case 0: /* mov to cr */
 		reg = (exit_qualification >> 8) & 15;
-		val = kvm_register_readl(vcpu, reg);
+		val = kvm_register_read(vcpu, reg);
 		switch (cr) {
 		case 0:
 			if (vmcs12->cr0_guest_host_mask &
@@ -5743,7 +5743,7 @@ static bool nested_vmx_exit_handled_vmcs_access(struct kvm_vcpu *vcpu,
 
 	/* Decode instruction info and find the field to access */
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+	field = kvm_register_read(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
 
 	/* Out-of-range fields always cause a VM exit from L2 to L1 */
 	if (field >> 15)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 7ef4c11d655b..13ac981fae1d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5090,7 +5090,7 @@ static int handle_cr(struct kvm_vcpu *vcpu)
 	reg = (exit_qualification >> 8) & 15;
 	switch ((exit_qualification >> 4) & 3) {
 	case 0: /* mov to cr */
-		val = kvm_register_readl(vcpu, reg);
+		val = kvm_register_read(vcpu, reg);
 		trace_kvm_cr_write(cr, val);
 		switch (cr) {
 		case 0:
@@ -5132,12 +5132,12 @@ static int handle_cr(struct kvm_vcpu *vcpu)
 		case 3:
 			WARN_ON_ONCE(enable_unrestricted_guest);
 			val = kvm_read_cr3(vcpu);
-			kvm_register_writel(vcpu, reg, val);
+			kvm_register_write(vcpu, reg, val);
 			trace_kvm_cr_read(cr, val);
 			return kvm_skip_emulated_instruction(vcpu);
 		case 8:
 			val = kvm_get_cr8(vcpu);
-			kvm_register_writel(vcpu, reg, val);
+			kvm_register_write(vcpu, reg, val);
 			trace_kvm_cr_read(cr, val);
 			return kvm_skip_emulated_instruction(vcpu);
 		}
@@ -5210,10 +5210,10 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		unsigned long val;
 
 		kvm_get_dr(vcpu, dr, &val);
-		kvm_register_writel(vcpu, reg, val);
+		kvm_register_write(vcpu, reg, val);
 		err = 0;
 	} else {
-		err = kvm_set_dr(vcpu, dr, kvm_register_readl(vcpu, reg));
+		err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg));
 	}
 
 out:
@@ -5565,7 +5565,7 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
 	}
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
 
 	if (type > 3) {
 		kvm_inject_gp(vcpu, 0);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 63af93211871..5e7d1cd2e2a5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6974,12 +6974,12 @@ static bool emulator_guest_has_fxsr(struct x86_emulate_ctxt *ctxt)
 
 static ulong emulator_read_gpr(struct x86_emulate_ctxt *ctxt, unsigned reg)
 {
-	return kvm_register_read(emul_to_vcpu(ctxt), reg);
+	return kvm_register_read_raw(emul_to_vcpu(ctxt), reg);
 }
 
 static void emulator_write_gpr(struct x86_emulate_ctxt *ctxt, unsigned reg, ulong val)
 {
-	kvm_register_write(emul_to_vcpu(ctxt), reg, val);
+	kvm_register_write_raw(emul_to_vcpu(ctxt), reg, val);
 }
 
 static void emulator_set_nmi_mask(struct x86_emulate_ctxt *ctxt, bool masked)
@@ -8660,7 +8660,7 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, char *buf)
 	put_smstate(u32, buf, 0x7ff0, kvm_rip_read(vcpu));
 
 	for (i = 0; i < 8; i++)
-		put_smstate(u32, buf, 0x7fd0 + i * 4, kvm_register_read(vcpu, i));
+		put_smstate(u32, buf, 0x7fd0 + i * 4, kvm_register_read_raw(vcpu, i));
 
 	kvm_get_dr(vcpu, 6, &val);
 	put_smstate(u32, buf, 0x7fcc, (u32)val);
@@ -8706,7 +8706,7 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, char *buf)
 	int i;
 
 	for (i = 0; i < 16; i++)
-		put_smstate(u64, buf, 0x7ff8 - i * 8, kvm_register_read(vcpu, i));
+		put_smstate(u64, buf, 0x7ff8 - i * 8, kvm_register_read_raw(vcpu, i));
 
 	put_smstate(u64, buf, 0x7f78, kvm_rip_read(vcpu));
 	put_smstate(u32, buf, 0x7f70, kvm_get_rflags(vcpu));
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index daccf20fbcd5..f2a4094532cf 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -232,19 +232,19 @@ static inline bool vcpu_match_mmio_gpa(struct kvm_vcpu *vcpu, gpa_t gpa)
 	return false;
 }
 
-static inline unsigned long kvm_register_readl(struct kvm_vcpu *vcpu, int reg)
+static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg)
 {
-	unsigned long val = kvm_register_read(vcpu, reg);
+	unsigned long val = kvm_register_read_raw(vcpu, reg);
 
 	return is_64_bit_mode(vcpu) ? val : (u32)val;
 }
 
-static inline void kvm_register_writel(struct kvm_vcpu *vcpu,
+static inline void kvm_register_write(struct kvm_vcpu *vcpu,
 				       int reg, unsigned long val)
 {
 	if (!is_64_bit_mode(vcpu))
 		val = (u32)val;
-	return kvm_register_write(vcpu, reg, val);
+	return kvm_register_write_raw(vcpu, reg, val);
 }
 
 static inline bool kvm_check_has_quirk(struct kvm *kvm, u64 quirk)
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 7f27bb65a572..ae17250e1efe 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -673,7 +673,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 	bool longmode;
 	u64 input, params[6];
 
-	input = (u64)kvm_register_readl(vcpu, VCPU_REGS_RAX);
+	input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
 
 	/* Hyper-V hypercalls get bit 31 set in EAX */
 	if ((input & 0x80000000) &&
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH v2 1/9] KVM: x86: Remove emulator's broken checks on CR0/CR3/CR4 loads

[Paolo Bonzini]

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;
>   }
>   
> 

Re: [PATCH v2 2/9] KVM: x86: Check CR3 GPA for validity regardless of vCPU mode

[Paolo Bonzini]

On 22/04/21 04:21, Sean Christopherson wrote:
> Check CR3 for an invalid GPA even if the vCPU isn't in long mode.  For
> bigger emulation flows, notably RSM, the vCPU mode may not be accurate
> if CR0/CR4 are loaded after CR3.  For MOV CR3 and similar flows, the
> caller is responsible for truncating the value.
> 
> Note, SMRAM.CR3 is read-only, so this is mostly a theoretical bug since
> KVM will not have stored an illegal CR3 into SMRAM during SMI emulation.

Well, the guest could have changed it...

Paolo

> Fixes: 660a5d517aaa ("KVM: x86: save/load state on SMM switch")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c9ba6f2d9bcd..63af93211871 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1078,10 +1078,15 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>   		return 0;
>   	}
>   
> -	if (is_long_mode(vcpu) && kvm_vcpu_is_illegal_gpa(vcpu, cr3))
> +	/*
> +	 * Do not condition the GPA check on long mode, this helper is used to
> +	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee that
> +	 * the current vCPU mode is accurate.
> +	 */
> +	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
>   		return 1;
> -	else if (is_pae_paging(vcpu) &&
> -		 !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
> +
> +	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))
>   		return 1;
>   
>   	kvm_mmu_new_pgd(vcpu, cr3, skip_tlb_flush, skip_tlb_flush);
> 

Re: [PATCH v2 0/9] KVM: x86: Fixes for (benign?) truncation bugs

[Paolo Bonzini]

On 22/04/21 04:21, Sean Christopherson wrote:
> Patches 01 and 02 fix theoretical bugs related to loading CRs through
> the emulator.  The rest of the patches are a bunch of small fixes for
> cases where KVM reads/writes a 64-bit register outside of 64-bit mode.
> 
> I stumbled on this when puzzling over commit 0107973a80ad ("KVM: x86:
> Introduce cr3_lm_rsvd_bits in kvm_vcpu_arch"), which stated that SEV
> guests failed to boot on PCID-enabled hosts.  Why only PCID hosts?
> 
> After much staring, I realized that the initial CR3 load in
> rsm_enter_protected_mode() would skip the MAXPHYADDR check due to the
> vCPU not being in long mode.  But due to the ordering problems with
> PCID, when PCID is enabled in the guest, the second load of CR3 would
> be done with long mode enabled and thus hit the SEV C-bit bug.
> 
> Changing kvm_set_cr3() made me look at the callers, and seeing that
> SVM didn't properly truncate the value made me look at everything else,
> and here we are.
> 
> Note, I strongly suspect the emulator still has bugs.  But, unless the
> guest is deliberately trying to hit these types of bugs, even the ones
> fixed here, they're likely benign.  I figured I was more likely to break
> something than I was to fix something by diving into the emulator, so I
> left it alone.  For now. :-)
> 
> v2: Rebase to kvm/queue, commit 89a22e37c8c2 ("KVM: avoid "deadlock"
>      between install_new_memslots and MMU notifier")
> 
> v1: https://lkml.kernel.org/r/20210213010518.1682691-1-seanjc@google.com
> 
> Sean Christopherson (9):
>    KVM: x86: Remove emulator's broken checks on CR0/CR3/CR4 loads
>    KVM: x86: Check CR3 GPA for validity regardless of vCPU mode
>    KVM: SVM: Truncate GPR value for DR and CR accesses in !64-bit mode
>    KVM: VMX: Truncate GPR value for DR and CR reads in !64-bit mode
>    KVM: nVMX: Truncate bits 63:32 of VMCS field on nested check in
>      !64-bit
>    KVM: nVMX: Truncate base/index GPR value on address calc in !64-bit
>    KVM: x86/xen: Drop RAX[63:32] when processing hypercall
>    KVM: SVM: Use default rAX size for INVLPGA emulation
>    KVM: x86: Rename GPR accessors to make mode-aware variants the
>      defaults
> 
>   arch/x86/kvm/emulate.c        | 68 +----------------------------------
>   arch/x86/kvm/kvm_cache_regs.h | 19 ++++++----
>   arch/x86/kvm/svm/svm.c        | 12 +++++--
>   arch/x86/kvm/vmx/nested.c     | 14 ++++----
>   arch/x86/kvm/vmx/vmx.c        |  6 ++--
>   arch/x86/kvm/x86.c            | 19 ++++++----
>   arch/x86/kvm/x86.h            |  8 ++---
>   7 files changed, 48 insertions(+), 98 deletions(-)
> 

Queued, thanks.

Paolo

Re: [PATCH v2 7/9] KVM: x86/xen: Drop RAX[63:32] when processing hypercall

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> Truncate RAX to 32 bits, i.e. consume EAX, when retrieving the hypecall
> index for a Xen hypercall.  Per Xen documentation[*], the index is EAX
> when the vCPU is not in 64-bit mode.
>
> [*] http://xenbits.xenproject.org/docs/sphinx-unstable/guest-guide/x86/hypercall-abi.html
>
> Fixes: 23200b7a30de ("KVM: x86/xen: intercept xen hypercalls if enabled")
> Cc: Joao Martins <joao.m.martins@oracle.com>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/xen.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index ae17250e1efe..7f27bb65a572 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -673,7 +673,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>  	bool longmode;
>  	u64 input, params[6];
>  
> -	input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
> +	input = (u64)kvm_register_readl(vcpu, VCPU_REGS_RAX);
>  
>  	/* Hyper-V hypercalls get bit 31 set in EAX */
>  	if ((input & 0x80000000) &&

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Alternatively, as a minor optimization, you could've used '!longmode'
check below, something like:

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index ae17250e1efe..7df1498d3a41 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -682,6 +682,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 
        longmode = is_64_bit_mode(vcpu);
        if (!longmode) {
+               input = (u32)input;
                params[0] = (u32)kvm_rbx_read(vcpu);
                params[1] = (u32)kvm_rcx_read(vcpu);
                params[2] = (u32)kvm_rdx_read(vcpu);

-- 
Vitaly

Re: [PATCH v2 7/9] KVM: x86/xen: Drop RAX[63:32] when processing hypercall

[Paolo Bonzini]

On 22/04/21 11:51, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
>> Truncate RAX to 32 bits, i.e. consume EAX, when retrieving the hypecall
>> index for a Xen hypercall.  Per Xen documentation[*], the index is EAX
>> when the vCPU is not in 64-bit mode.
>>
>> [*] http://xenbits.xenproject.org/docs/sphinx-unstable/guest-guide/x86/hypercall-abi.html
>>
>> Fixes: 23200b7a30de ("KVM: x86/xen: intercept xen hypercalls if enabled")
>> Cc: Joao Martins <joao.m.martins@oracle.com>
>> Cc: David Woodhouse <dwmw@amazon.co.uk>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
>>   arch/x86/kvm/xen.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>> index ae17250e1efe..7f27bb65a572 100644
>> --- a/arch/x86/kvm/xen.c
>> +++ b/arch/x86/kvm/xen.c
>> @@ -673,7 +673,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>>   	bool longmode;
>>   	u64 input, params[6];
>>   
>> -	input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
>> +	input = (u64)kvm_register_readl(vcpu, VCPU_REGS_RAX);
>>   
>>   	/* Hyper-V hypercalls get bit 31 set in EAX */
>>   	if ((input & 0x80000000) &&
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> Alternatively, as a minor optimization, you could've used '!longmode'
> check below, something like:
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index ae17250e1efe..7df1498d3a41 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -682,6 +682,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>   
>          longmode = is_64_bit_mode(vcpu);
>          if (!longmode) {
> +               input = (u32)input;
>                  params[0] = (u32)kvm_rbx_read(vcpu);
>                  params[1] = (u32)kvm_rcx_read(vcpu);
>                  params[2] = (u32)kvm_rdx_read(vcpu);
> 

You haven't seen patch 9 yet. :)

Paolo

Re: [PATCH v2 7/9] KVM: x86/xen: Drop RAX[63:32] when processing hypercall

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 22/04/21 11:51, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>>> Truncate RAX to 32 bits, i.e. consume EAX, when retrieving the hypecall
>>> index for a Xen hypercall.  Per Xen documentation[*], the index is EAX
>>> when the vCPU is not in 64-bit mode.
>>>
>>> [*] http://xenbits.xenproject.org/docs/sphinx-unstable/guest-guide/x86/hypercall-abi.html
>>>
>>> Fixes: 23200b7a30de ("KVM: x86/xen: intercept xen hypercalls if enabled")
>>> Cc: Joao Martins <joao.m.martins@oracle.com>
>>> Cc: David Woodhouse <dwmw@amazon.co.uk>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>   arch/x86/kvm/xen.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>>> index ae17250e1efe..7f27bb65a572 100644
>>> --- a/arch/x86/kvm/xen.c
>>> +++ b/arch/x86/kvm/xen.c
>>> @@ -673,7 +673,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>>>   	bool longmode;
>>>   	u64 input, params[6];
>>>   
>>> -	input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
>>> +	input = (u64)kvm_register_readl(vcpu, VCPU_REGS_RAX);
>>>   
>>>   	/* Hyper-V hypercalls get bit 31 set in EAX */
>>>   	if ((input & 0x80000000) &&
>> 
>> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> 
>> Alternatively, as a minor optimization, you could've used '!longmode'
>> check below, something like:
>> 
>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>> index ae17250e1efe..7df1498d3a41 100644
>> --- a/arch/x86/kvm/xen.c
>> +++ b/arch/x86/kvm/xen.c
>> @@ -682,6 +682,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>>   
>>          longmode = is_64_bit_mode(vcpu);
>>          if (!longmode) {
>> +               input = (u32)input;
>>                  params[0] = (u32)kvm_rbx_read(vcpu);
>>                  params[1] = (u32)kvm_rcx_read(vcpu);
>>                  params[2] = (u32)kvm_rdx_read(vcpu);
>> 
>
> You haven't seen patch 9 yet. :)
>

True; suggestion dismissed :-)

-- 
Vitaly

Re: [PATCH v2 2/9] KVM: x86: Check CR3 GPA for validity regardless of vCPU mode

[Sean Christopherson]

On Thu, Apr 22, 2021, Paolo Bonzini wrote:
> On 22/04/21 04:21, Sean Christopherson wrote:
> > Check CR3 for an invalid GPA even if the vCPU isn't in long mode.  For
> > bigger emulation flows, notably RSM, the vCPU mode may not be accurate
> > if CR0/CR4 are loaded after CR3.  For MOV CR3 and similar flows, the
> > caller is responsible for truncating the value.
> > 
> > Note, SMRAM.CR3 is read-only, so this is mostly a theoretical bug since
> > KVM will not have stored an illegal CR3 into SMRAM during SMI emulation.
> 
> Well, the guest could have changed it...

That's what I tried to address with "SMRAM.CR3 is read-only".  Both Intel and
AMD state that modifying read-only fields will result in unpredictable behavior,
i.e. KVM going into the weeds would be within spec.  IIRC, there's no real
danger to the host, it'll "just" fail VM-Enter.

SDM:
  Some register images are read-only, and must not be modified (modifying these
  registers will result in unpredictable behavior)

APM:

  Software should not modify offsets specified as read-only or reserved,
  otherwise unpredictable results can occur.