[PATCH v3] kvm: x86: fix stale mmio cache bug

[PATCH v3] kvm: x86: fix stale mmio cache bug

[David Matlack]

The following events can lead to an incorrect KVM_EXIT_MMIO bubbling
up to userspace:

(1) Guest accesses gpa X without a memory slot. The gfn is cached in
struct kvm_vcpu_arch (mmio_gfn). On Intel EPT-enabled hosts, KVM sets
the SPTE write-execute-noread so that future accesses cause
EPT_MISCONFIGs.

(2) Host userspace creates a memory slot via KVM_SET_USER_MEMORY_REGION
covering the page just accessed.

(3) Guest attempts to read or write to gpa X again. On Intel, this
generates an EPT_MISCONFIG. The memory slot generation number that
was incremented in (2) would normally take care of this but we fast
path mmio faults through quickly_check_mmio_pf(), which only checks
the per-vcpu mmio cache. Since we hit the cache, KVM passes a
KVM_EXIT_MMIO up to userspace.

This patch fixes the issue by using the memslot generation number
to validate the mmio cache.

Signed-off-by: David Matlack <dmatlack@google.com>
---
The patch diff is rather large because I had to pull some code out
of x86.h and mmu.c and into mmu.h. The main change is adding the
memslot generation in vcpu_cach_mmio_info() and then validating
that slot in vcpu_match_mmio_*().

Changes in v3:
  - remove memory barrier in vcpu_cache_mmio_info()
  - don't unconditionally clear mmio cache in mmu_synch_roots

