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

[PATCH v3 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

Best regards,
	Maxim Levitsky

Maxim Levitsky (4):
  KVM: x86: relax canonical check for some x86 architectural msrs
  KVM: x86: add X86_FEATURE_LA57 to governed_features
  KVM: nVMX: relax canonical checks on some x86 registers in vmx host
    state
  KVM: SVM: fix emulation of msr reads/writes of MSR_FS_BASE and
    MSR_GS_BASE

 arch/x86/kvm/cpuid.c             |  2 ++
 arch/x86/kvm/governed_features.h |  1 +
 arch/x86/kvm/svm/svm.c           | 12 ++++++++++++
 arch/x86/kvm/vmx/nested.c        | 30 +++++++++++++++++++++++-------
 arch/x86/kvm/x86.c               | 29 +++++++++++++++++++++++++++--
 5 files changed, 65 insertions(+), 9 deletions(-)

-- 
2.40.1

[PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Maxim Levitsky]

Several x86's arch msrs contain a linear base, and thus must
contain a canonical address.

This includes FS/GS base, addresses used for SYSENTER and SYSCALL
instructions and probably more.

As it turns out, when x86 architecture was updated to 5 level paging /
57 bit virtual addresses, these MSRs were allowed to contain a full
57 bit address regardless of the state of CR4.LA57.

The main reason behind this decision is that 5 level paging, and even
paging itself can be temporarily disabled (e.g by SMM entry) leaving non
canonical values in these fields.
Another reason is that OS might prepare these fields before it switches to
5 level paging.

Experemental tests on a Sapphire Rapids CPU and on a Zen4 CPU confirm this
behavior.

In addition to that, the Intel ISA extension manual mentions that this
might be the architectural behavior:

Architecture Instruction Set Extensions and Future Features Programming
Reference [1], Chapter 6.4:

"CANONICALITY CHECKING FOR DATA ADDRESSES WRITTEN TO CONTROL REGISTERS AND
MSRS"

"In Processors that support LAM continue to require the addresses written
tocontrol registers or MSRs to be 57-bit canonical if the processor
supports 5-level paging or 48-bit canonical if it supports only 4-level
paging"

[1]: https://cdrdv2.intel.com/v1/dl/getContent/671368

Suggested-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/x86.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ce7c00894f32..2e83f7d74591 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
 		       sizeof(kvm_vcpu_stats_desc),
 };
 
+
+/*
+ * Most x86 arch MSR values which contain linear addresses like
+ * segment bases, addresses that are used in instructions (e.g SYSENTER),
+ * have static canonicality checks,
+ * size of whose depends only on CPU's support for 5-level
+ * paging, rather than state of CR4.LA57.
+ *
+ * In addition to that, some of these MSRS are directly passed
+ * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
+ * doen't have LA57 enabled in its CPUID, for consistency with
+ * CPUs' ucode, it is better to pivot the check around host
+ * support for 5 level paging.
+ */
+
+static u8  max_host_supported_virt_addr_bits(void)
+{
+	return kvm_cpu_cap_has(X86_FEATURE_LA57) ? 57 : 48;
+}
+
+static bool is_host_noncanonical_msr_value(u64 la)
+{
+	return !__is_canonical_address(la, max_host_supported_virt_addr_bits());
+}
+
 static struct kmem_cache *x86_emulator_cache;
 
 /*
@@ -1829,7 +1854,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_host_noncanonical_msr_value(data))
 			return 1;
 		break;
 	case MSR_IA32_SYSENTER_EIP:
@@ -1846,7 +1871,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_supported_virt_addr_bits());
 		break;
 	case MSR_TSC_AUX:
 		if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX))
-- 
2.40.1

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson]

On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ce7c00894f32..2e83f7d74591 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
>  		       sizeof(kvm_vcpu_stats_desc),
>  };
>  
> +
> +/*
> + * Most x86 arch MSR values which contain linear addresses like

Is it most, or all?  I'm guessing all?

> + * segment bases, addresses that are used in instructions (e.g SYSENTER),
> + * have static canonicality checks,

Weird and early line breaks.

How about this?

/*
 * The canonicality checks for MSRs that hold linear addresses, e.g. segment
 * bases, SYSENTER targets, etc., are static, in the sense that they are based
 * on CPU _support_ for 5-level paging, not the state of CR4.LA57.

> + * size of whose depends only on CPU's support for 5-level
> + * paging, rather than state of CR4.LA57.
> + *
> + * In addition to that, some of these MSRS are directly passed
> + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> + * doen't have LA57 enabled in its CPUID, for consistency with
> + * CPUs' ucode, it is better to pivot the check around host
> + * support for 5 level paging.

I think we should elaborate on why it's better.  It only takes another line or
two, and that way we don't forget the edge cases that make properly emulating
guest CPUID a bad idea.

 * This creates a virtualization hole where a guest writes to passthrough MSRs
 * may incorrectly succeed if the CPU supports LA57, but the vCPU does not
 * (because hardware has no awareness of guest CPUID).  Do not try to plug this
 * hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
 * depending on whether or not KVM happens to emulate a WRMSR would result in
 * non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
 * passes through an MSR to L2, and then tries to save+restore L2's value.
 */

> +
> +static u8  max_host_supported_virt_addr_bits(void)

Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
Mostly to shorten the name, but also because "supported" suggests there's software
involvement, e.g. the max supported by the kernel/KVM, which isn't the case.

If you're ok with the above, I'll fixup when applying.

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson]

On Fri, Aug 16, 2024, Sean Christopherson wrote:
> On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> How about this?
> 
> /*
>  * The canonicality checks for MSRs that hold linear addresses, e.g. segment
>  * bases, SYSENTER targets, etc., are static, in the sense that they are based
>  * on CPU _support_ for 5-level paging, not the state of CR4.LA57.
> 
> > + * size of whose depends only on CPU's support for 5-level
> > + * paging, rather than state of CR4.LA57.
> > + *
> > + * In addition to that, some of these MSRS are directly passed
> > + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> > + * doen't have LA57 enabled in its CPUID, for consistency with
> > + * CPUs' ucode, it is better to pivot the check around host
> > + * support for 5 level paging.
> 
> I think we should elaborate on why it's better.  It only takes another line or
> two, and that way we don't forget the edge cases that make properly emulating
> guest CPUID a bad idea.
> 
>  * This creates a virtualization hole where a guest writes to passthrough MSRs
>  * may incorrectly succeed if the CPU supports LA57, but the vCPU does not
>  * (because hardware has no awareness of guest CPUID).  Do not try to plug this
>  * hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
>  * depending on whether or not KVM happens to emulate a WRMSR would result in
>  * non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
>  * passes through an MSR to L2, and then tries to save+restore L2's value.
>  */
> 
> > +
> > +static u8  max_host_supported_virt_addr_bits(void)
> 
> Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
> Mostly to shorten the name, but also because "supported" suggests there's software
> involvement, e.g. the max supported by the kernel/KVM, which isn't the case.
> 
> If you're ok with the above, I'll fixup when applying.

I take that back, I think we're going to need a v4 (see patch 3).

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[mlevitsk]

У пт, 2024-08-16 у 14:49 -0700, Sean Christopherson пише:
> > > > On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > > > > index ce7c00894f32..2e83f7d74591 100644
> > > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > > @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > > > > > > >                        sizeof(kvm_vcpu_stats_desc),
> > > > > > > >  };
> > > > > > > >  
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Most x86 arch MSR values which contain linear addresses like
> > > > 
> > > > Is it most, or all?  I'm guessing all?

I can't be sure that all of them are like that - there could be some outliers that behave differently.

One of the things my work at Intel taught me is that there is nothing consistent
in x86 spec, anything is possible and nothing can be assumed.

I dealt only with those msrs, that KVM checks for canonicality, therefore I use the word 
'most'. There could be other msrs that are not known to me and/or to KVM.

I can write 'some' if you prefer.

> > > > 
> > > > > > > > + * segment bases, addresses that are used in instructions (e.g SYSENTER),
> > > > > > > > + * have static canonicality checks,
> > > > 
> > > > Weird and early line breaks.
> > > > 
> > > > How about this?
> > > > 
> > > > /*
> > > >  * The canonicality checks for MSRs that hold linear addresses, e.g. segment
> > > >  * bases, SYSENTER targets, etc., are static, in the sense that they are based
> > > >  * on CPU _support_ for 5-level paging, not the state of CR4.LA57.
> > > > 
> > > > > > > > + * size of whose depends only on CPU's support for 5-level
> > > > > > > > + * paging, rather than state of CR4.LA57.
> > > > > > > > + *
> > > > > > > > + * In addition to that, some of these MSRS are directly passed
> > > > > > > > + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> > > > > > > > + * doen't have LA57 enabled in its CPUID, for consistency with
> > > > > > > > + * CPUs' ucode, it is better to pivot the check around host
> > > > > > > > + * support for 5 level paging.
> > > > 
> > > > I think we should elaborate on why it's better.  It only takes another line or
> > > > two, and that way we don't forget the edge cases that make properly emulating
> > > > guest CPUID a bad idea.

OK, will do.

> > > > 
> > > >  * This creates a virtualization hole where a guest writes to passthrough MSRs
> > > >  * may incorrectly succeed if the CPU supports LA57, but the vCPU does not
> > > >  * (because hardware has no awareness of guest CPUID).  Do not try to plug this
> > > >  * hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
> > > >  * depending on whether or not KVM happens to emulate a WRMSR would result in
> > > >  * non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
> > > >  * passes through an MSR to L2, and then tries to save+restore L2's value.
> > > >  */
> > > > 
> > > > > > > > +
> > > > > > > > +static u8  max_host_supported_virt_addr_bits(void)
> > > > 
> > > > Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
> > > > Mostly to shorten the name, but also because "supported" suggests there's software
> > > > involvement, e.g. the max supported by the kernel/KVM, which isn't the case.

