mmu audit update

PATCH In review Marcelo Tosatti 2 participants 23 messages Jun 2, 2009 – Jun 11, 2009

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2009-06-02 21:44 UTC #0 lore raw

cover mmu audit update

Some updates to the MMU audit code.

The third patch is "guessy" because I could not find the notrap spte documentation, all I can see is the page-fault error code mask and match fields in the VMCS, but can't see the link of that to sptes. Can someone point it out please?

Marcelo Tosatti

author

2009-06-02 21:44 UTC #1 lore raw

PATCH 1/4 KVM: MMU audit: update count_writable_mappings / count_rmaps

Under testing, count_writable_mappings returns a value that is 2 integers larger than what count_rmaps returns.

Suspicion is that either of the two functions is counting a duplicate (either positively or negatively).

Modifying check_writable_mappings_rmap to check for rmap existance on all present MMU pages fails to trigger an error, which should keep Avi happy.

Also introduce mmu_spte_walk to invoke a callback on all present sptes visible to the current vcpu, might be useful in the future.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +90 ~~-10~~

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3017,6 +3017,55 @@ static gva_t canonicalize(gva_t gva)
 	return gva;
 }
 
+
+typedef void (*inspect_spte_fn) (struct kvm *kvm, struct kvm_mmu_page *sp,
+				 u64 *sptep);
+
+static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    inspect_spte_fn fn)
+{
+	int i;
+
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		u64 ent = sp->spt[i];
+
+		if (is_shadow_present_pte(ent)) {
+			if (sp->role.level > 1) {
+				struct kvm_mmu_page *child;
+				child = page_header(ent & PT64_BASE_ADDR_MASK);
+				__mmu_spte_walk(kvm, child, fn);
+			}
+			if (sp->role.level == 1)
+				fn(kvm, sp, &sp->spt[i]);
+		}
+	}
+}
+
+static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
+{
+	int i;
+	struct kvm_mmu_page *sp;
+
+	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		return;
+	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
+		hpa_t root = vcpu->arch.mmu.root_hpa;
+		sp = page_header(root);
+		__mmu_spte_walk(vcpu->kvm, sp, fn);
+		return;
+	}
+	for (i = 0; i < 4; ++i) {
+		hpa_t root = vcpu->arch.mmu.pae_root[i];
+
+		if (root && VALID_PAGE(root)) {
+			root &= PT64_BASE_ADDR_MASK;
+			sp = page_header(root);
+			__mmu_spte_walk(vcpu->kvm, sp, fn);
+		}
+	}
+	return;
+}
+
 static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
 				gva_t va, int level)
 {

@@ -3109,9 +3158,43 @@ static int count_rmaps(struct kvm_vcpu *
 	return nmaps;
 }
 
-static int count_writable_mappings(struct kvm_vcpu *vcpu)
+void inspect_spte_has_rmap(struct kvm *kvm, struct kvm_mmu_page *sp, u64 *sptep)
+{
+	unsigned long *rmapp;
+	struct kvm_mmu_page *rev_sp;
+	gfn_t gfn;
+
+	if (*sptep & PT_WRITABLE_MASK) {
+		rev_sp = page_header(__pa(sptep));
+		gfn = rev_sp->gfns[sptep - rev_sp->spt];
+
+		if (!gfn_to_memslot(kvm, gfn)) {
+			printk(KERN_ERR "%s: no memslot for gfn %ld\n",
+					 audit_msg, gfn);
+			printk(KERN_ERR "%s: index %ld of sp (gfn=%lx)\n",
+					audit_msg, sptep - rev_sp->spt,
+					rev_sp->gfn);
+			dump_stack();
+			return;
+		}
+
+		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
+		if (!*rmapp) {
+			printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
+					 audit_msg, *sptep);
+			dump_stack();
+		}
+	}
+
+}
+
+void audit_writable_sptes_have_rmaps(struct kvm_vcpu *vcpu)
+{
+	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
+}
+
+static void check_writable_mappings_rmap(struct kvm_vcpu *vcpu)
 {
-	int nmaps = 0;
 	struct kvm_mmu_page *sp;
 	int i;

@@ -3128,20 +3211,16 @@ static int count_writable_mappings(struc
 				continue;
 			if (!(ent & PT_WRITABLE_MASK))
 				continue;
-			++nmaps;
+			inspect_spte_has_rmap(vcpu->kvm, sp, &pt[i]);
 		}
 	}