Changes in v2:
  - Use memslot generation to invalidate the mmio cache rather than
    actively invalidating the cache.
  - Update patch description with new cache invalidation technique.
  - Pull mmio cache/clear code up out of x86.h and mmu.c and into
    mmu.h.

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/mmu.c              | 15 +----------
 arch/x86/kvm/mmu.h              | 58 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.h              | 36 -------------------------
 4 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 49205d0..f518d14 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -479,6 +479,7 @@ struct kvm_vcpu_arch {
 	u64 mmio_gva;
 	unsigned access;
 	gfn_t mmio_gfn;
+	unsigned int mmio_gen;
 
 	struct kvm_pmu pmu;
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9314678..dd5a30d 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -206,11 +206,8 @@ EXPORT_SYMBOL_GPL(kvm_mmu_set_mmio_spte_mask);
 #define MMIO_SPTE_GEN_LOW_SHIFT		3
 #define MMIO_SPTE_GEN_HIGH_SHIFT	52
 
-#define MMIO_GEN_SHIFT			19
 #define MMIO_GEN_LOW_SHIFT		9
 #define MMIO_GEN_LOW_MASK		((1 << MMIO_GEN_LOW_SHIFT) - 1)
-#define MMIO_GEN_MASK			((1 << MMIO_GEN_SHIFT) - 1)
-#define MMIO_MAX_GEN			((1 << MMIO_GEN_SHIFT) - 1)
 
 static u64 generation_mmio_spte_mask(unsigned int gen)
 {
@@ -234,16 +231,6 @@ static unsigned int get_mmio_spte_generation(u64 spte)
 	return gen;
 }
 
-static unsigned int kvm_current_mmio_generation(struct kvm *kvm)
-{
-	/*
-	 * Init kvm generation close to MMIO_MAX_GEN to easily test the
-	 * code of handling generation number wrap-around.
-	 */
-	return (kvm_memslots(kvm)->generation +
-		      MMIO_MAX_GEN - 150) & MMIO_GEN_MASK;
-}
-
 static void mark_mmio_spte(struct kvm *kvm, u64 *sptep, u64 gfn,
 			   unsigned access)
 {
@@ -3163,7 +3150,7 @@ static void mmu_sync_roots(struct kvm_vcpu *vcpu)
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;
 
-	vcpu_clear_mmio_info(vcpu, ~0ul);
+	vcpu_clear_mmio_info(vcpu, MMIO_GVA_ANY);
 	kvm_mmu_audit(vcpu, AUDIT_PRE_SYNC);
 	if (vcpu->arch.mmu.root_level == PT64_ROOT_LEVEL) {
 		hpa_t root = vcpu->arch.mmu.root_hpa;
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index b982112..a98a060 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -82,6 +82,64 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *context,
 void update_permission_bitmask(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 		bool ept);
 
+#define MMIO_GEN_SHIFT                 19
+#define MMIO_GEN_MASK                  ((1 << MMIO_GEN_SHIFT) - 1)
+#define MMIO_MAX_GEN                   ((1 << MMIO_GEN_SHIFT) - 1)
+static inline unsigned int kvm_current_mmio_generation(struct kvm *kvm)
+{
+	/*
+	 * Init kvm generation close to MMIO_MAX_GEN to easily test the
+	 * code of handling generation number wrap-around.
+	 */
+	return (kvm_memslots(kvm)->generation +
+			MMIO_MAX_GEN - 150) & MMIO_GEN_MASK;
+}
+
+static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
+					gva_t gva, gfn_t gfn, unsigned access)
+{
+	vcpu->arch.mmio_gen = kvm_current_mmio_generation(vcpu->kvm);
+	vcpu->arch.mmio_gva = gva & PAGE_MASK;
+	vcpu->arch.access = access;
+	vcpu->arch.mmio_gfn = gfn;
+}
+
+/*
+ * Clear the mmio cache info for the given gva. If gva is MMIO_GVA_ANY,
+ * unconditionally clear the mmio cache.
+ */
+#define MMIO_GVA_ANY ~((gva_t) 0)
+static inline void vcpu_clear_mmio_info(struct kvm_vcpu *vcpu, gva_t gva)
+{
+	if (gva != MMIO_GVA_ANY && vcpu->arch.mmio_gva != (gva & PAGE_MASK))
+		return;
+
+	vcpu->arch.mmio_gva = 0;
+}
+
+static inline bool vcpu_match_mmio_gen(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.mmio_gen == kvm_current_mmio_generation(vcpu->kvm);
+}
+
+static inline bool vcpu_match_mmio_gva(struct kvm_vcpu *vcpu, unsigned long gva)
+{
+	u64 mmio_gva = vcpu->arch.mmio_gva;
+
+	return vcpu_match_mmio_gen(vcpu) &&
+	       mmio_gva &&
+	       mmio_gva == (gva & PAGE_MASK);
+}
+
+static inline bool vcpu_match_mmio_gpa(struct kvm_vcpu *vcpu, gpa_t gpa)
+{
+	gfn_t mmio_gfn = vcpu->arch.mmio_gfn;
+
+	return vcpu_match_mmio_gen(vcpu) &&
+	       mmio_gfn &&
+	       mmio_gfn == (gpa >> PAGE_SHIFT);
+}
+
 static inline unsigned int kvm_mmu_available_pages(struct kvm *kvm)
 {
 	if (kvm->arch.n_max_mmu_pages > kvm->arch.n_used_mmu_pages)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 8c97bac..c686c91 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -72,42 +72,6 @@ static inline u32 bit(int bitno)
 	return 1 << (bitno & 31);
 }
 
-static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
-					gva_t gva, gfn_t gfn, unsigned access)
-{
-	vcpu->arch.mmio_gva = gva & PAGE_MASK;
-	vcpu->arch.access = access;
-	vcpu->arch.mmio_gfn = gfn;
-}
-
-/*
- * Clear the mmio cache info for the given gva,
- * specially, if gva is ~0ul, we clear all mmio cache info.
- */
-static inline void vcpu_clear_mmio_info(struct kvm_vcpu *vcpu, gva_t gva)
-{
-	if (gva != (~0ul) && vcpu->arch.mmio_gva != (gva & PAGE_MASK))
-		return;
-
-	vcpu->arch.mmio_gva = 0;
-}
-
-static inline bool vcpu_match_mmio_gva(struct kvm_vcpu *vcpu, unsigned long gva)
-{
-	if (vcpu->arch.mmio_gva && vcpu->arch.mmio_gva == (gva & PAGE_MASK))
-		return true;
-
-	return false;
-}
-
-static inline bool vcpu_match_mmio_gpa(struct kvm_vcpu *vcpu, gpa_t gpa)
-{
-	if (vcpu->arch.mmio_gfn && vcpu->arch.mmio_gfn == gpa >> PAGE_SHIFT)
-		return true;
-
-	return false;
-}
-
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
 int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
-- 
2.0.0.526.g5318336

Re: [PATCH v3] kvm: x86: fix stale mmio cache bug

[Xiao Guangrong]

On 08/08/2014 02:32 AM, David Matlack wrote:
> The following events can lead to an incorrect KVM_EXIT_MMIO bubbling
> up to userspace:
> 
> (1) Guest accesses gpa X without a memory slot. The gfn is cached in
> struct kvm_vcpu_arch (mmio_gfn). On Intel EPT-enabled hosts, KVM sets
> the SPTE write-execute-noread so that future accesses cause
> EPT_MISCONFIGs.
> 
> (2) Host userspace creates a memory slot via KVM_SET_USER_MEMORY_REGION
> covering the page just accessed.
> 
> (3) Guest attempts to read or write to gpa X again. On Intel, this
> generates an EPT_MISCONFIG. The memory slot generation number that
> was incremented in (2) would normally take care of this but we fast
> path mmio faults through quickly_check_mmio_pf(), which only checks
> the per-vcpu mmio cache. Since we hit the cache, KVM passes a
> KVM_EXIT_MMIO up to userspace.
> 
> This patch fixes the issue by using the memslot generation number
> to validate the mmio cache.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
> The patch diff is rather large because I had to pull some code out
> of x86.h and mmu.c and into mmu.h. The main change is adding the
> memslot generation in vcpu_cach_mmio_info() and then validating
> that slot in vcpu_match_mmio_*().

Why not just move vcpu_cach_mmio_info() into mmu.c where is
the only place vcpu_cach_mmio_info is called. :)

