[PATCH v4 0/4] Relax canonical checks on some arch msrs

[PATCH v4 0/4] Relax canonical checks on some arch msrs

[Maxim Levitsky]

Recently we came up upon a failure where likely the guest writes
0xff4547ceb1600000 to MSR_KERNEL_GS_BASE and later on, qemu
sets this value via KVM_PUT_MSRS, and is rejected by the
kernel, likely due to not being canonical in 4 level paging.

One of the way to trigger this is to make the guest enter SMM,
which causes paging to be disabled, which SMM bios re-enables
but not the whole 5 level. MSR_KERNEL_GS_BASE on the other
hand continues to contain old value.

I did some reverse engineering and to my surprise I found out
that both Intel and AMD indeed ignore CR4.LA57 when doing
canonical checks on this and other msrs and/or other arch
registers (like GDT base) which contain linear addresses.

V2: addressed a very good feedback from Chao Gao. Thanks!

V3: also fix the nested VMX, and also fix the
MSR_IA32_SYSENTER_EIP / MSR_IA32_SYSENTER_ESP

V4:
  - added PT and PEBS msrs
  - corrected emulation of SGDT/SIDT/STR/SLDT instructions
  - corrected canonical checks for TLB invalidation instructions

Best regards,
	Maxim Levitsky

Maxim Levitsky (4):
  KVM: x86: drop x86.h include from cpuid.h
  KVM: x86: implement emul_is_noncanonical_address using
    is_noncanonical_address
  KVM: x86: model canonical checks more precisely
  KVM: nVMX: fix canonical check of vmcs12 HOST_RIP

 arch/x86/kvm/cpuid.h         |  1 -
 arch/x86/kvm/emulate.c       | 15 ++++++-----
 arch/x86/kvm/kvm_emulate.h   |  5 ++++
 arch/x86/kvm/mmu.h           |  1 +
 arch/x86/kvm/mmu/mmu.c       |  2 +-
 arch/x86/kvm/vmx/hyperv.c    |  1 +
 arch/x86/kvm/vmx/nested.c    | 35 +++++++++++++++++---------
 arch/x86/kvm/vmx/pmu_intel.c |  2 +-
 arch/x86/kvm/vmx/sgx.c       |  5 ++--
 arch/x86/kvm/vmx/vmx.c       |  4 +--
 arch/x86/kvm/x86.c           | 13 +++++++---
 arch/x86/kvm/x86.h           | 49 ++++++++++++++++++++++++++++++++++--
 12 files changed, 102 insertions(+), 31 deletions(-)

-- 
2.26.3

[PATCH v4 1/4] KVM: x86: drop x86.h include from cpuid.h

[Maxim Levitsky]

Drop x86.h include from cpuid.h to allow the x86.h to include the cpuid.h
instead.

Also fix various places where x86.h was implicitly included via cpuid.h

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/cpuid.h      | 1 -
 arch/x86/kvm/mmu.h        | 1 +
 arch/x86/kvm/vmx/hyperv.c | 1 +
 arch/x86/kvm/vmx/nested.c | 2 +-
 arch/x86/kvm/vmx/sgx.c    | 3 +--
 5 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 41697cca354e6..c8dc66eddefda 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -2,7 +2,6 @@
 #ifndef ARCH_X86_KVM_CPUID_H
 #define ARCH_X86_KVM_CPUID_H
 
-#include "x86.h"
 #include "reverse_cpuid.h"
 #include <asm/cpu.h>
 #include <asm/processor.h>
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 4341e0e285712..9243a2863c8bb 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -4,6 +4,7 @@
 
 #include <linux/kvm_host.h>
 #include "kvm_cache_regs.h"
+#include "x86.h"
 #include "cpuid.h"
 
 extern bool __read_mostly enable_mmio_caching;
diff --git a/arch/x86/kvm/vmx/hyperv.c b/arch/x86/kvm/vmx/hyperv.c
index fab6a1ad98dc1..fa41d036acd49 100644
--- a/arch/x86/kvm/vmx/hyperv.c
+++ b/arch/x86/kvm/vmx/hyperv.c
@@ -4,6 +4,7 @@
 #include <linux/errno.h>
 #include <linux/smp.h>
 
+#include "x86.h"
 #include "../cpuid.h"
 #include "hyperv.h"
 #include "nested.h"
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 2392a7ef254df..3d64c14d4fd76 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -7,6 +7,7 @@
 #include <asm/debugreg.h>
 #include <asm/mmu_context.h>
 
+#include "x86.h"
 #include "cpuid.h"
 #include "hyperv.h"
 #include "mmu.h"
@@ -16,7 +17,6 @@
 #include "sgx.h"
 #include "trace.h"
 #include "vmx.h"
-#include "x86.h"
 #include "smm.h"
 
 static bool __read_mostly enable_shadow_vmcs = 1;
diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
index 6fef01e0536e5..016db12631d66 100644
--- a/arch/x86/kvm/vmx/sgx.c
+++ b/arch/x86/kvm/vmx/sgx.c
@@ -4,12 +4,11 @@
 
 #include <asm/sgx.h>
 
-#include "cpuid.h"
+#include "x86.h"
 #include "kvm_cache_regs.h"
 #include "nested.h"
 #include "sgx.h"
 #include "vmx.h"
-#include "x86.h"
 
 bool __read_mostly enable_sgx = 1;
 module_param_named(sgx, enable_sgx, bool, 0444);
-- 
2.26.3

Re: [PATCH v4 1/4] KVM: x86: drop x86.h include from cpuid.h

[Sean Christopherson]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> Drop x86.h include from cpuid.h to allow the x86.h to include the cpuid.h
> instead.
> 
> Also fix various places where x86.h was implicitly included via cpuid.h
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/cpuid.h      | 1 -
>  arch/x86/kvm/mmu.h        | 1 +
>  arch/x86/kvm/vmx/hyperv.c | 1 +
>  arch/x86/kvm/vmx/nested.c | 2 +-
>  arch/x86/kvm/vmx/sgx.c    | 3 +--
>  5 files changed, 4 insertions(+), 4 deletions(-)

This missed a necessary include in mtrr.c (maybe only for W=1 builds?)

arch/x86/kvm/mtrr.c:95:5: error: no previous prototype for ‘kvm_mtrr_set_msr’ [-Werror=missing-prototypes]
   95 | int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
      |     ^~~~~~~~~~~~~~~~
arch/x86/kvm/mtrr.c:110:5: error: no previous prototype for ‘kvm_mtrr_get_msr’ [-Werror=missing-prototypes]
  110 | int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
      |     ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

[PATCH v4 2/4] KVM: x86: implement emul_is_noncanonical_address using is_noncanonical_address

[Maxim Levitsky]

Implement the emul_is_noncanonical_address() using
is_noncanonical_address().

This will allow to extend the is_noncanonical_address() to support
different flavors of canonical checks.

Also add X86EMUL_F_MSR and X86EMUL_F_DT_LOAD emulation flags which will be
used to indicate an emulation of a msr or a segment base load, which
will affect the required canonical check.

No functional change is intended.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/emulate.c     | 15 +++++++++------
 arch/x86/kvm/kvm_emulate.h |  5 +++++
 arch/x86/kvm/x86.c         |  7 +++++++
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e72aed25d7212..8c8061884a019 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -651,9 +651,10 @@ static inline u8 ctxt_virt_addr_bits(struct x86_emulate_ctxt *ctxt)
 }
 
 static inline bool emul_is_noncanonical_address(u64 la,
-						struct x86_emulate_ctxt *ctxt)
+						struct x86_emulate_ctxt *ctxt,
+						unsigned int flags)
 {
-	return !__is_canonical_address(la, ctxt_virt_addr_bits(ctxt));
+	return !ctxt->ops->is_canonical_addr(ctxt, la, 0);
 }
 
 /*
@@ -1733,7 +1734,8 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		if (ret != X86EMUL_CONTINUE)
 			return ret;
 		if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
-						 ((u64)base3 << 32), ctxt))
+						 ((u64)base3 << 32), ctxt,
+						 X86EMUL_F_DT_LOAD))
 			return emulate_gp(ctxt, err_code);
 	}
 
@@ -2516,8 +2518,8 @@ static int em_sysexit(struct x86_emulate_ctxt *ctxt)
 		ss_sel = cs_sel + 8;
 		cs.d = 0;
 		cs.l = 1;
-		if (emul_is_noncanonical_address(rcx, ctxt) ||
-		    emul_is_noncanonical_address(rdx, ctxt))
+		if (emul_is_noncanonical_address(rcx, ctxt, 0) ||
+		    emul_is_noncanonical_address(rdx, ctxt, 0))
 			return emulate_gp(ctxt, 0);
 		break;
 	}
@@ -3494,7 +3496,8 @@ static int em_lgdt_lidt(struct x86_emulate_ctxt *ctxt, bool lgdt)
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 	if (ctxt->mode == X86EMUL_MODE_PROT64 &&
-	    emul_is_noncanonical_address(desc_ptr.address, ctxt))
+	    emul_is_noncanonical_address(desc_ptr.address, ctxt,
+					 X86EMUL_F_DT_LOAD))
 		return emulate_gp(ctxt, 0);
 	if (lgdt)
 		ctxt->ops->set_gdt(ctxt, &desc_ptr);
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 55a18e2f2dcd9..86bde1c9d9183 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -94,6 +94,8 @@ struct x86_instruction_info {
 #define X86EMUL_F_FETCH			BIT(1)
 #define X86EMUL_F_IMPLICIT		BIT(2)
 #define X86EMUL_F_INVLPG		BIT(3)
+#define X86EMUL_F_MSR			BIT(4)
+#define X86EMUL_F_DT_LOAD		BIT(5)
 
 struct x86_emulate_ops {
 	void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
@@ -235,6 +237,9 @@ struct x86_emulate_ops {
 
 	gva_t (*get_untagged_addr)(struct x86_emulate_ctxt *ctxt, gva_t addr,
 				   unsigned int flags);
+
+	bool (*is_canonical_addr)(struct x86_emulate_ctxt *ctxt,
+				  gva_t addr, unsigned int flags);
 };
 
 /* Type, address-of, and value of an instruction's operand. */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f72e5d89e942d..f496830445355 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8617,6 +8617,12 @@ static gva_t emulator_get_untagged_addr(struct x86_emulate_ctxt *ctxt,
 					       addr, flags);
 }
 