-	return nmaps;
+	return;
 }
 
 static void audit_rmap(struct kvm_vcpu *vcpu)
 {
-	int n_rmap = count_rmaps(vcpu);
-	int n_actual = count_writable_mappings(vcpu);
-
-	if (n_rmap != n_actual)
-		printk(KERN_ERR "%s: (%s) rmap %d actual %d\n",
-		       __func__, audit_msg, n_rmap, n_actual);
+	check_writable_mappings_rmap(vcpu);
+	count_rmaps(vcpu);
 }
 
 static void audit_write_protection(struct kvm_vcpu *vcpu)

@@ -3175,6 +3254,7 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
 	audit_mappings(vcpu);
+	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }

Avi Kivity

2009-06-08 09:24 UTC #2 lore raw

Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps

Quoted 28 lines · Marcelo Tosatti

Under testing, count_writable_mappings returns a value that is 2 integers larger than what count_rmaps returns.
Suspicion is that either of the two functions is counting a duplicate (either positively or negatively).
Modifying check_writable_mappings_rmap to check for rmap existance on all present MMU pages fails to trigger an error, which should keep Avi happy.
Also introduce mmu_spte_walk to invoke a callback on all present sptes visible to the current vcpu, might be useful in the future.
Signed-off-by Marcelo Tosatti <[email protected]>
Index: kvm/arch/x86/kvm/mmu.c
===================================================================
arch/x86/kvm/mmu.c +5 -0
@@ -3017,6 +3017,55 @@ static gva_t canonicalize(gva_t gva)
 	return gva;
 }
 
+
+typedef void (*inspect_spte_fn) (struct kvm *kvm, struct kvm_mmu_page *sp,
+				 u64 *sptep);
+
+static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,

arch/x86/kvm/mmu.c static gva_t canonicalize(gva_t gva) · line 3034

Quoted 10 lines

arch/x86/kvm/mmu.c +9 -0

