x86: fix page refcount handling in page table pin error path

In the original patch 7 of the series addressing XSA-45 I mistakenly
took the addition of the call to get_page_light() in alloc_page_type()
to cover two decrements that would happen: One for the PGT_partial bit
that is getting set along with the call, and the other for the page
reference the caller hold (and would be dropping on its error path).
But of course the additional page reference is tied to the PGT_partial
bit, and hence any caller of a function that may leave
->arch.old_guest_table non-NULL for error cleanup purposes has to make
sure a respective page reference gets retained.

Similar issues were then also spotted elsewhere: In effect all callers
of get_page_type_preemptible() need to deal with errors in similar
ways. To make sure error handling can work this way without leaking
page references, a respective assertion gets added to that function.

This is CVE-2013-1432 / XSA-58.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 48f3487102ed61d643ab07f156b070b0e28ebf99..52c9040f59b3e920cba63d36c48f8698e59e8e11 100644 (file)
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -829,6 +829,10 @@ int arch_set_info_guest(
     if ( v->vcpu_id == 0 )
         d->vm_assist = c(vm_assist);
 
+    rc = put_old_guest_table(current);
+    if ( rc )
+        return rc;
+
     if ( !compat )
         rc = (int)set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
     else
@@ -864,18 +868,24 @@ int arch_set_info_guest(
     }
     else
     {
-        /*
-         * Since v->arch.guest_table{,_user} are both NULL, this effectively
-         * is just a call to put_old_guest_table().
-         */
         if ( !compat )
-            rc = vcpu_destroy_pagetables(v);
+            rc = put_old_guest_table(v);
         if ( !rc )
             rc = get_page_type_preemptible(cr3_page,
                                            !compat ? PGT_root_page_table
                                                    : PGT_l3_page_table);
-        if ( rc == -EINTR )
+        switch ( rc )
+        {
+        case -EINTR:
             rc = -EAGAIN;
+        case -EAGAIN:
+        case 0:
+            break;
+        default:
+            if ( cr3_page == current->arch.old_guest_table )
+                cr3_page = NULL;
+            break;
+        }
     }
     if ( rc )
         /* handled below */;
@@ -901,6 +911,11 @@ int arch_set_info_guest(
                         pagetable_get_page(v->arch.guest_table);
                     v->arch.guest_table = pagetable_null();
                     break;
+                default:
+                    if ( cr3_page == current->arch.old_guest_table )
+                        cr3_page = NULL;
+                case 0:
+                    break;
                 }
             }
             if ( !rc )

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5123860ffd82da4705ea488ad4fb8a7d3d0ebc8e..77dcafc2614b48749bd12a78ef8f50972d64c5bd 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -640,7 +640,8 @@ static int get_page_and_type_from_pagenr(unsigned long page_nr,
           get_page_type_preemptible(page, type) :
           (get_page_type(page, type) ? 0 : -EINVAL));
 
-    if ( unlikely(rc) && partial >= 0 )
+    if ( unlikely(rc) && partial >= 0 &&
+         (!preemptible || page != current->arch.old_guest_table) )
         put_page(page);
 
     return rc;
@@ -2427,6 +2428,7 @@ int put_page_type_preemptible(struct page_info *page)
 
 int get_page_type_preemptible(struct page_info *page, unsigned long type)
 {
+    ASSERT(!current->arch.old_guest_table);
     return __get_page_type(page, type, 1);
 }
 
@@ -2617,7 +2619,7 @@ static void put_superpage(unsigned long mfn)
     return;
 }
 
-static int put_old_guest_table(struct vcpu *v)
+int put_old_guest_table(struct vcpu *v)
 {
     int rc;
 
@@ -2988,7 +2990,8 @@ long do_mmuext_op(
                     rc = -EAGAIN;
                 else if ( rc != -EAGAIN )
                     MEM_LOG("Error while pinning mfn %lx", page_to_mfn(page));
-                put_page(page);
+                if ( page != curr->arch.old_guest_table )
+                    put_page(page);
                 break;
             }
 

diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 6b367c0feed51e1243e4bfb21defd2f6d69ef235..213fc9cb37cd557f9738f621dd8adee908a73886 100644 (file)
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -354,6 +354,7 @@ void put_page_type(struct page_info *page);
 int  get_page_type(struct page_info *page, unsigned long type);
 int  put_page_type_preemptible(struct page_info *page);
 int  get_page_type_preemptible(struct page_info *page, unsigned long type);
+int  put_old_guest_table(struct vcpu *);
 int  get_page_from_l1e(
     l1_pgentry_t l1e, struct domain *l1e_owner, struct domain *pg_owner);
 void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner);