Re: [patch] kvm: make cr3 loading more robust

Avi Kivity
2007-01-03T08:29:16Z

Ingo Molnar wrote:
> From: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
> Subject: [patch] kvm: make cr3 loading more robust
>
> rmap_write_protect() has a BUG_ON() if a physical address is not found
> the the memslot. But this is a possible scenario if a buggy guest OS
> loads an invalid or corrupted cr3 value. So exit more gracefully.
>
>

I think a better solution is to detect the invalid cr3 in new_cr3().
That way the entire chain of error returns is avoided.

> [ Avi: this patch needs kvm_mmu_get_page() NULL return to not be ignored
> by mmu_alloc_roots()/nonpaging_init_context()/paging_new_cr3()/etc.
> and passed further down - i assume you are working on those
> issues already, or should i fix those too? ]
>

kvm_mmu_get_page() cannot return NULL (before this patch), as we ensure
there are free pages before entry to page_fault.

> Signed-off-by: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
>
> Index: linux/drivers/kvm/mmu.c
> ===================================================================
> --- linux.orig/drivers/kvm/mmu.c
> +++ linux/drivers/kvm/mmu.c
> @@ -378,7 +378,7 @@ static void rmap_remove(struct kvm_vcpu
> }
> }
>
> -static void rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
> +static int rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
> {
> struct kvm *kvm = vcpu->kvm;
> struct page *page;
> @@ -387,7 +387,8 @@ static void rmap_write_protect(struct kv
> u64 *spte;
>
> slot = gfn_to_memslot(kvm, gfn);
> - BUG_ON(!slot);
> + if (!slot)
> + return -1;
> page = gfn_to_page(slot, gfn);
>
> while (page->private) {
> @@ -407,6 +408,7 @@ static void rmap_write_protect(struct kv
> kvm_arch_ops->tlb_flush(vcpu);
> *spte &= ~(u64)PT_WRITABLE_MASK;
> }
> + return 0;
> }
>
> static int is_empty_shadow_page(hpa_t page_hpa)
> @@ -592,12 +594,17 @@ static struct kvm_mmu_page *kvm_mmu_get_
> page = kvm_mmu_alloc_page(vcpu, parent_pte);
> if (!page)
> return page;
> + if (!metaphysical) {
> + if (rmap_write_protect(vcpu, gfn) < 0) {
> + kvm_mmu_free_page(vcpu, page->page_hpa);
> + return NULL;
> + }
> + }
> pgprintk("%s: adding gfn %lx role %x\n", __FUNCTION__, gfn, role.word);
> page->gfn = gfn;
> page->role = role;
> hlist_add_head(&page->hash_link, bucket);
> - if (!metaphysical)
> - rmap_write_protect(vcpu, gfn);
> +
> return page;
> }