[PATCH 0/2] Fix for a very old KVM bug in the segment cache

[PATCH 0/2] Fix for a very old KVM bug in the segment cache

[Maxim Levitsky]

Hi,

Recently, while trying to understand why the pmu_counters_test
selftest sometimes fails when run nested I stumbled
upon a very interesting and old bug:

It turns out that KVM caches guest segment state,
but this cache doesn't have any protection against concurrent use.

This usually works because the cache is per vcpu, and should
only be accessed by vCPU thread, however there is an exception:

If the full preemption is enabled in the host kernel,
it is possible that vCPU thread will be preempted, for
example during the vmx_vcpu_reset.

vmx_vcpu_reset resets the segment cache bitmask and then initializes
the segments in the vmcs, however if the vcpus is preempted in the
middle of this code, the kvm_arch_vcpu_put is called which
reads SS's AR bytes to determine if the vCPU is in the kernel mode,
which caches the old value.

Later vmx_vcpu_reset will set the SS's AR field to the correct value
in vmcs but the cache still contains an invalid value which
can later for example leak via KVM_GET_SREGS and such.

In particular, kvm selftests will do KVM_GET_SREGS,
and then KVM_SET_SREGS, with a broken SS's AR field passed as is,
which will lead to vm entry failure.

This issue is not a nested issue, and actually I was able
to reproduce it on bare metal, but due to timing it happens
much more often nested. The only requirement for this to happen
is to have full preemption enabled in the kernel which runs the selftest.

pmu_counters_test reproduces this issue well, because it creates
lots of short lived VMs, but the issue as was noted
about is not related to pmu.

To fix this issue, I wrapped the places which write the segment
fields with preempt_disable/enable. It's not an ideal fix, other options are
possible. Please tell me if you prefer these:

1. Getting rid of the segment cache. I am not sure how much it helps
these days - this code is very old.

2. Using a read/write lock - IMHO the cleanest solution but might
also affect performance.

3. Making the kvm_arch_vcpu_in_kernel not touch the cache
and instead do a vmread directly.
This is a shorter solution but probably less future proof.

Best regards,
	Maxim Levitsky

Maxim Levitsky (2):
  KVM: nVMX: use vmx_segment_cache_clear
  KVM: VMX: disable preemption when writing guest segment state

 arch/x86/kvm/vmx/nested.c |  7 ++++++-
 arch/x86/kvm/vmx/vmx.c    | 22 ++++++++++++++++++----
 arch/x86/kvm/vmx/vmx.h    |  5 +++++
 3 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.26.3

[PATCH 1/2] KVM: nVMX: use vmx_segment_cache_clear

[Maxim Levitsky]

In prepare_vmcs02_rare, call vmx_segment_cache_clear, instead
of setting the segment_cache.bitmask directly.