+static bool emulator_is_canonical_addr(struct x86_emulate_ctxt *ctxt,
+				       gva_t addr, unsigned int flags)
+{
+	return !is_noncanonical_address(addr, emul_to_vcpu(ctxt));
+}
+
 static const struct x86_emulate_ops emulate_ops = {
 	.vm_bugged           = emulator_vm_bugged,
 	.read_gpr            = emulator_read_gpr,
@@ -8663,6 +8669,7 @@ static const struct x86_emulate_ops emulate_ops = {
 	.triple_fault        = emulator_triple_fault,
 	.set_xcr             = emulator_set_xcr,
 	.get_untagged_addr   = emulator_get_untagged_addr,
+	.is_canonical_addr   = emulator_is_canonical_addr,
 };
 
 static void toggle_interruptibility(struct kvm_vcpu *vcpu, u32 mask)
-- 
2.26.3

Re: [PATCH v4 2/4] KVM: x86: implement emul_is_noncanonical_address using is_noncanonical_address

[Sean Christopherson]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> Implement the emul_is_noncanonical_address() using
> is_noncanonical_address().
> 
> This will allow to extend the is_noncanonical_address() to support
> different flavors of canonical checks.
> 
> Also add X86EMUL_F_MSR and X86EMUL_F_DT_LOAD emulation flags which will be
> used to indicate an emulation of a msr or a segment base load, which
> will affect the required canonical check.

Adding the flags belongs in a separate patch, there's no reason to squeeze them
into this patch.  Just an FYI, I'll split/fixup when applying.

[PATCH v4 3/4] KVM: x86: model canonical checks more precisely

[Maxim Levitsky]

As a result of a recent investigation, it was determined that x86 CPUs
which support 5-level paging, don't always respect CR4.LA57 when doing
canonical checks.

In particular:

1. MSRs which contain a linear address, allow full 57-bitcanonical address
regardless of CR4.LA57 state. For example: MSR_KERNEL_GS_BASE.

2. All hidden segment bases and GDT/IDT bases also behave like MSRs.
This means that full 57-bit canonical address can be loaded to them
regardless of CR4.LA57, both using MSRS (e.g GS_BASE) and instructions
(e.g LGDT).

3. TLB invalidation instructions also allow the user to use full 57-bit
address regardless of the CR4.LA57.

Finally, it must be noted that the CPU doesn't prevent the user from
disabling 5-level paging, even when the full 57-bit canonical address is
present in one of the registers mentioned above (e.g GDT base).

In fact, this can happen without any userspace help, when the CPU enters
SMM mode - some MSRs, for example MSR_KERNEL_GS_BASE are left to contain
a non-canonical address in regard to the new mode.

Since most of the affected MSRs and all segment bases can be read and
written freely by the guest without any KVM intervention, this patch makes
the emulator closely follow hardware behavior, which means that the
emulator doesn't take in the account the guest CPUID support for 5-level
paging, and only takes in the account the host CPU support.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/emulate.c       |  2 +-
 arch/x86/kvm/mmu/mmu.c       |  2 +-
 arch/x86/kvm/vmx/nested.c    | 22 ++++++++--------
 arch/x86/kvm/vmx/pmu_intel.c |  2 +-
 arch/x86/kvm/vmx/sgx.c       |  2 +-
 arch/x86/kvm/vmx/vmx.c       |  4 +--
 arch/x86/kvm/x86.c           |  8 +++---
 arch/x86/kvm/x86.h           | 49 ++++++++++++++++++++++++++++++++++--
 8 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8c8061884a019..60986f67c35a8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -654,7 +654,7 @@ static inline bool emul_is_noncanonical_address(u64 la,
 						struct x86_emulate_ctxt *ctxt,
 						unsigned int flags)
 {
-	return !ctxt->ops->is_canonical_addr(ctxt, la, 0);
+	return !ctxt->ops->is_canonical_addr(ctxt, la, flags);
 }
 
 /*
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index f107ec2557c1e..b9fe85ccdc095 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6113,7 +6113,7 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 	/* It's actually a GPA for vcpu->arch.guest_mmu.  */
 	if (mmu != &vcpu->arch.guest_mmu) {
 		/* INVLPG on a non-canonical address is a NOP according to the SDM.  */
-		if (is_noncanonical_address(addr, vcpu))
+		if (is_noncanonical_invlpg_address(addr, vcpu))
 			return;
 
 		kvm_x86_call(flush_tlb_gva)(vcpu, addr);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 3d64c14d4fd76..a7b0674094473 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2979,8 +2979,8 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    CC(!kvm_vcpu_is_legal_cr3(vcpu, vmcs12->host_cr3)))
 		return -EINVAL;
 
-	if (CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu)))
+	if (CC(is_noncanonical_msr_address(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
+	    CC(is_noncanonical_msr_address(vmcs12->host_ia32_sysenter_eip, vcpu)))
 		return -EINVAL;
 
 	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
@@ -3014,12 +3014,12 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    CC(vmcs12->host_ss_selector == 0 && !ia32e))
 		return -EINVAL;
 
-	if (CC(is_noncanonical_address(vmcs12->host_fs_base, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_gs_base, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_gdtr_base, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_idtr_base, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_tr_base, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))
+	if (CC(is_noncanonical_base_address(vmcs12->host_fs_base, vcpu)) ||
+	    CC(is_noncanonical_base_address(vmcs12->host_gs_base, vcpu)) ||
+	    CC(is_noncanonical_base_address(vmcs12->host_gdtr_base, vcpu)) ||
+	    CC(is_noncanonical_base_address(vmcs12->host_idtr_base, vcpu)) ||
+	    CC(is_noncanonical_base_address(vmcs12->host_tr_base, vcpu)) ||
+	    CC(is_noncanonical_address(vmcs12->host_rip, vcpu, 0)))
 		return -EINVAL;
 
 	/*
@@ -3137,7 +3137,7 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 	}
 
 	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
-	    (CC(is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu)) ||
+	    (CC(is_noncanonical_msr_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu)) ||
 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
 		return -EINVAL;
 
@@ -5093,7 +5093,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
 		 * non-canonical form. This is the only check on the memory
 		 * destination for long mode!
 		 */
-		exn = is_noncanonical_address(*ret, vcpu);
+		exn = is_noncanonical_address(*ret, vcpu, 0);
 	} else {
 		/*
 		 * When not in long mode, the virtual/linear address is
@@ -5898,7 +5898,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 		 * invalidation.
 		 */
 		if (!operand.vpid ||
-		    is_noncanonical_address(operand.gla, vcpu))
+		    is_noncanonical_invlpg_address(operand.gla, vcpu))
 			return nested_vmx_fail(vcpu,
 				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
 		vpid_sync_vcpu_addr(vpid02, operand.gla);
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 83382a4d1d66f..9c9d4a3361664 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -365,7 +365,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		}
 		break;
 	case MSR_IA32_DS_AREA:
-		if (is_noncanonical_address(data, vcpu))
+		if (is_noncanonical_msr_address(data, vcpu))
 			return 1;
 
 		pmu->ds_area = data;
diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
index 016db12631d66..8fd1ca9f8dbb5 100644
--- a/arch/x86/kvm/vmx/sgx.c
+++ b/arch/x86/kvm/vmx/sgx.c
@@ -37,7 +37,7 @@ static int sgx_get_encls_gva(struct kvm_vcpu *vcpu, unsigned long offset,
 		fault = true;
 	} else if (likely(is_64_bit_mode(vcpu))) {
 		*gva = vmx_get_untagged_addr(vcpu, *gva, 0);
-		fault = is_noncanonical_address(*gva, vcpu);
+		fault = is_noncanonical_address(*gva, vcpu, 0);
 	} else {
 		*gva &= 0xffffffff;
 		fault = (s.unusable) ||
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 89682832dded7..2628c899c94fc 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2287,7 +2287,7 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    (!msr_info->host_initiated &&
 		     !guest_cpuid_has(vcpu, X86_FEATURE_MPX)))
 			return 1;
-		if (is_noncanonical_address(data & PAGE_MASK, vcpu) ||
+		if (is_noncanonical_msr_address(data & PAGE_MASK, vcpu) ||
 		    (data & MSR_IA32_BNDCFGS_RSVD))
 			return 1;
 
@@ -2452,7 +2452,7 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
 		if (index >= 2 * vmx->pt_desc.num_address_ranges)
 			return 1;
-		if (is_noncanonical_address(data, vcpu))
+		if (is_noncanonical_msr_address(data, vcpu))
 			return 1;
 		if (index % 2)
 			vmx->pt_desc.guest.addr_b[index / 2] = data;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f496830445355..f378e4ff03505 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1829,7 +1829,7 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 	case MSR_KERNEL_GS_BASE:
 	case MSR_CSTAR:
 	case MSR_LSTAR:
-		if (is_noncanonical_address(data, vcpu))
+		if (is_noncanonical_msr_address(data, vcpu))
 			return 1;
 		break;
 	case MSR_IA32_SYSENTER_EIP:
@@ -1846,7 +1846,7 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 		 * value, and that something deterministic happens if the guest
 		 * invokes 64-bit SYSENTER.
 		 */
-		data = __canonical_address(data, vcpu_virt_addr_bits(vcpu));
+		data = __canonical_address(data, max_host_virt_addr_bits());
 		break;
 	case MSR_TSC_AUX:
 		if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX))