Doesn't matter to me.

> > > > 
> > > > If you're ok with the above, I'll fixup when applying.
> > > > 

Best regards,
   Maxim Levitsky

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[mlevitsk]

У вт, 2024-08-20 у 15:13 +0300, mlevitsk@redhat.com пише:
> У пт, 2024-08-16 у 14:49 -0700, Sean Christopherson пише:
> > > > > On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > > > > > index ce7c00894f32..2e83f7d74591 100644
> > > > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > > > @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > > > > > > > >                        sizeof(kvm_vcpu_stats_desc),
> > > > > > > > >  };
> > > > > > > > >  
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * Most x86 arch MSR values which contain linear addresses like
> > > > > 
> > > > > Is it most, or all?  I'm guessing all?
> 
> I can't be sure that all of them are like that - there could be some outliers that behave differently.
> 
> One of the things my work at Intel taught me is that there is nothing consistent
> in x86 spec, anything is possible and nothing can be assumed.
> 
> I dealt only with those msrs, that KVM checks for canonicality, therefore I use the word 
> 'most'. There could be other msrs that are not known to me and/or to KVM.
> 
> I can write 'some' if you prefer.

Hi,


So I did some more reverse engineering and indeed, 'some' is the right word:

I audited all places in KVM which check an linear address for being canonical and this is what I found:

- MSR_IA32_BNDCFGS - since it is not supported on CPUs with 5 level paging, its not possible to know
  what the hardware does.


- MSR_IA32_DS_AREA: - Ignores CR4.LA57 as expected. Tested by booting into kernel with 5 level paging
  disabled and then using userspace 'wrmsr' program to set this msr.
  I attached the bash script that I used

- MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B: - Exactly the same story, but for some reason the
  host doesn't suport (not even read) from MSR_IA32_RTIT_ADDR2_*, MSR_IA32_RTIT_ADDR3_*.
  Probably the system is not new enough for these.


- invpcid instruction. It is exposed to the guest without interception (unless !npt or !ept),
  and yes, it works just fine on 57-canonical address without CR4.LA57 set....


- invvpid - this one belongs to VMX set, so technically its for nesting although it is run by L1,
  it is always emulated by KVM, but still executed on the host just with different vpid,
  so I booted the host without 5 level paging, and patched KVM to avoid canonical check.

  Also 57-canonical adddress worked just fine, and fully non canonical address failed.
  and gave a warning in 'invvpid_error'


Should I fix all of these too?


About fixing the emulator this is what see:

	emul_is_noncanonical_address
		__load_segment_descriptor
			load_segment_descriptor
				em_lldt
				em_ltr

		em_lgdt_lidt



While em_lgdt_lidt should be easy to fix because it calls emul_is_noncanonical_address
directly, the em_lldt, em_ltr will be harder because these use load_segment_descriptor
which calls __load_segment_descriptor which in turn is also used for emulating of far jumps/calls/rets,
for which I do believe that canonical check does respect CR4.LA57, but can't be sure either.

It is possible that far jumps/calls/rets also ignore CR4.LA57, and instead set RIP to
non canonical instruction, and then on first fetch, #GP happens.

I'll setup another unit test for this. RIP of the #GP will determine if the instruction
failed or the next fetch.

Best regards,
	Maxim Levitsky


> 
> > > > > 
> > > > > > > > > + * segment bases, addresses that are used in instructions (e.g SYSENTER),
> > > > > > > > > + * have static canonicality checks,
> > > > > 
> > > > > Weird and early line breaks.
> > > > > 
> > > > > How about this?
> > > > > 
> > > > > /*
> > > > >  * The canonicality checks for MSRs that hold linear addresses, e.g. segment
> > > > >  * bases, SYSENTER targets, etc., are static, in the sense that they are based
> > > > >  * on CPU _support_ for 5-level paging, not the state of CR4.LA57.
> > > > > 
> > > > > > > > > + * size of whose depends only on CPU's support for 5-level
> > > > > > > > > + * paging, rather than state of CR4.LA57.
> > > > > > > > > + *
> > > > > > > > > + * In addition to that, some of these MSRS are directly passed
> > > > > > > > > + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> > > > > > > > > + * doen't have LA57 enabled in its CPUID, for consistency with
> > > > > > > > > + * CPUs' ucode, it is better to pivot the check around host
> > > > > > > > > + * support for 5 level paging.
> > > > > 
> > > > > I think we should elaborate on why it's better.  It only takes another line or
> > > > > two, and that way we don't forget the edge cases that make properly emulating
> > > > > guest CPUID a bad idea.
> 
> OK, will do.
> 
> > > > > 
> > > > >  * This creates a virtualization hole where a guest writes to passthrough MSRs
> > > > >  * may incorrectly succeed if the CPU supports LA57, but the vCPU does not
> > > > >  * (because hardware has no awareness of guest CPUID).  Do not try to plug this
> > > > >  * hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
> > > > >  * depending on whether or not KVM happens to emulate a WRMSR would result in
> > > > >  * non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
> > > > >  * passes through an MSR to L2, and then tries to save+restore L2's value.
> > > > >  */
> > > > > 
> > > > > > > > > +
> > > > > > > > > +static u8  max_host_supported_virt_addr_bits(void)
> > > > > 
> > > > > Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
> > > > > Mostly to shorten the name, but also because "supported" suggests there's software
> > > > > involvement, e.g. the max supported by the kernel/KVM, which isn't the case.
> 
> Doesn't matter to me.
> 
> > > > > 
> > > > > If you're ok with the above, I'll fixup when applying.
> > > > > 
> 
> Best regards,
>    Maxim Levitsky

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson]

On Wed, Aug 21, 2024, mlevitsk@redhat.com wrote:
> У вт, 2024-08-20 у 15:13 +0300, mlevitsk@redhat.com пише:
> > У пт, 2024-08-16 у 14:49 -0700, Sean Christopherson пише:
> > > > > > On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> > > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > > > > > > index ce7c00894f32..2e83f7d74591 100644
> > > > > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > > > > @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > > > > > > > > >                        sizeof(kvm_vcpu_stats_desc),
> > > > > > > > > >  };
> > > > > > > > > >  
> > > > > > > > > > +
> > > > > > > > > > +/*
> > > > > > > > > > + * Most x86 arch MSR values which contain linear addresses like
> > > > > > 
> > > > > > Is it most, or all?  I'm guessing all?
> > 
> > I can't be sure that all of them are like that - there could be some
> > outliers that behave differently.
> > 
> > One of the things my work at Intel taught me is that there is nothing
> > consistent in x86 spec, anything is possible and nothing can be assumed.
> > 
> > I dealt only with those msrs, that KVM checks for canonicality, therefore I
> > use the word  'most'. There could be other msrs that are not known to me
> > and/or to KVM.
> > 
> > I can write 'some' if you prefer.
> 
> Hi,
> 
> 
> So I did some more reverse engineering and indeed, 'some' is the right word:

Is it?  IIUC, we have yet to find an MSR that honors, CR4.LA57, i.e. it really
is "all", so far as we know.

> I audited all places in KVM which check an linear address for being canonical
> and this is what I found:
> 
> - MSR_IA32_BNDCFGS - since it is not supported on CPUs with 5 level paging,
>   its not possible to know what the hardware does.

Heh, yeah, but I would be very surprised if MSR_IA32_BNDCFGS didn't follow all
other system-ish MSRs.

> - MSR_IA32_DS_AREA: - Ignores CR4.LA57 as expected. Tested by booting into kernel
>   with 5 level paging disabled and then using userspace 'wrmsr' program to
>   set this msr.  I attached the bash script that I used
> 
> - MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B: - Exactly the same story,
>   but for some reason the host doesn't suport (not even read) from
>   MSR_IA32_RTIT_ADDR2_*, MSR_IA32_RTIT_ADDR3_*.  Probably the system is not new
>   enough for these.
>
> - invpcid instruction. It is exposed to the guest without interception
>   (unless !npt or !ept), and yes, it works just fine on 57-canonical address
>   without CR4.LA57 set....

Did you verify the behavior for the desciptor, the target, or both?  I assume
the memory operand, i.e. the address of the _descriptor_, honors CR4.LA57, but
the target within the descriptor does not.

If that assumption is correct, then this code in vm_mmu_invalidate_addr() is broken,
as KVM actually needs to do a TLB flush if the address is canonical for the vCPU
model, even if it's non-canonical for the current vCPU state.

	/* 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))
			return;

		kvm_x86_call(flush_tlb_gva)(vcpu, addr);
	}

Assuming INVPCID is indicative of how INVLPG and INVVPID behave (they are nops if
the _target_ is non-canonical), then the code is broken for INVLPG and INVVPID.

And I think it's probably a safe assumption that a TLB flush is needed.  E.g. the
primary (possible only?) use case for INVVPID with a linear address is to flush
TLB entries for a specific GVA=>HPA mapping, and honoring CR4.LA57 would prevent
shadowing 5-level paging with a hypervisor that is using 4-level paging for itself.

> - invvpid - this one belongs to VMX set, so technically its for nesting
>   although it is run by L1, it is always emulated by KVM, but still executed on
>   the host just with different vpid, so I booted the host without 5 level
>   paging, and patched KVM to avoid canonical check.
> 
>   Also 57-canonical adddress worked just fine, and fully non canonical
>   address failed.  and gave a warning in 'invvpid_error'
>
> Should I fix all of these too?

Yeah, though I believe we're at the point where we need to figure out a better
naming scheme, because usage of what is currently is_noncanonical_address() will
be, by far, in the minority.

Hmm, actually, what if we extend the X86EMUL_F_* flags that were added for LAM
(and eventually for LASS) to handle the non-canonical checks?  That's essentially
what LAM does already, there are just a few more flavors we now need to handle.

E.g. I think we just need flags for MSRs and segment/DT bases.  The only (or at
least, most) confusing thing is that LAM/LASS do NOT apply to INVPLG accesses,
but are exempt from LA57.  But that's an arch oddity, not a problem KVM can solve.

diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 55a18e2f2dcd..6da03a37bdd5 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_BASE                 BIT(5)
 
 struct x86_emulate_ops {
        void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
---

And then with that, we can do the below, and have emul_is_noncanonical_address()
redirect to is_noncanonical_address() instead of being an open coded equivalent.

---
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 u8 max_host_virt_addr_bits(void)
{
	return kvm_cpu_cap_has(X86_FEATURE_LA57) ? 57 : 48;
}

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

That will make it _much_ harder to incorrectly use is_noncanonical_address(),
as all callers will be forced to specify the emulation type, i.e. there is no
automatic, implied default type.

Line lengths could get annoying, but with per-type flags, we could do selectively
add a few wrappers, e.g.

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

static inline bool is_noncanonical_invlpg_address(u64 la, struct kvm_vcpu *vcpu)
{
	return is_noncanonical_address(la, vcpu, X86EMUL_F_INVLPG);
}
---

We wouldn't want wrapper for everything, e.g. to minimize the risk of creating a
de factor implicit default, but I think those three, and maybe a code/fetch
variant, will cover all but a few users.

> About fixing the emulator this is what see:
> 
> 	emul_is_noncanonical_address
> 		__load_segment_descriptor
> 			load_segment_descriptor
> 				em_lldt
> 				em_ltr
> 
> 		em_lgdt_lidt
> 
> 
> 
> While em_lgdt_lidt should be easy to fix because it calls
> emul_is_noncanonical_address directly,

Those don't need to be fixed, they are validating the memory operand, not the
base of the descriptor, i.e. aren't exempt from CR4.LA57.

> the em_lldt, em_ltr will be harder
> because these use load_segment_descriptor which calls
> __load_segment_descriptor which in turn is also used for emulating of far
> jumps/calls/rets, for which I do believe that canonical check does respect
> CR4.LA57, but can't be sure either.

I'm fairly certain this is a non-issue.  CS, DS, ES, and SS have a fixed base of
'0' in 64-bit mode, i.e. are completely exempt from canonical checks because the
base address is always ignored.  And while FS and GS do have base addresses, the
segment descriptors themselves can only load 32-bit bases, i.e. _can't_ generate
non-canonical addresses.

There _is_ an indirect canonical check on the _descriptor_ (the actual descriptor
pointed at by the selector, not the memory operand).  The SDM is calls this case
out in the LFS/LDS docs:

  If the FS, or GS register is being loaded with a non-NULL segment selector and
  any of the following is true: the segment selector index is not within descriptor
  table limits, the memory address of the descriptor is non-canonical
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
and similarly, when using a CALL GATE, the far transfer docs say:

  If the segment descriptor from a 64-bit call gate is in non-canonical space.

And given that those implicit accesses are not subjected to LAM/LASS, I strongly
suspect they honor CR4.LA57.  So the explicit emul_is_noncanonical_address()
check in __load_segment_descriptor() needs to be tagged X86EMUL_F_BASE, but
otherwise it all should Just Work (knock wood).

> It is possible that far jumps/calls/rets also ignore CR4.LA57, and instead
> set RIP to non canonical instruction, and then on first fetch, #GP happens.

I doubt this is the case for the final RIP check, especially since ucode does
check vmcs.HOST_RIP against vmcs.HOST_CR4.LA57, but it's worth testing to confirm.

> I'll setup another unit test for this. RIP of the #GP will determine if the
> instruction failed or the next fetch.

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[mlevitsk]

У ср, 2024-08-21 у 09:04 -0700, Sean Christopherson пише:
> > On Wed, Aug 21, 2024, mlevitsk@redhat.com wrote:
> > > > У вт, 2024-08-20 у 15:13 +0300, mlevitsk@redhat.com пише:
> > > > > > У пт, 2024-08-16 у 14:49 -0700, Sean Christopherson пише:
> > > > > > > > > > > > > > On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> > > > > > > > > > > > > > > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > > > > > > > > > > > > > > > > > > index ce7c00894f32..2e83f7d74591 100644
> > > > > > > > > > > > > > > > > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > > > > > > > > > > > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > > > > > > > > > > > > > > > > @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > > > > > > > > > > > > > > > > > > > > >                        sizeof(kvm_vcpu_stats_desc),
> > > > > > > > > > > > > > > > > > > > > >  };
> > > > > > > > > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > > > > > > +/*
> > > > > > > > > > > > > > > > > > > > > > + * Most x86 arch MSR values which contain linear addresses like
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Is it most, or all?  I'm guessing all?
> > > > > > 
> > > > > > I can't be sure that all of them are like that - there could be some
> > > > > > outliers that behave differently.
> > > > > > 
> > > > > > One of the things my work at Intel taught me is that there is nothing
> > > > > > consistent in x86 spec, anything is possible and nothing can be assumed.
> > > > > > 
> > > > > > I dealt only with those msrs, that KVM checks for canonicality, therefore I
> > > > > > use the word  'most'. There could be other msrs that are not known to me
> > > > > > and/or to KVM.
> > > > > > 
> > > > > > I can write 'some' if you prefer.
> > > > 
> > > > Hi,
> > > > 
> > > > 
> > > > So I did some more reverse engineering and indeed, 'some' is the right word:
> > 
> > Is it?  IIUC, we have yet to find an MSR that honors, CR4.LA57, i.e. it really
> > is "all", so far as we know.

This is more or less what I meant to say: I meant to say that I verified only 'some'
of the msrs/instructions, and for others I can't know, although it's likely that
indeed our theory is correct.

> > 
> > > > I audited all places in KVM which check an linear address for being canonical
> > > > and this is what I found:
> > > > 
> > > > - MSR_IA32_BNDCFGS - since it is not supported on CPUs with 5 level paging,
> > > >   its not possible to know what the hardware does.
> > 
> > Heh, yeah, but I would be very surprised if MSR_IA32_BNDCFGS didn't follow all
> > other system-ish MSRs.
> > 
> > > > - MSR_IA32_DS_AREA: - Ignores CR4.LA57 as expected. Tested by booting into kernel
> > > >   with 5 level paging disabled and then using userspace 'wrmsr' program to
> > > >   set this msr.  I attached the bash script that I used
> > > > 
> > > > - MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B: - Exactly the same story,
> > > >   but for some reason the host doesn't suport (not even read) from
> > > >   MSR_IA32_RTIT_ADDR2_*, MSR_IA32_RTIT_ADDR3_*.  Probably the system is not new
> > > >   enough for these.
> > > > 
> > > > - invpcid instruction. It is exposed to the guest without interception
> > > >   (unless !npt or !ept), and yes, it works just fine on 57-canonical address
> > > >   without CR4.LA57 set....
> > 
> > Did you verify the behavior for the desciptor, the target, or both?  I assume
> > the memory operand, i.e. the address of the _descriptor_, honors CR4.LA57, but
> > the target within the descriptor does not.

I verified only the target address. I assume that memory fetches do have to honor the CR.LA57.

> > 
> > If that assumption is correct, then this code in vm_mmu_invalidate_addr() is broken,
> > as KVM actually needs to do a TLB flush if the address is canonical for the vCPU
> > model, even if it's non-canonical for the current vCPU state.
> > 
> >         /* 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))
> >                         return;
> > 
> >                 kvm_x86_call(flush_tlb_gva)(vcpu, addr);
> >         }
> > 
> > Assuming INVPCID is indicative of how INVLPG and INVVPID behave (they are nops if
> > the _target_ is non-canonical), then the code is broken for INVLPG and INVVPID.
> > 
> > And I think it's probably a safe assumption that a TLB flush is needed.  E.g. the
> > primary (possible only?) use case for INVVPID with a linear address is to flush
> > TLB entries for a specific GVA=>HPA mapping, and honoring CR4.LA57 would prevent
> > shadowing 5-level paging with a hypervisor that is using 4-level paging for itself.

I also think so.

> > 
> > > > - invvpid - this one belongs to VMX set, so technically its for nesting
> > > >   although it is run by L1, it is always emulated by KVM, but still executed on
> > > >   the host just with different vpid, so I booted the host without 5 level
> > > >   paging, and patched KVM to avoid canonical check.
> > > > 
> > > >   Also 57-canonical adddress worked just fine, and fully non canonical
> > > >   address failed.  and gave a warning in 'invvpid_error'
> > > > 
> > > > Should I fix all of these too?
> > 
> > Yeah, though I believe we're at the point where we need to figure out a better
> > naming scheme, because usage of what is currently is_noncanonical_address() will
> > be, by far, in the minority.

Yes, I will take this into account.

> > 
> > Hmm, actually, what if we extend the X86EMUL_F_* flags that were added for LAM
> > (and eventually for LASS) to handle the non-canonical checks?  That's essentially
> > what LAM does already, there are just a few more flavors we now need to handle.
> > 
> > E.g. I think we just need flags for MSRs and segment/DT bases.  The only (or at
> > least, most) confusing thing is that LAM/LASS do NOT apply to INVPLG accesses,
> > but are exempt from LA57.  But that's an arch oddity, not a problem KVM can solve.
> > 
> > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> > index 55a18e2f2dcd..6da03a37bdd5 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_BASE                 BIT(5)
> >  
> >  struct x86_emulate_ops {
> >         void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
> > ---
> > 
> > And then with that, we can do the below, and have emul_is_noncanonical_address()
> > redirect to is_noncanonical_address() instead of being an open coded equivalent.
> > 
> > ---
> > 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 u8 max_host_virt_addr_bits(void)
> > {
> >         return kvm_cpu_cap_has(X86_FEATURE_LA57) ? 57 : 48;
> > }

This is a good name for this function, thanks.

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

This can work in principle, although are you OK with using these emulator flags
outside of the emulator code?

I am asking because the is_noncanonical_address is used in various places across KVM.

> > ---
> > 
> > That will make it _much_ harder to incorrectly use is_noncanonical_address(),
> > as all callers will be forced to specify the emulation type, i.e. there is no
> > automatic, implied default type.
> > 
> > Line lengths could get annoying, but with per-type flags, we could do selectively
> > add a few wrappers, e.g.
> > 
> > ---
> > 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_BASE);
> > }
> > 
> > static inline bool is_noncanonical_invlpg_address(u64 la, struct kvm_vcpu *vcpu)
> > {
> >         return is_noncanonical_address(la, vcpu, X86EMUL_F_INVLPG);
> > }
> > ---
> > 
> > We wouldn't want wrapper for everything, e.g. to minimize the risk of creating a
> > de factor implicit default, but I think those three, and maybe a code/fetch
> > variant, will cover all but a few users.
> > 
> > > > About fixing the emulator this is what see:
> > > > 
> > > >         emul_is_noncanonical_address
> > > >                 __load_segment_descriptor
> > > >                         load_segment_descriptor
> > > >                                 em_lldt
> > > >                                 em_ltr
> > > > 
> > > >                 em_lgdt_lidt
> > > > 
> > > > 
> > > > 
> > > > While em_lgdt_lidt should be easy to fix because it calls
> > > > emul_is_noncanonical_address directly,
> > 
> > Those don't need to be fixed, they are validating the memory operand, not the
> > base of the descriptor, i.e. aren't exempt from CR4.LA57.

Are you sure?

em_lgdt_lidt reads its memory operands (and checks that it is canonical through
linearize) with read_descriptor and that is fine because this is memory fetch, 
and then it checks that the base address within the operand is canonical.

This check needs to be updated, as it is possible to load non canonical GDT and IDT
base via lgdt/lidt (I tested this).

For em_lldt, em_ltr, the check on the system segment descriptor base is
in __load_segment_descriptor:

...
 ret = linear_read_system(ctxt, desc_addr+8, &base3, sizeof(base3));
 if (ret != X86EMUL_CONTINUE)
 return ret;
 if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
 ((u64)base3 << 32), ctxt))
 return emulate_gp(ctxt, err_code);

...


64 bases are possible only for system segments, which are
TSS, LDT, and call gates/IDT descriptors.


We don't emulate IDT fetches in protected mode, and as I found out the hard way after
I wrote a unit test to do a call through a call gate, the emulator doesn't
support call gates either)

Thus I can safely patch __load_segment_descriptor.


> > 
> > > > the em_lldt, em_ltr will be harder
> > > > because these use load_segment_descriptor which calls
> > > > __load_segment_descriptor which in turn is also used for emulating of far
> > > > jumps/calls/rets, for which I do believe that canonical check does respect
> > > > CR4.LA57, but can't be sure either.
> > 
> > I'm fairly certain this is a non-issue.  CS, DS, ES, and SS have a fixed base of
> > '0' in 64-bit mode, i.e. are completely exempt from canonical checks because the
> > base address is always ignored.  And while FS and GS do have base addresses, the
> > segment descriptors themselves can only load 32-bit bases, i.e. _can't_ generate
> > non-canonical addresses.

Yes - all data segments in 64 bit mode, still have 32 bit base in the GDT/LDT,
so this is indeed not an issue.

> > 

> > There _is_ an indirect canonical check on the _descriptor_ (the actual descriptor
> > pointed at by the selector, not the memory operand).  The SDM is calls this case
> > out in the LFS/LDS docs:
> > 
> >   If the FS, or GS register is being loaded with a non-NULL segment selector and
> >   any of the following is true: the segment selector index is not within descriptor
> >   table limits, the memory address of the descriptor is non-canonical
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I tested that both ltr and lldt do ignore CR4.LA57 by loading from a descriptor
which had a non canonical value and CR4.LA57 clear.

> > and similarly, when using a CALL GATE, the far transfer docs say:
> > 
> >   If the segment descriptor from a 64-bit call gate is in non-canonical space.

For the academic reference, when loading offset from a call gate descriptor,
the CPU does the canonical check, and it does honour the CR4.LA57.


A variation of the below test can prove it:

void set_callgate_entry(u16 sel, void *offset, int dpl)
{
	idt_entry_t *e = (idt_entry_t *)&gdt[sel >> 3];
	set_desc_entry(e, sizeof *e, offset, read_cs(), 12, dpl);
}

int main(int argc, char **argv)
{
	u64 gate_offset = (u64)&&gate_entry_point;

	// toggle below to check various cases:
	//setup_5level_page_table();
	//gate_offset = 0xff4547ceb1600000;

	set_callgate_entry(FIRST_SPARE_SEL, (void *)gate_offset, 0);

	struct { u64 offset; u16 sel; } call_target =
		{ 0 /* ignored */,
		  FIRST_SPARE_SEL};

	// Perform the far call
	asm volatile goto (
//	    KVM_FEP
	    ".byte 0x48; rex.w\n" // optional, just for fun
	    "lcall *%0\n"
	    "jmp %l1\n"
	:
	: "m" (call_target)
	:
	: return_address
	);

	gate_entry_point:
	printf("Call gate worked\n");

	asm volatile (
	     "retfq\n"
	);

	return_address:

	printf("Exit from call gate worked\n");
	return 0;
}



