kvm: make cr3 loading more robust

From: Ingo Molnar <[email protected]>
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.

[ 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? ]

Signed-off-by Ingo Molnar <[email protected]>

Index: linux/drivers/kvm/mmu.c
===================================================================

drivers/kvm/mmu.c +11 -4

@@ -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;
 }
 

Quoted 4 lines

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.

Quoted 8 lines · Avi Kivity

Quoted 5 lines

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.

i'm wondering what the right semantics would be though. Kill the VM context?

on a real CPU an invalid cr3 does nothing explicitly (besides being a sure way to get a triple fault) - physical memory is always accessible, non-present physical memory at most generates an #MCE, but is typically just returning 0xff, right? So perhaps keep a non-mapped page filled with 0xff and map non-existent RAM to that? But then this 'RAM' needs to be made non-writable.

Looks a bit messy. Cleanest would be to kill the
VM context (without injecting any fault into the guest), but that would then require an error return chain from set_cr3() ...

Quoted 15 lines · Ingo Molnar

Quoted 8 lines · Avi Kivity

Quoted 5 lines

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.

i'm wondering what the right semantics would be though. Kill the VM context?

Quoted 6 lines

on a real CPU an invalid cr3 does nothing explicitly (besides being a sure way to get a triple fault) - physical memory is always accessible, non-present physical memory at most generates an #MCE, but is typically just returning 0xff, right? So perhaps keep a non-mapped page filled with 0xff and map non-existent RAM to that? But then this 'RAM' needs to be made non-writable.

That's a good solution. I don't see why it has to be made non-writable -- it has undefined content, and any old value will do. We have (or maybe had) something like that somewhere.

Quoted 15 lines · Ingo Molnar

Quoted 8 lines · Avi Kivity

Quoted 5 lines

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.

i'm wondering what the right semantics would be though. Kill the VM context?

Quoted 6 lines

on a real CPU an invalid cr3 does nothing explicitly (besides being a sure way to get a triple fault) - physical memory is always accessible, non-present physical memory at most generates an #MCE, but is typically just returning 0xff, right? So perhaps keep a non-mapped page filled with 0xff and map non-existent RAM to that? But then this 'RAM' needs to be made non-writable.

That's a good solution. I don't see why it has to be made non-writable -- it has undefined content, and any old value will do. We have (or maybe had) something like that somewhere.

Quoted 3 lines

Looks a bit messy. Cleanest would be to kill the VM context (without injecting any fault into the guest), but that would then require an error return chain from set_cr3() ...

set_cr3() is fairly close to the top. I'm mostly worried about keeping the innards of the mmu clean.

An alternative is to add a flag to the vcpu which would be examined on entry (vcpu->triple_faulted).

Quoted 4 lines

That's a good solution. I don't see why it has to be made non-writable -- it has undefined content, and any old value will do. We have (or maybe had) something like that somewhere.

it should always return 0xff content because that's how real hardware behaves. It's essentially ROM-alike, with 0xff content. Writes are ignored. (last i checked)

also having it writable means it's an information leak as we want to share this page amongst guests, etc. Then explicitly faulting the guest would be alot cleaner.

Quoted 4 lines

An alternative is to add a flag to the vcpu which would be examined on entry (vcpu->triple_faulted).

well, the triple fault isnt really explicit behavior of the cr3 loading, it is "just" a side-effect of having an all-0xff piece of physical memory holding the CPU's page tables. Such a cr3 can be loaded fine, but the next instruction fetched will be 0xff 0xff, which should be an undefined opcode. The resulting fault will try to execute based off an invalid IDT so we get a double fault, which then also tries to execute 0xff 0xff (if the IDT entry didnt generate a #GPF beforehand, due to an invalid segment descriptor) so it results in a triple fault.

Does VMX
report triple faults?

Note that the corner cases will never be 100% emulatable. For example, you can set cr3 to point at your IDE DMA mmio space or something like that. It's quite all right to kill the guest quietly at that point, as no real-life guest will do that.

The kvm goals do not include cycle accurate emulation. [...]

Quoted 1 line

The kvm goals do not include cycle accurate emulation. [...]

yes. That's why i'm suggesting to kill the VM in such a scenario. A cr3 value is only valid if it points to real RAM.

Quoted 2 lines · Ingo Molnar

Quoted 2 lines

The kvm goals do not include cycle accurate emulation. [...]

Quoted 3 lines

yes. That's why i'm suggesting to kill the VM in such a scenario. A cr3 value is only valid if it points to real RAM.

and if it ever causes real incompatibility it should be easy to address it by returning a per-vcpu (or per-vm) special memory slot in gfn_to_memslot(), for invalid memory, instead of the current NULL, right?

Yes.

A read-only memory slot may have other uses too (for ROMs).

Quoted 2 lines · Avi Kivity

Yes.

Quoted 2 lines

A read-only memory slot may have other uses too (for ROMs).

a related question: i was wondering about the purpose of the KVM_MEM_LOG_DIRTY_PAGES feature that is attached to physical memory slots. Is that used so that you can get a compact bitmap of all modified pages within a Qemu-emulated framebuffer, resulting in a delta update, instead of having to do the full framebuffer update every time?

Quoted 5 lines · Avi Kivity

Note that the corner cases will never be 100% emulatable. For example, you can set cr3 to point at your IDE DMA mmio space or something like that. It's quite all right to kill the guest quietly at that point, as no real-life guest will do that.