No functional change intended.

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

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 643935a0f70ab..d3ca1a772ae67 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2469,6 +2469,9 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 
 	if (!hv_evmcs || !(hv_evmcs->hv_clean_fields &
 			   HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP2)) {
+
+		vmx_segment_cache_clear(vmx);
+
 		vmcs_write16(GUEST_ES_SELECTOR, vmcs12->guest_es_selector);
 		vmcs_write16(GUEST_CS_SELECTOR, vmcs12->guest_cs_selector);
 		vmcs_write16(GUEST_SS_SELECTOR, vmcs12->guest_ss_selector);
@@ -2505,8 +2508,6 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 		vmcs_writel(GUEST_TR_BASE, vmcs12->guest_tr_base);
 		vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
 		vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
-
-		vmx->segment_cache.bitmask = 0;
 	}
 
 	if (!hv_evmcs || !(hv_evmcs->hv_clean_fields &
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b3c83c06f8265..fa9f307d9b18b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -524,10 +524,6 @@ static const struct kvm_vmx_segment_field {
 	VMX_SEGMENT_FIELD(LDTR),
 };
 
-static inline void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
-{
-	vmx->segment_cache.bitmask = 0;
-}
 
 static unsigned long host_idt_base;
 
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 7b64e271a9319..1689f0d59f435 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -755,4 +755,9 @@ static inline bool vmx_can_use_ipiv(struct kvm_vcpu *vcpu)
 	return  lapic_in_kernel(vcpu) && enable_ipiv;
 }
 
+static inline void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
+{
+	vmx->segment_cache.bitmask = 0;
+}
+
 #endif /* __KVM_X86_VMX_H */
-- 
2.26.3

[PATCH 2/2] KVM: VMX: disable preemption when writing guest segment state

[Maxim Levitsky]

VMX code uses a segment cache to avoid reading guest segment fields
from the vmcs.

The cache is reset each time a field that belongs to the guest segment
state is written.

However if the vCPU is preempted after the cache is reset but before a new
field value is written, a race can happen:

If during the preemption period the same field is read,
its old value is put in the cache and the cache is never updated when
execution returns to the preempted code which finally writes the new
value to the field.

Usually a lock is required to avoid a race in such cases but since
vCPU segment state should only be accessed by its vCPU thread,
we can avoid a lock and opt to only disable preemption,
in places where the segment cache is invalidated and
segment fields are updated.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/nested.c |  4 ++++
 arch/x86/kvm/vmx/vmx.c    | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index d3ca1a772ae67..62c3c12b4c41d 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2470,6 +2470,8 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 	if (!hv_evmcs || !(hv_evmcs->hv_clean_fields &
 			   HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP2)) {
 
+		preempt_disable();
+
 		vmx_segment_cache_clear(vmx);
 
 		vmcs_write16(GUEST_ES_SELECTOR, vmcs12->guest_es_selector);
@@ -2508,6 +2510,8 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 		vmcs_writel(GUEST_TR_BASE, vmcs12->guest_tr_base);
 		vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
 		vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
+
+		preempt_enable();
 	}
 
 	if (!hv_evmcs || !(hv_evmcs->hv_clean_fields &
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fa9f307d9b18b..7b27723f787cc 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2171,12 +2171,16 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 #ifdef CONFIG_X86_64
 	case MSR_FS_BASE:
+		preempt_disable();
 		vmx_segment_cache_clear(vmx);
 		vmcs_writel(GUEST_FS_BASE, data);
+		preempt_enable();
 		break;
 	case MSR_GS_BASE:
+		preempt_disable();
 		vmx_segment_cache_clear(vmx);
 		vmcs_writel(GUEST_GS_BASE, data);
+		preempt_enable();
 		break;
 	case MSR_KERNEL_GS_BASE:
 		vmx_write_guest_kernel_gs_base(vmx, data);
@@ -3088,6 +3092,7 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
 
 	vmx->rmode.vm86_active = 1;
 
+	preempt_disable();
 	vmx_segment_cache_clear(vmx);
 
 	vmcs_writel(GUEST_TR_BASE, kvm_vmx->tss_addr);
@@ -3109,6 +3114,8 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
 	fix_rmode_seg(VCPU_SREG_DS, &vmx->rmode.segs[VCPU_SREG_DS]);
 	fix_rmode_seg(VCPU_SREG_GS, &vmx->rmode.segs[VCPU_SREG_GS]);
 	fix_rmode_seg(VCPU_SREG_FS, &vmx->rmode.segs[VCPU_SREG_FS]);
+
+	preempt_enable();
 }
 
 int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
@@ -3140,6 +3147,7 @@ static void enter_lmode(struct kvm_vcpu *vcpu)
 {
 	u32 guest_tr_ar;
 
+	preempt_disable();
 	vmx_segment_cache_clear(to_vmx(vcpu));
 
 	guest_tr_ar = vmcs_read32(GUEST_TR_AR_BYTES);
@@ -3150,6 +3158,9 @@ static void enter_lmode(struct kvm_vcpu *vcpu)
 			     (guest_tr_ar & ~VMX_AR_TYPE_MASK)
 			     | VMX_AR_TYPE_BUSY_64_TSS);
 	}
+
+	preempt_enable();
+
 	vmx_set_efer(vcpu, vcpu->arch.efer | EFER_LMA);
 }
 
@@ -3571,6 +3582,7 @@ void __vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
 