> > 
> > And given that those implicit accesses are not subjected to LAM/LASS, I strongly
> > suspect they honor CR4.LA57.  So the explicit emul_is_noncanonical_address()
> > check in __load_segment_descriptor() needs to be tagged X86EMUL_F_BASE, but
> > otherwise it all should Just Work (knock wood).
> > 
> > > > It is possible that far jumps/calls/rets also ignore CR4.LA57, and instead
> > > > set RIP to non canonical instruction, and then on first fetch, #GP happens.
> > 
> > I doubt this is the case for the final RIP check, especially since ucode does
> > check vmcs.HOST_RIP against vmcs.HOST_CR4.LA57, but it's worth testing to confirm.

See above.

So now I'll try to do a v4 of the patches with all of the feedback included.
Thanks!

> > 
> > > > I'll setup another unit test for this. RIP of the #GP will determine if the
> > > > instruction failed or the next fetch.
> > 

Best regards,
	Maxim Levitsky

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson]

On Fri, Aug 23, 2024, mlevitsk@redhat.com wrote:
> У ср, 2024-08-21 у 09:04 -0700, Sean Christopherson пише:
> > > 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));
> > > }
> 
> This can work in principle, although are you OK with using these emulator flags
> outside of the emulator code?

Yep, they're already used in VMX's vmx_get_untagged_addr().  

> I am asking because the is_noncanonical_address is used in various places across KVM.

...

> > > We wouldn't want wrapper for everything, e.g. to minimize the risk of creating a
> > > de factor implicit default, but I think those three, and maybe a code/fetch
> > > variant, will cover all but a few users.
> > > 
> > > > > About fixing the emulator this is what see:
> > > > > 
> > > > >         emul_is_noncanonical_address
> > > > >                 __load_segment_descriptor
> > > > >                         load_segment_descriptor
> > > > >                                 em_lldt
> > > > >                                 em_ltr
> > > > > 
> > > > >                 em_lgdt_lidt
> > > > > 
> > > > > 
> > > > > 
> > > > > While em_lgdt_lidt should be easy to fix because it calls
> > > > > emul_is_noncanonical_address directly,
> > > 
> > > Those don't need to be fixed, they are validating the memory operand, not the
> > > base of the descriptor, i.e. aren't exempt from CR4.LA57.
> 
> Are you sure?

Nope.  Re-reading what I wrote, I have no idea what code past me was looking at.

I even contradicted myself later on:

  So the explicit emul_is_noncanonical_address() check in __load_segment_descriptor()
  needs to be tagged X86EMUL_F_BASE, but otherwise it all should Just Work (knock wood).

so yeah, ignore this.

> em_lgdt_lidt reads its memory operands (and checks that it is canonical through
> linearize) with read_descriptor and that is fine because this is memory fetch, 
> and then it checks that the base address within the operand is canonical.
> 
> This check needs to be updated, as it is possible to load non canonical GDT and IDT
> base via lgdt/lidt (I tested this).
> 
> For em_lldt, em_ltr, the check on the system segment descriptor base is
> in __load_segment_descriptor:
> 
> ...
>  ret = linear_read_system(ctxt, desc_addr+8, &base3, sizeof(base3));
>  if (ret != X86EMUL_CONTINUE)
>  return ret;
>  if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
>  ((u64)base3 << 32), ctxt))
>  return emulate_gp(ctxt, err_code);
> 
> ...
> 
> 
> 64 bases are possible only for system segments, which are
> TSS, LDT, and call gates/IDT descriptors.
> 
> 
> We don't emulate IDT fetches in protected mode, and as I found out the hard way after
> I wrote a unit test to do a call through a call gate, the emulator doesn't
> support call gates either)
> 
> Thus I can safely patch __load_segment_descriptor.

Agreed.

And thinking more about how this is likely implemented in ucode, this is probably
working as intended.  The the SDM gives CPUs a _lot_ of leeway:

  In 64-bit mode, an address is considered to be in canonical form if address
  bits 63 through to the most-significant implemented bit by the microarchitecture
  are set to either all ones or all zeros.

as does the APM:

  Long mode defines 64 bits of virtual address, but implementations of the AMD64
  architecture may support fewer bits of virtual address. Although implementations
  might not use all 64 bits of the virtual address, they check bits 63 through the
  most-significant implemented bit to see if those bits are all zeros or all ones.
  An address that complies with this property is said to be in canonical address
  form. If a virtual-memory reference is not in canonical form, the implementation
  causes a general-protection exception or stack fault.

I suspect that CR4.LA_57 is only consulted when the CPU is actually consuming the
address, i.e. is (or is about to, e.g. for code fetches) generating a memory
access.

Heh, and for MPX, the SDM kinda sorta confirms that LA57 is ignored, though I
doubt the author of this section intended their words to be taken this way :-)

  WRMSR to BNDCFGS will #GP if any of the reserved bits of BNDCFGS is not zero or
  if the base address of the bound directory is not canonical. XRSTOR of BNDCFGU
  ignores the reserved bits and does not fault if any is non-zero; similarly, it
  ignores the upper bits of the base address of the bound directory and sign-extends
  the highest implemented bit of the linear address to guarantee the canonicality
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  of this address.

> > > There _is_ an indirect canonical check on the _descriptor_ (the actual descriptor
> > > pointed at by the selector, not the memory operand).  The SDM is calls this case
> > > out in the LFS/LDS docs:
> > > 
> > >   If the FS, or GS register is being loaded with a non-NULL segment selector and
> > >   any of the following is true: the segment selector index is not within descriptor
> > >   table limits, the memory address of the descriptor is non-canonical
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I tested that both ltr and lldt do ignore CR4.LA57 by loading from a descriptor
> which had a non canonical value and CR4.LA57 clear.

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Jim Mattson]

On Fri, Aug 23, 2024 at 6:59 AM Sean Christopherson <seanjc@google.com> wrote:
> Heh, and for MPX, the SDM kinda sorta confirms that LA57 is ignored, though I
> doubt the author of this section intended their words to be taken this way :-)
>
>   WRMSR to BNDCFGS will #GP if any of the reserved bits of BNDCFGS is not zero or
>   if the base address of the bound directory is not canonical. XRSTOR of BNDCFGU
>   ignores the reserved bits and does not fault if any is non-zero; similarly, it
>   ignores the upper bits of the base address of the bound directory and sign-extends
>   the highest implemented bit of the linear address to guarantee the canonicality
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   of this address.

I don't believe there was ever a CPU that supported both MPX and LA57. :)

Late to the party, as usual, but my interest was piqued by the failure
of KVM_SET_NESTED_STATE prior to v6.13 if L1 had CR4.LA57 set, L2 did
not, and the VMCS12.HOST_GSBASE had a kernel address > 48 bits wide.
The canonicalization checks for the *host* state in the VMCS were done
using the guest's CR4.LA57.

Shouldn't this series have been cc'd to stable?

Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson]

On Fri, Sep 12, 2025, Jim Mattson wrote:
> On Fri, Aug 23, 2024 at 6:59 AM Sean Christopherson <seanjc@google.com> wrote:
> > Heh, and for MPX, the SDM kinda sorta confirms that LA57 is ignored, though I
> > doubt the author of this section intended their words to be taken this way :-)
> >
> >   WRMSR to BNDCFGS will #GP if any of the reserved bits of BNDCFGS is not zero or
> >   if the base address of the bound directory is not canonical. XRSTOR of BNDCFGU
> >   ignores the reserved bits and does not fault if any is non-zero; similarly, it
> >   ignores the upper bits of the base address of the bound directory and sign-extends
> >   the highest implemented bit of the linear address to guarantee the canonicality
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   of this address.
> 
> I don't believe there was ever a CPU that supported both MPX and LA57. :)
> 
> Late to the party, as usual, but my interest was piqued by the failure
> of KVM_SET_NESTED_STATE prior to v6.13 if L1 had CR4.LA57 set, L2 did
> not, and the VMCS12.HOST_GSBASE had a kernel address > 48 bits wide.
> The canonicalization checks for the *host* state in the VMCS were done
> using the guest's CR4.LA57.
> 
> Shouldn't this series have been cc'd to stable?

Yes :-(

That's my fault.  I balked at the size/scope of the changes, but in hindsight
that was a mistake.

[PATCH v3 2/4] KVM: x86: add X86_FEATURE_LA57 to governed_features

[Maxim Levitsky]

Guest support for X86_FEATURE_LA57 will need to be tested to determine
canonical width check for nested VMX VM entries.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/cpuid.c             | 2 ++
 arch/x86/kvm/governed_features.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 2617be544480..1a2d3fbe9a59 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -373,6 +373,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	if (allow_gbpages)
 		kvm_governed_feature_set(vcpu, X86_FEATURE_GBPAGES);
 
+	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LA57);
+
 	best = kvm_find_cpuid_entry(vcpu, 1);
 	if (best && apic) {
 		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
index ad463b1ed4e4..475171f2fd49 100644
--- a/arch/x86/kvm/governed_features.h
+++ b/arch/x86/kvm/governed_features.h
@@ -17,6 +17,7 @@ KVM_GOVERNED_X86_FEATURE(PFTHRESHOLD)
 KVM_GOVERNED_X86_FEATURE(VGIF)
 KVM_GOVERNED_X86_FEATURE(VNMI)
 KVM_GOVERNED_X86_FEATURE(LAM)
+KVM_GOVERNED_X86_FEATURE(LA57)
 
 #undef KVM_GOVERNED_X86_FEATURE
 #undef KVM_GOVERNED_FEATURE
-- 
2.40.1

[PATCH v3 3/4] KVM: nVMX: relax canonical checks on some x86 registers in vmx host state

[Maxim Levitsky]

Several x86's architecture registers contain a linear base, and thus must
contain a canonical address.

This includes segment and segment like bases (FS/GS base, GDT,IDT,LDT,TR),
addresses used for SYSENTER and SYSCALL instructions and probably more.

As it turns out, when x86 architecture was updated to 5 level paging /
57 bit virtual addresses, these fields were allowed to contain a full
57 bit address regardless of the state of CR4.LA57.

The main reason behind this decision is that 5 level paging, and even
paging itself can be temporarily disabled (e.g by SMM entry) leaving non
canonical values in these fields.
Another reason is that OS might prepare these fields before it switches to
5 level paging.

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

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 2392a7ef254d..3f18edff80ac 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2969,6 +2969,22 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static bool is_l1_noncanonical_address_static(u64 la, struct kvm_vcpu *vcpu)
+{
+	u8 max_guest_address_bits = guest_can_use(vcpu, X86_FEATURE_LA57) ? 57 : 48;
+	/*
+	 * Most x86 arch registers which contain linear addresses like
+	 * segment bases, addresses that are used in instructions (e.g SYSENTER),
+	 * have static canonicality checks,
+	 * size of whose depends only on CPU's support for 5-level
+	 * paging, rather than state of CR4.LA57.
+	 *
+	 * In other words the check only depends on the CPU model,
+	 * rather than on runtime state.
+	 */
+	return !__is_canonical_address(la, max_guest_address_bits);
+}
+
 static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 				       struct vmcs12 *vmcs12)
 {
@@ -2979,8 +2995,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_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
+	    CC(is_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_eip, vcpu)))
 		return -EINVAL;
 
 	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
@@ -3014,11 +3030,11 @@ 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)) ||
+	if (CC(is_l1_noncanonical_address_static(vmcs12->host_fs_base, vcpu)) ||
+	    CC(is_l1_noncanonical_address_static(vmcs12->host_gs_base, vcpu)) ||
+	    CC(is_l1_noncanonical_address_static(vmcs12->host_gdtr_base, vcpu)) ||
+	    CC(is_l1_noncanonical_address_static(vmcs12->host_idtr_base, vcpu)) ||
+	    CC(is_l1_noncanonical_address_static(vmcs12->host_tr_base, vcpu)) ||
 	    CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))
 		return -EINVAL;
 
-- 
2.40.1

Re: [PATCH v3 3/4] KVM: nVMX: relax canonical checks on some x86 registers in vmx host state

[mlevitsk]

У чт, 2024-08-15 у 15:33 +0300, Maxim Levitsky пише:
> Several x86's architecture registers contain a linear base, and thus must
> contain a canonical address.
> 
> This includes segment and segment like bases (FS/GS base, GDT,IDT,LDT,TR),
> addresses used for SYSENTER and SYSCALL instructions and probably more.
> 
> As it turns out, when x86 architecture was updated to 5 level paging /
> 57 bit virtual addresses, these fields were allowed to contain a full
> 57 bit address regardless of the state of CR4.LA57.
> 
> The main reason behind this decision is that 5 level paging, and even
> paging itself can be temporarily disabled (e.g by SMM entry) leaving non
> canonical values in these fields.
> Another reason is that OS might prepare these fields before it switches to
> 5 level paging.

Hi,

