[PATCH 0/5] KVM: x86: Handle reserved CR4 bit interception in VMX

[PATCH 0/5] KVM: x86: Handle reserved CR4 bit interception in VMX

[Sean Christopherson]

This series stems from Lai's RFC patches to intercept LA57 and let the
guest own FSGSBASE[*].  Discussion and inspection revealed that KVM does
not handle the case where LA57 is supported in hardware but not exposed to
the guest.  This is actually true for all CR4 bits, but LA57 is currently
the only bit that can be reserved and also owned by the guest.  I have
a unit test for this that I'll post separately.

Intercepting LA57 was by far the easiest fix for the immedidate bug, and
is likely the right change in the long term as there's no justification
for letting the guest own LA57.

The middle three patches adjust VMX's CR4 guest/host mask to intercept
reserved bits.  This required reworking CPUID updates to also refresh said
mask at the correct time.

The last past is Lai's, which let's the guest own FSGSBASE.  This depends
on the reserved bit handling being in place.

Ran everything through unit tests, and ran the kernel's FSGSBASE selftests
in a VM.

[*] https://lkml.kernel.org/r/20200928083047.3349-1-jiangshanlai@gmail.com

Lai Jiangshan (2):
  KVM: x86: Intercept LA57 to inject #GP fault when it's reserved
  KVM: x86: Let the guest own CR4.FSGSBASE

Sean Christopherson (3):
  KVM: x86: Invoke vendor's vcpu_after_set_cpuid() after all common
    updates
  KVM: x86: Move call to update_exception_bitmap() into VMX code
  KVM: VMX: Intercept guest reserved CR4 bits to inject #GP fault

 arch/x86/kvm/cpuid.c          |  6 +++---
 arch/x86/kvm/kvm_cache_regs.h |  2 +-
 arch/x86/kvm/vmx/vmx.c        | 18 +++++++++++++-----
 3 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.28.0

[PATCH 1/5] KVM: x86: Intercept LA57 to inject #GP fault when it's reserved

[Sean Christopherson]

From: Lai Jiangshan <laijs@linux.alibaba.com>

Unconditionally intercept changes to CR4.LA57 so that KVM correctly
injects a #GP fault if the guest attempts to set CR4.LA57 when it's
supported in hardware but not exposed to the guest.

Long term, KVM needs to properly handle CR4 bits that can be under guest
control but also may be reserved from the guest's perspective.  But, KVM
currently sets the CR4 guest/host mask only during vCPU creation, and
reworking flows to change that will take a bit of elbow grease.

Even if/when generic support for intercepting reserved bits exists, it's
probably not worth letting the guest set CR4.LA57 directly.  LA57 can't
be toggled while long mode is enabled, thus it's all but guaranteed to
be set once (maybe twice, e.g. by BIOS and kernel) during boot and never
touched again.  On the flip side, letting the guest own CR4.LA57 may
incur extra VMREADs.  In other words, this temporary "hack" is probably
also the right long term fix.

Fixes: fd8cb433734e ("KVM: MMU: Expose the LA57 feature to VM.")
Cc: stable@vger.kernel.org
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
[sean: rewrote changelog]
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/kvm_cache_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index cfe83d4ae625..ca0781b41df9 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -7,7 +7,7 @@
 #define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
 #define KVM_POSSIBLE_CR4_GUEST_BITS				  \
 	(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR  \
-	 | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_PGE | X86_CR4_TSD)
+	 | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD)
 
 #define BUILD_KVM_GPR_ACCESSORS(lname, uname)				      \
 static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu *vcpu)\
-- 
2.28.0

[PATCH 2/5] KVM: x86: Invoke vendor's vcpu_after_set_cpuid() after all common updates

[Sean Christopherson]

Move the call to kvm_x86_ops.vcpu_after_set_cpuid() to the very end of
kvm_vcpu_after_set_cpuid() to allow the vendor implementation to react
to changes made by the common code.  In the near future, this will be
used by VMX to update its CR4 guest/host masks to account for reserved
bits.  In the long term, SGX support will update the allowed XCR0 mask
for enclaves based on the vCPU's allowed XCR0.

vcpu_after_set_cpuid() (nee kvm_update_cpuid()) was originally added by
commit 2acf923e38fb ("KVM: VMX: Enable XSAVE/XRSTOR for guest"), and was
called separately after kvm_x86_ops.vcpu_after_set_cpuid() (nee
kvm_x86_ops->cpuid_update()).  There is no indication that the placement
of the common code updates after the vendor updates was anything more
than a "new function at the end" decision.