@@ -8620,7 +8620,7 @@ static gva_t emulator_get_untagged_addr(struct x86_emulate_ctxt *ctxt,
 static bool emulator_is_canonical_addr(struct x86_emulate_ctxt *ctxt,
 				       gva_t addr, unsigned int flags)
 {
-	return !is_noncanonical_address(addr, emul_to_vcpu(ctxt));
+	return !is_noncanonical_address(addr, emul_to_vcpu(ctxt), flags);
 }
 
 static const struct x86_emulate_ops emulate_ops = {
@@ -13781,7 +13781,7 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
 		 * invalidation.
 		 */
 		if ((!pcid_enabled && (operand.pcid != 0)) ||
-		    is_noncanonical_address(operand.gla, vcpu)) {
+		    is_noncanonical_invlpg_address(operand.gla, vcpu)) {
 			kvm_inject_gp(vcpu, 0);
 			return 1;
 		}
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 50596f6f83208..5cd67477214fa 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -8,6 +8,7 @@
 #include <asm/pvclock.h>
 #include "kvm_cache_regs.h"
 #include "kvm_emulate.h"
+#include "cpuid.h"
 
 struct kvm_caps {
 	/* control of guest tsc rate supported? */
@@ -226,9 +227,53 @@ static inline u8 vcpu_virt_addr_bits(struct kvm_vcpu *vcpu)
 	return kvm_is_cr4_bit_set(vcpu, X86_CR4_LA57) ? 57 : 48;
 }
 
-static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
+static inline u8 max_host_virt_addr_bits(void)
 {
-	return !__is_canonical_address(la, vcpu_virt_addr_bits(vcpu));
+	return kvm_cpu_cap_has(X86_FEATURE_LA57) ? 57 : 48;
+}
+
+/*
+ * X86 MSRs which contain linear addresses, x86 hidden segment bases, and
+ * IDT/GDT bases have static canonicality checks, size of which depends only on
+ * CPU's support for  5-level paging, rather than on the state of CR4.LA57.
+ * This applies to both WRMSR and to other instructions that set their values,
+ * e.g. SGDT.
+ *
+ * KVM passes through most of these MSRS and also doesn't intercept the
+ * instructions that set the hidden segment bases.
+ *
+ * Because of this, for consistency with ucode, even if the guest doesn't
+ * have LA57 enabled in its CPUID, it is better to base the check on the *host*
+ * support for 5 level paging.
+ *
+ * Finally, instructions which are related to MMU invalidation of a given
+ * linear address, also have a similar static canonical check on address,
+ * (this allows for example to invalidate 5-level addresses of a guest from a
+ * host which uses 4-level paging).
+ */
+
+static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu,
+		unsigned int flags)
+{
+	if (flags & (X86EMUL_F_INVLPG | X86EMUL_F_MSR | X86EMUL_F_DT_LOAD))
+		return !__is_canonical_address(la, max_host_virt_addr_bits());
+	else
+		return !__is_canonical_address(la, vcpu_virt_addr_bits(vcpu));
+}
+
+static inline bool is_noncanonical_msr_address(u64 la, struct kvm_vcpu *vcpu)
+{
+	return is_noncanonical_address(la, vcpu, X86EMUL_F_MSR);
+}
+
+static inline bool is_noncanonical_base_address(u64 la, struct kvm_vcpu *vcpu)
+{
+	return is_noncanonical_address(la, vcpu, X86EMUL_F_DT_LOAD);
+}
+
+static inline bool is_noncanonical_invlpg_address(u64 la, struct kvm_vcpu *vcpu)
+{
+	return is_noncanonical_address(la, vcpu, X86EMUL_F_INVLPG);
 }
 
 static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
-- 
2.26.3

Re: [PATCH v4 3/4] KVM: x86: model canonical checks more precisely

[Sean Christopherson]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> As a result of a recent investigation, it was determined that x86 CPUs
> which support 5-level paging, don't always respect CR4.LA57 when doing
> canonical checks.
> 
> In particular:
> 
> 1. MSRs which contain a linear address, allow full 57-bitcanonical address
> regardless of CR4.LA57 state. For example: MSR_KERNEL_GS_BASE.
> 
> 2. All hidden segment bases and GDT/IDT bases also behave like MSRs.
> This means that full 57-bit canonical address can be loaded to them
> regardless of CR4.LA57, both using MSRS (e.g GS_BASE) and instructions
> (e.g LGDT).
> 
> 3. TLB invalidation instructions also allow the user to use full 57-bit
> address regardless of the CR4.LA57.
> 
> Finally, it must be noted that the CPU doesn't prevent the user from
> disabling 5-level paging, even when the full 57-bit canonical address is
> present in one of the registers mentioned above (e.g GDT base).
> 
> In fact, this can happen without any userspace help, when the CPU enters
> SMM mode - some MSRs, for example MSR_KERNEL_GS_BASE are left to contain
> a non-canonical address in regard to the new mode.
> 
> Since most of the affected MSRs and all segment bases can be read and
> written freely by the guest without any KVM intervention, this patch makes
> the emulator closely follow hardware behavior, which means that the
> emulator doesn't take in the account the guest CPUID support for 5-level
> paging, and only takes in the account the host CPU support.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/emulate.c       |  2 +-
>  arch/x86/kvm/mmu/mmu.c       |  2 +-
>  arch/x86/kvm/vmx/nested.c    | 22 ++++++++--------
>  arch/x86/kvm/vmx/pmu_intel.c |  2 +-
>  arch/x86/kvm/vmx/sgx.c       |  2 +-
>  arch/x86/kvm/vmx/vmx.c       |  4 +--
>  arch/x86/kvm/x86.c           |  8 +++---
>  arch/x86/kvm/x86.h           | 49 ++++++++++++++++++++++++++++++++++--
>  8 files changed, 68 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 8c8061884a019..60986f67c35a8 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -654,7 +654,7 @@ static inline bool emul_is_noncanonical_address(u64 la,
>  						struct x86_emulate_ctxt *ctxt,
>  						unsigned int flags)
>  {
> -	return !ctxt->ops->is_canonical_addr(ctxt, la, 0);
> +	return !ctxt->ops->is_canonical_addr(ctxt, la, flags);

And conversely, passing flags to ->is_canonical_addr() belongs in the patch that
adds the plumbing.  Even though flags isn't truly consumed until this patch,
passing '0' in this helper is confusing and an unnecessary risk.

[PATCH v4 4/4] KVM: nVMX: fix canonical check of vmcs12 HOST_RIP

[Maxim Levitsky]

HOST_RIP canonical check should check the L1 of CR4.LA57 stored in
the vmcs12 rather than the current L1's because it is legal to change
the CR4.LA57 value during VM exit from L2 to L1.

This is a theoretical bug though, because it is highly unlikely that a
VM exit will change the CR4.LA57 from the value it had on VM entry.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index a7b0674094473..38c9d3077d17a 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2969,6 +2969,17 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static bool is_l1_noncanonical_address_on_vmexit(u64 la, struct vmcs12 *vmcs12)
+{
+	/*
+	 * Check that the given linear address is canonical after a VM exit
+	 * from L2, based on HOST_CR4.LA57 value that will be loaded then.
+	 */
+	u8 l1_address_bits_on_exit = (vmcs12->host_cr4 & X86_CR4_LA57) ? 57 : 48;
+
+	return !__is_canonical_address(la, l1_address_bits_on_exit);
+}
+
 static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 				       struct vmcs12 *vmcs12)
 {
@@ -3019,7 +3030,7 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    CC(is_noncanonical_base_address(vmcs12->host_gdtr_base, vcpu)) ||
 	    CC(is_noncanonical_base_address(vmcs12->host_idtr_base, vcpu)) ||
 	    CC(is_noncanonical_base_address(vmcs12->host_tr_base, vcpu)) ||
-	    CC(is_noncanonical_address(vmcs12->host_rip, vcpu, 0)))
+	    CC(is_l1_noncanonical_address_on_vmexit(vmcs12->host_rip, vmcs12)))
 		return -EINVAL;
 
 	/*
-- 
2.26.3

Re: [PATCH v4 0/4] Relax canonical checks on some arch msrs

[Maxim Levitsky]

On Fri, 2024-09-06 at 18:18 -0400, Maxim Levitsky wrote:
> Recently we came up upon a failure where likely the guest writes
> 0xff4547ceb1600000 to MSR_KERNEL_GS_BASE and later on, qemu
> sets this value via KVM_PUT_MSRS, and is rejected by the
> kernel, likely due to not being canonical in 4 level paging.
> 
> One of the way to trigger this is to make the guest enter SMM,
> which causes paging to be disabled, which SMM bios re-enables
> but not the whole 5 level. MSR_KERNEL_GS_BASE on the other
> hand continues to contain old value.
> 
> I did some reverse engineering and to my surprise I found out
> that both Intel and AMD indeed ignore CR4.LA57 when doing
> canonical checks on this and other msrs and/or other arch
> registers (like GDT base) which contain linear addresses.
> 
> V2: addressed a very good feedback from Chao Gao. Thanks!
> 
> V3: also fix the nested VMX, and also fix the
> MSR_IA32_SYSENTER_EIP / MSR_IA32_SYSENTER_ESP
> 
> V4:
>   - added PT and PEBS msrs
>   - corrected emulation of SGDT/SIDT/STR/SLDT instructions
>   - corrected canonical checks for TLB invalidation instructions
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (4):
>   KVM: x86: drop x86.h include from cpuid.h
>   KVM: x86: implement emul_is_noncanonical_address using
>     is_noncanonical_address
>   KVM: x86: model canonical checks more precisely
>   KVM: nVMX: fix canonical check of vmcs12 HOST_RIP
> 
>  arch/x86/kvm/cpuid.h         |  1 -
>  arch/x86/kvm/emulate.c       | 15 ++++++-----
>  arch/x86/kvm/kvm_emulate.h   |  5 ++++
>  arch/x86/kvm/mmu.h           |  1 +
>  arch/x86/kvm/mmu/mmu.c       |  2 +-
>  arch/x86/kvm/vmx/hyperv.c    |  1 +
>  arch/x86/kvm/vmx/nested.c    | 35 +++++++++++++++++---------
>  arch/x86/kvm/vmx/pmu_intel.c |  2 +-
>  arch/x86/kvm/vmx/sgx.c       |  5 ++--
>  arch/x86/kvm/vmx/vmx.c       |  4 +--
>  arch/x86/kvm/x86.c           | 13 +++++++---
>  arch/x86/kvm/x86.h           | 49 ++++++++++++++++++++++++++++++++++--
>  12 files changed, 102 insertions(+), 31 deletions(-)
> 
> -- 
> 2.26.3
> 
> 

Hi,
A very gentle ping on this patch series.

Best regards,
	Maxim Levitsky

Re: [PATCH v4 0/4] Relax canonical checks on some arch msrs

[Sean Christopherson]

On Wed, Oct 30, 2024, Maxim Levitsky wrote:
> On Fri, 2024-09-06 at 18:18 -0400, Maxim Levitsky wrote:
> > Recently we came up upon a failure where likely the guest writes
> > 0xff4547ceb1600000 to MSR_KERNEL_GS_BASE and later on, qemu
> > sets this value via KVM_PUT_MSRS, and is rejected by the
> > kernel, likely due to not being canonical in 4 level paging.
> > 
> > One of the way to trigger this is to make the guest enter SMM,
> > which causes paging to be disabled, which SMM bios re-enables
> > but not the whole 5 level. MSR_KERNEL_GS_BASE on the other
> > hand continues to contain old value.
> > 
> > I did some reverse engineering and to my surprise I found out
> > that both Intel and AMD indeed ignore CR4.LA57 when doing
> > canonical checks on this and other msrs and/or other arch
> > registers (like GDT base) which contain linear addresses.
> > 
> > V2: addressed a very good feedback from Chao Gao. Thanks!
> > 
> > V3: also fix the nested VMX, and also fix the
> > MSR_IA32_SYSENTER_EIP / MSR_IA32_SYSENTER_ESP
> > 
> > V4:
> >   - added PT and PEBS msrs
> >   - corrected emulation of SGDT/SIDT/STR/SLDT instructions
> >   - corrected canonical checks for TLB invalidation instructions
> > 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > Maxim Levitsky (4):
> >   KVM: x86: drop x86.h include from cpuid.h
> >   KVM: x86: implement emul_is_noncanonical_address using
> >     is_noncanonical_address
> >   KVM: x86: model canonical checks more precisely
> >   KVM: nVMX: fix canonical check of vmcs12 HOST_RIP
> > 
> >  arch/x86/kvm/cpuid.h         |  1 -
> >  arch/x86/kvm/emulate.c       | 15 ++++++-----
> >  arch/x86/kvm/kvm_emulate.h   |  5 ++++
> >  arch/x86/kvm/mmu.h           |  1 +
> >  arch/x86/kvm/mmu/mmu.c       |  2 +-
> >  arch/x86/kvm/vmx/hyperv.c    |  1 +
> >  arch/x86/kvm/vmx/nested.c    | 35 +++++++++++++++++---------
> >  arch/x86/kvm/vmx/pmu_intel.c |  2 +-
> >  arch/x86/kvm/vmx/sgx.c       |  5 ++--
> >  arch/x86/kvm/vmx/vmx.c       |  4 +--
> >  arch/x86/kvm/x86.c           | 13 +++++++---
> >  arch/x86/kvm/x86.h           | 49 ++++++++++++++++++++++++++++++++++--
> >  12 files changed, 102 insertions(+), 31 deletions(-)
> > 
> > -- 
> > 2.26.3
> > 
> > 
> 
> Hi,
> A very gentle ping on this patch series.

Heh, good timing, I literally (like, 2 seconds ago) applied this (still need to
test before you'll see a "thank you" email).

Re: [PATCH v4 0/4] Relax canonical checks on some arch msrs

[Maxim Levitsky]

On Wed, 2024-10-30 at 14:22 -0700, Sean Christopherson wrote:
> On Wed, Oct 30, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-09-06 at 18:18 -0400, Maxim Levitsky wrote:
> > > Recently we came up upon a failure where likely the guest writes
> > > 0xff4547ceb1600000 to MSR_KERNEL_GS_BASE and later on, qemu
> > > sets this value via KVM_PUT_MSRS, and is rejected by the
> > > kernel, likely due to not being canonical in 4 level paging.
> > > 
> > > One of the way to trigger this is to make the guest enter SMM,
> > > which causes paging to be disabled, which SMM bios re-enables
> > > but not the whole 5 level. MSR_KERNEL_GS_BASE on the other
> > > hand continues to contain old value.
> > > 
> > > I did some reverse engineering and to my surprise I found out
> > > that both Intel and AMD indeed ignore CR4.LA57 when doing
> > > canonical checks on this and other msrs and/or other arch
> > > registers (like GDT base) which contain linear addresses.
> > > 
> > > V2: addressed a very good feedback from Chao Gao. Thanks!
> > > 
> > > V3: also fix the nested VMX, and also fix the
> > > MSR_IA32_SYSENTER_EIP / MSR_IA32_SYSENTER_ESP
> > > 
> > > V4:
> > >   - added PT and PEBS msrs
> > >   - corrected emulation of SGDT/SIDT/STR/SLDT instructions
> > >   - corrected canonical checks for TLB invalidation instructions
> > > 
> > > Best regards,
> > > 	Maxim Levitsky
> > > 
> > > Maxim Levitsky (4):
> > >   KVM: x86: drop x86.h include from cpuid.h
> > >   KVM: x86: implement emul_is_noncanonical_address using
> > >     is_noncanonical_address
> > >   KVM: x86: model canonical checks more precisely
> > >   KVM: nVMX: fix canonical check of vmcs12 HOST_RIP
> > > 
> > >  arch/x86/kvm/cpuid.h         |  1 -
> > >  arch/x86/kvm/emulate.c       | 15 ++++++-----
> > >  arch/x86/kvm/kvm_emulate.h   |  5 ++++
> > >  arch/x86/kvm/mmu.h           |  1 +
> > >  arch/x86/kvm/mmu/mmu.c       |  2 +-
> > >  arch/x86/kvm/vmx/hyperv.c    |  1 +
> > >  arch/x86/kvm/vmx/nested.c    | 35 +++++++++++++++++---------
> > >  arch/x86/kvm/vmx/pmu_intel.c |  2 +-
> > >  arch/x86/kvm/vmx/sgx.c       |  5 ++--
> > >  arch/x86/kvm/vmx/vmx.c       |  4 +--
> > >  arch/x86/kvm/x86.c           | 13 +++++++---
> > >  arch/x86/kvm/x86.h           | 49 ++++++++++++++++++++++++++++++++++--
> > >  12 files changed, 102 insertions(+), 31 deletions(-)
> > > 
> > > -- 
> > > 2.26.3
> > > 
> > > 
> > 
> > Hi,
> > A very gentle ping on this patch series.
> 
> Heh, good timing, I literally (like, 2 seconds ago) applied this (still need to
> test before you'll see a "thank you" email).
> 

Thank you!

Best regards,
	Maxim Levitsky

Re: [PATCH v4 0/4] Relax canonical checks on some arch msrs

[Sean Christopherson]

On Fri, 06 Sep 2024 18:18:20 -0400, Maxim Levitsky wrote:
> Recently we came up upon a failure where likely the guest writes
> 0xff4547ceb1600000 to MSR_KERNEL_GS_BASE and later on, qemu
> sets this value via KVM_PUT_MSRS, and is rejected by the
> kernel, likely due to not being canonical in 4 level paging.
> 
> One of the way to trigger this is to make the guest enter SMM,
> which causes paging to be disabled, which SMM bios re-enables
> but not the whole 5 level. MSR_KERNEL_GS_BASE on the other
> hand continues to contain old value.
> 
> [...]

Applied to kvm-x86 misc, with some massaging (see responsed to individual
patches).  Thanks!

[1/4] KVM: x86: drop x86.h include from cpuid.h
      https://github.com/kvm-x86/linux/commit/391bd0c520c1
[2/4] KVM: x86: implement emul_is_noncanonical_address using is_noncanonical_address
      https://github.com/kvm-x86/linux/commit/6c45d62536d0
[3/4] KVM: x86: model canonical checks more precisely
      https://github.com/kvm-x86/linux/commit/1b1336d1d858
[4/4] KVM: nVMX: fix canonical check of vmcs12 HOST_RIP
      https://github.com/kvm-x86/linux/commit/14a95598b6e7

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH v4 0/4] Relax canonical checks on some arch msrs

[Sean Christopherson]

On Thu, Oct 31, 2024, Sean Christopherson wrote:
> On Fri, 06 Sep 2024 18:18:20 -0400, Maxim Levitsky wrote:
> > Recently we came up upon a failure where likely the guest writes
> > 0xff4547ceb1600000 to MSR_KERNEL_GS_BASE and later on, qemu
> > sets this value via KVM_PUT_MSRS, and is rejected by the
> > kernel, likely due to not being canonical in 4 level paging.
> > 
> > One of the way to trigger this is to make the guest enter SMM,
> > which causes paging to be disabled, which SMM bios re-enables
> > but not the whole 5 level. MSR_KERNEL_GS_BASE on the other
> > hand continues to contain old value.
> > 
> > [...]
> 
> Applied to kvm-x86 misc, with some massaging (see responsed to individual
> patches).  Thanks!
> 
> [1/4] KVM: x86: drop x86.h include from cpuid.h
>       https://github.com/kvm-x86/linux/commit/391bd0c520c1
> [2/4] KVM: x86: implement emul_is_noncanonical_address using is_noncanonical_address
>       https://github.com/kvm-x86/linux/commit/6c45d62536d0
> [3/4] KVM: x86: model canonical checks more precisely
>       https://github.com/kvm-x86/linux/commit/1b1336d1d858
> [4/4] KVM: nVMX: fix canonical check of vmcs12 HOST_RIP
>       https://github.com/kvm-x86/linux/commit/14a95598b6e7

FYI, I rebased misc to v6.12-rc5, as patches in another series had already been
taken through the tip tree.  New hashes:

[1/5] KVM: x86: drop x86.h include from cpuid.h
      https://github.com/kvm-x86/linux/commit/e52ad1ddd0a3
[2/5] KVM: x86: Route non-canonical checks in emulator through emulate_ops
      https://github.com/kvm-x86/linux/commit/16ccadefa295
[3/5] KVM: x86: Add X86EMUL_F_MSR and X86EMUL_F_DT_LOAD to aid canonical checks
      https://github.com/kvm-x86/linux/commit/c534b37b7584
[4/5] KVM: x86: model canonical checks more precisely
      https://github.com/kvm-x86/linux/commit/9245fd6b8531
[5/5] KVM: nVMX: fix canonical check of vmcs12 HOST_RIP
      https://github.com/kvm-x86/linux/commit/90a877216e6b