Note that I haven't included a fix for HOST_RIP. I did today a bare metal check
and indeed the microcode does check CR4.LA57, the one that is stored in the vmcs
as you suspected.

I add a patch to this patch series with this mostly theoretical fix, when I send a new revision.

Second thing, I kept the canonical check on 'vmcs12->guest_bndcfgs because Intel
deprecated this feature and none of CPUs which support 5 level paging support MPX.

Also I think that since this is a guest state field, it might be possible to just
remove the check, because the value of this field is copied to vmcs02 and the
CPU's microcode should do the same check that KVM does.

Best regards,
	Maxim Levitsky


> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 2392a7ef254d..3f18edff80ac 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2969,6 +2969,22 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
>         return 0;
>  }
>  
> +static bool is_l1_noncanonical_address_static(u64 la, struct kvm_vcpu *vcpu)
> +{
> +       u8 max_guest_address_bits = guest_can_use(vcpu, X86_FEATURE_LA57) ? 57 : 48;
> +       /*
> +        * Most x86 arch registers which contain linear addresses like
> +        * segment bases, addresses that are used in instructions (e.g SYSENTER),
> +        * have static canonicality checks,
> +        * size of whose depends only on CPU's support for 5-level
> +        * paging, rather than state of CR4.LA57.
> +        *
> +        * In other words the check only depends on the CPU model,
> +        * rather than on runtime state.
> +        */
> +       return !__is_canonical_address(la, max_guest_address_bits);
> +}
> +
>  static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
>                                        struct vmcs12 *vmcs12)
>  {
> @@ -2979,8 +2995,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_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
> +           CC(is_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_eip, vcpu)))
>                 return -EINVAL;
>  
>         if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
> @@ -3014,11 +3030,11 @@ 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)) ||
> +       if (CC(is_l1_noncanonical_address_static(vmcs12->host_fs_base, vcpu)) ||
> +           CC(is_l1_noncanonical_address_static(vmcs12->host_gs_base, vcpu)) ||
> +           CC(is_l1_noncanonical_address_static(vmcs12->host_gdtr_base, vcpu)) ||
> +           CC(is_l1_noncanonical_address_static(vmcs12->host_idtr_base, vcpu)) ||
> +           CC(is_l1_noncanonical_address_static(vmcs12->host_tr_base, vcpu)) ||
>             CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))
>                 return -EINVAL;
>  

Re: [PATCH v3 3/4] KVM: nVMX: relax canonical checks on some x86 registers in vmx host state

[Sean Christopherson]

On Fri, Aug 16, 2024, mlevitsk@redhat.com wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 2392a7ef254d..3f18edff80ac 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2969,6 +2969,22 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
> >         return 0;
> >  }
> >  
> > +static bool is_l1_noncanonical_address_static(u64 la, struct kvm_vcpu *vcpu)
> > +{
> > +       u8 max_guest_address_bits = guest_can_use(vcpu, X86_FEATURE_LA57) ? 57 : 48;

I don't see any reason to use LA57 support from guest CPUID for the VMCS checks.
The virtualization hole exists can't be safely plugged for all cases, so why
bother trying to plug it only for some cases?

It'd be very odd that an L1 could set a "bad" value via WRMSR, but then couldn't
load that same value on VM-Exit, e.g. if L1 gets the VMCS value by doing RDMSR.

> > +       /*
> > +        * Most x86 arch registers which contain linear addresses like
> > +        * segment bases, addresses that are used in instructions (e.g SYSENTER),
> > +        * have static canonicality checks,
> > +        * size of whose depends only on CPU's support for 5-level
> > +        * paging, rather than state of CR4.LA57.
> > +        *
> > +        * In other words the check only depends on the CPU model,
> > +        * rather than on runtime state.
> > +        */
> > +       return !__is_canonical_address(la, max_guest_address_bits);
> > +}
> > +
> >  static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> >                                        struct vmcs12 *vmcs12)
> >  {
> > @@ -2979,8 +2995,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_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_eip, vcpu)))
> >                 return -EINVAL;
> >  
> >         if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
> > @@ -3014,11 +3030,11 @@ 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)) ||
> > +       if (CC(is_l1_noncanonical_address_static(vmcs12->host_fs_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_gs_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_gdtr_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_idtr_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_tr_base, vcpu)) ||

If loads via LTR, LLDT, and LGDT are indeed exempt, then we need to update
emul_is_noncanonical_address() too.

The best idea I have is to have a separate flow for system registers (not a great
name, but I can't think of anything better), and the

E.g. s/is_host_noncanonical_msr_value/is_non_canonical_system_reg, and then
wire that up to the emulator.

> >             CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))
> >                 return -EINVAL;
> >  
> 

Re: [PATCH v3 3/4] KVM: nVMX: relax canonical checks on some x86 registers in vmx host state

[mlevitsk]

[-- Attachment #1: Type: text/plain, Size: 6049 bytes --]

У пт, 2024-08-16 у 15:03 -0700, Sean Christopherson пише:
> On Fri, Aug 16, 2024, mlevitsk@redhat.com wrote:
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++++++++++-------
> > >  1 file changed, 23 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index 2392a7ef254d..3f18edff80ac 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -2969,6 +2969,22 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
> > >         return 0;
> > >  }
> > >  
> > > +static bool is_l1_noncanonical_address_static(u64 la, struct kvm_vcpu *vcpu)
> > > +{
> > > +       u8 max_guest_address_bits = guest_can_use(vcpu, X86_FEATURE_LA57) ? 57 : 48;
> 
> I don't see any reason to use LA57 support from guest CPUID for the VMCS checks.
> The virtualization hole exists can't be safely plugged for all cases, so why
> bother trying to plug it only for some cases?

I also thought like that but then there is another argument:

My idea was that the guest really ought to not put non canonical values if its CPUID doesn't
support 5 level paging. There is absolutely no reason for doing so.

If the guest does this though via WRMSR, most of the time the MSR is not intercepted, thus
it makes sense to allow this in emulation patch as well, as we discussed to be consistent.

But when VMRESUME/VMLAUNCH instruction, which is *always* emulated, writes those MSRS on VM exit,
then I don't see a reason to allow a virtualization hole.

But then as it turns out (I didn't expect that) that instructions like LGDT also don't check CR4.LA57,
and these are also passed through, then I guess singling out the VMX instructions is no longer better.

> 
> It'd be very odd that an L1 could set a "bad" value via WRMSR, but then couldn't
> load that same value on VM-Exit, e.g. if L1 gets the VMCS value by doing RDMSR.
> 
> > > +       /*
> > > +        * Most x86 arch registers which contain linear addresses like
> > > +        * segment bases, addresses that are used in instructions (e.g SYSENTER),
> > > +        * have static canonicality checks,
> > > +        * size of whose depends only on CPU's support for 5-level
> > > +        * paging, rather than state of CR4.LA57.
> > > +        *
> > > +        * In other words the check only depends on the CPU model,
> > > +        * rather than on runtime state.
> > > +        */
> > > +       return !__is_canonical_address(la, max_guest_address_bits);
> > > +}
> > > +
> > >  static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> > >                                        struct vmcs12 *vmcs12)
> > >  {
> > > @@ -2979,8 +2995,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_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
> > > +           CC(is_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_eip, vcpu)))
> > >                 return -EINVAL;
> > >  
> > >         if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
> > > @@ -3014,11 +3030,11 @@ 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)) ||
> > > +       if (CC(is_l1_noncanonical_address_static(vmcs12->host_fs_base, vcpu)) ||
> > > +           CC(is_l1_noncanonical_address_static(vmcs12->host_gs_base, vcpu)) ||
> > > +           CC(is_l1_noncanonical_address_static(vmcs12->host_gdtr_base, vcpu)) ||
> > > +           CC(is_l1_noncanonical_address_static(vmcs12->host_idtr_base, vcpu)) ||
> > > +           CC(is_l1_noncanonical_address_static(vmcs12->host_tr_base, vcpu)) ||
> 
> If loads via LTR, LLDT, and LGDT are indeed exempt, then we need to update
> emul_is_noncanonical_address() too.

Sadly the answer to this is yes, at least on Intel. I will test on AMD soon, as soon as I grab
a Zen4 machine again.

And since these instructions are also all unintercepted, it also makes sense to use host cpuid
for them as well.

I attached two kvm unit tests, which I will hopefully polish for publishing soon, which pass
with flying colors with this patch series, and unless I made a mistake prove most of my
research.

The HOST_RIP field I checked separately by patching the L0 kernel, and observing it
either hang/crash or fail VM entry of the first guest.

Best regards,
	Maxim Levitsky

> 
> The best idea I have is to have a separate flow for system registers (not a great
> name, but I can't think of anything better), and the
> 
> E.g. s/is_host_noncanonical_msr_value/is_non_canonical_system_reg, and then
> wire that up to the emulator.
> 
> > >             CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))
> > >                 return -EINVAL;
> > >  



> 


[-- Attachment #2: 0001-Add-test-for-writing-canonical-values-to-various-msr.patch --]
[-- Type: text/x-patch, Size: 7913 bytes --]

From e6d7bd2aa4f185881714a6103e9e672b0dbd12ab Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <mlevitsk@redhat.com>
Date: Fri, 16 Aug 2024 12:40:20 +0300
Subject: [PATCH 1/2] Add test for writing canonical values to various msrs
 Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

---
 lib/x86/asm/setup.h |   1 +
 lib/x86/processor.h |   5 +
 lib/x86/vm.h        |   1 +
 x86/Makefile.x86_64 |   1 +
 x86/cstart64.S      |  35 +++++++
 x86/msr_canonical.c | 236 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 279 insertions(+)
 create mode 100644 x86/msr_canonical.c

diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
index 458eac858..399ced1f9 100644
--- a/lib/x86/asm/setup.h
+++ b/lib/x86/asm/setup.h
@@ -14,6 +14,7 @@ unsigned long setup_tss(u8 *stacktop);
 
 efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo);
 void setup_5level_page_table(void);