+			    inspect_spte_fn fn)
+{
+	int i;
+
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		u64 ent = sp->spt[i];
+
+		if (is_shadow_present_pte(ent)) {
+			if (sp->role.level > 1) {

I think this is broken wrt large pages. We should recurse if role.level > 1 or the G bit is set.

Quoted 12 lines

arch/x86/kvm/mmu.c +12 -0

+	if (*sptep & PT_WRITABLE_MASK) {
+		rev_sp = page_header(__pa(sptep));
+		gfn = rev_sp->gfns[sptep - rev_sp->spt];
+
+		if (!gfn_to_memslot(kvm, gfn)) {
+			printk(KERN_ERR "%s: no memslot for gfn %ld\n",
+					 audit_msg, gfn);
+			printk(KERN_ERR "%s: index %ld of sp (gfn=%lx)\n",
+					audit_msg, sptep - rev_sp->spt,
+					rev_sp->gfn);
+			dump_stack();
+			return;

arch/x86/kvm/mmu.c static int count_rmaps(struct kvm_vcpu * · line 3200

Quoted 10 lines

arch/x86/kvm/mmu.c +9 -0

+		}
+
+		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
+		if (!*rmapp) {
+			printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
+					 audit_msg, *sptep);
+			dump_stack();
+		}
+	}

Semi-related: we should set up a new exit code to halt the VM so it can be inspected, otherwise all those printks and dump_stack()s will quickly overwhelm the logging facilities.

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

author

2009-06-09 12:34 UTC #3 lore raw

Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps

Quoted 5 lines · Avi Kivity

Quoted 5 lines

arch/x86/kvm/mmu.c +5 -0

+static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    inspect_spte_fn fn)
+{
+	int i;
+

Quoted 9 lines

arch/x86/kvm/mmu.c static gva_t canonicalize(gva_t gva) · line 3033
Quoted 6 lines
arch/x86/kvm/mmu.c +5 -0
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		u64 ent = sp->spt[i];
+
+		if (is_shadow_present_pte(ent)) {
+			if (sp->role.level > 1) {
I think this is broken wrt large pages. We should recurse if role.level
Quoted 1 line
1 or the G bit is set.

Yes, fixed. Plan to add largepages validity checks later.

Quoted 3 lines

Semi-related: we should set up a new exit code to halt the VM so it can be inspected, otherwise all those printks and dump_stack()s will quickly overwhelm the logging facilities.

Can you clarify on the halt exit code?

Because for other exit codes which similar behaviour is wanted, say, unhandled vm exit, the policy can be handled in userspace (and the decision to halt or not seems better suited to happen there). So perhaps KVM_EXIT_MMU_AUDIT_FAILED?

I wondered before whether it would be good to stop auditing on the first error, but gave up on the idea.

Avi Kivity

2009-06-09 12:40 UTC #4 lore raw

Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps

Quoted 5 lines · Marcelo Tosatti

Quoted 2 lines
Semi-related: we should set up a new exit code to halt the VM so it can be inspected, otherwise all those printks and dump_stack()s will quickly overwhelm the logging facilities.
Can you clarify on the halt exit code?

set a bit that tells KVM_RUN to quit (like KVM_EXIT_UNKNOWN), when userspace sees it it vm_stop()s, waiting for someone to dig in.

Clean logs, clean state.

Quoted 5 lines

Because for other exit codes which similar behaviour is wanted, say, unhandled vm exit, the policy can be handled in userspace (and the decision to halt or not seems better suited to happen there). So perhaps KVM_EXIT_MMU_AUDIT_FAILED?

Yes, the name was bad. We can do KVM_EXIT_INTERNAL_ERROR and have a maintainer_name field in the union to choose who will handle it.

Quoted 3 lines

I wondered before whether it would be good to stop auditing on the first error, but gave up on the idea.

It's harder without exceptions.

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

author

2009-06-02 21:44 UTC #5 lore raw

PATCH 2/4 KVM: MMU audit: update audit_write_protection

In reply to Marcelo Tosatti

- Unsync pages contain writable sptes in the rmap.
- rmaps do not exclusively contain writable sptes anymore.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +11 -3

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3228,20 +3228,28 @@ static void audit_write_protection(struc
 	struct kvm_mmu_page *sp;
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;
+	u64 *spte;
 	gfn_t gfn;
 
 	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
 		if (sp->role.direct)
 			continue;
+		if (sp->unsync)
+			continue;
 
 		gfn = unalias_gfn(vcpu->kvm, sp->gfn);
 		slot = gfn_to_memslot_unaliased(vcpu->kvm, sp->gfn);
 		rmapp = &slot->rmap[gfn - slot->base_gfn];
-		if (*rmapp)
-			printk(KERN_ERR "%s: (%s) shadow page has writable"
-			       " mappings: gfn %lx role %x\n",
+
+		spte = rmap_next(vcpu->kvm, rmapp, NULL);
+		while (spte) {
+			if (*spte & PT_WRITABLE_MASK)
+				printk(KERN_ERR "%s: (%s) shadow page has "
+				"writable mappings: gfn %lx role %x\n",
 			       __func__, audit_msg, sp->gfn,
 			       sp->role.word);
+			spte = rmap_next(vcpu->kvm, rmapp, spte);
+		}
 	}
 }

Marcelo Tosatti

author

2009-06-02 21:44 UTC #6 lore raw

PATCH 3/4 KVM: MMU audit: nontrapping ptes in nonleaf level

In reply to Marcelo Tosatti

update_pte sets nonleaf sptes as notrap, so either it or the audit code is broken.

Choose the audit code.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +1 -6

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3081,12 +3081,7 @@ static void audit_mappings_page(struct k
 
 		va = canonicalize(va);
 		if (level > 1) {
-			if (ent == shadow_notrap_nonpresent_pte)
-				printk(KERN_ERR "audit: (%s) nontrapping pte"
-				       " in nonleaf level: levels %d gva %lx"
-				       " level %d pte %llx\n", audit_msg,
-				       vcpu->arch.mmu.root_level, va, level, ent);
-			else
+			if (is_shadow_present_pte(ent))
 				audit_mappings_page(vcpu, ent, va, level - 1);
 		} else {
 			gpa_t gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, va);

Marcelo Tosatti

author

2009-06-02 21:44 UTC #7 lore raw

PATCH 4/4 KVM: MMU audit: audit_mappings tweaks

In reply to Marcelo Tosatti

- Fail early in case gfn_to_pfn returns is_error_pfn.
- For the pre pte write case, avoid spurious "gva is valid but spte is notrap" messages (the emulation code does the guest write first, so this particular case is OK).

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +7 -1

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3089,6 +3089,11 @@ static void audit_mappings_page(struct k
 			pfn_t pfn = gfn_to_pfn(vcpu->kvm, gfn);
 			hpa_t hpa = (hpa_t)pfn << PAGE_SHIFT;
 
+			if (is_error_pfn(pfn)) {
+				kvm_release_pfn_clean(pfn);
+				continue;
+			}
+
 			if (is_shadow_present_pte(ent)
 			    && (ent & PT64_BASE_ADDR_MASK) != hpa)
 				printk(KERN_ERR "xx audit error: (%s) levels %d"

@@ -3256,7 +3261,8 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_msg = msg;
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
-	audit_mappings(vcpu);
+	if (strcmp("pre pte write", audit_msg))
+		audit_mappings(vcpu);
 	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }

Avi Kivity

2009-06-08 09:29 UTC #8 lore raw

Re: [patch 4/4] KVM: MMU audit: audit_mappings tweaks

arch/x86/kvm/mmu.c static void kvm_mmu_audit(struct kvm_vcp · line 3268

Quoted 11 lines · Marcelo Tosatti

arch/x86/kvm/mmu.c +2 -1

@@ -3256,7 +3261,8 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_msg = msg;
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
-	audit_mappings(vcpu);
+	if (strcmp("pre pte write", audit_msg))
+		audit_mappings(vcpu);
 	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }

strcmp() doesn't return a truth value, please add != 0.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2009-06-07 07:14 UTC #9 lore raw

In reply to Marcelo Tosatti

Quoted 7 lines · Marcelo Tosatti

Some updates to the MMU audit code.
The third patch is "guessy" because I could not find the notrap spte documentation, all I can see is the page-fault error code mask and match fields in the VMCS, but can't see the link of that to sptes. Can someone point it out please?

When bypass_guest_pf is set, we tell vmx not to trap if the fault is due to page-not-present. So if we know gpte.p == 0, we set spte.p = 0 and allow not-present page faults to go directly to the guest without trapping.

Of course, we still need to trap cases where gpte.p = 1 but we haven't mapped the page yet. So we set a reserved bit in the spte and trap on that.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Marcelo Tosatti

author

2009-06-10 15:31 UTC #10 lore raw

cover mmu audit update v4

Addressing comments, introducing a new helper, handling largepages.

Marcelo Tosatti

author

2009-06-10 15:31 UTC #11 lore raw

PATCH 1/6 KVM: MMU: introduce is_last_spte helper

Hiding some of the last largepage / level interaction (which is useful for gbpages and for zero based levels).

Also merge the PT_PAGE_TABLE_LEVEL clearing loop in unlink_children.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +13 ~~-13~~

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -250,6 +250,15 @@ static int is_rmap_spte(u64 pte)
 	return is_shadow_present_pte(pte);
 }
 
+static int is_last_spte(u64 pte, int level)
+{
+	if (level == PT_PAGE_TABLE_LEVEL)
+		return 1;
+	if (level == PT_DIRECTORY_LEVEL && is_large_pte(pte))
+		return 1;
+	return 0;
+}
+
 static pfn_t spte_to_pfn(u64 pte)
 {
 	return (pte & PT64_BASE_ADDR_MASK) >> PAGE_SHIFT;

@@ -1293,25 +1302,17 @@ static void kvm_mmu_page_unlink_children
 
 	pt = sp->spt;
 
-	if (sp->role.level == PT_PAGE_TABLE_LEVEL) {
-		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
-			if (is_shadow_present_pte(pt[i]))
-				rmap_remove(kvm, &pt[i]);
-			pt[i] = shadow_trap_nonpresent_pte;
-		}
-		return;
-	}
-
 	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
 		ent = pt[i];
 
 		if (is_shadow_present_pte(ent)) {
-			if (!is_large_pte(ent)) {
+			if (!is_last_spte(ent, sp->role.level)) {
 				ent &= PT64_BASE_ADDR_MASK;
 				mmu_page_remove_parent_pte(page_header(ent),
 							   &pt[i]);
 			} else {
-				--kvm->stat.lpages;
+				if (is_large_pte(ent))
+					--kvm->stat.lpages;
 				rmap_remove(kvm, &pt[i]);
 			}
 		}

@@ -2357,8 +2358,7 @@ static void mmu_pte_write_zap_pte(struct
 
 	pte = *spte;
 	if (is_shadow_present_pte(pte)) {
-		if (sp->role.level == PT_PAGE_TABLE_LEVEL ||
-		    is_large_pte(pte))
+		if (is_last_spte(pte, sp->role.level))
 			rmap_remove(vcpu->kvm, spte);
 		else {
 			child = page_header(pte & PT64_BASE_ADDR_MASK);

Marcelo Tosatti

author

2009-06-10 15:31 UTC #12 lore raw

PATCH 2/6 KVM: MMU audit: update count_writable_mappings / count_rmaps

In reply to Marcelo Tosatti

Under testing, count_writable_mappings returns a value that is 2 integers larger than what count_rmaps returns.

Suspicion is that either of the two functions is counting a duplicate (either positively or negatively).

Modifying check_writable_mappings_rmap to check for rmap existance on all present MMU pages fails to trigger an error, which should keep Avi happy.

Also introduce mmu_spte_walk to invoke a callback on all present sptes visible to the current vcpu, might be useful in the future.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +94 ~~-10~~

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3020,6 +3020,55 @@ static gva_t canonicalize(gva_t gva)
 	return gva;
 }
 
+
+typedef void (*inspect_spte_fn) (struct kvm *kvm, struct kvm_mmu_page *sp,
+				 u64 *sptep);
+
+static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    inspect_spte_fn fn)
+{
+	int i;
+
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		u64 ent = sp->spt[i];
+
+		if (is_shadow_present_pte(ent)) {
+			if (sp->role.level > 1 && !is_large_pte(ent)) {
+				struct kvm_mmu_page *child;
+				child = page_header(ent & PT64_BASE_ADDR_MASK);
+				__mmu_spte_walk(kvm, child, fn);
+			}
+			if (sp->role.level == 1)
+				fn(kvm, sp, &sp->spt[i]);
+		}
+	}
+}
+
+static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
+{
+	int i;
+	struct kvm_mmu_page *sp;
+
+	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		return;
+	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
+		hpa_t root = vcpu->arch.mmu.root_hpa;
+		sp = page_header(root);
+		__mmu_spte_walk(vcpu->kvm, sp, fn);
+		return;
+	}
+	for (i = 0; i < 4; ++i) {
+		hpa_t root = vcpu->arch.mmu.pae_root[i];
+
+		if (root && VALID_PAGE(root)) {
+			root &= PT64_BASE_ADDR_MASK;
+			sp = page_header(root);
+			__mmu_spte_walk(vcpu->kvm, sp, fn);
+		}
+	}
+	return;
+}
+
 static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
 				gva_t va, int level)
 {

@@ -3112,9 +3161,47 @@ static int count_rmaps(struct kvm_vcpu *
 	return nmaps;
 }
 
-static int count_writable_mappings(struct kvm_vcpu *vcpu)
+void inspect_spte_has_rmap(struct kvm *kvm, struct kvm_mmu_page *sp, u64 *sptep)
+{
+	unsigned long *rmapp;
+	struct kvm_mmu_page *rev_sp;
+	gfn_t gfn;
+
+	if (*sptep & PT_WRITABLE_MASK) {
+		rev_sp = page_header(__pa(sptep));
+		gfn = rev_sp->gfns[sptep - rev_sp->spt];
+
+		if (!gfn_to_memslot(kvm, gfn)) {
+			if (!printk_ratelimit())
+				return;
+			printk(KERN_ERR "%s: no memslot for gfn %ld\n",
+					 audit_msg, gfn);
+			printk(KERN_ERR "%s: index %ld of sp (gfn=%lx)\n",
+					audit_msg, sptep - rev_sp->spt,
+					rev_sp->gfn);
+			dump_stack();
+			return;
+		}
+
+		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
+		if (!*rmapp) {
+			if (!printk_ratelimit())
+				return;
+			printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
+					 audit_msg, *sptep);
+			dump_stack();
+		}
+	}
+
+}
+
+void audit_writable_sptes_have_rmaps(struct kvm_vcpu *vcpu)
+{
+	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
+}
+
+static void check_writable_mappings_rmap(struct kvm_vcpu *vcpu)
 {
-	int nmaps = 0;
 	struct kvm_mmu_page *sp;
 	int i;

@@ -3131,20 +3218,16 @@ static int count_writable_mappings(struc
 				continue;
 			if (!(ent & PT_WRITABLE_MASK))
 				continue;
-			++nmaps;
+			inspect_spte_has_rmap(vcpu->kvm, sp, &pt[i]);
 		}
 	}
-	return nmaps;
+	return;
 }
 
 static void audit_rmap(struct kvm_vcpu *vcpu)
 {
-	int n_rmap = count_rmaps(vcpu);
-	int n_actual = count_writable_mappings(vcpu);
-
-	if (n_rmap != n_actual)
-		printk(KERN_ERR "%s: (%s) rmap %d actual %d\n",
-		       __func__, audit_msg, n_rmap, n_actual);
+	check_writable_mappings_rmap(vcpu);
+	count_rmaps(vcpu);
 }
 
 static void audit_write_protection(struct kvm_vcpu *vcpu)

@@ -3178,6 +3261,7 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
 	audit_mappings(vcpu);
+	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }

Marcelo Tosatti

author

2009-06-10 15:31 UTC #13 lore raw

PATCH 3/6 KVM: MMU audit: update audit_write_protection

In reply to Marcelo Tosatti

- Unsync pages contain writable sptes in the rmap.
- rmaps do not exclusively contain writable sptes anymore.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +11 -3

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3235,20 +3235,28 @@ static void audit_write_protection(struc
 	struct kvm_mmu_page *sp;
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;
+	u64 *spte;
 	gfn_t gfn;
 
 	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
 		if (sp->role.direct)
 			continue;
+		if (sp->unsync)
+			continue;
 
 		gfn = unalias_gfn(vcpu->kvm, sp->gfn);
 		slot = gfn_to_memslot_unaliased(vcpu->kvm, sp->gfn);
 		rmapp = &slot->rmap[gfn - slot->base_gfn];
-		if (*rmapp)
-			printk(KERN_ERR "%s: (%s) shadow page has writable"
-			       " mappings: gfn %lx role %x\n",
+
+		spte = rmap_next(vcpu->kvm, rmapp, NULL);
+		while (spte) {
+			if (*spte & PT_WRITABLE_MASK)
+				printk(KERN_ERR "%s: (%s) shadow page has "
+				"writable mappings: gfn %lx role %x\n",
 			       __func__, audit_msg, sp->gfn,
 			       sp->role.word);
+			spte = rmap_next(vcpu->kvm, rmapp, spte);
+		}
 	}
 }

Marcelo Tosatti

author

2009-06-10 15:31 UTC #14 lore raw

PATCH 4/6 KVM: MMU audit: nontrapping ptes in nonleaf level

In reply to Marcelo Tosatti

It is valid to set non leaf sptes as notrap.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +1 -6

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3084,12 +3084,7 @@ static void audit_mappings_page(struct k
 
 		va = canonicalize(va);
 		if (level > 1) {
-			if (ent == shadow_notrap_nonpresent_pte)
-				printk(KERN_ERR "audit: (%s) nontrapping pte"
-				       " in nonleaf level: levels %d gva %lx"
-				       " level %d pte %llx\n", audit_msg,
-				       vcpu->arch.mmu.root_level, va, level, ent);
-			else
+			if (is_shadow_present_pte(ent))
 				audit_mappings_page(vcpu, ent, va, level - 1);
 		} else {
 			gpa_t gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, va);

Marcelo Tosatti

author

2009-06-10 15:31 UTC #15 lore raw

PATCH 5/6 KVM: MMU audit: audit_mappings tweaks

In reply to Marcelo Tosatti

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +7 -1

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3092,6 +3092,11 @@ static void audit_mappings_page(struct k
 			pfn_t pfn = gfn_to_pfn(vcpu->kvm, gfn);
 			hpa_t hpa = (hpa_t)pfn << PAGE_SHIFT;
 
+			if (is_error_pfn(pfn)) {
+				kvm_release_pfn_clean(pfn);
+				continue;
+			}
+
 			if (is_shadow_present_pte(ent)
 			    && (ent & PT64_BASE_ADDR_MASK) != hpa)
 				printk(KERN_ERR "xx audit error: (%s) levels %d"

@@ -3263,7 +3268,8 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_msg = msg;
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
-	audit_mappings(vcpu);
+	if (strcmp("pre pte write", audit_msg) != 0)
+		audit_mappings(vcpu);
 	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }

Marcelo Tosatti

author

2009-06-10 15:31 UTC #16 lore raw

PATCH 6/6 KVM: MMU audit: largepage handling

In reply to Marcelo Tosatti

Make the audit code aware of largepages.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +7 -8

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3033,12 +3033,11 @@ static void __mmu_spte_walk(struct kvm *
 		u64 ent = sp->spt[i];
 
 		if (is_shadow_present_pte(ent)) {
-			if (sp->role.level > 1 && !is_large_pte(ent)) {
+			if (!is_last_spte(ent, sp->role.level)) {
 				struct kvm_mmu_page *child;
 				child = page_header(ent & PT64_BASE_ADDR_MASK);
 				__mmu_spte_walk(kvm, child, fn);
-			}
-			if (sp->role.level == 1)
+			} else
 				fn(kvm, sp, &sp->spt[i]);
 		}
 	}

@@ -3083,10 +3082,9 @@ static void audit_mappings_page(struct k
 			continue;
 
 		va = canonicalize(va);
-		if (level > 1) {
-			if (is_shadow_present_pte(ent))
-				audit_mappings_page(vcpu, ent, va, level - 1);
-		} else {
+		if (is_shadow_present_pte(ent) && !is_last_spte(ent, level))
+			audit_mappings_page(vcpu, ent, va, level - 1);
+		else {
 			gpa_t gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, va);
 			gfn_t gfn = gpa >> PAGE_SHIFT;
 			pfn_t pfn = gfn_to_pfn(vcpu->kvm, gfn);

@@ -3183,7 +3181,8 @@ void inspect_spte_has_rmap(struct kvm *k
 			return;
 		}
 
-		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
+		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt],
+				    is_large_pte(*sptep));
 		if (!*rmapp) {
 			if (!printk_ratelimit())
 				return;

Avi Kivity

Applied

2009-06-11 14:24 UTC #17 lore raw

Re: [patch 0/6] mmu audit update v4

In reply to Marcelo Tosatti

Quoted 3 lines · Marcelo Tosatti

Addressing comments, introducing a new helper, handling largepages.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

author

2009-06-09 13:15 UTC #18 lore raw

cover mmu audit update v2

In reply to Marcelo Tosatti

Addressing comments.

Marcelo Tosatti

author

2009-06-09 13:15 UTC #19 lore raw

PATCH 1/4 KVM: MMU audit: update count_writable_mappings / count_rmaps

Under testing, count_writable_mappings returns a value that is 2 integers larger than what count_rmaps returns.

Suspicion is that either of the two functions is counting a duplicate (either positively or negatively).

Modifying check_writable_mappings_rmap to check for rmap existance on all present MMU pages fails to trigger an error, which should keep Avi happy.

Also introduce mmu_spte_walk to invoke a callback on all present sptes visible to the current vcpu, might be useful in the future.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +90 ~~-10~~

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3020,6 +3020,55 @@ static gva_t canonicalize(gva_t gva)
 	return gva;
 }
 
+
+typedef void (*inspect_spte_fn) (struct kvm *kvm, struct kvm_mmu_page *sp,
+				 u64 *sptep);
+
+static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    inspect_spte_fn fn)
+{
+	int i;
+
+	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
+		u64 ent = sp->spt[i];
+
+		if (is_shadow_present_pte(ent)) {
+			if (sp->role.level > 1 && !is_large_pte(ent)) {
+				struct kvm_mmu_page *child;
+				child = page_header(ent & PT64_BASE_ADDR_MASK);
+				__mmu_spte_walk(kvm, child, fn);
+			}
+			if (sp->role.level == 1)
+				fn(kvm, sp, &sp->spt[i]);
+		}
+	}
+}
+
+static void mmu_spte_walk(struct kvm_vcpu *vcpu, inspect_spte_fn fn)
+{
+	int i;
+	struct kvm_mmu_page *sp;
+
+	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		return;
+	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
+		hpa_t root = vcpu->arch.mmu.root_hpa;
+		sp = page_header(root);
+		__mmu_spte_walk(vcpu->kvm, sp, fn);
+		return;
+	}
+	for (i = 0; i < 4; ++i) {
+		hpa_t root = vcpu->arch.mmu.pae_root[i];
+
+		if (root && VALID_PAGE(root)) {
+			root &= PT64_BASE_ADDR_MASK;
+			sp = page_header(root);
+			__mmu_spte_walk(vcpu->kvm, sp, fn);
+		}
+	}
+	return;
+}
+
 static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
 				gva_t va, int level)
 {

@@ -3112,9 +3161,43 @@ static int count_rmaps(struct kvm_vcpu *
 	return nmaps;
 }
 
-static int count_writable_mappings(struct kvm_vcpu *vcpu)
+void inspect_spte_has_rmap(struct kvm *kvm, struct kvm_mmu_page *sp, u64 *sptep)
+{
+	unsigned long *rmapp;
+	struct kvm_mmu_page *rev_sp;
+	gfn_t gfn;
+
+	if (*sptep & PT_WRITABLE_MASK) {
+		rev_sp = page_header(__pa(sptep));
+		gfn = rev_sp->gfns[sptep - rev_sp->spt];
+
+		if (!gfn_to_memslot(kvm, gfn)) {
+			printk(KERN_ERR "%s: no memslot for gfn %ld\n",
+					 audit_msg, gfn);
+			printk(KERN_ERR "%s: index %ld of sp (gfn=%lx)\n",
+					audit_msg, sptep - rev_sp->spt,
+					rev_sp->gfn);
+			dump_stack();
+			return;
+		}
+
+		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
+		if (!*rmapp) {
+			printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
+					 audit_msg, *sptep);
+			dump_stack();
+		}
+	}
+
+}
+
+void audit_writable_sptes_have_rmaps(struct kvm_vcpu *vcpu)
+{
+	mmu_spte_walk(vcpu, inspect_spte_has_rmap);
+}
+
+static void check_writable_mappings_rmap(struct kvm_vcpu *vcpu)
 {
-	int nmaps = 0;
 	struct kvm_mmu_page *sp;
 	int i;

@@ -3131,20 +3214,16 @@ static int count_writable_mappings(struc
 				continue;
 			if (!(ent & PT_WRITABLE_MASK))
 				continue;
-			++nmaps;
+			inspect_spte_has_rmap(vcpu->kvm, sp, &pt[i]);
 		}
 	}
-	return nmaps;
+	return;
 }
 
 static void audit_rmap(struct kvm_vcpu *vcpu)
 {
-	int n_rmap = count_rmaps(vcpu);
-	int n_actual = count_writable_mappings(vcpu);
-
-	if (n_rmap != n_actual)
-		printk(KERN_ERR "%s: (%s) rmap %d actual %d\n",
-		       __func__, audit_msg, n_rmap, n_actual);
+	check_writable_mappings_rmap(vcpu);
+	count_rmaps(vcpu);
 }
 
 static void audit_write_protection(struct kvm_vcpu *vcpu)

