[PATCH] load_pdptrs cleanups

[PATCH] load_pdptrs cleanups

[Rusty Russell]

load_pdptrs can be handed an invalid cr3, and it should not oops.
This can happen because we injected #gp in set_cr3() after we set
vcpu->cr3 to the invalid value, or from kvm_vcpu_ioctl_set_sregs(), or
possibly (?) memory configuration changes after the guest did
set_cr3().

We should also copy the pdpte array once, before checking and
assigning, otherwise an SMP guest can potentially alter the values
between the check and the set.

Finally one nitpick: ret = 1 should be done as late as possible: this
allows GCC to check for unset "ret" should the function change in
future.

Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>

diff -r 98f010081c9f drivers/kvm/kvm_main.c
--- a/drivers/kvm/kvm_main.c	Wed Jul 25 10:39:07 2007 +1000
+++ b/drivers/kvm/kvm_main.c	Wed Jul 25 13:03:22 2007 +1000
@@ -432,30 +432,32 @@ static int load_pdptrs(struct kvm_vcpu *
 	gfn_t pdpt_gfn = cr3 >> PAGE_SHIFT;
 	unsigned offset = ((cr3 & (PAGE_SIZE-1)) >> 5) << 2;
 	int i;
-	u64 pdpte;
 	u64 *pdpt;
 	int ret;
 	struct page *page;
+	u64 pdpte[ARRAY_SIZE(vcpu->pdptrs)];
 
 	spin_lock(&vcpu->kvm->lock);
 	page = gfn_to_page(vcpu->kvm, pdpt_gfn);
-	/* FIXME: !page - emulate? 0xff? */
+	if (!page) {
+		ret = 0;
+		goto out;
+	}
+
 	pdpt = kmap_atomic(page, KM_USER0);
-
+	memcpy(pdpte, pdpt+offset, sizeof(pdpte));
+	kunmap_atomic(pdpt, KM_USER0);
+
+	for (i = 0; i < ARRAY_SIZE(pdpte); ++i) {
+		if ((pdpte[i] & 1) && (pdpte[i] & 0xfffffff0000001e6ull)) {
+			ret = 0;
+			goto out;
+		}
+	}
 	ret = 1;
-	for (i = 0; i < 4; ++i) {
-		pdpte = pdpt[offset + i];
-		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
-			ret = 0;
-			goto out;
-		}
-	}
-
-	for (i = 0; i < 4; ++i)
-		vcpu->pdptrs[i] = pdpt[offset + i];
-
+
+	memcpy(vcpu->pdptrs, pdpte, sizeof(vcpu->pdptrs));
 out:
-	kunmap_atomic(pdpt, KM_USER0);
 	spin_unlock(&vcpu->kvm->lock);
 
 	return ret;



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [PATCH] load_pdptrs cleanups

[Avi Kivity]

Rusty Russell wrote:
> load_pdptrs can be handed an invalid cr3, and it should not oops.
> This can happen because we injected #gp in set_cr3() after we set
> vcpu->cr3 to the invalid value, or from kvm_vcpu_ioctl_set_sregs(), or
> possibly (?) memory configuration changes after the guest did
> set_cr3().
>
> We should also copy the pdpte array once, before checking and
> assigning, otherwise an SMP guest can potentially alter the values
> between the check and the set.
>
> Finally one nitpick: ret = 1 should be done as late as possible: this
> allows GCC to check for unset "ret" should the function change in
> future.
>
>   

Applied, thanks.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/