+void setup_4level_page_table(void);
 #endif /* CONFIG_EFI */
 
 void save_id(void);
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index da1ed6628..d478eff91 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -468,6 +468,11 @@ static inline int rdmsr_safe(u32 index, uint64_t *val)
 	return rdreg64_safe("rdmsr", index, val);
 }
 
+static inline int rdmsr_fep_safe(u32 index, uint64_t *val)
+{
+	return __rdreg64_safe(KVM_FEP, "rdmsr", index, val);
+}
+
 static inline int wrmsr_safe(u32 index, u64 val)
 {
 	return wrreg64_safe("wrmsr", index, val);
diff --git a/lib/x86/vm.h b/lib/x86/vm.h
index cf39787aa..60ace1a84 100644
--- a/lib/x86/vm.h
+++ b/lib/x86/vm.h
@@ -8,6 +8,7 @@
 #include "asm/bitops.h"
 
 void setup_5level_page_table(void);
+void setup_4level_page_table(void);
 
 struct pte_search {
 	int level;
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index 2771a6fad..1bc1c10b0 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -38,6 +38,7 @@ tests += $(TEST_DIR)/rdpru.$(exe)
 tests += $(TEST_DIR)/pks.$(exe)
 tests += $(TEST_DIR)/pmu_lbr.$(exe)
 tests += $(TEST_DIR)/pmu_pebs.$(exe)
+tests += $(TEST_DIR)/msr_canonical.$(exe)
 
 ifeq ($(CONFIG_EFI),y)
 tests += $(TEST_DIR)/amd_sev.$(exe)
diff --git a/x86/cstart64.S b/x86/cstart64.S
index 4dff11027..a91d55d00 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -92,6 +92,27 @@ switch_to_5level:
 	call enter_long_mode
 	jmpl $8, $lvl5
 
+
+switch_to_4level:
+	mov %cr0, %eax
+	btr $31, %eax
+	mov %eax, %cr0
+
+	mov $ptl4, %eax
+	mov %eax, pt_root
+
+	/* Disable CR4.LA57 */
+	mov %cr4, %eax
+	btr $12, %eax
+	mov %eax, %cr4
+
+	mov $0x10, %ax
+	mov %ax, %ss
+
+	call enter_long_mode
+	jmpl $8, $lvl5
+
+
 smp_stacktop:	.long stacktop - 4096
 
 .align 16
@@ -139,3 +160,17 @@ setup_5level_page_table:
 	lretq
 lvl5:
 	retq
+
+
+.globl setup_4level_page_table
+setup_4level_page_table:
+	/* Check if 4-level paging has already enabled */
+	mov %cr4, %rax
+	test $0x1000, %eax
+	jz lvl4
+
+	pushq $32
+	pushq $switch_to_4level
+	lretq
+lvl4:
+	retq
diff --git a/x86/msr_canonical.c b/x86/msr_canonical.c
new file mode 100644
index 000000000..89e809d90
--- /dev/null
+++ b/x86/msr_canonical.c
@@ -0,0 +1,236 @@
+#include "libcflat.h"
+
+#include "apic.h"
+#include "processor.h"
+#include "msr.h"
+#include "x86/vm.h"
+#include "asm/setup.h"
+
+static ulong msr_list[] = {
+	/*MSR_GS_BASE - tests needs this msr for _safe macros */
+	MSR_IA32_SYSENTER_ESP,
+	MSR_IA32_SYSENTER_EIP,
+	MSR_FS_BASE,
+	MSR_KERNEL_GS_BASE,
+	MSR_LSTAR,
+	MSR_CSTAR
+};
+
+#define TEST_VALUE0 0xffffffceb1600000
+#define TEST_VALUE1 0xff4547ceb1600000
+#define TEST_VALUE2 0xfa4547ceb1600000
+
+
+static void test_msrs0(void)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(msr_list) ; i++) {
+
+		u64 value = rdmsr(msr_list[i]);
+		u64 value1;
+		int vector;
+
+		// write test value via kvm
+		vector = wrmsr_fep_safe(msr_list[i], TEST_VALUE0);
+
+		if (vector)
+			report_fail("%d Fail to write msr via emulation", i);
+		else {
+			// read test value via hardware
+			if (rdmsr(msr_list[i]) != TEST_VALUE0)
+				report_fail("%d: Wrong msr value set via emulation", i);
+		}
+
+		// restore the original value
+		wrmsr(msr_list[i], value);
+
+		// now write test value via hardware
+		wrmsr(msr_list[i], TEST_VALUE0);
+
+		// now read test value via kvm
+		vector = rdmsr_fep_safe(msr_list[i], &value1);
+
+		if (vector)
+			report_fail("%d Fail to read msr via emulation\n", i);
+		else {
+			if( value1 != TEST_VALUE0)
+				report_fail("%d: Wrong value read via emulation", i);
+		}
+
+
+		// restore the original value
+		wrmsr(msr_list[i], value);
+	}
+}
+
+
+
+
+/*
+ * write non canonical for 4 level but canonical for 5 level paging
+ * value to the set of tested msrs
+ */
+static void test_msrs1(u64 test_value)
+{
+	int i, vector1, vector2;
+
+	for (i = 0 ; i < ARRAY_SIZE(msr_list) ; i++) {
+
+		u64 value = rdmsr(msr_list[i]);
+
+		vector1 = wrmsr_safe(msr_list[i], test_value);
+		wrmsr(msr_list[i], value);
+
+		vector2 = wrmsr_fep_safe(msr_list[i], test_value);
+		wrmsr(msr_list[i], value);
+
+		printf("%d: hw exception: %d kvm exception %d\n", i, vector1, vector2);
+
+	}
+}
+
+
+/*
+ * write non canonical for 4 level but canonical for 5 level paging
+ * value to the set of tested msrs while using 5 level paging,
+ * and then switch to 4 level paging
+ *
+ *
+ */
+static void test_msrs2(void)
+{
+	int i;
+
+	setup_5level_page_table();
+
+	for (i = 0 ; i < ARRAY_SIZE(msr_list) ; i++) {
+		wrmsr(msr_list[i], TEST_VALUE1);
+	}
+
+	setup_4level_page_table();
+
+	for (i = 0 ; i < ARRAY_SIZE(msr_list) ; i++)
+		if (rdmsr(msr_list[i]) != TEST_VALUE1)
+			report_fail("MSR %i didn't preserve value when switching back to 4 level paging", i);
+
+}
+
+
+static void test_lldt_host(u64 value)
+{
+	u16 orignal_ldt = sldt();
+
+	set_gdt_entry(FIRST_SPARE_SEL, value, 0x100, 0x82, 0);
+	lldt(FIRST_SPARE_SEL);
+	lldt(orignal_ldt);
+}
+
+static void test_ltr_host(u64 value)
+{
+	size_t tss_offset;
+
+	set_gdt_entry(FIRST_SPARE_SEL, value, 0x100, 0x89, 0);
+	ltr(FIRST_SPARE_SEL);
+
+	/* restore TSS*/
+	tss_offset = setup_tss(NULL);
+	load_gdt_tss(tss_offset);
+}
+
+static void test_lgdt_host(u64 value)
+{
+	struct descriptor_table_ptr dt_ptr;
+	u64 orig_base;
+
+	sgdt(&dt_ptr);
+	orig_base = dt_ptr.base;
+
+	dt_ptr.base = value;
+	lgdt(&dt_ptr);
+
+	dt_ptr.base = orig_base;
+	lgdt(&dt_ptr);
+}
+
+static void test_lidt_host(u64 value)
+{
+	struct descriptor_table_ptr dt_ptr;
+	u64 orig_base;
+
+	sidt(&dt_ptr);
+	orig_base = dt_ptr.base;
+
+	dt_ptr.base = value;
+	lidt(&dt_ptr);
+
+	dt_ptr.base = orig_base;
+	lidt(&dt_ptr);
+}
+
+
+static void test_special_bases(u64 value)
+{
+	test_lgdt_host(value);
+	test_lidt_host(value);
+
+	test_lldt_host(value);
+	test_ltr_host(value);
+
+	printf("Special bases test done for %lx\n", value);
+}
+
+
+int main(int argc, char **argv)
+{
+#ifndef CONFIG_EFI
+
+	printf("Basic msr test\n");
+	test_msrs0();
+
+	printf("testing msrs with 4 level paging\n");
+	test_msrs1(TEST_VALUE0);
+	printf("\n");
+
+	printf("testing msrs with 4 level paging (4 level non canonical value)\n");
+	test_msrs1(TEST_VALUE1);
+	printf("\n");
+
+	printf("testing msrs with 4 level paging (fully non canonical value)\n");
+	test_msrs1(TEST_VALUE2);
+	printf("\n");
+
+
+	setup_5level_page_table();
+
+	printf("testing msrs with 5 level paging\n");
+	test_msrs1(TEST_VALUE0);
+	printf("\n");
+
+
+	printf("testing msrs with 5 level paging (4 level non canonical value)\n");
+	test_msrs1(TEST_VALUE1);
+	printf("\n");
+
+	printf("testing msrs with 5 level paging (fully non canonical value)\n");
+	test_msrs1(TEST_VALUE2);
+	printf("\n");
+
+
+	printf("testing that msrs remain with non canonical values after switch to 4 level paging\n");
+	test_msrs2();
+
+	setup_5level_page_table();
+
+	test_special_bases(TEST_VALUE0);
+	test_special_bases(TEST_VALUE1);
+
+	setup_4level_page_table();
+
+	test_special_bases(TEST_VALUE0);
+	test_special_bases(TEST_VALUE1);
+
+#endif
+	return report_summary();
+}
+
-- 
2.40.1