+	preempt_disable();
 	vmx_segment_cache_clear(vmx);
 
 	if (vmx->rmode.vm86_active && seg != VCPU_SREG_LDTR) {
@@ -3601,6 +3613,8 @@ void __vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg)
 		var->type |= 0x1; /* Accessed */
 
 	vmcs_write32(sf->ar_bytes, vmx_segment_access_rights(var));
+
+	preempt_enable();
 }
 
 void vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg)
@@ -4870,6 +4884,8 @@ void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vmx->hv_deadline_tsc = -1;
 	kvm_set_cr8(vcpu, 0);
 
+	preempt_disable();
+
 	vmx_segment_cache_clear(vmx);
 	kvm_register_mark_available(vcpu, VCPU_EXREG_SEGMENTS);
 
@@ -4899,6 +4915,8 @@ void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vmcs_writel(GUEST_IDTR_BASE, 0);
 	vmcs_write32(GUEST_IDTR_LIMIT, 0xffff);
 
+	preempt_enable();
+
 	vmcs_write32(GUEST_ACTIVITY_STATE, GUEST_ACTIVITY_ACTIVE);
 	vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, 0);
 	vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS, 0);
-- 
2.26.3

Re: [PATCH 0/2] Fix for a very old KVM bug in the segment cache

[Paolo Bonzini]

On 7/13/24 03:38, Maxim Levitsky wrote:
> 1. Getting rid of the segment cache. I am not sure how much it helps
> these days - this code is very old.
> 
> 2. Using a read/write lock - IMHO the cleanest solution but might
> also affect performance.

A read/write lock would cause a deadlock between the writer and the 
sched_out callback, since they run on the same CPU.

I think the root cause of the issue is that clearing the cache should be 
done _after_ the writes (and should have a barrier() at the beginning, 
if only for cleanliness).  So your patch 1 should leave the clearing of 
vmx->segment_cache.bitmask where it was.

However, that would still leave an assumption: that it's okay that a 
sched_out during vmx_vcpu_reset() (or other functions that write segment 
data in the VMCS) accesses stale data, as long as the stale data is not 
used after vmx_vcpu_reset() returns.  Your patch is a safer approach, 
but maybe wrap preempt_disable()/preempt_enable() with

	vmx_invalidate_segment_cache_start() {
		preempt_disable();
	}
	vmx_invalidate_segment_cache_end() {
		vmx->segment_cache.bitmask = 0;
		preempt_enable();
	}

Paolo

Re: [PATCH 0/2] Fix for a very old KVM bug in the segment cache

[Maxim Levitsky]

On Sat, 2024-07-13 at 12:22 +0200, Paolo Bonzini wrote:
> On 7/13/24 03:38, Maxim Levitsky wrote:
> > 1. Getting rid of the segment cache. I am not sure how much it helps
> > these days - this code is very old.
> > 
> > 2. Using a read/write lock - IMHO the cleanest solution but might
> > also affect performance.
> 
> A read/write lock would cause a deadlock between the writer and the 
> sched_out callback, since they run on the same CPU.
> 
> I think the root cause of the issue is that clearing the cache should be 
> done _after_ the writes (and should have a barrier() at the beginning, 
> if only for cleanliness).  So your patch 1 should leave the clearing of 
> vmx->segment_cache.bitmask where it was.
> 
> However, that would still leave an assumption: that it's okay that a 
> sched_out during vmx_vcpu_reset() (or other functions that write segment 
> data in the VMCS) accesses stale data, as long as the stale data is not 
> used after vmx_vcpu_reset() returns.  Your patch is a safer approach, 
> but maybe wrap preempt_disable()/preempt_enable() with
> 
> 	vmx_invalidate_segment_cache_start() {
> 		preempt_disable();
> 	}
> 	vmx_invalidate_segment_cache_end() {
> 		vmx->segment_cache.bitmask = 0;
> 		preempt_enable();
> 	}
> 
> Paolo
> 

Hi Paolo!

This looks like a very good idea, I'll do this in v2.

Thanks,
Best regards,
	Maxim Levitsky