Inspection of the current code reveals no dependency on kvm_x86_ops'
vcpu_after_set_cpuid() in kvm_vcpu_after_set_cpuid() or any of its
helpers.  The bulk of the common code depends only on the guest's CPUID
configuration, kvm_mmu_reset_context() does not consume dynamic vendor
state, and there are no collisions between kvm_pmu_refresh() and VMX's
update of PT state.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 37c3668a774f..963bad7bc0ff 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -121,8 +121,6 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct kvm_cpuid_entry2 *best;
 
-	kvm_x86_ops.vcpu_after_set_cpuid(vcpu);
-
 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
 	if (best && apic) {
 		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
@@ -146,6 +144,9 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	kvm_pmu_refresh(vcpu);
 	vcpu->arch.cr4_guest_rsvd_bits =
 	    __cr4_reserved_bits(guest_cpuid_has, vcpu);
+
+	/* Invoke the vendor callback only after the above state is updated. */
+	kvm_x86_ops.vcpu_after_set_cpuid(vcpu);
 	kvm_x86_ops.update_exception_bitmap(vcpu);
 }
 
-- 
2.28.0

[PATCH 3/5] KVM: x86: Move call to update_exception_bitmap() into VMX code

[Sean Christopherson]

Now that vcpu_after_set_cpuid() and update_exception_bitmap() are called
back-to-back, subsume the exception bitmap update into the common CPUID
update.  Drop the SVM invocation entirely as SVM's exception bitmap
doesn't vary with respect to guest CPUID.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c   | 1 -
 arch/x86/kvm/vmx/vmx.c | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 963bad7bc0ff..dd62156b8868 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -147,7 +147,6 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 
 	/* Invoke the vendor callback only after the above state is updated. */
 	kvm_x86_ops.vcpu_after_set_cpuid(vcpu);
-	kvm_x86_ops.update_exception_bitmap(vcpu);
 }
 
 static int is_efer_nx(void)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4551a7e80ebc..223e070c48b2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7232,6 +7232,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 			vmx_set_guest_uret_msr(vmx, msr, enabled ? 0 : TSX_CTRL_RTM_DISABLE);
 		}
 	}
+
+	/* Refresh #PF interception to account for MAXPHYADDR changes. */
+	update_exception_bitmap(vcpu);
 }
 
 static __init void vmx_set_cpu_caps(void)
-- 
2.28.0

[PATCH 4/5] KVM: VMX: Intercept guest reserved CR4 bits to inject #GP fault

[Sean Christopherson]

Intercept CR4 bits that are guest reserved so that KVM correctly injects
a #GP fault if the guest attempts to set a reserved bit.  If a feature
is supported by the CPU but is not exposed to the guest, and its
associated CR4 bit is not intercepted by KVM by default, then KVM will
fail to inject a #GP if the guest sets the CR4 bit without triggering
an exit, e.g. by toggling only the bit in question.

Note, KVM doesn't give the guest direct access to any CR4 bits that are
also dependent on guest CPUID.  Yet.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 223e070c48b2..4ff440e7518e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4037,13 +4037,16 @@ void vmx_set_constant_host_state(struct vcpu_vmx *vmx)
 
 void set_cr4_guest_host_mask(struct vcpu_vmx *vmx)
 {
-	vmx->vcpu.arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS;
+	struct kvm_vcpu *vcpu = &vmx->vcpu;
+
+	vcpu->arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS &
+					  ~vcpu->arch.cr4_guest_rsvd_bits;
 	if (!enable_ept)
-		vmx->vcpu.arch.cr4_guest_owned_bits &= ~X86_CR4_PGE;
+		vcpu->arch.cr4_guest_owned_bits &= ~X86_CR4_PGE;
 	if (is_guest_mode(&vmx->vcpu))
-		vmx->vcpu.arch.cr4_guest_owned_bits &=
-			~get_vmcs12(&vmx->vcpu)->cr4_guest_host_mask;
-	vmcs_writel(CR4_GUEST_HOST_MASK, ~vmx->vcpu.arch.cr4_guest_owned_bits);
+		vcpu->arch.cr4_guest_owned_bits &=
+			~get_vmcs12(vcpu)->cr4_guest_host_mask;
+	vmcs_writel(CR4_GUEST_HOST_MASK, ~vcpu->arch.cr4_guest_owned_bits);
 }
 
 u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
@@ -7233,6 +7236,8 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		}
 	}
 
+	set_cr4_guest_host_mask(vmx);
+
 	/* Refresh #PF interception to account for MAXPHYADDR changes. */
 	update_exception_bitmap(vcpu);
 }
-- 
2.28.0

[PATCH 5/5] KVM: x86: Let the guest own CR4.FSGSBASE

[Sean Christopherson]

From: Lai Jiangshan <laijs@linux.alibaba.com>

Add FSGSBASE to the set of possible guest-owned CR4 bits, i.e. let the
guest own it on VMX.  KVM never queries the guest's CR4.FSGSBASE value,
thus there is no reason to force VM-Exit on FSGSBASE being toggled.