[-- Attachment #3: 0002-vmx-add-test-for-canonical-checks-on-various-fields.patch --]
[-- Type: text/x-patch, Size: 4678 bytes --]

From e14288f50896caaafbdd8e66d58fe757f237b13c Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <mlevitsk@redhat.com>
Date: Mon, 22 Jul 2024 11:09:40 -0400
Subject: [PATCH 2/2] vmx: add test for canonical checks on various fields

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 x86/vmx_tests.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ffe7064c9..8f9784360 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -10732,6 +10732,7 @@ static void handle_exception_in_l1(u32 vector)
 	vmcs_write(EXC_BITMAP, old_eb);
 }
 
+
 static void vmx_exception_test(void)
 {
 	struct vmx_exception_test *t;
@@ -10754,6 +10755,155 @@ static void vmx_exception_test(void)
 	test_set_guest_finished();
 }
 
+
+#define TEST_VALUE_CANONICAL  0xffffffceb1600000
+#define TEST_VALUE_5CANONICAL 0xff4547ceb1600000
+
+static void vmx_canonical_test_guest(void)
+{
+	while (true) {
+		vmcall();
+	}
+}
+
+static int get_host_value(u64 vmcs_field, u64 *value)
+{
+	struct descriptor_table_ptr dt_ptr;
+
+	switch(vmcs_field) {
+	case HOST_SYSENTER_ESP:
+		*value = rdmsr(MSR_IA32_SYSENTER_ESP);
+		break;
+	case HOST_SYSENTER_EIP:
+		*value =  rdmsr(MSR_IA32_SYSENTER_EIP);
+		break;
+	case HOST_BASE_FS:
+		*value =  rdmsr(MSR_FS_BASE);
+		break;
+	case HOST_BASE_GS:
+		*value =  rdmsr(MSR_GS_BASE);
+		break;
+	case HOST_BASE_GDTR:
+		sgdt(&dt_ptr);
+		*value =  dt_ptr.base;
+		break;
+	case HOST_BASE_IDTR:
+		sidt(&dt_ptr);
+		*value =  dt_ptr.base;
+		break;
+	case HOST_BASE_TR:
+		*value = get_gdt_entry_base(get_tss_descr());
+		/* value might not reflect the actual base if changed by VMX */
+		return 1;
+	default:
+		assert(0);
+	}
+	return 0;
+}
+
+static void set_host_value(u64 vmcs_field, u64 value)
+{
+	struct descriptor_table_ptr dt_ptr;
+
+	switch(vmcs_field) {
+	case HOST_SYSENTER_ESP:
+		wrmsr(MSR_IA32_SYSENTER_ESP, value);
+		break;
+	case HOST_SYSENTER_EIP:
+		wrmsr(MSR_IA32_SYSENTER_EIP, value);
+		break;
+	case HOST_BASE_FS:
+		wrmsr(MSR_FS_BASE, value);
+		break;
+	case HOST_BASE_GS:
+		wrmsr(MSR_GS_BASE, value);
+		break;
+	case HOST_BASE_GDTR:
+		sgdt(&dt_ptr);
+		dt_ptr.base = value;
+		lgdt(&dt_ptr);
+		break;
+	case HOST_BASE_IDTR:
+		sidt(&dt_ptr);
+		dt_ptr.base = value;
+		lidt(&dt_ptr);
+		break;
+	case HOST_BASE_TR:
+		/* set the base and clear the busy bit */
+		set_gdt_entry(FIRST_SPARE_SEL, value, 0x200, 0x89, 0);
+		ltr(FIRST_SPARE_SEL);
+		break;
+	}
+}
+
+static void do_vmx_canonical_test_one_field(const char* name, u64 field)
+{
+	/* backup the msr and field values */
+	u64 host_org_value, test_value;
+	u64 field_org_value = vmcs_read(field);
+
+	get_host_value(field, &host_org_value);
+
+	/* write 57-canonical value on the host and check that it was written */
+	set_host_value(field, TEST_VALUE_5CANONICAL);
+	if (!get_host_value(field, &test_value)) {
+		report(test_value == TEST_VALUE_5CANONICAL, "%s: HOST value is set to test value directly", name);
+	}
+
+	/* write 57-canonical value via VMLANUCH/VMRESUME instruction*/
+	set_host_value(field, TEST_VALUE_CANONICAL);
+	vmcs_write(field, TEST_VALUE_5CANONICAL);
+
+	enter_guest();
+	skip_exit_vmcall();
+
+	if (!get_host_value(field, &test_value)) {
+		/* check that now msr value is the same as the field value*/
+		report(test_value == TEST_VALUE_5CANONICAL, "%s: HOST value is set to test value via VMLAUNCH/VMRESUME", name);
+	}
+
+	/* Restore original values */
+	vmcs_write(field, field_org_value);
+	set_host_value(field, host_org_value);
+}
+
+#define vmx_canonical_test_one_field(field) \
+	do_vmx_canonical_test_one_field(#field, field);
+
+
+
+static void test_lldt_host(u64 value)
+{
+	u16 orignal_ldt = sldt();
+
+	set_gdt_entry(FIRST_SPARE_SEL, value, 0x100, 0x82, 0);
+	lldt(FIRST_SPARE_SEL);
+	lldt(orignal_ldt);
+}
+
+static void vmx_canonical_test(void)
+{
+	report(!(read_cr4() & X86_CR4_LA57), "4 level paging");
+
+	test_set_guest(vmx_canonical_test_guest);
+
+	test_lldt_host(TEST_VALUE_5CANONICAL);
+
+	vmx_canonical_test_one_field(HOST_SYSENTER_ESP);
+	vmx_canonical_test_one_field(HOST_SYSENTER_EIP);
+
+	vmx_canonical_test_one_field(HOST_BASE_FS);
+	vmx_canonical_test_one_field(HOST_BASE_GS);
+
+	vmx_canonical_test_one_field(HOST_BASE_GDTR);
+	vmx_canonical_test_one_field(HOST_BASE_IDTR);
+
+	vmx_canonical_test_one_field(HOST_BASE_TR);
+
+
+	test_set_guest_finished();
+}
+
 enum Vid_op {
 	VID_OP_SET_ISR,
 	VID_OP_NOP,
@@ -11262,5 +11412,6 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_pf_invvpid_test),
 	TEST(vmx_pf_vpid_test),
 	TEST(vmx_exception_test),
+	TEST(vmx_canonical_test),
 	{ NULL, NULL, NULL, NULL, NULL, {0} },
 };
-- 
2.40.1

[PATCH v3 4/4] KVM: SVM: fix emulation of msr reads/writes of MSR_FS_BASE and MSR_GS_BASE

[Maxim Levitsky]

If these msrs are read by the emulator (e.g due to 'force emulation'
prefix), SVM code currently fails to extract the corresponding segment
bases, and return them to the emulator.

Fix that.

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

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a04f6627b237..be3fc54700e3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2876,6 +2876,12 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_CSTAR:
 		msr_info->data = svm->vmcb01.ptr->save.cstar;
 		break;
+	case MSR_GS_BASE:
+		msr_info->data = svm->vmcb01.ptr->save.gs.base;
+		break;
+	case MSR_FS_BASE:
+		msr_info->data = svm->vmcb01.ptr->save.fs.base;
+		break;
 	case MSR_KERNEL_GS_BASE:
 		msr_info->data = svm->vmcb01.ptr->save.kernel_gs_base;
 		break;
@@ -3101,6 +3107,12 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_CSTAR:
 		svm->vmcb01.ptr->save.cstar = data;
 		break;
+	case MSR_GS_BASE:
+		svm->vmcb01.ptr->save.gs.base = data;
+		break;
+	case MSR_FS_BASE:
+		svm->vmcb01.ptr->save.fs.base = data;
+		break;
 	case MSR_KERNEL_GS_BASE:
 		svm->vmcb01.ptr->save.kernel_gs_base = data;
 		break;
-- 
2.40.1

Re: [PATCH v3 4/4] KVM: SVM: fix emulation of msr reads/writes of MSR_FS_BASE and MSR_GS_BASE

[Sean Christopherson]

On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> If these msrs are read by the emulator (e.g due to 'force emulation'
> prefix), SVM code currently fails to extract the corresponding segment
> bases, and return them to the emulator.

I'll apply this one for 6.11 and tag it for stable, i.e. no need to include this
patch in v4.

Re: [PATCH v3 4/4] KVM: SVM: fix emulation of msr reads/writes of MSR_FS_BASE and MSR_GS_BASE

[Sean Christopherson]

On Fri, Aug 16, 2024, Sean Christopherson wrote:
> On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> > If these msrs are read by the emulator (e.g due to 'force emulation'
> > prefix), SVM code currently fails to extract the corresponding segment
> > bases, and return them to the emulator.
> 
> I'll apply this one for 6.11 and tag it for stable, i.e. no need to include this
> patch in v4.

I appear to have missed my normal "thank you" for this, so here it is, in kvm-x86
fixes:

[1/4] KVM: SVM: fix emulation of msr reads/writes of MSR_FS_BASE and MSR_GS_BASE
      https://github.com/kvm-x86/linux/commit/dad1613e0533