@@ -3178,6 +3257,7 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
 	audit_mappings(vcpu);
+	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }

Marcelo Tosatti

author

2009-06-09 13:15 UTC #20 lore raw

PATCH 2/4 KVM: MMU audit: update audit_write_protection

In reply to Marcelo Tosatti

- Unsync pages contain writable sptes in the rmap.
- rmaps do not exclusively contain writable sptes anymore.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +11 -3

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3231,20 +3231,28 @@ static void audit_write_protection(struc
 	struct kvm_mmu_page *sp;
 	struct kvm_memory_slot *slot;
 	unsigned long *rmapp;
+	u64 *spte;
 	gfn_t gfn;
 
 	list_for_each_entry(sp, &vcpu->kvm->arch.active_mmu_pages, link) {
 		if (sp->role.direct)
 			continue;
+		if (sp->unsync)
+			continue;
 
 		gfn = unalias_gfn(vcpu->kvm, sp->gfn);
 		slot = gfn_to_memslot_unaliased(vcpu->kvm, sp->gfn);
 		rmapp = &slot->rmap[gfn - slot->base_gfn];
-		if (*rmapp)
-			printk(KERN_ERR "%s: (%s) shadow page has writable"
-			       " mappings: gfn %lx role %x\n",
+
+		spte = rmap_next(vcpu->kvm, rmapp, NULL);
+		while (spte) {
+			if (*spte & PT_WRITABLE_MASK)
+				printk(KERN_ERR "%s: (%s) shadow page has "
+				"writable mappings: gfn %lx role %x\n",
 			       __func__, audit_msg, sp->gfn,
 			       sp->role.word);
+			spte = rmap_next(vcpu->kvm, rmapp, spte);
+		}
 	}
 }

Marcelo Tosatti

author

2009-06-09 13:15 UTC #21 lore raw

PATCH 3/4 KVM: MMU audit: nontrapping ptes in nonleaf level

In reply to Marcelo Tosatti

update_pte sets nonleaf sptes as notrap, so either it or the audit code is broken.

Choose the audit code.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +1 -6

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3084,12 +3084,7 @@ static void audit_mappings_page(struct k
 
 		va = canonicalize(va);
 		if (level > 1) {
-			if (ent == shadow_notrap_nonpresent_pte)
-				printk(KERN_ERR "audit: (%s) nontrapping pte"
-				       " in nonleaf level: levels %d gva %lx"
-				       " level %d pte %llx\n", audit_msg,
-				       vcpu->arch.mmu.root_level, va, level, ent);
-			else
+			if (is_shadow_present_pte(ent))
 				audit_mappings_page(vcpu, ent, va, level - 1);
 		} else {
 			gpa_t gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, va);

Marcelo Tosatti

author

2009-06-09 13:15 UTC #22 lore raw

PATCH 4/4 KVM: MMU audit: audit_mappings tweaks

In reply to Marcelo Tosatti

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/mmu.c +7 -1

Index: kvm/arch/x86/kvm/mmu.c
===================================================================
@@ -3092,6 +3092,11 @@ static void audit_mappings_page(struct k
 			pfn_t pfn = gfn_to_pfn(vcpu->kvm, gfn);
 			hpa_t hpa = (hpa_t)pfn << PAGE_SHIFT;
 
+			if (is_error_pfn(pfn)) {
+				kvm_release_pfn_clean(pfn);
+				continue;
+			}
+
 			if (is_shadow_present_pte(ent)
 			    && (ent & PT64_BASE_ADDR_MASK) != hpa)
 				printk(KERN_ERR "xx audit error: (%s) levels %d"

@@ -3259,7 +3264,8 @@ static void kvm_mmu_audit(struct kvm_vcp
 	audit_msg = msg;
 	audit_rmap(vcpu);
 	audit_write_protection(vcpu);
-	audit_mappings(vcpu);
+	if (strcmp("pre pte write", audit_msg) != 0)
+		audit_mappings(vcpu);
 	audit_writable_sptes_have_rmaps(vcpu);
 	dbg = olddbg;
 }