Note, because FSGSBASE is conditionally available, this is dependent on
recent changes to intercept reserved CR4 bits and to update the CR4
guest/host mask in response to guest CPUID changes.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
[sean: added justification in changelog]
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/kvm_cache_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index ca0781b41df9..a889563ad02d 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -7,7 +7,7 @@
 #define KVM_POSSIBLE_CR0_GUEST_BITS X86_CR0_TS
 #define KVM_POSSIBLE_CR4_GUEST_BITS				  \
 	(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR  \
-	 | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD)
+	 | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE)
 
 #define BUILD_KVM_GPR_ACCESSORS(lname, uname)				      \
 static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu *vcpu)\
-- 
2.28.0

Re: [PATCH 0/5] KVM: x86: Handle reserved CR4 bit interception in VMX

[Paolo Bonzini]

On 30/09/20 06:16, Sean Christopherson wrote:
> This series stems from Lai's RFC patches to intercept LA57 and let the
> guest own FSGSBASE[*].  Discussion and inspection revealed that KVM does
> not handle the case where LA57 is supported in hardware but not exposed to
> the guest.  This is actually true for all CR4 bits, but LA57 is currently
> the only bit that can be reserved and also owned by the guest.  I have
> a unit test for this that I'll post separately.
> 
> Intercepting LA57 was by far the easiest fix for the immedidate bug, and
> is likely the right change in the long term as there's no justification
> for letting the guest own LA57.
> 
> The middle three patches adjust VMX's CR4 guest/host mask to intercept
> reserved bits.  This required reworking CPUID updates to also refresh said
> mask at the correct time.
> 
> The last past is Lai's, which let's the guest own FSGSBASE.  This depends
> on the reserved bit handling being in place.
> 
> Ran everything through unit tests, and ran the kernel's FSGSBASE selftests
> in a VM.
> 
> [*] https://lkml.kernel.org/r/20200928083047.3349-1-jiangshanlai@gmail.com
> 
> Lai Jiangshan (2):
>   KVM: x86: Intercept LA57 to inject #GP fault when it's reserved
>   KVM: x86: Let the guest own CR4.FSGSBASE
> 
> Sean Christopherson (3):
>   KVM: x86: Invoke vendor's vcpu_after_set_cpuid() after all common
>     updates
>   KVM: x86: Move call to update_exception_bitmap() into VMX code
>   KVM: VMX: Intercept guest reserved CR4 bits to inject #GP fault
> 
>  arch/x86/kvm/cpuid.c          |  6 +++---
>  arch/x86/kvm/kvm_cache_regs.h |  2 +-
>  arch/x86/kvm/vmx/vmx.c        | 18 +++++++++++++-----
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 

Queued, thanks.

Paolo

Re: [PATCH 0/5] KVM: x86: Handle reserved CR4 bit interception in VMX

[Paolo Bonzini]

On 30/09/20 06:16, Sean Christopherson wrote:
> This series stems from Lai's RFC patches to intercept LA57 and let the
> guest own FSGSBASE[*].  Discussion and inspection revealed that KVM does
> not handle the case where LA57 is supported in hardware but not exposed to
> the guest.  This is actually true for all CR4 bits, but LA57 is currently
> the only bit that can be reserved and also owned by the guest.  I have
> a unit test for this that I'll post separately.
> 
> Intercepting LA57 was by far the easiest fix for the immedidate bug, and
> is likely the right change in the long term as there's no justification
> for letting the guest own LA57.
> 
> The middle three patches adjust VMX's CR4 guest/host mask to intercept
> reserved bits.  This required reworking CPUID updates to also refresh said
> mask at the correct time.
> 
> The last past is Lai's, which let's the guest own FSGSBASE.  This depends
> on the reserved bit handling being in place.
> 
> Ran everything through unit tests, and ran the kernel's FSGSBASE selftests
> in a VM.
> 
> [*] https://lkml.kernel.org/r/20200928083047.3349-1-jiangshanlai@gmail.com
> 
> Lai Jiangshan (2):
>   KVM: x86: Intercept LA57 to inject #GP fault when it's reserved
>   KVM: x86: Let the guest own CR4.FSGSBASE
> 
> Sean Christopherson (3):
>   KVM: x86: Invoke vendor's vcpu_after_set_cpuid() after all common
>     updates
>   KVM: x86: Move call to update_exception_bitmap() into VMX code
>   KVM: VMX: Intercept guest reserved CR4 bits to inject #GP fault
> 
>  arch/x86/kvm/cpuid.c          |  6 +++---
>  arch/x86/kvm/kvm_cache_regs.h |  2 +-
>  arch/x86/kvm/vmx/vmx.c        | 18 +++++++++++++-----
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 

Queued, thanks.

Paolo