BTW, i will post a patch to fix the generation-number issue
soon.

Re: [PATCH v3] kvm: x86: fix stale mmio cache bug

[David Matlack]

On Thu, Aug 7, 2014 at 6:36 PM, Xiao Guangrong
<xiaoguangrong@linux.vnet.ibm.com> wrote:
> On 08/08/2014 02:32 AM, David Matlack wrote:
>> The following events can lead to an incorrect KVM_EXIT_MMIO bubbling
>> up to userspace:
>>
>> (1) Guest accesses gpa X without a memory slot. The gfn is cached in
>> struct kvm_vcpu_arch (mmio_gfn). On Intel EPT-enabled hosts, KVM sets
>> the SPTE write-execute-noread so that future accesses cause
>> EPT_MISCONFIGs.
>>
>> (2) Host userspace creates a memory slot via KVM_SET_USER_MEMORY_REGION
>> covering the page just accessed.
>>
>> (3) Guest attempts to read or write to gpa X again. On Intel, this
>> generates an EPT_MISCONFIG. The memory slot generation number that
>> was incremented in (2) would normally take care of this but we fast
>> path mmio faults through quickly_check_mmio_pf(), which only checks
>> the per-vcpu mmio cache. Since we hit the cache, KVM passes a
>> KVM_EXIT_MMIO up to userspace.
>>
>> This patch fixes the issue by using the memslot generation number
>> to validate the mmio cache.
>>
>> Signed-off-by: David Matlack <dmatlack@google.com>
>> ---
>> The patch diff is rather large because I had to pull some code out
>> of x86.h and mmu.c and into mmu.h. The main change is adding the
>> memslot generation in vcpu_cach_mmio_info() and then validating
>> that slot in vcpu_match_mmio_*().
>
> Why not just move vcpu_cach_mmio_info() into mmu.c where is
> the only place vcpu_cach_mmio_info is called. :)

If only it was so simple :). vcpu_match_mmio_*() now needs
kvm_current_mmio_generation() which *was* in mmu.c.
vcpu_match_mmio_*() is called from x86.c and mmu.c so all that
has to go in a header. I decided to keep vcpu_cache_mmio_info()
with the  rest for organization's sake.

> BTW, i will post a patch to fix the generation-number issue
> soon.
>
>

Great! Thanks.