yes. Or to map the lapic to the IDT ;-) (as yours truly has tried it years ago)

that's why my suggestion is to just kill the guest. Loading such a cr3 is a serious bug that might be hard to debug in the guest. I had to debug at least one such bug in Linux before (years ago, in the lazy TLB switching code) and it was a royal PITA to track down. Having a hypervisor that points any cr3 load error out /before/ the effects of the error propagate further is a bonus, not an incompatibility. The CPU does not implement this not because the semantics is important, but i suspect mostly because it doesnt really know the boundaries and type of RAM.

Quoted 2 lines

The kvm goals do not include cycle accurate emulation. [...]

yes. That's why i'm suggesting to kill the VM in such a scenario. A cr3 value is only valid if it points to real RAM.

Quoted 4 lines

[ 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.

Quoted 8 lines · Avi Kivity

Quoted 5 lines

[ 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.

hm, where do we ensure that? kvm_mmu_get_page() calls kvm_mmu_alloc_page(), which might return NULL:

static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,

                                                u64 *parent_pte)
 {
         struct kvm_mmu_page *page;

         if (list_empty(&vcpu->free_pages))
                 return NULL;

and i dont see where we ensure that ->free_pages is not empty. The relevant callchain would be kvm_vmx_return() -> handle_cr() -> set_cr3() -> paging_new_cr3() -> mmu_alloc_roots() -> kvm_mmu_get_page().

is there some accounting that makes a NULL pointer return impossible? The only one i can see is the direct quering of vcpu->free_pages or the querying of n_free_mmu_pages, but neither seems to be done in this codepath. (but i might have missed it)

Quoted 16 lines

hm, where do we ensure that? kvm_mmu_get_page() calls kvm_mmu_alloc_page(), which might return NULL:

static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,

                                               u64 *parent_pte)
{
        struct kvm_mmu_page *page;

        if (list_empty(&vcpu->free_pages))
                return NULL;

and i dont see where we ensure that ->free_pages is not empty. The relevant callchain would be kvm_vmx_return() -> handle_cr() -> set_cr3() -> paging_new_cr3() -> mmu_alloc_roots() -> kvm_mmu_get_page().

Quoted 6 lines

is there some accounting that makes a NULL pointer return impossible? The only one i can see is the direct quering of vcpu->free_pages or the querying of n_free_mmu_pages, but neither seems to be done in this codepath. (but i might have missed it)

The page fault path does this:

static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,

                    u32 error_code)
{
   if (unlikely(vcpu->kvm->n_free_mmu_pages < KVM_MIN_FREE_MMU_PAGES))
       kvm_mmu_free_some_pages(vcpu);
   return vcpu->mmu.page_fault(vcpu, gva, error_code);
}

The context switch paths need to do the same.

Quoted 3 lines

The context switch paths need to do the same.

yeah. Although, as this example has shown it, such implicit assumptions carried into code tends to be volatile. Couldnt the kvm_mmu_zap_page() be done implicitly within kvm_mmu_alloc_page()? As long as the list is LRU, and the number of allocations done within a logical operation doesnt exceed the size of the LRU list (which it doesnt), this should be doable.

Right not the list is not LRU, so a zap can kill your parent page table. I'm also worried about preserving invariants, as the page tables are linked through a variety of data structures. Calling zap_page() while another operation is in progress could cause corruption if zap_page() kills one of your parents or children.

ok. How about the patch below then? This only addresses the OOM scenario, not the !memslot case.

ok. How about the patch below then? This only addresses the OOM scenario, not the !memslot case.

instead of a host crash. Note that i chose to put this into the generic cr3 loading function, so that it covers real-mode too. I think we can safely ignore a BIOS loading crap into cr3 and after that loading the right value into it. (if that ever happens we 1) want to know about it 2) can push the test down into paging_new_cr3()) Agreed?

another small detail is that currently KVM_SET_MEMORY_REGION appears to be an add-only interface - it is not possible to 'unregister' RAM from a VM.

That keeps things easy for now, but if it's ever implemented then the current cr3 of all vcpus of the VM needs to be validated against the reduced memory slot map. (besides migrating all existing mappings from the removed memory slot to other memory slots and redirecting all in-flight DMA transactions, etc., etc. Which all needs heavy guest-OS awareness as well.)

The guest needs to cooperate, but it can do so using the native memory hotlpug mechanisms (whatever they are). [...]

Quoted 3 lines · Ingo Molnar

ok. How about the patch below then? This only addresses the OOM scenario, not the !memslot case.

the !memslot case is covered by the patch below. Injecting a #GPF is the easiest one to do here, although we could do a triple fault too - i just dont see the infrastructure for that in KVM, so i went for the easier solution ;-)

I have tested this with an intentionally bad cr3 value in a Linux guest, and the result is a relatively clean guest abort crash:

inject_general_protection: rip 0xc012093e
kvm_handle_exit: unexpected, valid vectoring info and exit reason is 0x9

at the right RIP:

c012093e: 0f 22 d8 mov %eax,%cr3

instead of a host crash. Note that i chose to put this into the generic cr3 loading function, so that it covers real-mode too. I think we can safely ignore a BIOS loading crap into cr3 and after that loading the right value into it. (if that ever happens we 1) want to know about it
2) can push the test down into paging_new_cr3()) Agreed?

Ingo